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 contribute to icon registry from icon contribution point #114942

Closed
aeschli opened this issue Jan 25, 2021 · 10 comments
Closed

Allow to contribute to icon registry from icon contribution point #114942

aeschli opened this issue Jan 25, 2021 · 10 comments
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan
Milestone

Comments

@aeschli
Copy link
Contributor

aeschli commented Jan 25, 2021

Extracted from #92791.

Allow extensions to define a new icon by id, along with a default icon.
That icon id can then be used by the extension (or other extensions that depend on the extension) at the places where ThemeIcon can be used.

@aeschli aeschli self-assigned this Jan 25, 2021
@aeschli aeschli added the feature-request Request for new features or functionality label Jan 25, 2021
@aeschli aeschli added this to the Backlog milestone Jan 25, 2021
@aeschli aeschli modified the milestones: Backlog, February 2021 Jan 26, 2021
@aeschli aeschli changed the title allow to contribute to icon registry from icon contribution point Allow to contribute to icon registry from icon contribution point Feb 4, 2021
@aeschli aeschli closed this as completed in 684f61b Feb 4, 2021
@aeschli
Copy link
Contributor Author

aeschli commented Feb 4, 2021

I pushed new contribution points as proposed API:

"contributes": {
	"icons": [
		{
			"id": "distro-ubuntu",
			"description": "Ubuntu icon",
			"default": {
				"fontId": "distro-icon-font",
				"fontCharacter": "\\E001"
			}
		},
		{
			"id": "distro-fedora",
			"description": "Fedora icon",
			"default": {
				"fontId": "distro-icon-font",
				"fontCharacter": "\\E002"
			}
		}
	],
	"iconFonts": [
		{
			"id": "distro-icon-font",
			"src": [
				{
					"path": "./distroicons.woff",
					"format": "woff"
				}
			]
		}
	]
}

In this example, an extension defines two new icon ids distro-ubuntu and distro-fedora along with default icon definitions. The icons are defined in an icon font at the given font character. The icon font is defined in a new contribution point iconFonts.

  • The icon ids distro-ubuntu and distro-fedora can be used in markdown strings ("$(distro-ubuntu)" Ubuntu 20.04") and at all places that take a ThemeIcon ( new ThemeIcon("distro-ubuntu"))
  • The icon id can also be used by other extensions (if they know about the icon id)
  • Product icon themes can redefine the icon (if they know about the icon id)

Current limitations:

  • There can be name clashes for icon ids. Right now we leave it up to extensions to make sure the icon ids are unique. We could enforce the extension id as qualifier, but that would lead to very long icon names
  • Icons must come from icon fonts. Simpler for us, but admittedly a bit of extra work for extension. See the product-icon-theme-sample for build script to create a icon font.
  • The icon font id is local to the extension. It can't be referenced from other extensions. Motivation for that was to avoid name clashes

Feedback is welcome!

@eamodio
Copy link
Contributor

eamodio commented Feb 4, 2021

@aeschli This is AWESOME!

I'm a bit concerned about the uniqueness, and having extension collisions with our names as they evolve. What happens if an id is a defined codicon and contributed by an extension? What happens if 2 extensions contribute the same id?

The icon font id is local to the extension. It can't be referenced from other extensions. Motivation for that was to avoid name clashes

What is the implication of this? Other extensions can't define new icon names from that font?

@aeschli
Copy link
Contributor Author

aeschli commented Feb 4, 2021

@eamodio Yes, right now it's one big icon namespace: The built-in icon ids as well as the contributed icon ids. Agree that some qualifier would be good. Either enforced or by convention.
We have the same for ThemeIcons where id uniqueness is based on consensus.

I wasn't sure if reusing an icon font across extensions is needed/useful/a good idea.
I'd expect a contributed icon font to just contain the icons the extension needs and that the extension that contributes the font also defines icon ids for all the icons contained in the font. The icons then can be reused across extensions.
Keeping the icon font local to the extension has the advantage that the extension can modify the font without having to worry about breaking another user.

But let me know if you have a good use case for sharing of fonts.

@eamodio
Copy link
Contributor

eamodio commented Feb 4, 2021

Forcing the extension id is ugly as you said, but maybe there should be a required prefix on each icon? And the prefix would just be added to the id, e.g. ${prefix}:${id}. While not guaranteed to be unique it allows for some name-spacing and avoids conflicts with our codicons.

But let me know if you have a good use case for sharing of fonts.

Nope, just making sure I understood properly. Makes sense to me.

@gjsjohnmurray
Copy link
Contributor

gjsjohnmurray commented Feb 4, 2021

I think it would be OK (actually, advisable) to prefix the icon id with the extension id when adding it to the registry.

For instance if extension ms-vscode.vscode-github-issue-notebooks contributes icon foobar it would be referenced in Markdown as "$(ms-vscode.vscode-github-issue-notebooks.foobar)"

If publishers want to minimize the length of their icon ids they could publish an extension that just contributes the icon(s) and icon font(s), giving that extension a short name, e.g. ms-vscode.icons. Their other extensions (e.g. ms-vscode.vscode-github-issue-notebooks) could declare a dependency on the icons extension, then reference the icons more concisely, e.g. "$(ms-vscode.icons.foobar)"

@jrieken
Copy link
Member

jrieken commented Feb 5, 2021

For instance if extension ms-vscode.vscode-github-issue-notebooks contributes icon foobar it would be referenced in Markdown as "$(ms-vscode.vscode-github-issue-notebooks.foobar)"

Just wanna say that we have the same challenge with command ids. With them it's up to extensions and that hasn't turned out to be big problem.

@eamodio
Copy link
Contributor

eamodio commented Feb 8, 2021

Yeah, but at least with commands our getting started/scaffolding contains a command that is namespaced, so it's easier to fall into the desired pattern. But at the same time, I agree it isn't much of an issue -- we should just recommend a prefix in the docs.

@aeschli
Copy link
Contributor Author

aeschli commented Feb 9, 2021

I will change to allow to have dots in icon names and I will show an error (or warning?) if the icon contribution id doesn't contain a dot.

@aeschli
Copy link
Contributor Author

aeschli commented Feb 23, 2021

I added a check that the icon name consists of at least two segments separated by a minus
(e.g. distro-ubuntu or wsl-distro-ubuntu)

@aeschli
Copy link
Contributor Author

aeschli commented Feb 24, 2021

As discussed in #117492, I renamed iconFontId to fontId and character to fontCharacter for consistency with similar things in product and file icon themes.

I updated the sample in #114942 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders on-testplan
Projects
None yet
Development

No branches or pull requests

5 participants
@eamodio @jrieken @aeschli @gjsjohnmurray and others