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

Introduce 'window.withProgress' API endpoint #2979

Merged
merged 1 commit into from
Nov 30, 2018
Merged

Introduce 'window.withProgress' API endpoint #2979

merged 1 commit into from
Nov 30, 2018

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Sep 26, 2018

fixes #2802 Introduce 'window.withProgress' API endpoint
ezgif-1-028e4db99f

This is needed to run vsCode Kubernetes plugin in Theia. The plugin calls window.withProgress method

@akosyakov
Copy link
Member

@AlexTugarev please review core and messages extensions

@akosyakov akosyakov removed their request for review September 26, 2018 07:52
@AlexTugarev
Copy link
Contributor

I'm going to review and test this PR today.

@AlexTugarev
Copy link
Contributor

we need to synchronize this feature with https://github.com/theia-ide/theia/pull/2461/files somehow. it looks competing at first appearance, but I think there are different use cases in mind.

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

I reviewed core/message parts, and would like to do a next iteration. Currently I don't see this working. There is no caller in FE, and jsonrpc won't work. Proposal for changes to the API of MessageService coming next...

packages/core/src/common/message-service-protocol.ts Outdated Show resolved Hide resolved
packages/core/src/common/message-service.ts Outdated Show resolved Hide resolved
packages/core/src/common/message-service-protocol.ts Outdated Show resolved Hide resolved
packages/messages/src/browser/notifications.ts Outdated Show resolved Hide resolved
packages/messages/src/browser/notifications.ts Outdated Show resolved Hide resolved
packages/messages/src/browser/notifications.ts Outdated Show resolved Hide resolved
packages/messages/src/browser/notifications.ts Outdated Show resolved Hide resolved
@AlexTugarev
Copy link
Contributor

@vinokurig, could your try with a different progress API in the core? In the end it should also work for progress reports from BE.

Maybe something as simple as this:

// an alternative to `newProgress` could be a token factory as function
newProgress(): Promise<ProgressToken> {
    return Promise.resolve({ id: 'GENERATED-UUID' }); 
}
reportProgress(progress: ProgressToken, update: ProgressUpdate): Promise<void> {
    return this.client.reportProgress(progress, update);
}

interface ProgressToken {
    id: string;
}

interface ProgressUpdate {
    value: string;
    requestCancellation: boolean;
}

@vinokurig
Copy link
Contributor Author

@AlexTugarev Thank you for your feedback, I am going on vocation, so I will follow it in 3 weeks.

@vinokurig vinokurig force-pushed the plugin-2802 branch 5 times, most recently from ec83eed to 30d07ae Compare October 29, 2018 07:34
@vinokurig
Copy link
Contributor Author

@AlexTugarev Fixed all your comments, could you please take a look

@benoitf
Copy link
Contributor

benoitf commented Nov 7, 2018

hello @AlexTugarev did you were able to review this PR ?

@AlexTugarev
Copy link
Contributor

@benoitf, I'll provide a sample for the core part as mentioned previously. The current implementation won't work for the BE case, as callbacks cannot be passed via jsonrpc. Just synchronized with @vinokurig via PM on this.

@vinokurig
Copy link
Contributor Author

@AlexTugarev So what will be the next step?, Is it ready for merge now?

@AlexTugarev AlexTugarev dismissed their stale review November 23, 2018 16:23

Changes applied.

@vinokurig vinokurig force-pushed the plugin-2802 branch 2 times, most recently from 3e22837 to 38b7606 Compare November 28, 2018 09:45
@vinokurig
Copy link
Contributor Author

@AlexTugarev Could you please review my last changes

@AlexTugarev
Copy link
Contributor

LGTM! @vinokurig, please squash the commits. Let's wait for CI builds.

@vinokurig vinokurig force-pushed the plugin-2802 branch 2 times, most recently from da38db4 to e220094 Compare November 28, 2018 16:31
@vinokurig
Copy link
Contributor Author

@AlexTugarev looks like all CI builds have passed successfully, can we merge it?

@vinokurig vinokurig force-pushed the plugin-2802 branch 2 times, most recently from 30b28b5 to 42512e7 Compare November 29, 2018 12:25
Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

LGTM!

@vinokurig vinokurig force-pushed the plugin-2802 branch 2 times, most recently from ac4f93e to 744f9de Compare November 30, 2018 08:31
packages/plugin-ext/src/api/plugin-api.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/api/plugin-api.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/notification.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/plugin-context.ts Outdated Show resolved Hide resolved
packages/plugin-ext/src/plugin/plugin-context.ts Outdated Show resolved Hide resolved
@vinokurig vinokurig force-pushed the plugin-2802 branch 3 times, most recently from fca0eee to b064df5 Compare November 30, 2018 09:26
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@vinokurig vinokurig merged commit 72d94d0 into master Nov 30, 2018
@vinokurig vinokurig deleted the plugin-2802 branch November 30, 2018 12:25
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.

[plug-in] API endpoint for window.withProgress
5 participants