-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip multiprocessing if no callback actions requested; Add sound param; Fix signal handling #26
base: main
Are you sure you want to change the base?
Conversation
@@ -48,12 +49,13 @@ class NotificationManager(metaclass=Singleton): | |||
""" | |||
|
|||
def __init__(self): | |||
self._callback_queue: SimpleQueue = SimpleQueue() | |||
self._callback_queue: SimpleQueue | None = None | |||
self._callback_executor_event: Event = Event() | |||
self._callback_executor_thread: CallbackExecutorThread | None = None | |||
self._callback_listener_process: NotificationProcess | None = None | |||
# Specify that once we stop our application, self.cleanup should run | |||
atexit.register(self.cleanup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jorricks why do you need both atexit()
and custom SIGINT
handling that do the exact same thing (call self.cleanup()
)? Seems like just atexit()
should be sufficient to accomplish what you are going for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly do not recall.
I think we can indeed remove the catch_keyboard_interrupt
but let's save that for another MR.
It turns out that because the |
First of all, sorry for being so slow to respond. |
Changes:
sys.exit()
is just the return code for the shell; it does not re-raise theSIGINT
to callsys.exit(signal.SIGINT)
. Now it properly re-raises the trapped signal.sound
parameter as per the other PR Add sound param #25In order to implement (1) I had to move the
MacOSNotification
class outside of thecreate_notification()
method so it would not be redeclared every timecreate_notification()
is called - see my comment.Re: (2) I'm still not really convinced that trapping
SIGINT
in a package is the best idea and am kind of concerned about other unforeseen side effects but at least re-raising it seems to work as the user might expect.Feel free to cherry pick and/or request changes.