Skip to content
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

Merged
merged 35 commits into from
Jun 27, 2022
Merged

Add "Do Not Disturb" Mode #149645

merged 35 commits into from
Jun 27, 2022

Conversation

daviddossett
Copy link
Contributor

@daviddossett daviddossett commented May 16, 2022

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
vscode.window.showInformationMessage('Info Notification As Modal', { modal: true }); // Always shows	
vscode.window.showInformationMessage('Info Notification');
vscode.window.showWarningMessage('Warning Notification');
vscode.window.showErrorMessage('Error Notification'); // Always shows

@daviddossett daviddossett added ux User experience issues under-discussion Issue is under discussion for relevance, priority, approach workbench-notifications Notification widget issues labels May 16, 2022
@isidorn
Copy link
Contributor

isidorn commented May 17, 2022

Cool work!
Currently when users enter zen mode it also disables notifications like the do not disturb mode. So to make things simpler, it would be good if Zen mode just turns on Do Not Disturb (and in the case users have status bar visible in zen the new icon would appear).

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 :)

@bpasero
Copy link
Member

bpasero commented May 27, 2022

👍 cool, I will hold my review until you tackled the remaining todos, or do you want a review now?

@daviddossett daviddossett dismissed a stale review via 8a8a54a June 17, 2022 16:27
@daviddossett
Copy link
Contributor Author

daviddossett commented Jun 17, 2022

Talked with @misolori and arrived at a more simple starting point. The latest behavior:

  • Static icon in notification center toolbar (too much weirdness when swapping icons)
  • Do Not Disturb status is represented by the status bar icon
  • Lets error notifications through
  • Lets modal notifications through
do-not-disturb.mp4

@bpasero thoughts?

@daviddossett daviddossett marked this pull request as ready for review June 17, 2022 19:15
@VSCodeTriageBot VSCodeTriageBot added this to the June 2022 milestone Jun 17, 2022
@daviddossett
Copy link
Contributor Author

Thanks for the feedback + cleanup! Will take another pass at this to address the items.

I still wonder if we should show a little dot even when in DND mode to signal that notifications have arrived accessible when you click the bell

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.

CleanShot 2022-06-20 at 09 29 34@2x

why is this a setting?

I need to change it to be UI state instead of a setting.

@bpasero
Copy link
Member

bpasero commented Jun 20, 2022

👍 , icon works for me, even with 3 states.

@miguelsolorio
Copy link
Contributor

miguelsolorio commented Jun 21, 2022

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

@daviddossett
Copy link
Contributor Author

daviddossett commented Jun 21, 2022

Do progress notifications get hidden as part of do not disturb mode? @loganrosen @greazer @brettcannon were discussing this for the python/notebooks getting started experience

Currently, yes. Options for improving this could include:

  • Automatically move progress notifications to ProgressLocation.Window (status bar—example below) when do not disturb mode is enabled. Caveat—I don't know if this is possible today. @bpasero may know.
  • Suggest extensions move towards ProgressLocation.Window usage instead of ProgressLocation.Notification.
  • Like the bell-slash-with-dot proposed above, we could consider a bell-slash-with-progress icon, or replacing with the spinner altogether.
CleanShot.2022-06-21.at.11.53.45.mp4

@loganrosen
Copy link
Member

@misolori I think you meant to tag someone else. :)

@lramos15
Copy link
Member

@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

@bpasero
Copy link
Member

bpasero commented Jun 22, 2022

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:

case ProgressLocation.Notification:
return this.withNotificationProgress({ ...options, location }, task, onDidCancel);
case ProgressLocation.Window:
if ((options as IProgressWindowOptions).command) {
// Window progress with command get's shown in the status bar
return this.withWindowProgress({ ...options, location }, task);
}
// Window progress without command can be shown as silent notification
// which will first appear in the status bar and can then be brought to
// the front when clicking.
return this.withNotificationProgress({ delay: 150 /* default for ProgressLocation.Window */, ...options, silent: true, location: ProgressLocation.Notification }, task, onDidCancel);

@daviddossett
Copy link
Contributor Author

Some updates:

  • Moved from a setting to use storage service
  • Shows window progress instead of notification progress when mode is enabled
CleanShot.2022-06-23.at.20.44.40.mp4

Copy link
Member

@bpasero bpasero left a 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?

Copy link
Member

@bpasero bpasero left a 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/common/notifications.ts Outdated Show resolved Hide resolved
src/vs/platform/notification/common/notification.ts Outdated Show resolved Hide resolved
src/vs/platform/notification/common/notification.ts Outdated Show resolved Hide resolved
constructor(
@IStorageService private readonly storageService: IStorageService
) {
super();

this.registerListeners();
this.restoreDoNotDisturbModeState();
Copy link
Member

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.

Copy link
Contributor Author

@daviddossett daviddossett Jun 27, 2022

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

Copy link
Member

@bpasero bpasero Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see 359ee4c

Copy link
Member

@bpasero bpasero left a 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.

image

@daviddossett daviddossett merged commit f148d86 into main Jun 27, 2022
@daviddossett daviddossett deleted the ddossett/rear-halibut branch June 27, 2022 13:49
@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
under-discussion Issue is under discussion for relevance, priority, approach ux User experience issues workbench-notifications Notification widget issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants