Skip to content

Add badges extension API #139225

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

Merged

Conversation

matthewjamesadam
Copy link
Contributor

@matthewjamesadam matthewjamesadam commented Dec 15, 2021

This is a WIP for getting feedback for #62783

Alternative API can be seen here: #139229

@ghost
Copy link

ghost commented Dec 15, 2021

CLA assistant check
All CLA requirements met.

@alexr00
Copy link
Member

alexr00 commented Jan 18, 2022

@matthewjamesadam thanks for the proposal PR! Of your two proposals, this one is closer to what we want. A few notes from our discussion on it:

  • Instead of having the way to set the badge be on the TreeDataProvider, it should be on the TreeView, as this is view related. I think the equivalent place for webviews would be on WebviewView, but @mjbvz would need to confirm that.
  • Instead of an event for setting the badge, we could have just have a property badge on TreeView and WebviewView.
  • The type for badge would be number. At this time, we're not ready to support a way for views to have arbitrary characters in the badge.

@matthewjamesadam
Copy link
Contributor Author

Hi @alexr00 and @isidorn ,

Thanks for taking the time to discuss this! I have updated this PR to reflect your feedback:

  • Badges are now provided on TreeView and WebviewView.
  • Only numeric badges are allowed.

There are a couple of issues I'd appreciate further feedback on:

  1. I've specified the label as a simple string property here. The implementation in IBadge uses a callback function to provide this (IBadge.descriptorFn), I believe to delay resolving the description until a tooltip is displayed. Using a callback seemed like it might be unnecessarily complicated in the extension API, but please let me know if you think that's the right approach.

  2. I made the badge property an abstract base type (Badge), of which there is only one concrete implementation (NumberBadge). I thought this would allow VSCode to provide other badge types in the future (progress, text, etc). I am happy to replace this with a simple number if you would prefer, though this would prevent displaying a badge tooltip. Another alternative would be to allow providing one of a set of badge interfaces, similar to how for instance TerminalOptions.location allows one of a set of interfaces. The downside to this approach seemed to be on the implementation end, in that VSCode would have to determine which interface was passed in at runtime by checking property existence.

Thanks again!

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.

We had an opportunity to discuss the new proposal. A few more changes, then we can discuss implementing it.

# Conflicts:
#	src/vs/workbench/services/extensions/common/extensionsApiProposals.ts
@matthewjamesadam
Copy link
Contributor Author

Thanks for your feedback @alexr00 , I have updated the PR to reflect it.

@lgrammel
Copy link

@matthewjamesadam thanks for working on this! I'm an extension developer and have a question regarding the API. From what I understand, this PR seems to add badges to TreeView and WebviewView. However, on the activity viewContainers are being displayed, and they can contain multiple views. Since the badges are showing up on the activity bar, I'm wondering how the badges from the individual views are mapped to that? Is it an aggregation of the badge numbers of all the views in a view container?

@matthewjamesadam
Copy link
Contributor Author

Hi @lgrammel -- yes I believe the badge would be an aggregate of the values for all the container's views. There was an alternative proposal for a badge API here: #139229 that allowed providing a single badge for the entire viewContainer, but it is a more complicated API (it requires a separate Provider interface, since viewContainers don't have any interface representation in the API). @alexr00 might have more thoughts on this though!

@alexr00
Copy link
Member

alexr00 commented Jan 28, 2022

I think we'd do one of two things:

  • One of the view's would "win" showing the badge. How a view wins would be TBD, but likely done by extension install order since I think we do that else where.
  • There would be a "there're multiple badges" badge. Maybe a badge with ellipses would work for that.

I don't think we'll ever show multiple badges on one container though as this will get very cluttered very quickly.

@lgrammel
Copy link

Thanks - both of those options would work well for my use case. Really excited about this feature, looking forward to having it part of VS Code!

@matthewjamesadam
Copy link
Contributor Author

@alexr00 that makes sense! Do you have any other feedback for this proposal? Is there anything else I can do to help push this forward? I'd be happy to prepare a draft PR implementing this as well, with the understanding that this proposal might change in the future.

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.

Just adding a few minor comments on the api proposal

/**
* A label to present in tooltips for the badge
*/
label: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These properties should be readonly as I don't think we support updating the badge if you do treeView.badge.value = 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

/**
* A label to present in tooltips for the badge
*/
label: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we use tooltip (instead of label) in other places in vscode.d.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@alexr00
Copy link
Member

alexr00 commented Jan 31, 2022

@matthewjamesadam We'll try to discuss during our weekly API meeting this week (there was no meeting last week because of our endgame). After that, we can look into a PR.

@matthewjamesadam
Copy link
Contributor Author

@mjbvz thanks for your PR feedback, I addressed both items.

@alexr00 thanks for the update!

@matthewjamesadam matthewjamesadam changed the title WIP: Add badges extension API Add badges extension API Jan 31, 2022
@matthewjamesadam matthewjamesadam marked this pull request as ready for review January 31, 2022 17:54
@matthewjamesadam
Copy link
Contributor Author

Hi @alexr00, do you have any updates on this? Did you manage to discuss this proposal at the API meeting? Thanks!

@alexr00
Copy link
Member

alexr00 commented Feb 16, 2022

I haven't had a chance to bring this to our API discussion. I will post an update as soon as I have one!

@alexr00 alexr00 added this to the March 2022 milestone Feb 16, 2022
@alexr00 alexr00 added api api-proposal feature-request Request for new features or functionality labels Mar 1, 2022
/**
* A badge presenting a value for a view
*/
export interface Badge {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call them ViewBadge?

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.

@matthewjamesadam we had a chance to discuss last week. This proposal is heading in the right direction! If you want to make a PR that implements this feature/API I will be happy to review it.

@alexr00 alexr00 merged commit 9be0d5e into microsoft:main Mar 8, 2022
@matthewjamesadam matthewjamesadam deleted the matthewjamesadam/badges-api-1 branch March 8, 2022 17:02
@matthewjamesadam
Copy link
Contributor Author

Thanks @alexr00 ! I have added a PR to implement this API here: #144775

@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-proposal feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants