-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
Add "Do Not Disturb" Mode #149645
Add "Do Not Disturb" Mode #149645
Conversation
Cool work! I agree with Kai that this should not be a setting, but just a local UI state. Will provide more feedback once I try it out :) |
👍 cool, I will hold my review until you tackled the remaining todos, or do you want a review now? |
Talked with @misolori and arrived at a more simple starting point. The latest behavior:
do-not-disturb.mp4@bpasero thoughts? |
Thanks for the feedback + cleanup! Will take another pass at this to address the items.
Miguel and I talked about that too—here's what it could look like. Thoughts? I'm a little worried about an icon handling two states at once but open to the idea.
I need to change it to be UI state instead of a setting. |
👍 , icon works for me, even with 3 states. |
Do progress notifications get hidden as part of do not disturb mode? @lramos15 @greazer @brettcannon were discussing this for the python/notebooks getting started experience |
Currently, yes. Options for improving this could include:
CleanShot.2022-06-21.at.11.53.45.mp4 |
@misolori I think you meant to tag someone else. :) |
You will forever be my tagging buddy. I think this must be like the 10th time you get tagged when I was meant to get tagged :p |
I like the idea of automatically moving a progress notification to the status bar when DND is enabled, I would not suggest to ask extensions to change their code though because it is perfectly fine to use a notification for progress. As for the implementation, maybe somewhere here we can additionally check for DND mode and then turn a progress notification into a silent one which will show in the status bar: vscode/src/vs/workbench/services/progress/browser/progressService.ts Lines 72 to 82 in f361c5b
|
Some updates:
CleanShot.2022-06-23.at.20.44.40.mp4 |
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 believe things would look nicer and be easier if we were to add the concept of do not disturb to the INotificationService
. That service already has a dependency to storage service, so it would work nicely. If we also had an event for when DND mode changes, listeners can easily be setup. It would also make sense conceptually: enabling DND mode should not be owned by the notification center (which is a UI piece), but rather by the notification core service, same as filters are there already.
In a nutshell:
- add methods to notification service for getting and changing DND mode
- add event to notification service for when DND mode changes
- add a
static readonly
constant for the storage key for reuse in notification service - adopt service in commands and UI elements
Some minor feedback:
- I wonder if the storage key should be
StorageTarget.MACHINE
so that it does not roam to other machines, I feel DND mode is a local, machine specific thing to enable based on situation and not something to sync to other devices - similar, should it be
StorageScope.APPLICATION
though to work across profiles?
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.
Almost there 👏
Btw one thing I now realised is: once this landed we should probably review any user of setFilter(filter: NotificationsFilter)
(e.g. Zen mode) and change it to set the DND mode accordingly.
In other words: once we have DND mode, entering Zen mode should simply toggle DND mode so that the user is aware. And we can probably then remove setFilter
from the interface and make it private.
src/vs/workbench/browser/parts/notifications/notificationsCenter.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/notification/common/notificationService.ts
Outdated
Show resolved
Hide resolved
constructor( | ||
@IStorageService private readonly storageService: IStorageService | ||
) { | ||
super(); | ||
|
||
this.registerListeners(); | ||
this.restoreDoNotDisturbModeState(); |
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 think this is not needed and would send out an event right on startup: rather anyone that depends on DND mode should ask the notification service for the mode when needed and then listen to changes going forward.
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.
The main reasoning for doing this was to ensure that the filters were actually set on startup/reload. Otherwise the user ends up in a state where DND is enabled but notifications will still come through. Open to other ideas on how to solve for that case though.
E.g.
CleanShot.2022-06-26.at.21.30.13.mp4
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.
Please see 359ee4c
src/vs/workbench/services/notification/common/notificationService.ts
Outdated
Show resolved
Hide resolved
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.
@daviddossett LGTM now, please take note of:
- 1bca7ce which uses this new DND mode from Zen mode so that there is no inconsistent state and we use the same concepts
- 359ee4c ensures to set the filters on startup based on state
I think we can 🚢 this in this state. One 💄 item maybe would be to consider changing the bell icon in the notifications center based on DND mode enabled or not. Now it is the crossed out bell even when you want to disable DND mode.
This PR solves for #144324 by implementing a "Do Not Disturb" mode that hides non-error toast notifications. Any hidden notifications can still be viewed in the notification center. It toggled directly from the notification center’s toolbar or from the command palette.
Demo
do-not-disturb.mp4