Fix progress notification issues #8479

DoroNahari · 2020-09-08T08:19:57Z

Signed-off-by: Doron Nahari doron.nahari@sap.com

What it does

Fixes #6506
Fixes #8421
Fixes #8430
Fixes #8587

Hide progress notifications without title #6506 - Fix empty notifications and progress notifications.
progress notification cancelable default is different than vscode #8421 - Align progress notification cancelable property default with vscode (from true to false) (in vscode cancelable considered as true if cancelable=true (boolean) or any non-empty value except boolean false.)
progress without title but with progress.report message shows wrong colon prefix ':' #8430 - Remove redundant colon when using progress without title.

Progress notification which is not cancellable can be cancelled through the 'X' button #8587 - Progress notification which is not cancellable can be cancelled through the 'X' button.

How to test

Use this vscode extension sample to reproduce and see the fixes.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

DoroNahari · 2020-09-14T06:55:46Z

@AlexTugarev Can you review please?

AlexTugarev · 2020-09-14T06:56:50Z

@DoroNahari, yes sure! I'm happy to do it.

AlexTugarev · 2020-09-14T07:52:08Z

packages/messages/src/browser/notifications-manager.ts

@@ -66,8 +66,12 @@ export class NotificationManager extends MessageClient {
    protected readonly onUpdatedEmitter = new Emitter<NotificationUpdateEvent>();
    readonly onUpdated = this.onUpdatedEmitter.event;
    protected readonly fireUpdatedEvent = throttle(() => {
-        const notifications = deepClone(Array.from(this.notifications.values()));
-        const toasts = deepClone(Array.from(this.toasts.values()));
+        const notifications = deepClone(Array.from(this.notifications.values()).filter((notification: Notification) =>


showProgress/reportProgress should handle it.

if it's called with empty message a log entry should be created to give developers (not users!) a hint.

I wonder how vscode does it. if you provide no message in in showProgress but in reportProgress, especially if reportProgress is called with empty message in between.
does it flicker then? which is my concern this the approach ☝️

maybe we should just accept showProgress with non-empty message.

showProgress/reportProgress should handle it.

I didn't want it in showProgress because i wanted to handle empty 'regular' notification as well.

Here, is the place i found that handle all kind of notifications. otherwise i will need to look for empty message in many places (showProgress, reportProgress, showMessage and maybe more).
Can you think about other and better place that centralized all of the notifications?

I wonder how vscode does it. if you provide no message in in showProgress but in reportProgress, especially if reportProgress is called with empty message in between.
does it flicker then?

In vscode (and in Theia with this fix) if no message in showProgress it doesn't show the notification until you reportProgress with a message.
if reportProgress is called with empty message but with increment, the message stays as is and the progress bar updated accordingly. no flickering.

otherwise i will need to look for empty message in many places (showProgress, reportProgress, showMessage and maybe more).

as far as as I see in showProgress and in showMessage, yes. that's where they are added.

So do you agree with this approach?

Not sure yet. We need to check with clients which do not require labels at all, e.g. git progress with is handled in the progress locations...

My fix is for progress notification.
AFAIK, all git progresses are in the "scm" pane and not as notification. i couldn't found any use of progress notification in Theia.

AlexTugarev · 2020-09-14T07:54:29Z

packages/core/src/common/message-service-protocol.ts

-        return !message.options
-            || message.options.cancelable === undefined
-            || message.options.cancelable === true;
+        return !!message.options && !!message.options.cancelable;


@akosyakov would that require any change log as it changes the default behavior in theory?

@akosyakov @AlexTugarev
It's been few days since last comment here.
What should be done to promote this pr?

DoroNahari · 2020-09-24T08:25:25Z

@vince-fugnitto hi,
Can something be done from your side to promote this pr?

vince-fugnitto · 2020-09-24T15:39:35Z

@vince-fugnitto hi,
Can something be done from your side to promote this pr?

I haven't been following discussions related to the pull-request, I initially requested review from Alex since he is the original author of progress messages and it looks like he still has concerns/questions related to the implementation.

DoroNahari · 2020-09-24T16:20:34Z

@vince-fugnitto hi,
Can something be done from your side to promote this pr?

I haven't been following discussions related to the pull-request, I initially requested review from Alex since he is the original author of progress messages and it looks like he still has concerns/questions related to the implementation.

Alex asked Anton a question and since Anton didn't answer, i thought maybe you can.

Regarding the second thread, i answered all Alex's questions.

vince-fugnitto · 2020-09-24T16:26:07Z

@vince-fugnitto hi,
Can something be done from your side to promote this pr?

I haven't been following discussions related to the pull-request, I initially requested review from Alex since he is the original author of progress messages and it looks like he still has concerns/questions related to the implementation.

Alex asked Anton a question and since Anton didn't answer, i thought maybe you can.

It would of course be good to document the new behavior, if something is breaking (api, behavior or otherwise) then it should be documented as such as well.

Regarding the second thread, i answered all Alex's questions.

I think he had a concern about other clients and determining if they were properly handled such as git progress.
It would be good to identify if anything is broken by the changes for our internal clients.

The provided extension to test with is minimal (it only verifies the notification location such as toasts), it would be good to test other types of notifications for any regressions (I believe this was Alex's comment regarding scm).

DoroNahari · 2020-09-30T06:23:54Z

Added details to changelog.
Attached vscode extension which you can use to see all kind of progress locations and more:
- you can use the commands:
  1. "Show Notification Progress"
  2. "Show Cancelable Notification Progress"
  3. "Show SCM Progress"
  4. "Show Window Progress"
  5. "Show Empty Notification"
  6. "Show Notification"
    you can make changes to this extension as you want and re-run or ask me to add another variations.
I checked again all usages of withProgress in Theia.
- Regarding the change of the default cancelable option: There is one call to messageService.showProgress with explicitly declaring cancelable: true. the rest (debug-session-manager.ts, git-contribution.ts, git-quick-open-service.ts, navigator-model.ts, hosted-plugin.ts, vsc-extensions-model.ts) are calling progressService.withProgress which is explicitly declaring cancelable: true.
- Regarding not showing empty notification/progress-notification i didn't identified any usages of notification/progress-notification with empty content (what is the meaning of it anyway?). empty text in withProgress is ok unless the location of the progress is in the Notification area.

@AlexTugarev Are there any other concerns i didn't cover here?

amiramw

Tested all notification and they seems fine.
One thing is that even when executing "Show Notification Progress" I still see the X button that allows the user to close the notification (no cancel button though).

amiramw · 2020-10-01T21:42:51Z

packages/core/src/common/message-service-protocol.ts

-        return !message.options
-            || message.options.cancelable === undefined
-            || message.options.cancelable === true;
+        return !!message.options && !!message.options.cancelable;


consider return !!message.options?.cancelable

amiramw

I think you can open a separate issue for the X button on not cancellable. This PR seems fine to me and tested successfully.

Signed-off-by: Doron Nahari <doron.nahari@sap.com>

DoroNahari · 2020-10-04T11:17:17Z

Tested all notification and they seems fine.
One thing is that even when executing "Show Notification Progress" I still see the X button that allows the user to close the notification (no cancel button though).

I think you can open a separate issue for the X button on not cancellable. This PR seems fine to me and tested successfully.

Thank you for noticing this additional issue. i created another issue and fixed it here.

amiramw · 2020-10-05T22:17:45Z

Thank you for noticing this additional issue. i created another issue and fixed it here.

Thanks! Fix LGTM and tested again. Now no progress shows with X button, only simple notifications.

vince-fugnitto added progress issues related to the progress functionality vscode issues related to VSCode compatibility labels Sep 8, 2020

vince-fugnitto requested a review from AlexTugarev September 10, 2020 14:04

AlexTugarev reviewed Sep 14, 2020

View reviewed changes

DoroNahari force-pushed the progress-notif branch from 17a17a3 to 6bd9865 Compare September 29, 2020 22:30

DoroNahari requested a review from AlexTugarev October 1, 2020 13:40

amiramw reviewed Oct 1, 2020

View reviewed changes

amiramw approved these changes Oct 4, 2020

View reviewed changes

DoroNahari force-pushed the progress-notif branch from 6bd9865 to 6cb47c3 Compare October 4, 2020 09:45

Fix progress notification issues

3caa1da

Signed-off-by: Doron Nahari <doron.nahari@sap.com>

DoroNahari force-pushed the progress-notif branch from 6cb47c3 to 3caa1da Compare October 4, 2020 09:52

DoroNahari closed this Oct 4, 2020

DoroNahari reopened this Oct 4, 2020

amiramw merged commit d262753 into eclipse-theia:master Oct 8, 2020

vince-fugnitto mentioned this pull request Feb 5, 2021

core: fix progress documentation #9033

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix progress notification issues #8479

Fix progress notification issues #8479

DoroNahari commented Sep 8, 2020 •

edited

Loading

DoroNahari commented Sep 14, 2020

AlexTugarev commented Sep 14, 2020

AlexTugarev Sep 14, 2020

DoroNahari Sep 14, 2020 •

edited

Loading

AlexTugarev Sep 14, 2020

DoroNahari Sep 14, 2020

AlexTugarev Sep 14, 2020

DoroNahari Sep 17, 2020

AlexTugarev Sep 14, 2020

DoroNahari Sep 21, 2020

DoroNahari commented Sep 24, 2020

vince-fugnitto commented Sep 24, 2020

DoroNahari commented Sep 24, 2020

vince-fugnitto commented Sep 24, 2020

DoroNahari commented Sep 30, 2020

amiramw left a comment

amiramw Oct 1, 2020

DoroNahari Oct 4, 2020

amiramw left a comment

DoroNahari commented Oct 4, 2020

amiramw commented Oct 5, 2020

Fix progress notification issues #8479

Fix progress notification issues #8479

Conversation

DoroNahari commented Sep 8, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

DoroNahari commented Sep 14, 2020

AlexTugarev commented Sep 14, 2020

Choose a reason for hiding this comment

DoroNahari Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoroNahari commented Sep 24, 2020

vince-fugnitto commented Sep 24, 2020

DoroNahari commented Sep 24, 2020

vince-fugnitto commented Sep 24, 2020

DoroNahari commented Sep 30, 2020

amiramw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amiramw left a comment

Choose a reason for hiding this comment

DoroNahari commented Oct 4, 2020

amiramw commented Oct 5, 2020

DoroNahari commented Sep 8, 2020 •

edited

Loading

DoroNahari Sep 14, 2020 •

edited

Loading