Support Webview Views #10137

ChayaLau · 2021-09-19T13:23:46Z

Added API to support Webview based views contributions

Signed-off-by: ChayaLau [email protected]

What it does

This commit adds support to install and use webview views extensions - to be shows in the views - side panel
as described in the issue:
#8740

How to test

install the VSCode webview-view-sample extension.
go to the explorer side panel menu and click the button
the list of views should include the calico colors title
expand the view -> expected results - the webview should appear presenting the webview view contribution.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

Added API to support Webview based views contributions Signed-off-by: ChayaLau <[email protected]>

ChayaLau · 2021-09-19T19:43:08Z

@vince-fugnitto , @msujew, @colin-grant-work
I would like if you can take a look in this PR, so i can get prepared to the next version in advanced.
it is still one of my first commits in this repo - so i would like to hear any comment and recommendation and fix accordingly.
thanks

msujew

The functionality looks good to me so far. The sample webview-view extension works perfectly without impacting other webview related extensions like the webview-sample. 👍

I have a few remarks on the code. Nonetheless, impressive work for being quite new to the project.

packages/plugin-ext/src/common/plugin-protocol.ts

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

packages/plugin-ext/src/plugin/webview-views.ts

packages/plugin-ext/src/plugin/webviews.ts

packages/plugin/src/theia-proposed.d.ts

packages/plugin-ext/src/main/browser/webview-views/webview-views-main.ts

ChayaLau · 2021-09-20T13:21:58Z

The functionality looks good to me so far. The sample webview-view extension works perfectly without impacting other webview related extensions like the webview-sample. 👍

I have a few remarks on the code. Nonetheless, impressive work for being quite new to the project.

Hi - @msujew,
thank you so much for your review and the comments,
i will handle them and will let you know when done.

Added API to support Webview based views contributions Signed-off-by: ChayaLau <[email protected]>

into webview-view-support

ChayaLau · 2021-09-29T14:28:34Z

hi @msujew
I fixed the above according to your advice, thx.
how can i ask for a secondary review or a merge if possible ?

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts

packages/plugin-ext/src/main/browser/main-context.ts

packages/plugin-ext/src/main/browser/webview-views/webview-views-main.ts

packages/plugin/src/theia.d.ts

tsmaeder

A first batch of comments: I think this PR could be simpler, but maybe we should just get the code in an then refactor 🤷

packages/plugin-ext/src/main/browser/webview-views/webview-views.ts

packages/plugin/src/theia-proposed.d.ts

tsmaeder · 2021-10-05T12:19:14Z

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

        const view = this.views.get(viewId);
+        if (view?.[1]?.type === PluginViewType.Webview) {
+            const webviewWidget = this.widgetManager.getWidget(WebviewWidget.FACTORY_ID, <WebviewWidgetIdentifier>{ id: webviewId });


Welcome content (https://code.visualstudio.com/api/references/contribution-points#contributes.viewsWelcome) is not handled for webview views. I guess the right thing to do would be to move the welcome content handling out of the tree view and into the PluginViewWidget class. The less protocol we require from the view widget, the more we can reuse.

do you recommend stop working on this for now until the refactoring is done? or we should do the refactoring after all?

No, it's better to get the functionality in with limitations than not to have it at all: it's just missing functionality, it does not break anything beyond the new stuff.

packages/plugin-ext/src/main/browser/webview-views/webview-views.ts

tsmaeder · 2021-10-05T13:56:18Z

I'm thinking this area of the code needs some refactoring, in particular the "welcome content" support in views (which right now is tied to tree view, when it would not have to be). It's like the old joke about the englishman who wanted to go to Letterfrack: "I wouldn't start from here" ;-)

Added API to support Webview based views contributions Signed-off-by: ChayaLau <[email protected]>

into webview-view-support

ChayaLau · 2021-10-24T09:51:19Z

I'm thinking this area of the code needs some refactoring, in particular the "welcome content" support in views (which right now is tied to tree view, when it would not have to be). It's like the old joke about the englishman who wanted to go to Letterfrack: "I wouldn't start from here" ;-)

i want to try to get this merged before next release.
i see 3 limitations

the area of register view needs refactoring.
the welcome view is not supported
we might be able to skip the webview view service and register right into plugin view registry. - handled in below commit.
what is important for before merge, and what can wait to after merge?

…w registry

vince-fugnitto · 2021-10-25T12:30:19Z

i want to try to get this merged before next release

@ChayaLau I'm not sure it will be completed in time for the next release unfortunately (Thursday 28th), as there are currently limitations, and I believe CQs are still necessary:

Support Webview Views #10137 (comment) (not sure why it was previously resolved)

Is there a committer at SAP that can help you register CQs?

ChayaLau · 2021-10-25T18:58:08Z

i want to try to get this merged before next release

