Terminal decoration icon configuration doesn't go well with icon theming

[aeschli]

The settings to select an codicon for a decoration are not playing well with our product icon theming approach.

We want that a product icon theme can style all icons of our UI with a new icons. Each icon that we show should be defined with a component specific name
E.g. terminal-decoration-icon, terminal-decoration-error-icon, ...
That way the theme can come up with a custom icon for each icon location

The base icon names (circle-outline, primitive-dot, ...) should not be used directly, only as defaults, otherwise it gets complicated for a theme to change an icon as it affects many places, unknown to the theme. What might look good with our codicon based product icon theme, might look wrong with a theme that uses a different visual for primitive-dot

I suggest to remove the settings as we don't do that at other places. Instead we should refer to product icon themes as the way to change icons.

[Tyriar]

Requested the aliases in microsoft/vscode-codicons#126

[Tyriar]

We do allow switching icons for terminals in this way which I think is fine as they're very temporal:

I see what you're saying here though, let's remove the setting and leave it up to the icon theme 👍. We have this setting now so it solves that part of what these icon settings allowed:

"terminal.integrated.shellIntegration.decorationsEnabled": "never" | "gutter" | "overviewRuler" | "both"

[aeschli]

I was on vacation so I couldn't jump in here. It was not necessary to request new codicons or codicon aliases unless a new icon shape was needed, e.g. for a concept we didn't have yet.

There still seems to be a confusion of Codicons vs product icon icons.

The Codicon library is a set of default icons that are part of the Codicon font. We use these as default icons in VS Code.

See the spec of Codicon:

/**
 * The Codicon library is a set of default icons that are built-in in VS Code.
 *
 * In the product (outside of base) Codicons should only be used as defaults. In order to have all icons in VS Code
 * themeable, component should define new, UI component specific icons using `iconRegistry.registerIcon`.
 * In that call a Codicon can be named as default.
 */

codicon aliases should be component agnostic (no 'terminal', 'debug').

The componnet specific ids go in the product icon registry. These are the ids that wil be used in a product icon theme when styling an icon.

Product icon ids are component specific and are defined in the component itself, e.g. as done here:
https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/terminal/browser/terminalIcons.ts#L6

export const newTerminalIcon = registerIcon('terminal-new', Codicon.add, localize('newTerminalIcon', 'Icon for creating a new terminal instance.'));

This defines a new product icon terminal-new, and uses the codicon 'add' icon as the default icon.

When using these icons in code, terminalicons.newTerminalIcon must be used (or in labels by the id terminal-new).
We never want to reference Codicon.add in components, unless for the default in registerIcon.

Some years ago, all our components directly referenced codicons and their ids. When adding icon theming support we added this indirection and I and @misolori migrated most components, including terminal. But given the huge number of icon references that work is still not complete and there are still plenty of references to Codicon. That's why a codicon and a product icon (ThemeIcon) still have the same API and we don't enforce this with the compiler. Also there are some codicon aliases that have component specific names. We can't clean up the aliases in order to not break existing code in extensions. Eventually no one should use there anymore.

• please do not reference Codicon directly anymore unless in registerIcon
• please follow the pattern as seen in terminalIcons
• please don't add component specific Codicon aliases

FYI @misolori @daviddossett

[Tyriar]

@aeschli FYI I was following what seemed to be the thing to do as demonstrated by the debug- icons.

vscode/src/vs/base/common/codicons.ts

Lines 100 to 102 in e55863c

	public static readonly debugBreakpoint = new Codicon('debug-breakpoint', Codicon.circleFilled.definition);
	public static readonly debugBreakpointDisabled = new Codicon('debug-breakpoint-disabled', Codicon.circleFilled.definition);
	public static readonly debugHint = new Codicon('debug-hint', Codicon.circleFilled.definition);

Could we remove those and similar examples and/or add a @deprecated comment with this guidance?

[aeschli]

Good point, we should clean these up. The breakpoint- codicons have been around from old times before we did the product icons. There are product icon registration under the same name

vscode/src/vs/workbench/contrib/debug/browser/debugIcons.ts

Line 19 in 87d957e

regular: registerIcon('debug-breakpoint', Codicon.debugBreakpoint, localize('debugBreakpoint', 'Icon for breakpoints.')),

, so we can remove the codicons that are just aliases, and give the others a different name, to avoid confusion.