Skip to content

Commit

Permalink
Raise warning on invalid color scheme in commands (#8147)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
mpela81 authored Dec 1, 2020
1 parent 3a5042a commit f8edcf5
Show file tree
Hide file tree
Showing 6 changed files with 206 additions and 1 deletion.
136 changes: 136 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ namespace SettingsModelLocalTests

TEST_METHOD(ValidateKeybindingsWarnings);

TEST_METHOD(ValidateColorSchemeInCommands);

TEST_METHOD(ValidateExecuteCommandlineWarning);

TEST_METHOD(ValidateLegacyGlobalsWarning);
Expand Down Expand Up @@ -1325,6 +1327,140 @@ namespace SettingsModelLocalTests
VERIFY_ARE_EQUAL(L"Campbell", settings->_allProfiles.GetAt(2).ColorSchemeName());
}

void DeserializationTests::ValidateColorSchemeInCommands()
{
Log::Comment(NoThrowString().Format(
L"Ensure that setting a command's color scheme to a non-existent scheme causes a warning."));

const std::string settings0String{ R"(
{
"profiles": [
{
"name" : "profile0",
"colorScheme": "schemeOne"
}
],
"schemes": [
{
"name": "schemeOne",
"foreground": "#111111"
}
],
"actions": [
{
"command": { "action": "setColorScheme", "colorScheme": "schemeOne" }
},
{
"command": { "action": "setColorScheme", "colorScheme": "invalidScheme" }
}
]
})" };

const std::string settings1String{ R"(
{
"profiles": [
{
"name" : "profile0",
"colorScheme": "schemeOne"
}
],
"schemes": [
{
"name": "schemeOne",
"foreground": "#111111"
}
],
"actions": [
{
"command": { "action": "setColorScheme", "colorScheme": "schemeOne" }
},
{
"name": "parent",
"commands": [
{ "command": { "action": "setColorScheme", "colorScheme": "invalidScheme" } }
]
}
]
})" };

const std::string settings2String{ R"(
{
"profiles": [
{
"name" : "profile0",
"colorScheme": "schemeOne"
}
],
"schemes": [
{
"name": "schemeOne",
"foreground": "#111111"
}
],
"actions": [
{
"command": { "action": "setColorScheme", "colorScheme": "schemeOne" }
},
{
"name": "grandparent",
"commands": [
{
"name": "parent",
"commands": [
{
"command": { "action": "setColorScheme", "colorScheme": "invalidScheme" }
}
]
}
]
}
]
})" };

{
// Case 1: setColorScheme command with invalid scheme
Log::Comment(NoThrowString().Format(
L"Testing a simple command with invalid scheme"));
VerifyParseSucceeded(settings0String);

auto settings = winrt::make_self<implementation::CascadiaSettings>();
settings->_ParseJsonString(settings0String, false);
settings->LayerJson(settings->_userSettings);
settings->_ValidateColorSchemesInCommands();

VERIFY_ARE_EQUAL(1u, settings->_warnings.Size());
VERIFY_ARE_EQUAL(SettingsLoadWarnings::InvalidColorSchemeInCmd, settings->_warnings.GetAt(0));
}
{
// Case 2: nested setColorScheme command with invalid scheme
Log::Comment(NoThrowString().Format(
L"Testing a nested command with invalid scheme"));
VerifyParseSucceeded(settings1String);

auto settings = winrt::make_self<implementation::CascadiaSettings>();
settings->_ParseJsonString(settings1String, false);
settings->LayerJson(settings->_userSettings);
settings->_ValidateColorSchemesInCommands();

VERIFY_ARE_EQUAL(1u, settings->_warnings.Size());
VERIFY_ARE_EQUAL(SettingsLoadWarnings::InvalidColorSchemeInCmd, settings->_warnings.GetAt(0));
}
{
// Case 3: nested-in-nested setColorScheme command with invalid scheme
Log::Comment(NoThrowString().Format(
L"Testing a nested-in-nested command with invalid scheme"));
VerifyParseSucceeded(settings2String);

auto settings = winrt::make_self<implementation::CascadiaSettings>();
settings->_ParseJsonString(settings2String, false);
settings->LayerJson(settings->_userSettings);
settings->_ValidateColorSchemesInCommands();

VERIFY_ARE_EQUAL(1u, settings->_warnings.Size());
VERIFY_ARE_EQUAL(SettingsLoadWarnings::InvalidColorSchemeInCmd, settings->_warnings.GetAt(0));
}
}

void DeserializationTests::TestHelperFunctions()
{
const std::string settings0String{ R"(
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadWar
USES_RESOURCE(L"MissingRequiredParameter"),
USES_RESOURCE(L"LegacyGlobalsProperty"),
USES_RESOURCE(L"FailedToParseCommandJson"),
USES_RESOURCE(L"FailedToWriteToSettings")
USES_RESOURCE(L"FailedToWriteToSettings"),
USES_RESOURCE(L"InvalidColorSchemeInCmd")
};
static const std::array<std::wstring_view, static_cast<uint32_t>(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels {
USES_RESOURCE(L"NoProfilesText"),
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,10 @@
<value>Failed to expand a command with "iterateOn" set. This command will be ignored.</value>
<comment>{Locked="\"iterateOn\""} </comment>
</data>
<data name="InvalidColorSchemeInCmd" xml:space="preserve">
<value>Found a command with an invalid "colorScheme". This command will be ignored. Make sure that when setting a "colorScheme", the value matches the "name" of a color scheme in the "schemes" list.</value>
<comment>{Locked="\"colorScheme\"","\"name\"","\"schemes\""}</comment>
</data>
<data name="CmdCommandArgDesc" xml:space="preserve">
<value>An optional command, with arguments, to be spawned in the new tab or pane</value>
</data>
Expand Down
60 changes: 60 additions & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,8 @@ void CascadiaSettings::_ValidateSettings()
// This will also catch other keybinding warnings, like from GH#4239
_ValidateKeybindings();

_ValidateColorSchemesInCommands();

_ValidateNoGlobalsKey();
}

Expand Down Expand Up @@ -697,6 +699,64 @@ void CascadiaSettings::_ValidateKeybindings()
}
}

// Method Description:
// - Ensures that every "setColorScheme" command has a valid "color scheme" set.
// Arguments:
// - <none>
// Return Value:
// - <none>
// - Appends a SettingsLoadWarnings::InvalidColorSchemeInCmd to our list of warnings if
// we find any command with an invalid color scheme.
void CascadiaSettings::_ValidateColorSchemesInCommands()
{
bool foundInvalidScheme{ false };
for (const auto& nameAndCmd : _globals->Commands())
{
if (_HasInvalidColorScheme(nameAndCmd.Value()))
{
foundInvalidScheme = true;
break;
}
}

if (foundInvalidScheme)
{
_warnings.Append(SettingsLoadWarnings::InvalidColorSchemeInCmd);
}
}

bool CascadiaSettings::_HasInvalidColorScheme(const Model::Command& command)
{
bool invalid{ false };
if (command.HasNestedCommands())
{
for (const auto& nested : command.NestedCommands())
{
if (_HasInvalidColorScheme(nested.Value()))
{
invalid = true;
break;
}
}
}
else if (const auto& actionAndArgs = command.Action())
{
if (const auto& realArgs = actionAndArgs.Args().try_as<Model::SetColorSchemeArgs>())
{
auto cmdImpl{ winrt::get_self<Command>(command) };
// no need to validate iterable commands on color schemes
// they will be expanded to commands with a valid scheme name
if (cmdImpl->IterateOn() != ExpandCommandType::ColorSchemes &&
!_globals->ColorSchemes().HasKey(realArgs.SchemeName()))
{
invalid = true;
}
}
}

return invalid;
}

// Method Description:
// - Checks for the presence of the legacy "globals" key in the user's
// settings.json. If this key is present, then they've probably got a pre-0.11
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void _ValidateAllSchemesExist();
void _ValidateMediaResources();
void _ValidateKeybindings();
void _ValidateColorSchemesInCommands();
void _ValidateNoGlobalsKey();

bool _HasInvalidColorScheme(const Model::Command& command);

friend class SettingsModelLocalTests::SerializationTests;
friend class SettingsModelLocalTests::DeserializationTests;
friend class SettingsModelLocalTests::ProfileTests;
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/TerminalWarnings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace Microsoft.Terminal.Settings.Model
LegacyGlobalsProperty = 8,
FailedToParseCommandJson = 9,
FailedToWriteToSettings = 10,
InvalidColorSchemeInCmd = 11,
WARNINGS_SIZE // IMPORTANT: This MUST be the last value in this enum. It's an unused placeholder.
};

Expand Down

0 comments on commit f8edcf5

Please sign in to comment.