Skip to content

Implement badge API for webviews and treeviews#144775

Merged
alexr00 merged 4 commits intomicrosoft:mainfrom
matthewjamesadam:matthewjamesadam/badges-impl
Mar 30, 2022
Merged

Implement badge API for webviews and treeviews#144775
alexr00 merged 4 commits intomicrosoft:mainfrom
matthewjamesadam:matthewjamesadam/badges-impl

Conversation

@matthewjamesadam
Copy link
Contributor

This PR fixes #62783
This PR implements the API proposal here: #139225

I largely followed the existing patterns for how the TreeView and WebView processes state changes through the extension host into the main application. I do have a few questions of the details of what I did, which I've added as PR comments.

I have only tested this manually, by modifying the TreeView and WebviewView sample extensions to provide badges, and running them in my own local build. It works as expected. I am not sure what the expectations are for writing automated tests for something like this, please let me know where I should add tests to a particular test suite.

I did notice one thing while working on this, which I think is worth discussing: in the WebView View badge case, I am not sure how badges can be provided after extension activation. Webview Views specify a badge through WebviewView.badge, but you can only reference a WebViewView once it has been created (ie, once the user has opened up the Webview View, and WebviewViewProvider.resolveWebviewView has been called). I'm wondering if this is insufficient though: imagine a TODO extension, like https://github.com/Gruntfuggly/todo-tree (they are one extension that wants to use this API). If I used such an extension, I would expect that the badge would show up on the activation bar showing that I have TODOs as soon as I open up the workspace, even if I haven't yet opened up that extension's Webview View.

This is not a problem for TreeView badges, because an extension can call createTreeView to force the TreeView to be created when the extension is activated, and can provide badges at that point.

@matthewjamesadam matthewjamesadam marked this pull request as ready for review March 9, 2022 19:45
@isidorn isidorn assigned alexr00 and unassigned mjbvz Mar 10, 2022
@alexr00 alexr00 requested review from alexr00 and mjbvz March 10, 2022 09:27
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Webview parts of the change look good to me!

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor changes!

@matthewjamesadam
Copy link
Contributor Author

Thanks for the review @alexr00 and @mjbvz , I believe I've addressed all of the comments.

I am still wondering about the question I added in the PR description, about how badges can be provided before the webview view is created -- I believe this is a fairly important use case. Do either of you have any thoughts on this?

Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Everything's good from the tree view side. I'll wait for @mjbvz's input before merging.

@alexr00 alexr00 added this to the March 2022 milestone Mar 15, 2022
@alexr00 alexr00 modified the milestones: March 2022, April 2022 Mar 21, 2022
# Conflicts:
#	src/vs/workbench/api/browser/mainThreadTreeViews.ts
#	src/vs/workbench/api/common/extHostTreeViews.ts
#	src/vs/workbench/browser/parts/views/treeView.ts
@matthewjamesadam
Copy link
Contributor Author

I've cleaned up the conflicts this PR had with main. @alexr00 I believe @mjbvz has already approved this PR here: #144775 (review).

@mjbvz I'd appreciate any thoughts you have about the webview view initialization issue I outlined in the PR description, as I believe it might make the badge feature less useful for webview views.

@matthewjamesadam
Copy link
Contributor Author

@alexr00 @mjbvz we are excited to get this into nightly builds, and I believe I've addressed all the code review issues. Can we merge this in?

@alexr00 alexr00 merged commit a727329 into microsoft:main Mar 30, 2022
@alexr00
Copy link
Member

alexr00 commented Mar 30, 2022

Merged, thank you for the PR! @mjbvz we can continue to address the webview view questions during API review.

@matthewjamesadam
Copy link
Contributor Author

Thanks @alexr00 and @mjbvz -- I have opened a follow-on Issue here to discuss the webview view launching problem: #146330

@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2022
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.

badges on custom activities added to activity bar by extensions

5 participants