maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Raise warning on invalid color scheme in commands (#8147)

Browse source 
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
This commit is contained in:
MPela 2020-12-01 23:28:00 +01:00 committed by GitHub
parent 3a5042a774
commit f8edcf57bd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 206 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

136
src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file

@ -68,6 +68,8 @@ namespace SettingsModelLocalTests
TEST_METHOD(ValidateKeybindingsWarnings);
TEST_METHOD(ValidateColorSchemeInCommands);
TEST_METHOD(ValidateExecuteCommandlineWarning);
TEST_METHOD(ValidateLegacyGlobalsWarning);
@ -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"(

3
src/cascadia/TerminalApp/AppLogic.cpp
View file

@ -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"),

4
src/cascadia/TerminalApp/Resources/en-US/Resources.resw
View file

@ -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>

60
src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
View file

@ -282,6 +282,8 @@ void CascadiaSettings::_ValidateSettings()
// This will also catch other keybinding warnings, like from GH#4239
_ValidateKeybindings();
_ValidateColorSchemesInCommands();
_ValidateNoGlobalsKey();
}
@ -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

3
src/cascadia/TerminalSettingsModel/CascadiaSettings.h
View file

@ -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;

1
src/cascadia/TerminalSettingsModel/TerminalWarnings.idl
View file

@ -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.
};