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

Allow to use codicon in menu-contributions #84695

Closed
jrieken opened this issue Nov 13, 2019 · 25 comments
Closed

Allow to use codicon in menu-contributions #84695

jrieken opened this issue Nov 13, 2019 · 25 comments
Assignees
Labels
api api-finalization feature-request Request for new features or functionality icons-product Issues for in-product icons menus Menu items and widget issues on-release-notes Issue/pull request mentioned in release notes on-testplan
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Nov 13, 2019

From a different discussion

@akaroml

By the way, now we make a copy of the svg file and reference it in the command contribution. Is there a way of referencing vscode icons in the same fashion of octicons? (as described here https://code.visualstudio.com/api/references/icons-in-labels)

idea: add new URI scheme like vscode-icon://codicon/flat-list with which this becomes possible.

idea: allow the $(name) syntax in more places, e.g in json "icon": "$(zap)" and in markdown Hello *World* $(zap).

@jrieken jrieken added feature-request Request for new features or functionality api menus Menu items and widget issues labels Nov 13, 2019
@jrieken jrieken self-assigned this Nov 13, 2019
@miguelsolorio miguelsolorio added the icons-product Issues for in-product icons label Nov 13, 2019
@miguelsolorio
Copy link
Contributor

Related: Support to allow re-using VSCode icons in user extensions #31466

@eamodio
Copy link
Contributor

eamodio commented Nov 13, 2019

Related: allow codicon use in tree view contributions.

@eamodio
Copy link
Contributor

eamodio commented Nov 27, 2019

I'm going to use this issue as the umbrella issue for the 2 other related ones: #85579 & #85624.

Here is backstory on my thinking and approach here for all 3 issues. First, I wanted to ensure a somewhat unified experience across menus, hovers, and views. I would have also liked it to be consistent with quickpicks and statusbar items, but because of the needs/requirements I didn't feel that was possible.

Quickpicks and statusbar items use $(zap) style in their text for token replacement. At first I thought the same would work well for hovers, but that would not only be a change in existing behavior and general markdown rendering, it is also unclear if something in the form of $(zap) was actually intended to be a token replacement. Also from the implementation side it was far more complex, since it required either customization to our markdown parsing or a pre/post processor -- either of which would likely come with a performance impact. Instead I opted to just use the standard markdown image syntax with a custom uri (in the form of vscode-icon://codicon/zap). This keeps the intent very clear and also has both a very minimal performance impact and code surface area.

Given the hover learnings/requirements, I opted to use the same uri style for both menus and views as well. Which has very similar benefits to the hover case, explicit intent, minimal perf imact, and tiny code surface area.

At the same time, I did think about having something like ThemeIcon for custom views (and it still might be worth it). Although if we were to use something like ThemeIcon for custom view, I would want it to use the same url format under the covers if possible. Especially because the structure of the url format leave the door open for us to provide alternative icon sets (or even extension registered icons -- think vscode-icon://foo.extension/icon).

Also for menus, I also thought about just having $(zap) in the icon which would work, but it doesn't have the extensibility of the url format. Although that could be solved with some additional but optional structure inside the $(). I also thought about having icon take another type of object similar to ThemeIcon, but since icon is already a string or object this felt like it would be even harder for users to use. But again, I would still want to transform that into vscode-icon://codicon/zap internally to keep it consistent.

/cc @aeschli

@aeschli
Copy link
Contributor

aeschli commented Nov 27, 2019

@eamodio Thanks for the great writeup.
My secret plan was always to make icons themable too. The codicons are a great first step towards it, as it gives icons names and all references just use these names. A icon theme then can just define which icon is used for each name.
Your approach with the URIs also goes well with that. The only thing I'd suggest is to not use codicon in the URI's authority. Would vscode://icons/zap also work? We already use the vscode:// for things like generated schemas, e.g.

"url": "vscode://schemas/settings/user"

@eamodio
Copy link
Contributor

eamodio commented Nov 27, 2019

@aeschli 👍 I don't have really have a strong preference for either url formats: vscode://icons/zap works for me -- and I do like that icons is generic and could change with an "[icon] theme". And if eventually we supported extension contributed icons -- would it look like vscode://icons.foo.extension/bar? Or something like that?

@jrieken Do you have a preference?

@jrieken
Copy link
Member Author

jrieken commented Nov 28, 2019

Talked over this with @aeschli this morning and we came up with the slightly different proposal which builds on the existing $(xyz)-syntax and the ThemeIcon-type.

  • vscode.d.ts - there is a preference for ThemeIcon because it's already there (and actually already supported for tree items and a few other places) and because it makes clear where icons are possible, e.g. a showTextDocument(vscode.Uri.parse('vscode://icons/zap')) doesn't make sense but doesn't give you a compile error.
  • MarkdownString - we like the simplicity of$(xyz) because it's a known concept. To avoid unintended icon rendering we can add a flag to disable (or enable) this, e.g MarkdownString#isIconAware. Also, the $(xyz) syntax already supports modifiers, at least one which is ~spin, e.g SCM uses $(sync~spin) for the sync icon.
  • package.json - would be just like markdown and would use the $(xyz) syntax, e.g. "icon": "$(zap)"

Internally, we should only have one type and that should be uri, e.g an ThemeIcon will be translated into vscode://icons/<id>. The IconLabel which already support rendering of the two theme icons file and folder must be enriched to support rendering of all theme icons.

@eamodio
Copy link
Contributor

eamodio commented Nov 29, 2019

I'm in agreement with all of that, except for the MarkdownString. I agree that simplicity of $(icon) is nice, but imo it isn't enough of a signal that the content should be replaced. For example, in GitLens, I would want it to replace $(icon) in my content, but not in the content that comes from the commit messages. So unless there is a way of escaping $(icon), I would have no way to make sure my content area get transformed, while others don't. I much prefer using the standard markdown syntax for images, because it is not only built-in but is a clear signal that an image is desired.

Could we use ![]($(icon))? So it uses the standard markdown image format, clearly should be transformed, doesn't require a flag to turn it on or off (so you don't have to think extra about it -- or deal with extra custom escaping), and uses the $(icon) style as well.

@akaroml
Copy link
Member

akaroml commented Nov 29, 2019

+1 on using $() syntax to reference icons. The simplicity of it is so appealing. But of course, we need ways to escape for exceptions.

@jrieken
Copy link
Member Author

jrieken commented Nov 29, 2019

So unless there is a way of escaping $(icon), I would...

We should be supporting backslash to escape codicons - that's something we needed for messages already. So \$(zap) is just printed as $(zap)

@jrieken
Copy link
Member Author

jrieken commented Nov 29, 2019

Screenshot 2019-11-29 at 11 08 51

This is how it looks with the joh/themeicons branch 👯‍♂

@jrieken
Copy link
Member Author

jrieken commented Nov 29, 2019

Screenshot 2019-11-29 at 14 49 20

Merged my PR which also adds ThemeIcon support 🔝 for custom trees. There is currently no real API to drive this. Also, codicons for menu items are atm for proposed API users only.

@akaroml
Copy link
Member

akaroml commented Dec 3, 2019

Can't wait to adopt this in stable build.

@jrieken jrieken modified the milestones: November 2019, December 2019 Dec 3, 2019
@eamodio eamodio reopened this Dec 11, 2019
eamodio pushed a commit that referenced this issue Dec 16, 2019
`appendMarkdown` is now the only way to use ThemeIcons
jrieken added a commit that referenced this issue Dec 17, 2019
@jrieken
Copy link
Member Author

jrieken commented Jan 7, 2020

@chrmarti I have added you because vscode.QuickInputButton#iconPath. It looks like vscode.ThemeIcon isn't really supported here, which gives me trouble adding support for arbitrary theme icons. E.g in my sample below, only the back button works

quickpick.buttons = [
	vscode.QuickInputButtons.Back,
	{ iconPath: vscode.ThemeIcon.File },
	{ iconPath: vscode.ThemeIcon.Folder },
	{ iconPath: new vscode.ThemeIcon('zap') }
];

@chrmarti
Copy link
Contributor

chrmarti commented Jan 7, 2020

@jrieken An oversight, tracking as #72489.

@jrieken
Copy link
Member Author

jrieken commented Jan 7, 2020

Check. Closing this issue. The remainders are then #72489 and #87165

@akaroml
Copy link
Member

akaroml commented Jan 9, 2020

@jdneo @Eskibear

Theme icons can now be referenced using $() syntax.

@jdneo
Copy link
Member

jdneo commented Jan 9, 2020

Hey @jrieken,

Will codicon also support ~spin just like what VS Code has with the octicon ?

an example of usage is to make the loading icon to spin.

@jrieken
Copy link
Member Author

jrieken commented Jan 9, 2020

Yeah - it also supports the spin, e.g $(zap~spin). Tho, please don't use it in trees ;-)

@jdneo
Copy link
Member

jdneo commented Jan 10, 2020

That's exactly what I want 😆 ---- support spinning in the treeview.

In some cases, like Test Explorer, I want to show the status of running, it would be great if I can use $(loading~spin), otherwise I have to use the <style/> in the svg.

@jrieken
Copy link
Member Author

jrieken commented Jan 10, 2020

Hm... You should get a loading icon in tree by default, e.g when the promise that is returned from getChildren is taking longer than Xms (500?) then an inline progress is being shown

@jdneo
Copy link
Member

jdneo commented Jan 10, 2020

Yes, but I think that does not apply to the leaf node, right? 😄

@jrieken
Copy link
Member Author

jrieken commented Jan 10, 2020

Adding @alexr00 but I think it applies for all nodes, the twisty should turn into a spinning circle when fetching children is taking its time

@alexr00
Copy link
Member

alexr00 commented Jan 10, 2020

Since leaf nodes have no children, we don't have a way built into the tree to show a progress spinner on them.
One option to get a progress spinner for free: give that node children then update them.

@jrieken
Copy link
Member Author

jrieken commented Jan 10, 2020

@jdneo well, maybe in your case in does make sense to use spinning codicons in the tree then...

@jdneo
Copy link
Member

jdneo commented Jan 12, 2020

@jrieken It would be great if spinning codicons can be used in the tree. Cannot wait for it. 👍

jrieken added a commit that referenced this issue Jan 27, 2020
@jrieken jrieken added the on-release-notes Issue/pull request mentioned in release notes label Jan 30, 2020
@vscodebot vscodebot bot locked and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization feature-request Request for new features or functionality icons-product Issues for in-product icons menus Menu items and widget issues on-release-notes Issue/pull request mentioned in release notes on-testplan
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants