Progress extension #2461

JPinkney · 2018-07-29T23:56:22Z

This PR contributes a progress extension to Theia. With this PR different parts of theia will be able to contribute a progress report and users can filter through each progress report by their location in theia.

Related issue: #2299

Signed-off-by: jpinkney josh.pinkney@mail.utoronto.ca

svenefftinge · 2018-07-31T16:31:44Z

Nice!
I noticed that

the icon is always spinning even if there are no processes.
the pop-up doesn't close when focussing something else

Would be nice if the statusbar shows a bar or at least the percentage, so I don't need to open the pop-up..

simark · 2018-07-31T20:11:13Z

examples/browser/package.json

@@ -37,7 +37,8 @@
    "@theia/typescript": "^0.3.13",
    "@theia/userstorage": "^0.3.13",
    "@theia/variable-resolver": "^0.3.13",
-    "@theia/workspace": "^0.3.13"
+    "@theia/workspace": "^0.3.13",
+    "@theia/progress-monitor-extension": "^0.3.13"


I think the name could be simply "progress-monitor".

packages/progress-monitor/src/browser/progress-dialog.ts

simark · 2018-07-31T20:14:30Z

packages/progress-monitor/src/browser/progress-service.ts

+export class ProgressMonitorService {
+
+    protected readonly onContributionAddedOrUpdatedEmitter = new Emitter<ProgressReport>();
+    public onContributionAdded: Event<ProgressReport> = this.onContributionAddedOrUpdatedEmitter.event;


It's a bit strange that the names are not in sync. Should this be onContributionAddedOrUpdated?

akosyakov · 2018-08-01T04:54:41Z

packages/progress-monitor/src/browser/progress-widget.tsx

+        this.node.onclick = (e) => {
+            e.stopPropagation();
+        };
+        progressService.onContributionAdded((progressReport: ProgressReport) => {


I would manage the state in ProgressMonitorService to separate a model from a widget and here:

progressService.onChanged(() => this.update());

akosyakov · 2018-08-01T04:54:44Z

packages/progress-monitor/src/browser/progress-widget.tsx

+
+    SEARCH_DELAY = 200;
+
+    private progressItems: Map<string, ProgressReport> = new Map();


Should not it be a part of ProgressMonitorService?

packages/progress-monitor/src/browser/progress-widget.tsx

packages/progress-monitor/src/browser/style/index.css

simark · 2018-08-01T13:23:10Z

Just wondering, do you expect to have that many progress items in the progress monitor that it's worth having a search box? What scenario do you have in mind where this would be useful?

packages/progress-monitor/package.json

packages/progress-monitor/src/browser/progress-dialog.ts

packages/progress-monitor/src/browser/progress-monitor-frontend-module.ts

packages/progress-monitor/src/browser/progress-status-bar.ts

packages/progress-monitor/src/browser/progress-widget.tsx

packages/progress-monitor/src/browser/progress-dialog.ts

packages/progress-monitor/src/browser/progress-protocol.ts

akosyakov · 2018-08-08T04:51:46Z

packages/progress-monitor/src/browser/progress-dialog.ts

+        this.addKeyListener(document.body, Key.ESCAPE, e => {
+            this.closeDialog();
+        });
+        this.addEventListener(document.body, 'click', e => {


How about using a real callback: https://github.com/theia-ide/theia/blob/80f402c621fe197130eae7abd22a0d89008b4ef1/packages/core/src/browser/status-bar/status-bar.tsx#L45

The onclick for the status bar is set here to toggle whether the modal window is open. That part is needed so that it closes the dialog if anything but the modal window is clicked.

sorry, I don't understand then why code checks that a clicked element should not belong to status bar entry. Should not it check that it does not belong to this.node?

I was trying to make it so that you could click anywhere and close the modal window. However, that ran into some problems because when you would click the status bar item it would first hit that piece of code and close the dialog then the status bar item onclick would be triggered resulting in an opened dialog. To get around this I just made sure that if the status bar is the item being clicked then it shouldn't close the window here and instead wait for the onclick of the status bar item to close the dialog.

ok

You can get rid of coupling with the status bar entry by making sure that your event listener is first, either by using the capture phase or not attaching a dialog to a status bar entry (since it has higher z-index).

Also, you should use target not srcElement: https://developer.mozilla.org/en-US/docs/Web/API/Event/srcElement

packages/progress-monitor/src/browser/progress-dialog.ts

packages/progress-monitor/src/browser/progress-monitor-frontend-module.ts

JPinkney · 2018-09-23T00:50:02Z

@kittaakos @simark @akosyakov @svenefftinge Updated. Do you mind doing another review when you get a chance

AlexTugarev · 2018-10-02T09:11:01Z

packages/java/src/browser/java-client-contribution.ts

@@ -66,6 +63,7 @@ export class JavaClientContribution extends BaseLanguageClientContribution {
    protected onReady(languageClient: ILanguageClient): void {
        languageClient.onNotification(ActionableNotification.type, this.showActionableMessage.bind(this));
        languageClient.onNotification(StatusNotification.type, this.showStatusMessage.bind(this));
+        languageClient.onNotification(ProgressReportNotification.type, this.showProgressReportNotifications.bind(this));


Interesting, I just read up on microsoft/language-server-protocol#70, which proposes a similar protocol extension. Tough it might be not only focused on languages.

Is the JDT LS team aware of this it looks a bit competing, no?

@JPinkney could you check the LSP issue? maybe that contains hits for the protocol, which we can add/incorporate for the Progress protocol in Theia. Mapping ProgressReportNotification should not be a big problem.

@AlexTugarev While I think reporting progress on requests is a great addition, I think it's not necessarily the same thing: in jdt.ls, at least, background tasks are triggered on stuff that is not a reqest, think about a pom.xml being updated by a refactoring: the language server only gets notified with a "didChangeWatchedFiles()", but will start a large scale update, possibly a build as a consequence. As a user, I am interested in knowing when such things are going on.

I should have referred to the PR microsoft/vscode-languageserver-node#261, which I consider to be the proposal for LSP#70. As far as I see they've removed the boundary of a request.

packages/progress-monitor/src/browser/progress-widget.tsx

packages/core/src/browser/status-bar/status-bar.tsx

AlexTugarev · 2018-10-02T09:43:11Z

First of all, I think this is a very decent way to report progress. Accessing the progress ui by clicking on a statusbar item should not add more distraction, while adding a great possibility to inspect tasks running in background!

I tried this PR with the incorporation into the java extension. Unfortunately see this items. Though it looks like there were couple of updates afterwards, but they were not shown long enough to read them.

I'm mentioning this explicitly, as the status report of the JDT LS (which is still there) shows way more details.

I took a snapshot of the websocket frames to show, that there is not much traffic.

Signed-off-by: jpinkney <josh.pinkney@mail.utoronto.ca>

tsmaeder · 2018-11-05T11:10:05Z

@AlexTugarev just to make this clear: you're saying that the UI is not showing the last progress that was received?

AlexTugarev · 2018-11-05T11:31:34Z

'just tried it again with https://github.com/spring-guides/gs-spring-boot

the only progress item I saw was this:

in comparison to that, the status reporting shows many updates and indicates progress quite well.

also, I checked the JS console and that's what I found there:

bundle.js:100279 root ERROR Error: Widget is not attached.
    at Function.detach (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:23974:19)
    at ProgressDialog.../../node_modules/@phosphor/widgets/lib/widget.js.Widget.onCloseRequest (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:23625:20)
    at ProgressDialog.../../packages/core/lib/browser/widgets/widget.js.BaseWidget.onCloseRequest (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:98836:41)
    at ProgressDialog.../../node_modules/@phosphor/widgets/lib/widget.js.Widget.processMessage (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:23584:22)
    at invokeHandler (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:7518:21)
    at Object.sendMessage (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:7254:13)
    at ProgressDialog.../../node_modules/@phosphor/widgets/lib/widget.js.Widget.close (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:23421:33)
    at ProgressDialog.push.../../packages/progress-monitor/lib/browser/progress-dialog.js.ProgressDialog.closeDialog (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/53.bundle.js:141:14)
    at https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/53.bundle.js:303:38
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (https://3000-ba2325e4-5178-4432-97e1-fb84ebc30880.ws-eu.gitpod.io/bundle.js:100003:39)

olexii4 · 2019-05-08T09:43:51Z

@JPinkney Do you plan to continue working on this PR?

It is really needed for me. So I am going to continue work on it. WDYT?

JPinkney · 2019-05-08T13:02:11Z

@olexii4 It would be great if you could finish it off! Let me know if you have any questions

tsmaeder · 2019-05-09T08:10:25Z

@JPinkney Do you plan to continue working on this PR?

It is really needed for me. So I am going to continue work on it. WDYT?

@olexii4 How do you need this? It's is not the same as the proposed progress API in VS Code.

olexii4 · 2019-05-10T09:44:59Z

@tsmaeder I just need to add any progress when performing any git clone che#12564.
It is really annoying when you don't know what is happening for lots of minutes.

My old PR wasn't approved because of using the same progress as the proposed in API theia#4483.

So I am going to rework it.

olexii4 · 2019-05-17T07:48:16Z

I have created a new PR. Fixed a bug and rebased.

#5176

WDYT?

JPinkney · 2019-06-10T13:52:07Z

I'm going to close this because the other PR is open with the updates and fixes!

JPinkney force-pushed the progress-extension branch 2 times, most recently from 7a0f7f7 to 245a325 Compare July 30, 2018 00:01

simark reviewed Jul 31, 2018

View reviewed changes

packages/progress-monitor/src/browser/progress-dialog.ts Outdated Show resolved Hide resolved

simark reviewed Jul 31, 2018

View reviewed changes

akosyakov reviewed Aug 1, 2018

View reviewed changes

packages/progress-monitor/src/browser/style/index.css Outdated Show resolved Hide resolved

kittaakos reviewed Aug 1, 2018

View reviewed changes

tsmaeder mentioned this pull request Aug 2, 2018

Languages Sprint 153 eclipse-che/che#10571

Closed

12 tasks

JPinkney force-pushed the progress-extension branch 2 times, most recently from 6b71b48 to 790696d Compare August 7, 2018 20:35

JPinkney mentioned this pull request Aug 7, 2018

Add id option in status bar entry #2526

Closed

akosyakov reviewed Aug 8, 2018

View reviewed changes

packages/progress-monitor/src/browser/progress-dialog.ts Outdated Show resolved Hide resolved

akosyakov reviewed Aug 8, 2018

View reviewed changes

packages/progress-monitor/src/browser/progress-monitor-frontend-module.ts Outdated Show resolved Hide resolved

tsmaeder mentioned this pull request Sep 11, 2018

Languages Sprint 155 eclipse-che/che#11111

Closed

19 tasks

JPinkney force-pushed the progress-extension branch from 0725529 to 632fa49 Compare September 23, 2018 00:48

akosyakov mentioned this pull request Oct 1, 2018

General purpose progress monitor extension #2299

Open

svenefftinge requested a review from AlexTugarev October 1, 2018 12:10

AlexTugarev reviewed Oct 2, 2018

View reviewed changes

tsmaeder mentioned this pull request Oct 2, 2018

Languages Sprint 156 (K-1) eclipse-che/che#11438

Closed

15 tasks

JPinkney added 2 commits October 3, 2018 15:12

Added progress extension

222777a

Signed-off-by: jpinkney <josh.pinkney@mail.utoronto.ca>

fixup

b2b15a8

JPinkney force-pushed the progress-extension branch from 632fa49 to b2b15a8 Compare October 3, 2018 19:53

tsmaeder mentioned this pull request Oct 25, 2018

Languages Sprint 157 (K-2) eclipse-che/che#11733

Closed

13 tasks

tsmaeder mentioned this pull request Nov 12, 2018

Languages Sprint 158 (K-3) eclipse-che/che#11903

Closed

15 tasks

AlexTugarev mentioned this pull request Nov 26, 2018

Show the progress for Git clone operation #3631

Closed

tsmaeder mentioned this pull request Dec 6, 2018

Languages Sprint 159 (L-1) eclipse-che/che#12135

Closed

19 tasks

tsmaeder mentioned this pull request Jan 8, 2019

Languages Sprint 160 (L-2) eclipse-che/che#12352

Closed

16 tasks

tsmaeder mentioned this pull request Jan 15, 2019

Languages Sprint 161 (L-3) eclipse-che/che#12427

Closed

30 tasks

tsmaeder mentioned this pull request Feb 8, 2019

Sprint 162 (L-4) eclipse-che/che#12635

Closed

20 tasks

AlexTugarev mentioned this pull request Mar 7, 2019

add a progress notification when performing any git clone #4483

Closed

olexii4 mentioned this pull request May 17, 2019

Progress extension #5176

Closed

JPinkney closed this Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Progress extension #2461

Progress extension #2461

JPinkney commented Jul 29, 2018

svenefftinge commented Jul 31, 2018

simark Jul 31, 2018

simark Jul 31, 2018

akosyakov Aug 1, 2018 •

edited

Loading

akosyakov Aug 1, 2018

simark commented Aug 1, 2018

akosyakov Aug 8, 2018

JPinkney Aug 8, 2018

akosyakov Aug 8, 2018 •

edited

Loading

JPinkney Aug 8, 2018

akosyakov Aug 8, 2018 •

edited

Loading

JPinkney commented Sep 23, 2018

AlexTugarev Oct 2, 2018

AlexTugarev Oct 2, 2018

tsmaeder Oct 29, 2018 •

edited

Loading

AlexTugarev Oct 29, 2018

AlexTugarev commented Oct 2, 2018

tsmaeder commented Nov 5, 2018

AlexTugarev commented Nov 5, 2018

olexii4 commented May 8, 2019

JPinkney commented May 8, 2019

tsmaeder commented May 9, 2019

olexii4 commented May 10, 2019 •

edited

Loading

olexii4 commented May 17, 2019 •

edited

Loading

JPinkney commented Jun 10, 2019


		SEARCH_DELAY = 200;

		private progressItems: Map<string, ProgressReport> = new Map();

Progress extension #2461

Progress extension #2461

Conversation

JPinkney commented Jul 29, 2018

svenefftinge commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simark commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

JPinkney commented Sep 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev commented Oct 2, 2018

tsmaeder commented Nov 5, 2018

AlexTugarev commented Nov 5, 2018

olexii4 commented May 8, 2019

JPinkney commented May 8, 2019

tsmaeder commented May 9, 2019

olexii4 commented May 10, 2019 • edited Loading

olexii4 commented May 17, 2019 • edited Loading

JPinkney commented Jun 10, 2019

akosyakov Aug 1, 2018 •

edited

Loading

akosyakov Aug 8, 2018 •

edited

Loading

akosyakov Aug 8, 2018 •

edited

Loading

tsmaeder Oct 29, 2018 •

edited

Loading

olexii4 commented May 10, 2019 •

edited

Loading

olexii4 commented May 17, 2019 •

edited

Loading