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

Progress extension #5176

Closed
wants to merge 1 commit into from
Closed

Progress extension #5176

wants to merge 1 commit into from

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented May 17, 2019

As a continuation of the PR, with the preservation of the authorship of an origin commit.

Screenshot from 2019-06-11 17-21-07

Just tried it with 'git clone' - video

@olexii4 olexii4 mentioned this pull request May 17, 2019
@olexii4 olexii4 marked this pull request as ready for review May 17, 2019 07:49
Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

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

Awesome! I tested it out and it seems like the only thing that isn't working is when you focus something else the progress monitor still stays open.

@olexii4
Copy link
Contributor Author

olexii4 commented May 27, 2019

@JPinkney Several classes were added accidentally from your different commit. I have removed all that was unnecessary. Please, have a look.

@JPinkney
Copy link
Contributor

@olexii4 It looks like the progress monitor flickers if the progress reports are coming in really fast (Tested using Che), other then that everything is working as expected.

@olexii4
Copy link
Contributor Author

olexii4 commented Jun 11, 2019

@JPinkney I have updated it (fix the progress monitor flickers).

@olexii4
Copy link
Contributor Author

olexii4 commented Jun 11, 2019

@akosyakov @AlexTugarev Could you review this PR?

@olexii4
Copy link
Contributor Author

olexii4 commented Jun 11, 2019

Just tried it with 'git clone' - video

@AlexTugarev
Copy link
Contributor

'Looking into this today.
@olexii4, please take care of commit signing.

@olexii4
Copy link
Contributor Author

olexii4 commented Jun 13, 2019

@JPinkney Please, add your "Signed-off-by footer"

Co-authored-by: Oleksii Orel <oorel@redhat.com>

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
Signed-off-by: Oleksii Orel <oorel@redhat.com>
@westbury
Copy link
Contributor

westbury commented Jul 2, 2019

Thank you for your help in improving progress reporting in Theia. This is certainly something we need.

I was wondering how this relates to the existing support for progress reporting in the message package (showProgress and reportProgress in MessageClient and NotificationsMessageClient. That has a basic implementation (MessageClient) in the core package that simply reports to the console. The drop-down notifications are implemented in the message package. I believe this is the structure we should maintain because this means packages making notifications or progress reports need only depend on the core package. Extenders can then chose to use the message package or brand with their own MessageClient bindings.

I do not see any reason not to stick with the existing API. I can see there may be advantages in combining showProgress and reportProgress into your single addOrUpdateContribution function. However I would do that by ensuring calls to reportProgress imply showProgress not previously called.

This PR is regressing on functionality by not including a 'cancel' button in the progress, as that can be added to the current drop-down progress. However as existing progress reporters would not automatically switch to your progress dialog, that is ok.

My main concern is that we would now have two progress reporting systems, with some packages hooked up to one and some packages hooked up to another. Extenders currently will typically rebind MessageClient and delegate showMessage to one MessageClient implementation while delegating showProgress and reportProgress to another MessageClient implementation, and I am concerned this will become more confused.

@kittaakos kittaakos removed their request for review August 5, 2019 08:30
@olexii4 olexii4 closed this Aug 14, 2019
@olexii4 olexii4 deleted the progress-monitor branch August 14, 2019 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants