allow variables in the color-theme definition file for workbench color

[d-mahard]

Description

For feature request described in #105247

Mainly there are two new features proposed here

• Allow theme file to have a color palette, and the vars can be used to define a color for the workbench color item. EDIT 2023-12-15: also for syntax and semantic token.
• EDIT 2023-12-15 Add snippets and suggestion for the colors, which include all the defined variables in the palette

This does NOT include the following

• EDIT 2023-12-15 Usage of the palette var for token colors (I am interested to explore this, but would like to hear feedback first)
• Usage of the palette var in Setting #56855, EDIT 2023-12-15 as this needs to change the colorRegistry, which may affect many other things. (I would also love to explore this option, depending on the feedback)

EDIT 2023-12-15 (see comment for screenshots)

Some technical details

• The syntax using $ is based on this comment by @aeschli
• The parsing happens right when loading theme data. Any other service etc that uses color from the theme will always see the parsed hex, never the palette var.
• For the snippets/suggestion/validation, I cannot use workbenchColorsSchemaId because modifying it would affect many other things. So instead I create a new JSON Schema based on that (here), so it will be isolated, only in theme file. However, if there is no palette, there will essentially be no modification to the schema, it is just copied as it is.
• EDIT 2023-12-15 For the snippets/suggestion/validation, the palette needs to be loaded first, meaning that if there is any change in the palette, VSCode needs to be restarted after the change. However, I think it is safe to assume that a palette change, especially the change of the keys, only happens rarely.

How to test

I prepared a unit test for the parsing in themeColorParsing.test.ts

[aeschli]

I think the direction you are taking is good.
Please try to make the change as minimal as possible. That simplifies the review.

[d-mahard]

Hi, thank you for the feedback! Sure, I'll try to minimize the changes.

[d-mahard]

@aeschli I tried to reduce the changes, but not so much could be removed. Actually about half of the added lines are the unit test (two dummy theme files, and the unit test file itself). So for the logic, it is around 100 lines added and 30 removed.

To help with the review, I added some comments in this PR to explain the logic/design decision.
Please see the comments below.

Let me know if other information is needed.

[aeschli]

Thanks @d-mahard for all the work!
I don't like the solution chosen to use a dynamic schema for the validation. Schemas are intended to be static resources, identified by an resource URI. To try to validate a document with a schema that is dynamically generated based on the document being validated is a stretch, to say the least. I don't see how his will work when opening a random theme file. Or when you open multiple theme files at the same time.
If you want to add validation and completions, do that in code in the extension extension-editing. Or leave that to an external extension.

• remove the dynamic schema code. The only schema changes are to add the new property and allow $\w+ besides color-hex
• the palette should also work in tokenColors and semantic colors.
• variables are local to a theme file. Make sure that the schemas for workbench.colorCustomizations, editor.colorCustomizations and editor.semanticTokenColorCustomizations are not affected and don't allow variables
• when using a color that is not defined (e.g. because of a typo in the color name), I suggest to default to #f00 (red) as color, so that the problem is easy detectable

[d-mahard]

thank you @aeschli for the review!
I understand your points. I will leave the dynamic schema validation/suggestion of the variable enumeration out of this PR.
User will just need to satisfy the $\w+ pattern beside the color-hex, and if it is not in the var list, it will default to red for easy debugging.

Regarding the following two points:

• remove the dynamic schema code. The only schema changes are to add the new property and allow $\w+ besides color-hex
• Make sure that the schemas for workbench.colorCustomizations, editor.colorCustomizations and editor.semanticTokenColorCustomizations are not affected and don't allow variables

In my understanding, to satisfy both requirements we still need the dynamic schema (even though the var enum is removed). The reasons are the following:

Schemas for setting are controlled here. In line 84 below, we need to use workbenchColorsSchemaId, in which the color-hex format schema is defined.:

vscode/src/vs/workbench/services/themes/common/themeConfiguration.ts

Lines 81 to 90 in 4fce419

	const colorCustomizationsSchema: IConfigurationPropertySchema = {
	type: 'object',
	description: nls.localize('workbenchColors', "Overrides colors from the currently selected color theme."),
	allOf: [{ $ref: workbenchColorsSchemaId }],
	default: {},
	defaultSnippets: [{
	body: {
	}
	}]
	};

For the theme, the schema is (in the current main branch) as follows. In line 229 below, we need to also use workbenchColorsSchemaId, in which the color-hex format schema is defined.

vscode/src/vs/workbench/services/themes/common/colorThemeSchema.ts

Lines 222 to 253 in 4fce419

	const colorThemeSchema: IJSONSchema = {
	type: 'object',
	allowComments: true,
	allowTrailingCommas: true,
	properties: {
	colors: {
	description: nls.localize('schema.workbenchColors', 'Colors in the workbench'),
	$ref: workbenchColorsSchemaId,
	additionalProperties: false
	},
	tokenColors: {
	anyOf: [{
	type: 'string',
	description: nls.localize('schema.tokenColors.path', 'Path to a tmTheme file (relative to the current file).')
	},
	{
	description: nls.localize('schema.colors', 'Colors for syntax highlighting'),
	$ref: textmateColorsSchemaId
	}
	]
	},
	semanticHighlighting: {
	type: 'boolean',
	description: nls.localize('schema.supportsSemanticHighlighting', 'Whether semantic highlighting should be enabled for this theme.')
	},
	semanticTokenColors: {
	type: 'object',
	description: nls.localize('schema.semanticTokenColors', 'Colors for semantic tokens'),
	$ref: tokenStylingSchemaId
	}
	}
	};

The color-hex limitation is defined in line 139 below, (and then the whole schema is registered in line 687 of the same file.)

vscode/src/vs/platform/theme/common/colorRegistry.ts

Lines 136 to 153 in 4fce419

	public registerColor(id: string, defaults: ColorDefaults | null, description: string, needsTransparency = false, deprecationMessage?: string): ColorIdentifier {
	const colorContribution: ColorContribution = { id, description, defaults, needsTransparency, deprecationMessage };
	this.colorsById[id] = colorContribution;
	const propertySchema: IJSONSchema = { type: 'string', description, format: 'color-hex', defaultSnippets: [{ body: '${1:#ff0000}' }] };
	if (deprecationMessage) {
	propertySchema.deprecationMessage = deprecationMessage;
	}
	if (needsTransparency) {
	propertySchema.pattern = '^#(?:(?<rgba>[0-9a-fA-f]{3}[0-9a-eA-E])|(?:[0-9a-fA-F]{6}(?:(?![fF]{2})(?:[0-9a-fA-F]{2}))))?$';
	propertySchema.patternErrorMessage = 'This color must be transparent or it will obscure content';
	}
	this.colorSchema.properties[id] = propertySchema;
	this.colorReferenceSchema.enum.push(id);
	this.colorReferenceSchema.enumDescriptions.push(description);
	this._onDidChangeSchema.fire();
	return id;
	}

We cannot simply just change the schema of the theme, because the color-hex limitation is not defined there. But if we add the $\w+ to where it is defined (line 139 above), then it will definitely affect the setting.

So my proposal would still be using the dynamic schema, together with the scheduler, but removing the variable enum. So something like the following:

for (const key in workbenchColorSchemaForTheme.properties) {
		const property = workbenchColorSchemaForTheme.properties[key];
		delete property.format;

		property.anyOf = [
			{
				pattern: '^\\$\\w+', errorMessage: nls.localize('error.invalidFormat.colorEntryWithPalette', "Color must either in hex format (e.g. `#ffffff`) or one of the palette (e.g. `$accentName`)")
			}, // here, instead of using `$ref: themePaletteEnumSchemaId` like before, I just add the `$\w+` pattern
			{ format: 'color-hex' },
		];
	}

and of course themePaletteEnumSchemaId and all related logic everywhere will be removed.

(for completeness): The reason why I think even the scheduler is still needed is like I mentioned in my other comment:

This scheduler is necessary because the color items are contributed not just from the "core" items in colorRegistry.ts but from some other locations, and it seems (from my observation at least) the loading/registering of those colors happen in parallel also with the loading of the theme itself. I tried calling the registerColorThemeSchemas() only once (without the scheduler), and as a result some of the colors in the theme file were linted as not allowed in the schema (i.e. the newly schema, modified from the workbench scheme). I concluded that when the function was called, not all color-items had already been registered. So it has to be repeated when there is new color registered.

Is my understanding correct or did I miss something here? Do you have any other approach?

[aeschli]

@d-mahard So as you say, we can no longer have a single schema to describe colors for workbench.colorCustomizations and for colors in the theme file.
I think the proper approach is to have two different schemas, e.g. 'vscode://schemas/workbench-settings-colors' and `'vscode://schemas/workbench-theme-colors'.

I would suggest to collect two different schemas in the ColorRegistry:

vscode/src/vs/platform/theme/common/colorRegistry.ts

Line 128 in c587478

private colorSchema: IJSONSchema & { properties: IJSONSchemaMap } = { type: 'object', properties: {} };

The trick with these schemas is that they are modified while new colors come in. So you can not just take a copy of it.

[d-mahard]

@aeschli thank you for the hint!

I made the changes based on your suggestions:

• all failed parsing now defaults to red
• dynamic schema validation/suggestion of the variable enumeration is no longer in this PR
• expanded the feature to cover tokenColors and semantic colors. The approach is the same with workbench color: parsing happens when theme data is loaded, so any other thing that uses the data will not see the variable.
• a new separate schema is collected from the colorRegistry for the workbench theme.
• the same approach is also used for semantic token (separate schema for setting and theme), because I noticed that it is also updated every time there is a new semantic token rule added
• however for the token color, it seems not to be the case. So I just make a copy of the schema-for-setting and make the necessary changes for the theme schema
• I adjusted the unit test to cover also the parsing of token color and semantic token
• I wanted to add a unit test for the schema using something like ajv, but could not really figure out how module importing works in this project, so I decided to skip that one. If you can give me a hint I will try to create that.

In any case I tested the schema validation, see below:

in theme file, variable is allowed

in settings, variable is not allowed

A little caveat:
The newly added warning (#200052) still works if we use hex, but when use variable there won't be any warning even when the color of that var is not transparent

[aeschli]

@d-mahard Thanks for all your work! It's nicely done, I also tested it.
But now, seeing all the changes and talking about it with my co-workers, I'm hesitating to merge. I'm not happy with the added code complexity and added runtime memory around the schema generation. In the end, the color palettes are 'nice to have' but not essential. I'm not even sure that it will be good enough for ambitious theme authors, that probably want functions of colors.
So again I think this should better done as a tool on top on top of theme files. A tool that generates color themes from it's own syntax.
I'm sorry that I didn't stop you earlier from doing all the work. I was hoping it's ending up to be a simple change, but I don't see any other approach than what you just implemented.
So I'm planing to close the PR and I hope for your understanding.

[d-mahard]

@aeschli
Thank you for taking the time to test and give feedback, really appreciated it.
I can totally understand the consideration. In any case, that was a good practice on my side. 👍