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

Notification Center and Progress Service #5830

Merged
merged 5 commits into from
Aug 17, 2019
Merged

Conversation

AlexTugarev
Copy link
Contributor

@AlexTugarev AlexTugarev commented Jul 31, 2019

What this does

  • adds notification center to show user messages in the bottom right corner
  • three most recent notifications are shown as "toasts"
  • there is a toggle command (and bell icon in the status bar) to open the center
  • hide notifications using esc
  • makes progress reporting cancellable
  • supports links in messages
  • collapses looong messages
  • moved statusbar progress (aka window location) to core
  • adds progress location service and integrates with scm/git

2019-08-13 11 09 49

How to test

CQs

Fixes #1360
Fixes #5635
Fixes #5415

@AlexTugarev AlexTugarev force-pushed the at/notification-center branch 3 times, most recently from 0d2db8b to 56dda5f Compare July 31, 2019 13:31
@AlexTugarev AlexTugarev marked this pull request as ready for review July 31, 2019 16:15
@vince-fugnitto
Copy link
Member

@AlexTugarev

It looks much better! Really nice work 👍
I noticed that if no notifications are present, clicking the notifications icon will open the notifications tray. The only problem is that this tray can not be closed :( Toggling the icon or pressing 'x' does not close it.

Jul-31-2019 12-29-41

@vince-fugnitto
Copy link
Member

Should we use the close-all icon for the notifications tray (similarly to VSCode) ?

Screen Shot 2019-07-31 at 12 43 28 PM

yarn.lock Outdated Show resolved Hide resolved
yarn.lock Show resolved Hide resolved
@AlexTugarev AlexTugarev force-pushed the at/notification-center branch 2 times, most recently from 7ebf6b1 to 4d7afd0 Compare August 1, 2019 08:09
@evidolob
Copy link
Contributor

evidolob commented Aug 1, 2019

It works very good!
But I noticed one thing: when notification are closed by time out it removed from notification tray also, so I can loose some important notifications.

How it works now:
ezgif com-crop

How it works in VSCode:
ezgif com-video-to-gif

I use VSCode sample to test it.

@AlexTugarev
Copy link
Contributor Author

AlexTugarev commented Aug 1, 2019

You are right @evidolob!
edit: though this is the current behavior on master, it's time to fix it.

✔️

@akosyakov akosyakov added notifications issues related to notifications vscode issues related to VSCode compatibility labels Aug 2, 2019
@westbury
Copy link
Contributor

westbury commented Aug 2, 2019

Our UX team here think this is a definite improvement. Apparently users are expecting messages in the lower right corner and during UX testing were missing messages at the top.

There does appear to be a problem with the progress bars. They do not go all the way across. For example this screenshot shows progress over 50% yet the bar is far smaller. Making the same showProgress/reportProgress calls to our original UI shows the correct percentage.
image

@AlexTugarev
Copy link
Contributor Author

Apparently users are expecting messages in the lower right corner and during UX testing were missing messages at the top.

@westbury, thanks for sharing!

... a problem with the progress bars.

✔️

@westbury
Copy link
Contributor

westbury commented Aug 2, 2019

I notice that you allow location to be only 'window' (message consumed by StatusBarProgress) or 'notification' (message consumed by NotificationCenterComponent). If you see anything else you switch it to 'notification'. Surely the idea of an extensible platform is that extenders can add their own locations. I would expect to be able to inject NotificationManager into my own UI component to show messages, and to be able to set location to my own custom value. I can see that you may want to be sure that all messages are consumed by at least one UI component, so perhaps there should be a way of registering location ids, or perhaps consumers should have a way of indicating to NotificationManager that they have displayed a message and that it therefore should not go in the notifications component. 'scm' is just one example of a location unknown to core/messages.

@akosyakov
Copy link
Member

akosyakov commented Aug 7, 2019

@westbury We are planning to extend APIs to cover any location. Actually it is already possible in the current state of PR, see ProgressLocationService. After this PR is landed we are going to add progress reporting to the explorer, plugin trees, Monaco to report LSP requests progress and the quick palette. @AlexTugarev please open follow-up issues for each location

@akosyakov
Copy link
Member

It would be nice if someone else try it with VS Code extensions contributing progress information.

@akosyakov
Copy link
Member

using >TXT: commands from https://github.com/AlexTugarev/txt/raw/master/txt-0.0.1.vsix

@AlexTugarev Where can I see source code of a command to understand what it is supposed to do?

@akosyakov
Copy link
Member

Also please update changelog. Is it breaking btw?

@akosyakov
Copy link
Member

akosyakov commented Aug 7, 2019

  • I was testing proposed VS Code extension and somehow got in the state that scm progress is running forever.
  • I wonder if i collapsed all notification explicitly, should it pop up again? It can be quite annoying if i want to get rid of them. What VS Code does?
  • when i open the editor the bell is not in the right most position anymore

@AlexTugarev
Copy link
Contributor Author

I wonder if i collapsed all notification explicitly, should it pop up again? It can be quite annoying if i want to get rid of them. What VS Code does?

@akosyakov don’t get this. You’re referring to hiding of all notifications, right? That has the same icon as collapse; I’ll replace this to avoid confusion.

If you just hide them and not clear them, you should be able to see them again when you open the center, e.g from status bar.

What is it what you expect? I think this part meets vscode.

@akosyakov
Copy link
Member

@AlexTugarev i meant something like do not disturb mode. If it is supported by VS Code now, let's make a new issue, don't need to handle it in this PR. We should start wrapping it up. Let me know when i can have a look at changes.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I've tested with the git native and vscode extensions mentioned in How to test section. It behaves well and code looks good. But please sort out context keys before merging.

Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>
Rework of `withProgress` in plugin.

Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>
Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>
Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>
Signed-off-by: Alex Tugarev <alex.tugarev@typefox.io>
@akosyakov
Copy link
Member

akosyakov commented Aug 17, 2019

@AlexTugarev i've gave it a try again, it works nicely, please merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notifications issues related to notifications vscode issues related to VSCode compatibility
Projects
None yet
5 participants