@ChayaLau I'm not sure it will be completed in time for the next release unfortunately (Thursday 28th), as there are currently limitations, and I believe CQs are still necessary:

Support Webview Views #10137 (comment) (not sure why it was previously resolved)

Is there a committer at SAP that can help you register CQs?

yes, i will ask @amiramw to help me with CQ
what about code review changes, about the refactoring, are we going to wait for it?

amiramw · 2021-11-11T22:49:11Z

yes, i will ask @amiramw to help me with CQ

Done: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23825

bodarajeshkumar · 2021-11-16T12:27:52Z

Happy to see this feature getting implemented. I am waiting for this feature to be available in Eclipse-Che. Any update on when this can be merged and available?

bodarajeshkumar · 2021-11-29T11:08:00Z

Is there anything blocking this to be merged? Can someone please let me know If I can expect this to be available in the next few releases?

marcdumais-work · 2021-11-29T22:55:53Z

Looking at the history, I do not see major outstanding comments. It could be that reviewers have postponed a thorough review until the CQ was obtained and need to have another look.

The PR currently has conflicts. Please rebase it to the latest master branch.

bodarajeshkumar · 2021-11-30T05:19:39Z

@marcdumais-work Thank you for the update. @ChayaLau I am glad that you implemented this feature and looking forward to it being available. Can you please rebase this to the master branch which can help In merging this feature?

tsmaeder · 2021-12-21T15:43:25Z

@msujew or @vince-fugnitto I think we as reviewers kinda dropped the ball on this one. I won't be able to address this PR in the next month. Can one of you pick this one up?

msujew · 2021-12-21T15:56:48Z

@tsmaeder I will continue with the review at the start of January 👍

ChayaLau · 2021-12-21T16:01:20Z

@tsmaeder I will continue with the review at the start of January 👍

thanks

vince-fugnitto · 2022-01-05T19:32:47Z

@msujew or @vince-fugnitto I think we as reviewers kinda dropped the ball on this one.

@ChayaLau do you mind rebasing the pull-request against the latest master? There have been conflicts since #10137 (comment) and they would need to be resolved before we can proceed with a proper review (likely the review took time since there was no activity for a while in order to rebase).

msujew

The code looks mostly good to me. I have added some minor comments.

The feature still works as expected, @ChayaLau do you mind rebasing your branch soonish?

Everything looks good to me from an achitectural viewpoint, @colin-grant-work do you have something to add?

msujew · 2022-01-11T10:59:56Z

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

@@ -319,6 +326,68 @@ export class PluginViewRegistry implements FrontendApplicationContribution {
        return toDispose;
    }

+    public async registerWebviewView(viewId: string, resolver: WebviewViewResolver): Promise<Disposable> {


Minor: methods in JavaScript/TypeScript are public by default, the public modifier is not necessary.

msujew · 2022-01-11T11:20:43Z

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

+                            await this.prepareView(view, webviewView.webview.identifier.id);
+                        }
+                    }));
+                    const unsubscribe = () => toDisposeOnDidExpandView.dispose();


Suggestion: Rename this to dispose. Keeps it more in line with our usual naming conventions.

msujew · 2022-01-11T13:17:36Z

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

