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

Show a validation warning when someone sets a setColorScheme action with an invalid scheme #7221

Closed
zadjii-msft opened this issue Aug 7, 2020 · 3 comments · Fixed by #8147
Labels
Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@zadjii-msft
Copy link
Member

Follow up from #6993.

Basically the title.

We'll have to scan the tree of actions manually for all the "set color scheme" actions, and ensure each of them has a valid scheme. We can't do this during the parsing of the arg itself, we'll have to do this manually in the Validate phase

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Priority-3 A description (P3) good first issue This is a fix that might be easier for someone to do as a first contribution labels Aug 7, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 7, 2020
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 10, 2020
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Aug 26, 2020
@mpela81
Copy link
Contributor

mpela81 commented Oct 30, 2020

Some random thoughts while checking this:

  • iterate over all commands in the settings, if at least one "set color scheme" action refers to an unknown scheme, raise the warning
  • check for nested commands as well
  • noticed that the global commands list includes this non-expanded command from default json:
{
    // Select color scheme...
    "name": { "key": "SetColorSchemeParentCommandName" },
    "commands": [
        {
            "iterateOn": "schemes",
            "name": "${scheme.name}",
            "command": { "action": "setColorScheme", "colorScheme": "${scheme.name}" }
        }
    ]
}

What could be a good way to skip validating these? 🤔

@zadjii-msft
Copy link
Member Author

You could probably ignore any commands with "iterateOn": "schemes", at least for now. That's going to create one action for each color scheme, so you could reasonably assume that the provided colorScheme (which will be expanded out once we know what the set of schemes is) will be valid.

@ghost ghost added the In-PR This issue has a related PR label Nov 3, 2020
@ghost ghost closed this as completed in #8147 Dec 1, 2020
@ghost ghost removed the In-PR This issue has a related PR label Dec 1, 2020
ghost pushed a commit that referenced this issue Dec 1, 2020
Show a validation warning when someone sets a `setColorScheme` action
with an invalid scheme

In the setting validation phase, scan all commands for all the "set
color scheme" actions, and check each of them has a valid scheme. If any
of them has an invalid scheme name, raise a warning. Do not check
iterable commands that will be expanded to valid color schemes.

## Validation Steps Performed
- Added tests to LocalTests_SettingsModel
- Manual tests, add commands to settings.json with invalid color scheme
  and check the warning pops up. Try simple and nested commands.

Closes #7221
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Dec 1, 2020
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8147, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants