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 ThemeIcon's color property of the VS Code API #11243

Merged

Conversation

lucas-koehler
Copy link
Contributor

What it does

Fixes #11128

  • Add optional color property to ThemeIcon of the VS Code API
  • Hand over the ThemeIcon instead of just its id from plugin Ext to Main
  • Apply the color in the PluginTree
  • Adapt TreeViewNode.is to also recognize sub types such as CompositeTreeViewNode

Contributed on behalf of STMicroelectronics

Signed-off-by: Lucas Koehler lkoehler@eclipsesource.com

How to test

  • Download the test extension. Source for reference.

  • Launch and open the `TEST VIEW' in the Explorer:
    image

  • Expand the tree nodes. See that the tree nodes use file icons in the editor's warning color (usually some kind of yellow or orange)

  • See that file icons in other views (e.g. the File Explorer) still have their original color

  • Switch themes to see that the icons in the Test View use the warning color of the selected theme (select high contrast theme to see a noticeable difference)

Review checklist

Reminder for reviewers

@JonasHelming JonasHelming added the vscode issues related to VSCode compatibility label Jun 2, 2022
* Add optional color property to ThemeIcon
* Hand over the ThemeIcon instead of just its id from ext to main
* Apply the color in the `PluginTree`
* Adapt `TreeViewNode.is` to also recognize sub types such as `CompositeTreeViewNode`

Fixes eclipse-theia#11128

Contributed on behalf of STMicroelectronics

Signed-off-by: Lucas Koehler <lkoehler@eclipsesource.com>
@lucas-koehler
Copy link
Contributor Author

lucas-koehler commented Jun 2, 2022

@vince-fugnitto Thank you very much for the fast review. I added the documentation and explicitly added the id and color properties to ThemeIcon. With this, the class is now defined the same as in the VS Code API.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirm that the color is working well with the sample extension.

I made a comment about actually properly supporting all options for icons however that we can possibly tackle later if you don't want to update.

Comment on lines +46 to +47
if (node.themeIcon) {
if (node.themeIcon.id === 'file' || node.themeIcon.id === 'folder') {
Copy link
Member

Choose a reason for hiding this comment

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

I just wanted to highlight that this only allows us a subset of icons to be used which is different than the API actually allows. VS Code allows all icons listed here (https://code.visualstudio.com/api/references/icons-in-labels#icon-listing) which is documented in id:

/**
 * The id of the icon. The available icons are listed in https://code.visualstudio.com/api/references/icons-in-labels#icon-listing.
 */
readonly id: string;

With some modifications to the extension provided in VS Code I can see (while in Theia it does not work):

image

Code;

function getTreeItem(key: string): vscode.TreeItem {
	const treeElement = getTreeElement(key);
	// An example of how to use codicons in a MarkdownString in a tree item tooltip.
	const tooltip = new vscode.MarkdownString(`$(zap) Tooltip for ${key}`, true);
	const iconPath = new vscode.ThemeIcon('accounts-view-bar-icon', new vscode.ThemeColor('editorWarning.foreground'));
	return {
		label: /**vscode.TreeItemLabel**/<any>{ label: key, highlights: key.length > 1 ? [[key.length - 2, key.length - 1]] : void 0 },
		tooltip,
		iconPath,
		collapsibleState: treeElement && Object.keys(treeElement).length ? vscode.TreeItemCollapsibleState.Collapsed : vscode.TreeItemCollapsibleState.None
	};
}

@lucas-koehler
Copy link
Contributor Author

lucas-koehler commented Jun 2, 2022

@vince-fugnitto

I made a comment about actually properly supporting all options for icons however that we can possibly tackle later if you don't want to update.

I would prefer to have that as a separate issue because it does not seem trivial to support all these icons. For instance, I investigated a bit with the example you provided. The accounts-view-bar-icon theme icon is contributed in VSCode as part of its ActivitybarPart. It basically maps this identifier to the account codicon. I guess this is not even known to Theia (and possibly many other icons made available in such a way in the VS Code workbench).

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me, I confirmed that the color is properly applied 👍
I did not notice any regressions.

@JonasHelming JonasHelming merged commit 0398116 into eclipse-theia:master Jun 9, 2022
@JonasHelming JonasHelming added this to the 1.27.0 milestone Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vscode] Support optional property ThemeIcon#color
3 participants