+        let _description: string | undefined;
+
+        return {
+            webview: webview,


Nitpick:

Suggested change

webview: webview,

webview,

msujew · 2022-01-11T13:22:16Z

packages/plugin-ext/src/main/browser/view/plugin-view-registry.ts

+            dispose(): void {
+                webview.dispose();
+            },
+
+            show(preserveFocus: boolean): void {
+                webview.show();
+            }


Nitpick:

Suggested change

dispose(): void {

webview.dispose();

},

show(preserveFocus: boolean): void {

webview.show();

}

dispose: webview.dispose,

show: webview.show

msujew · 2022-01-11T13:24:27Z

packages/plugin-ext/src/main/browser/webview-views/webview-views-main.ts

+    protected getWebviewView(handle: string): WebviewView {
+        const webviewView = this.webviewViews.get(handle);
+        if (!webviewView) {
+            throw new Error('unknown webview view');


Suggestion: Add the handle to the error message to better determine what went wrong when this error actually occurs.

msujew · 2022-01-26T15:35:21Z

@ChayaLau Are you still interested in contributing this feature?

vince-fugnitto · 2022-01-26T15:38:11Z

@ChayaLau Are you still interested in contributing this feature?

@msujew it has been a while they haven't commented on this pull-request or other open pull-requests, should we aim to help complete them so they are landed?

msujew · 2022-01-26T16:24:36Z

@vince-fugnitto Yes, I think this one in particular is basically done 👍

ChayaLau force-pushed the webview-view-support branch 2 times, most recently from f322ac9 to ff70576 Compare September 19, 2021 14:28

Support Webview View

d12ce31

Added API to support Webview based views contributions Signed-off-by: ChayaLau <[email protected]>

ChayaLau force-pushed the webview-view-support branch from af0b1d8 to d12ce31 Compare September 19, 2021 14:39

msujew self-requested a review September 20, 2021 08:51

msujew reviewed Sep 20, 2021

View reviewed changes

msujew added vscode issues related to VSCode compatibility webviews issues related to webviews labels Sep 21, 2021

ChayaLau added 3 commits September 29, 2021 17:19

fixed commit after review

1815f20

Support Webview View

5f1565f

Added API to support Webview based views contributions Signed-off-by: ChayaLau <[email protected]>

Merge branch 'webview-view-support' of https://github.com/ChayaLau/theia

ea0baa7

into webview-view-support

vince-fugnitto requested review from RomanNikitenko and tsmaeder September 29, 2021 14:33

vince-fugnitto reviewed Oct 5, 2021

View reviewed changes

tsmaeder reviewed Oct 5, 2021

View reviewed changes

ChayaLau added 4 commits October 10, 2021 11:55

fixes

eb54f9c

Support Webview View

082c826

Added API to support Webview based views contributions Signed-off-by: ChayaLau <[email protected]>

Merge branch 'webview-view-support' of https://github.com/ChayaLau/theia

373593c

into webview-view-support

removed webview view creation to a function within registry

76d52e6

deleted webviewview service and moved functionality to the plugin vie…

14188fc

…w registry

msujew self-requested a review January 11, 2022 10:38

msujew reviewed Jan 11, 2022

View reviewed changes

msujew mentioned this pull request Feb 2, 2022

Support VSCode WebviewView API #10705

Merged

2 tasks

msujew closed this in #10705 Feb 6, 2022

msujew mentioned this pull request Feb 14, 2022

Fixed statusbar foreground color in debug mode #10430

Closed

1 task

msujew mentioned this pull request Mar 1, 2022

Implementation of registerWebviewViewProvider VSCode API #8740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Webview Views #10137

Support Webview Views #10137

ChayaLau commented Sep 19, 2021 •

edited

Loading

ChayaLau commented Sep 19, 2021

msujew left a comment

ChayaLau commented Sep 20, 2021

ChayaLau commented Sep 29, 2021 •

edited

Loading

tsmaeder left a comment

tsmaeder Oct 5, 2021

ChayaLau Oct 10, 2021

tsmaeder Oct 18, 2021

tsmaeder commented Oct 5, 2021

ChayaLau commented Oct 24, 2021 •

edited

Loading

vince-fugnitto commented Oct 25, 2021 •

edited

Loading

ChayaLau commented Oct 25, 2021

amiramw commented Nov 11, 2021

bodarajeshkumar commented Nov 16, 2021

bodarajeshkumar commented Nov 29, 2021

marcdumais-work commented Nov 29, 2021

bodarajeshkumar commented Nov 30, 2021

tsmaeder commented Dec 21, 2021 •

edited

Loading

msujew commented Dec 21, 2021

ChayaLau commented Dec 21, 2021

vince-fugnitto commented Jan 5, 2022

msujew left a comment

msujew Jan 11, 2022

msujew Jan 11, 2022

msujew Jan 11, 2022

msujew Jan 11, 2022

msujew Jan 11, 2022

msujew commented Jan 26, 2022

vince-fugnitto commented Jan 26, 2022

msujew commented Jan 26, 2022

Support Webview Views #10137

Support Webview Views #10137

Conversation

ChayaLau commented Sep 19, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

ChayaLau commented Sep 19, 2021

msujew left a comment

Choose a reason for hiding this comment

ChayaLau commented Sep 20, 2021

ChayaLau commented Sep 29, 2021 • edited Loading

tsmaeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Oct 5, 2021

ChayaLau commented Oct 24, 2021 • edited Loading

vince-fugnitto commented Oct 25, 2021 • edited Loading

ChayaLau commented Oct 25, 2021

amiramw commented Nov 11, 2021

bodarajeshkumar commented Nov 16, 2021

bodarajeshkumar commented Nov 29, 2021

marcdumais-work commented Nov 29, 2021

bodarajeshkumar commented Nov 30, 2021

tsmaeder commented Dec 21, 2021 • edited Loading

msujew commented Dec 21, 2021

ChayaLau commented Dec 21, 2021

vince-fugnitto commented Jan 5, 2022

msujew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msujew commented Jan 26, 2022

vince-fugnitto commented Jan 26, 2022

msujew commented Jan 26, 2022

ChayaLau commented Sep 19, 2021 •

edited

Loading

ChayaLau commented Sep 29, 2021 •

edited

Loading

ChayaLau commented Oct 24, 2021 •

edited

Loading

vince-fugnitto commented Oct 25, 2021 •

edited

Loading

tsmaeder commented Dec 21, 2021 •

edited

Loading