add a progress notification when performing any git clone #4483

olexii4 · 2019-03-05T09:08:36Z

Add a progress notification when performing any git clone.

Signed-off-by: Oleksii Orel oorel@redhat.com

Signed-off-by: Oleksii Orel <oorel@redhat.com>

akosyakov · 2019-03-06T06:07:40Z

vinokurig · 2019-03-06T07:45:12Z

It is strange for me that the notification can be closed by itself, even if clone is not finished yet. The button that restores the notification looks not obvious, I mean it is not clear what it does.

olexii4 · 2019-03-06T09:07:01Z

@vinokurig I add it like a proposal. Because it will be nice for me to hide an annoying notification after 10s.
Ok. I will improve the status bar widget. I will add a text "Tasks in progress: 3/10" nex the icon and change the tooltip.

olexii4 · 2019-03-06T18:30:16Z

olexii4 · 2019-03-06T18:31:46Z

WDYT?

svenefftinge · 2019-03-07T05:23:59Z

Yes, why not. Ideally we would do the clone in the terminal and rely on the natural output of git.
@AlexTugarev could you have a look at the changes regarding the notifications?

AlexTugarev · 2019-03-07T07:56:14Z

I really like the idea to show the progress git clone, actually of all kind of long running operations. However I think the UX didn't work in my test:

Starting a git clone operation

It shows Cloning the git repo progress notification 👍

The progress notification disappears after a short while, but I see spinner icon in the status bar for just two seconds.

Starting a second clone operation

... of a git repo containing a lot of objects.

And now I see purpose of the status bar item. I says Tasks in Progress 1/1 and it reveals the progress notification which was hidden previously.

It's not clear why the progress is hidden in the first place. Especially the progress notifications are meant to report progress continuously:
https://github.com/theia-ide/theia/blob/56222486fd076d3d71887e5e629152015faf88e5/packages/core/src/common/message-service-protocol.ts#L61-L71

And we can easily add the progress reporting from dugite-extra in the Git interface, see ICloneProgress
which is to be add here:
https://github.com/theia-ide/theia/blob/56222486fd076d3d71887e5e629152015faf88e5/packages/git/src/node/dugite-git.ts#L339

@olexii4 could you explain what the benefit of the status bar item is? Also, have you seen #2461? The generic progress reporting mechanism seems very similar.

AlexTugarev

Besides the UX concerns mentioned above, there is a Emitter leak, cf.

