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

Extended theme customisation syntax #122969

Merged
merged 12 commits into from Jul 19, 2021
Merged

Extended theme customisation syntax #122969

merged 12 commits into from Jul 19, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 5, 2021

Closes #103694.

  • Multiple [scope] per customisation
  • Wildcard scopes using *

Similar themes in large packs might need to be customised the same way.
Can lead to verbose code as explained in #103694 (already existent):

{
    "workbench.colorCustomizations": {
        "[Material Theme Palenight]": {
            "activityBarBadge.background": "#c792ea",
            "activityBar.activeBorder": "#c792ea",
            "breadcrumb.activeSelectionForeground": "#c792ea",
            "statusBarItem.remoteBackground": "#c792ea"
        },
        "[Material Theme Palenight High Contrast]": {
            "activityBarBadge.background": "#c792ea",
            "activityBar.activeBorder": "#c792ea",
            "breadcrumb.activeSelectionForeground": "#c792ea",
            "statusBarItem.remoteBackground": "#c792ea"
        },
        "[Community Material Theme Palenight]": {
            "activityBarBadge.background": "#c792ea",
            "activityBar.activeBorder": "#c792ea",
            "breadcrumb.activeSelectionForeground": "#c792ea",
            "statusBarItem.remoteBackground": "#c792ea"
        },
        "[Community Material Theme Palenight High Contrast]": {
            "activityBarBadge.background": "#c792ea",
            "activityBar.activeBorder": "#c792ea",
            "breadcrumb.activeSelectionForeground": "#c792ea",
            "statusBarItem.remoteBackground": "#c792ea"
        }
    }
}

This pull adds a new syntax that shortens all of that to 1 wildcard scope:

{
    "workbench.colorCustomizations": {
        "[*Material Theme Palenight*]": {
            "activityBarBadge.background": "#c792ea",
            "activityBar.activeBorder": "#c792ea",
            "breadcrumb.activeSelectionForeground": "#c792ea",
            "statusBarItem.remoteBackground": "#c792ea"
        }
    }
}

The * wildcards matches zero or more of any character.
Multiple scopes are legal, delimited by the regex /\]\s*\[/. This will enhance settings.json keys:

  • workbench.colorCustomizations
  • editor.colorCustomizations editor.tokenColorCustomizations editor.semanticTokenColorCustomizations

@aeschli

Multiple scopes per customisation
Wildcard scopes using `...`
@ghost
Copy link

ghost commented May 5, 2021

CLA assistant check
All CLA requirements met.

@@ -635,7 +668,7 @@ export class ColorThemeData implements IWorkbenchColorTheme {
}

function toCSSSelector(extensionId: string, path: string) {
if (startsWith(path, './')) {
if (path.startsWith('./')) {
Copy link
Author

Choose a reason for hiding this comment

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

Used String.startsWith to pass code hygiene test.

@aeschli
Copy link
Contributor

aeschli commented May 11, 2021

Can you describe in the description what problem you want to solve and how what syntax you propose?

@ghost
Copy link
Author

ghost commented May 12, 2021

Done.

@ghost
Copy link
Author

ghost commented May 12, 2021

Do I change the configuration schema?

for (let t of themes) {
// add theme specific color customization ("[Abyss]":{ ... })
const themeId = `[${t.settingsId}]`;
themeSpecificWorkbenchColors.properties![themeId] = workbenchColors;
themeSpecificTokenColors.properties![themeId] = tokenColors;
themeSpecificSemanticTokenColors.properties![themeId] = semanticTokenColorSchema;
experimentalThemeSpecificSemanticTokenColors.properties![themeId] = { $ref: tokenStylingSchemaId, additionalProperties: false };
}

Late addition - how do I add tests?

4086606 added 2 commits May 12, 2021 04:03
Enable subloop key index by removing Iterable assert and for-of
@ghost
Copy link
Author

ghost commented May 14, 2021

A) Test rerun please B) few questions above.

@aeschli
Copy link
Contributor

aeschli commented May 17, 2021

We currently have no tests covering how the settings are read out. If you want to add unit tests for your code, that would be welcome. They would go to src/vs/workbench/services/themes/test/electron-browser

Not sure what you mean by Do I change the configuration schema?.
To get code completion in the settings JSON editor, we should adapt the configuration schema.
I think it can be done with the 'patternProperties' property: https://json-schema.org/understanding-json-schema/reference/object.html#pattern-properties

@ghost
Copy link
Author

ghost commented May 25, 2021

I've read the JSON schema doc. But I'm still unsure on how I implement the code completion on multiple scopes e.g. [Monokai] [Monokai Dimmed]?

@aeschli
Copy link
Contributor

aeschli commented May 26, 2021

The schema based completions are too limited, but validation will work. Better completions could be added programmatically, but I don't think we should go there. You can add some defaultSnippets as seen here:
https://github.com/microsoft/vscode/blob/main/extensions/configuration-editing/schemas/attachContainer.schema.json#L24

@ghost
Copy link
Author

ghost commented May 26, 2021

Hmm. Alright. Should I remove autocompletion of [...] scopes altogether? I suspect it's a a rather inconsistent UX to provide autocompletion for the first scope only (might get a lot of bug reports too)

@aeschli
Copy link
Contributor

aeschli commented May 26, 2021

The completions for single theme customizations (what we currently support) can stay in, no?

@ghost
Copy link
Author

ghost commented May 26, 2021

Alright, I'll look into the patternProperties validation

@ghost
Copy link
Author

ghost commented May 27, 2021

I don't understand how to write tests, nor what naming convention to use.
Leave it to an engineer when test coverage becomes a concern?

@aeschli
Copy link
Contributor

aeschli commented May 27, 2021

@4086606 That's ok. Let me know when the PR is ready to be reviewed

@ghost
Copy link
Author

ghost commented May 27, 2021

Should be okay for review. I don't like the as usage in getThemeSpecificColors. Maybe workbenchThemeService module could specify theme scopes as a separate optional property? Not sure how to change that and make isThemeScope a type assert

4086606 added 2 commits May 27, 2021 23:30
Introduce concept of theme scopes in the theme setting types.
Template literal types are broken in vscode repo, so we dupe interfaces.
@ghost
Copy link
Author

ghost commented May 27, 2021

Using this pattern, I can create scoped property types:

export type ThemeScopeKey = `[${string}$]`;

So we need template literal types in eslint which means @typescript-eslint/parser^4.7.0 😔

[styleRuleOrThemeSettingsId: string]: ISemanticTokenRules | ISemanticTokenColorCustomizations | boolean | undefined;
}

export interface IThemeScopedExperimentalSemanticTokenColorCustomizations {
Copy link
Author

Choose a reason for hiding this comment

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

Tongue twister of a name 🙈 any suggestions or is verbose better

@ghost
Copy link
Author

ghost commented Jun 6, 2021

After second thought, the changes to workbenchThemeService module are fine. The as usage is unpleasant but seems to work at compile time and that's fine by me (instead of waiting for TS to get new features). I think this pull is ready for merging.

@ghost
Copy link
Author

ghost commented Jul 8, 2021

@aeschli now that #103694 has the required number of 👍🏾, could this pull be added to the v1.59.0 release?

@aeschli aeschli added this to the July 2021 milestone Jul 19, 2021
@aeschli aeschli added feature-request Request for new features or functionality themes Color theme issues labels Jul 19, 2021
@aeschli aeschli merged commit d22e9b4 into microsoft:main Jul 19, 2021
@aeschli
Copy link
Contributor

aeschli commented Jul 19, 2021

Thanks a lot, @4086606 , and sorry for the wait!

@ghost ghost deleted the themeScopeSyntax branch July 28, 2021 15:28
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2021
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 themes Color theme issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[themes] settings: support for multiple theme names in VSCode colorCustomization
1 participant