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

Support indeterminate progress notifications #10945

Merged

Conversation

planger
Copy link
Contributor

@planger planger commented Mar 29, 2022

What it does

  • Default to progress notification type if showProgress() is used instead of overwriting it with info
  • Pass progress notification type through to view to make it aware that this is a progress without having to specify a value for the progress
  • Show indeterminate progress animation if notification type is progress but no numeric progress value is specified
  • Use info icon by default in view for progress notifications (this was implicitly done before too because we overwrote the notification type with info if the client didn't specify one
  • Add sample menu contributions to document and test progress notifications with determinate and indeterminate progress

Fixes #10944

How to test

Use the added sample commands "Sample Command With Indeterminate Progress Message" and "Sample Command With Progress Message". See also their handler implementation in sample-menu-contribution.ts, in particular how one reports no progress (indeterminate) and the other one does.

With progress reporting:
Peek 2022-03-29 13-21

Without progress reporting:
Peek 2022-03-29 13-22

Other Observations

  • Theoretically you can specify a message type with messageService.showProgress({ type: MessageType.Error, ...}) even though the argument type ProgressMessage dictates the optional type parameter to be of type MessageType.Progress. If clients still set another type, such as MessageType.Error this would affect the icon being shown in the notification. This PR doesn't change that behavior, but we only show an indeterminate progress, if the message type is MessageType.Progress.
  • Reporting a progress { done: 0, total: 100 } is equal to specifying no progress, which is after this PR indeterminate. This might be surprising, but I felt it is fine as no progress bar would otherwise be visible anyway. This is because of the way how a progress update is handled in NotificationManager#toPlainProgress().
  • Clients can't switch for existing notifications from a determinate progress to an indeterminate progress, because NotificationManager#reportProgress(...) falls back to previous progress message, if the new one is undefined. I guess this behavior is fine, because otherwise clients would be forced to always provide a work in their update, even if they just want to set a new message.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added notifications issues related to notifications progress issues related to the progress functionality labels Mar 29, 2022
@msujew msujew self-requested a review March 29, 2022 12:33
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

LGTM!

  • Normal progress notifications with progress are displayed as usual
  • Progress notifications without a progress property are displayed as in VSCode
  • Normal notifications continue to work as expected

@@ -195,12 +195,13 @@ export class MessageService {
const report = (update: ProgressUpdate) => {
this.client.reportProgress(id, update, message, cancellationSource.token);
};
const type = message.type ? message.type : MessageType.Progress;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick:

Suggested change
const type = message.type ? message.type : MessageType.Progress;
const type = message.type ?? MessageType.Progress;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the fast feedback and your suggestion!
I applied this change and also used the ?? operator in notification-component.tsx with the newly force-pushed commit 9f7c090.

  * Default to `progress` notification type if `showProgress()` is used
     instead of overwriting it with `info`
  * Pass through `progress` notification type to view
  * Show indeterminate progress animation if type `progress`
     but without specifying a numeric `progress` value > 0
  * Use `info` icon by default in view for `progress` notifications
  * Add sample menu contributions to document and test progress
    notifications with determinate and indeterminate progress

Fixes eclipse-theia#10944

Change-Id: Ieaf0e775cd1ad282b396c26c9047f4930f483712
@JonasHelming JonasHelming merged commit 31793be into eclipse-theia:master Mar 30, 2022
@JonasHelming JonasHelming added this to the 1.24.0 milestone Mar 30, 2022
@planger planger deleted the planger/issues/10944 branch March 30, 2022 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notifications issues related to notifications progress issues related to the progress functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support indeterminate progress notifications in message service
4 participants