WARN Error: Possible Emitter memory leak detected. 30 exit listeners added. Use event.maxListeners to increase limit
    at Emitter.../../packages/core/lib/common/event.js.Emitter.checkMaxListeners (https://3000-a1d2c682-c255-4618-a5e6-60887fe22915.ws-eu0.gitpod.io/bundle.js:97020:26)
    at Notifications._event.Object.assign.maxListeners [as onShow] (https://3000-a1d2c682-c255-4618-a5e6-60887fe22915.ws-eu0.gitpod.io/bundle.js:96987:27)
    at Notifications.push.../../packages/messages/lib/browser/notifications.js.Notifications.createNotificationElement (https://3000-a1d2c682-c255-4618-a5e6-60887fe22915.ws-eu0.gitpod.io/78.bundle.js:503:18)
    at Notifications.push.../../packages/messages/lib/browser/notifications.js.Notifications.create (https://3000-a1d2c682-c255-4618-a5e6-60887fe22915.ws-eu0.gitpod.io/78.bundle.js:463:66)

olexii4 · 2019-03-07T15:56:33Z

@AlexTugarev
I have fixed the error and want to explain the benefit of the status bar item:
For this moment we have a possibility to show progress message with possible actions to the user. But, in general case, the progress message is shown under the command palette. And we should close all notifications for working with the command palette.
But, in the case when we close the existing progress message - lost the opportunity to know when the progress will be canceled.

With my improvement, we close any progress message in time from the preferences(notification.timeout). Then can reopen with the status bar widget.

And I added minimal changes to improve existing progress message, to become more convenient to users because we are using it with our API. And this improvement was added in the area of the issue "add progress when performing any git clone" like a separate commit to improving their behavior.

Progress extension looks very nice. But still not merged from the 2018 summer season. And the big improvement cannot be done in the issue of adding existing progress notification for git clone.

I can create The separate issue for progress message improvement when the progress extension will be merged. Like a part for withProgress improvement(theia.d.ts).

Signed-off-by: Oleksii Orel <oorel@redhat.com>

AlexTugarev · 2019-03-07T16:12:38Z

packages/messages/src/browser/notifications-message-client.ts

@@ -48,21 +52,59 @@ export class NotificationsMessageClient extends MessageClient {
    }

    showProgress(progressId: string, message: ProgressMessage, cancellationToken: CancellationToken, update?: ProgressUpdate): Promise<string | undefined> {
-        const messageArguments = { ...message, type: MessageType.Progress, options: { ...(message.options || {}), timeout: 0 } };
-        if (this.visibleProgressNotifications.has(progressId)) {
+        const messageArguments = {


why should the default value for timeout be changed now?

If we remove , timeout: 0 it will get it from preferences['notification.timeout']

https://github.com/theia-ide/theia/blob/4382a27f709c60cacfe1113534fda0f2e7d0d80e/packages/messages/src/browser/notifications-message-client.ts#L160

got it. the question is, why should that be the case in general? what is wrong about it in general? besides the git clone operation, the main use case when introducing this feature was to allow progress reporting. what happens with that now?

if the source for progress reports stops updating, will this dismiss the progress notification?

if you think this should be a short timeout value for git clone in this PR, why not just setting it explicitly?

I cannot understand you. Previously it was shown without a close timeout at all timeout: 0.

If the source for progress reports stops updating(as I understand - cancel this progress notification
https://github.com/theia-ide/theia/pull/4483/files#diff-cde0e8b29563cef99910b9e41f4daab7R89
), will this dismiss the progress notification.

AlexTugarev · 2019-03-07T17:02:01Z

But, in general case, the progress message is shown under the command palette. And we should close all notifications for working with the command palette.

The problem is, that the UI for notification is worn out and needs a refresh.

But here, a fix of the z-order of the notification would solve the issue, right? If you still can use the command palette while the progress notification is visible, you don't need to dismiss it automatically.

With my improvement, we close any progress message in time from the preferences(notification.timeout).

What is the reason to change this default?

Progress extension looks very nice. But still not merged from the 2018 summer season. And the big improvement cannot be done in the issue of adding existing progress notification for git clone.

Well, isn't it worth spending time on finishing stuff? The progress UI just had no (well) working application. The git clone would be a good candidate, especially with the hints about the progress reporting from above. I'd rather see progress extension redux using the git clone case as an application of it, instead of adding yet another shortcut.

I'm releasing the break as the memory leak is gone. But please get more people convinced of the UX.

the leak is gone

AlexTugarev · 2019-03-07T17:15:21Z

packages/git/src/browser/git-quick-open-service.ts

@@ -77,11 +77,16 @@ export class GitQuickOpenService {
                    dynamicItems.push(new SingleStringInputOpenItem(
                        `Clone the Git repository: ${lookFor}. ${suffix}`,
                        async () => {
+                            const progress = await gitQuickOpenService.messageService.showProgress({


cf. links in:
eclipse-che/che#12564 (comment)
eclipse-che/che#12564 (comment)

it was discussed couple of times before. and it would really make sense so tackle this in a more generic way.

Ok. There's more general issue #759.

What about approve improvements for inProgress notifications only?
We are using them in our API and it will be nice to improve the existing behavior. In this case, I will remove the first commit and change the PR description

olexii4 · 2019-03-11T14:21:20Z

@AlexTugarev
Could we discuss the second commit for inProgress notification improvements only? I am sure it will be a good improvement

add progress when performing a git clone

4f619b0

Signed-off-by: Oleksii Orel <oorel@redhat.com>

olexii4 force-pushed the CHE-12564_1 branch from 09e5b34 to 048f6d3 Compare March 5, 2019 16:03

olexii4 requested review from vitaliy-guliy, vinokurig, akosyakov, sunix and evidolob March 5, 2019 16:04

olexii4 changed the title ~~add progress when performing a git clone~~ add a progress notification when performing any git clone Mar 5, 2019

olexii4 marked this pull request as ready for review March 6, 2019 08:46

AlexTugarev previously requested changes Mar 7, 2019

View reviewed changes

vinokurig approved these changes Mar 7, 2019

View reviewed changes

improve inProgress notifications

4382a27

Signed-off-by: Oleksii Orel <oorel@redhat.com>

olexii4 force-pushed the CHE-12564_1 branch from c0f7b63 to 4382a27 Compare March 7, 2019 16:01

AlexTugarev reviewed Mar 7, 2019

View reviewed changes

AlexTugarev self-requested a review March 7, 2019 17:02

AlexTugarev reviewed Mar 7, 2019

View reviewed changes

AlexTugarev requested a review from kittaakos March 7, 2019 17:15

olexii4 closed this Mar 15, 2019

olexii4 mentioned this pull request May 10, 2019

Progress extension #2461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a progress notification when performing any git clone #4483

add a progress notification when performing any git clone #4483

olexii4 commented Mar 5, 2019 •

edited

Loading

akosyakov commented Mar 6, 2019

vinokurig commented Mar 6, 2019

olexii4 commented Mar 6, 2019 •

edited

Loading

olexii4 commented Mar 6, 2019

olexii4 commented Mar 6, 2019

svenefftinge commented Mar 7, 2019

AlexTugarev commented Mar 7, 2019

AlexTugarev left a comment

olexii4 commented Mar 7, 2019

AlexTugarev Mar 7, 2019

olexii4 Mar 7, 2019 •

edited

Loading

olexii4 Mar 7, 2019

AlexTugarev Mar 7, 2019

olexii4 Mar 11, 2019

AlexTugarev commented Mar 7, 2019

AlexTugarev Mar 7, 2019

olexii4 Mar 11, 2019

olexii4 commented Mar 11, 2019

add a progress notification when performing any git clone #4483

add a progress notification when performing any git clone #4483

Conversation

olexii4 commented Mar 5, 2019 • edited Loading

akosyakov commented Mar 6, 2019

vinokurig commented Mar 6, 2019

olexii4 commented Mar 6, 2019 • edited Loading

olexii4 commented Mar 6, 2019

olexii4 commented Mar 6, 2019

svenefftinge commented Mar 7, 2019

AlexTugarev commented Mar 7, 2019

Starting a git clone operation

Starting a second clone operation

AlexTugarev left a comment

Choose a reason for hiding this comment

olexii4 commented Mar 7, 2019

AlexTugarev Mar 7, 2019

Choose a reason for hiding this comment

olexii4 Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

olexii4 Mar 7, 2019

Choose a reason for hiding this comment

AlexTugarev Mar 7, 2019

Choose a reason for hiding this comment

olexii4 Mar 11, 2019

Choose a reason for hiding this comment

AlexTugarev commented Mar 7, 2019

AlexTugarev Mar 7, 2019

Choose a reason for hiding this comment

olexii4 Mar 11, 2019

Choose a reason for hiding this comment

olexii4 commented Mar 11, 2019

olexii4 commented Mar 5, 2019 •

edited

Loading

olexii4 commented Mar 6, 2019 •

edited

Loading

olexii4 Mar 7, 2019 •

edited

Loading