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

Add proposed webview view API #104601

Merged
merged 12 commits into from
Aug 20, 2020
Merged

Add proposed webview view API #104601

merged 12 commits into from
Aug 20, 2020

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Aug 13, 2020

For #46585

This adds a new WebviewView proposed api to VS Code that lets webview be used inside views. Webview views can be contributed using a contribution point such as :

    "views": {
      "explorer": [
        {
          "type": "webview",
          "id": "cats.cat",
          "name": "Cats",
          "visibility": "visible"
        }
      ]
    },

@mjbvz mjbvz requested review from jrieken and sandy081 August 13, 2020 22:53
@mjbvz mjbvz self-assigned this Aug 13, 2020
extensions/image-preview/package.json Outdated Show resolved Hide resolved
@@ -226,19 +236,76 @@ const viewsExtensionPoint: IExtensionPoint<ViewExtensionPointType> = ExtensionsR
jsonSchema: viewsContribution
});


export interface IViewResolver {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sandy081 Here's an explanation of the changes I made in the views file:

  1. View contributions now have an optional type. This can either be tree or webview.

    It would be great if we could determine this automatically for extension authors but I wasn't able to find a clean way to do so.

  2. The viewsExtensionPoint can now generate tree views and webview views.

    It selects which view to use based on the type

  3. In order to avoid a cycle, I added the new IViewResolverRegistry.

    This is a registry that maps view types to resolvers. A resolver takes a IViewDescriptor and fills in the missing details, such as the specific type of view to use.

  4. I updated viewExtensionPoint to register the resolver for tree views.

    This resolver should do the exact same thing viewExtensionPoint previously had hardcoded

  5. The webview then registers a new resolver for webview based views.

You know this code much better than I do so let me know if you have other suggestions on how this could be accomplished. The main thing is that the viewExtensionPoint cannot import any webview code since webviews are at a higher level in the codebase

Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could determine this automatically for extension authors but I wasn't able to find a clean way to do so.

It is cool but I think it adds confusion and complexity. It is clear if extension declares a tree/web view and creates the same using API.

Note: We need to validate if extension is creating the correct type of view for what it declared.

In order to avoid a cycle, I added the new IViewResolverRegistry.
This is a registry that maps view types to resolvers. A resolver takes a IViewDescriptor and fills in the missing details, such as the specific type of view to use.

Not sure if I understood the need for this resolver and why there is a cycle? For web view type, you have to pass the WebviewViewPane to ctorDescriptor , may I know where we have cycle here?

CCing @alexr00 as she owns custom tree views

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cleaned up the implementation so that this is no longer required. The changes in the view code should be much more minimal now

For microsoft#46585

This adds a new `WebviewView` proposed api to VS Code that lets webview be used inside views. Webview views can be contributed using a contribution point such as :

```json
    "views": {
      "explorer": [
        {
          "type": "webview",
          "id": "cats.cat",
          "name": "Cats",
          "visibility": "visible"
        }
      ]
    },
```
@@ -226,19 +236,76 @@ const viewsExtensionPoint: IExtensionPoint<ViewExtensionPointType> = ExtensionsR
jsonSchema: viewsContribution
});


export interface IViewResolver {
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could determine this automatically for extension authors but I wasn't able to find a clean way to do so.

It is cool but I think it adds confusion and complexity. It is clear if extension declares a tree/web view and creates the same using API.

Note: We need to validate if extension is creating the correct type of view for what it declared.

In order to avoid a cycle, I added the new IViewResolverRegistry.
This is a registry that maps view types to resolvers. A resolver takes a IViewDescriptor and fills in the missing details, such as the specific type of view to use.

Not sure if I understood the need for this resolver and why there is a cycle? For web view type, you have to pass the WebviewViewPane to ctorDescriptor , may I know where we have cycle here?

CCing @alexr00 as she owns custom tree views

By moving the webviews view into their own fodler, I was able to avoid the cycle the resolver was originally introduced for
@mjbvz mjbvz marked this pull request as ready for review August 19, 2020 22:35
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 19, 2020

This change should now be content complete and ready for review @alexr00

I'll remove cat example view from the image preview before check in

@alexr00
Copy link
Member

alexr00 commented Aug 20, 2020

Custom tree view related/adjacent changes look good.

@jrieken jrieken self-requested a review August 20, 2020 09:25
</body>
</html>`;

}
Copy link
Member

Choose a reason for hiding this comment

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

I assume this won't be merged ;-)

*
* @return Optional promise indicating that the view has been fully resolved.
*/
resolveWebviewView(webviewView: WebviewView, context: WebviewViewResolveContext, token: CancellationToken): Promise<void> | void;
Copy link
Member

Choose a reason for hiding this comment

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

Return Thenable instead

@sandy081
Copy link
Member

Not sure if I missed this validation - checking if extension is registering tree data provider to the tree view type and the same for web view?

@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 20, 2020

Made the suggested changes.

@sandy081 Registering a tree data provider for a webview based view will be treated the same as if you never registered the provider at all. I think this is reasonable behavior but if we see that extension authors are confused by which type of view to use, we could look into adding a better error message

@mjbvz mjbvz merged commit 61f799f into microsoft:master Aug 20, 2020
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 20, 2020

Merging the initial work. Will continue to iterate on the implementation and polishing the API

@usernamehw
Copy link
Contributor

Is this api going to be available in Stable 1.49.0 with --enable-proposed-api flag enabled?

@mjbvz
Copy link
Collaborator Author

mjbvz commented Sep 1, 2020

Yes it should already be available in the latest insiders. See https://github.com/microsoft/vscode-extension-samples/tree/master/webview-view-sample for an example extension

@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants