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

Support extending icon themes #110830

Closed
wants to merge 3 commits into from

Conversation

SPGoding
Copy link
Contributor

@SPGoding SPGoding commented Nov 17, 2020

This PR resolves #14662.

It enables extension authors to extend existing icon themes. I chose not to go with the "default icon" idea proposed by sentry07, as (from my point of view) it's more complicated to implement and only provides a subset of the features which this PR adds.

This is my first time contributing real changes to VS Code, so I apologize in advance for any hacky implementation/negligence ;)

Changes

A new optional property named extends is added under contributes.iconThemes[].
When extends is present, the value of label is ignored as extenders won't be shown in icon theme list at all.

{
	"contributes": {
		"iconThemes": [
			{
				"id": "general-extender",
				"path": "./general-extender-icon-theme.json",
				"extends": {
					"all": true
				}
			},
			{
				"id": "vs-seti-extender",
				"path": "./vs-seti-extender-icon-theme.json",
				"extends": {
					"ids": [
						"vs-seti"
					]
				}
			},
			{
				"id": "i-dont-know-why-but-you-can-do-this-extender",
				"path": "./i-dont-know-why-but-you-can-do-this-icon-theme.json",
				"extends": {
					"all": true,
					"ids": [
						"vs-seti"
					]
				}
			}
		]
	}
}
  • extends
    • all: Whether to extend all icon themes generally or not. Defaults to false.
    • ids: Ids of file icon themes that should be extended specifically.

Extending Generally

When the extender is extending another icon theme because and only because "all" is set to true, the following properties in the extender will be ignored:

  • file
  • folder
  • folderExpanded
  • rootFolder
  • rootFolderExpanded

The following properties in the extender will be merged into the icon theme if and only if the icon theme already contains specific folder icons:

  • folderNames
  • folderNamesExpanded

The following properties in the extender will be merged into the icon theme if and only if the icon theme already contains specific file icons:

  • languageIds
  • fileExtensions
  • fileNames

Things under light and highContrast are merged using the same strategy.

Properties in iconDefinitions and fonts are always merged.

Note: the extender cannot override existing associations/definitions in other icon themes when extending generally.

Extending Specifically

When the extender is extending other icon themes because the IDs of those themes are specifically listed in ids, there are no limits like the ones shown in Extends Generally.
The extender is free to override existing associations and definitions in the listed themes.

Conflicts

When multiple extenders are extending the same icon theme with the same association, the one extending specifically wins.
If multiple extenders are extending specifically, or if all of the extenders are extending generally, there's no guarantee on who will win.
Therefore, it's not encouraged to extend existing icon themes to add icons for popular languages.

To Test

I couldn't find existing tests for the icon theme feature, and also couldn't figure out how to add tests for the extends feature... I have a stupid manual test environment set-up at here.

Thanks

Thank everyone from #14662 who provided really helpful information and/or thoughts!

@aaronfranke
Copy link

Does this mean that "all": true has the behavior described in this comment? #14662 (comment)

@SPGoding
Copy link
Contributor Author

Does this mean that "all": true has the behavior described in this comment? #14662 (comment)

Yes, with the limitations mentioned by aeschli at here implemented.

@hugonteifeh
Copy link

Any news on this?

@JiriTrecak
Copy link

Any news on this? This would make adding new language-specific icons a breeze when your extension is providing the language implementation itself before the language becomes more mainstream, for example. Good job on this PR @SPGoding , really well done

@aeschli
Copy link
Contributor

aeschli commented Jan 25, 2021

Thanks @SPGoding for taking the time. The implementation it is simple and pragmatic, that's great. But for my taste the feature is too generic.

I'd prefer a more targeted solution along the lines of a icon contribution and a default icon association, similar as seen here: #14662 (comment)
I have planned such a icon contribution point for #92791. That would allow extensions to contribute new new named icons (ThemeIcon) along with icon definitions. That would not only be for file icons but also product icons.
Having that, then we can allow language contributions to also specify a default icon id.

There's already the the icon registry (https://github.com/microsoft/vscode/blob/master/src/vs/platform/theme/common/iconRegistry.ts) and what's missing is a the code that wires it up an extension point, similar to https://github.com/microsoft/vscode/blob/master/src/vs/workbench/services/themes/common/colorExtensionPoint.ts

@SPGoding
Copy link
Contributor Author

Would you like to see a PR for that?

@aeschli
Copy link
Contributor

aeschli commented Jan 26, 2021

#114942 is issue for the registry.
Thanks for offering help. I think I can work on the next week, that will be easier.

@SPGoding SPGoding closed this Jan 26, 2021
@SPGoding SPGoding deleted the feature/default-icons branch January 26, 2021 17:28
@Hezkore Hezkore mentioned this pull request Feb 27, 2021
14 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Mar 12, 2021
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.

[file icons] Adding default fileicon support to language contributions
5 participants