-
Notifications
You must be signed in to change notification settings - Fork 15k
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
refactor: ginify Notification #22821
Conversation
cc @sethlu |
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.
Can you add code to ensure the notification object is destroyed/cleared on exit?
It is a problem with Windows 10 that, if you don't clear the notification it would stay in the list even when it does not meant to.
@zcbenz hmm, that seems like it might be the intended behavior on Windows in the underlying API? If an app crashes, it won't be able to call any OS functions to remove notifications. |
It think this is similar to Tray icons, it is fine to leave a ghost icon there if the process crashed, but it should clear the icon if it exited normally. But anyway I tried to find out the original issue about this but didn't succeed, let's just leave it leaked on exit. |
Also I think this should be marked as |
This comment has been minimized.
This comment has been minimized.
NB this removes the |
No Release Notes |
Description of Change
Also, add some tests.
Checklist
npm test
passesRelease Notes
Notes: none