From 3f65b46c623a38ec6302e736a3386b668b3bd5cb Mon Sep 17 00:00:00 2001 From: Marco Pelagatti <1140981+mpela81@users.noreply.github.com> Date: Tue, 3 Nov 2020 19:34:18 +0100 Subject: [PATCH 1/3] Raise warning on invalid color scheme in commands --- .../DeserializationTests.cpp | 136 ++++++++++++++++++ src/cascadia/TerminalApp/AppLogic.cpp | 3 +- .../Resources/en-US/Resources.resw | 4 + .../CascadiaSettings.cpp | 60 ++++++++ .../TerminalSettingsModel/CascadiaSettings.h | 3 + .../TerminalWarnings.idl | 1 + 6 files changed, 206 insertions(+), 1 deletion(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index e6cf1b3abcd..93bc8ca3a3d 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -68,6 +68,8 @@ namespace SettingsModelLocalTests TEST_METHOD(ValidateKeybindingsWarnings); + TEST_METHOD(ValidateColorSchemeInCommands); + TEST_METHOD(ValidateExecuteCommandlineWarning); TEST_METHOD(ValidateLegacyGlobalsWarning); @@ -1323,6 +1325,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(); + 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(); + 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(); + 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"( diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index fedcbe05944..f804a6e36a4 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -41,7 +41,8 @@ static const std::array(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(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels { USES_RESOURCE(L"NoProfilesText"), diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index c7b08834f56..704283ebe94 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -235,6 +235,10 @@ Failed to expand a command with "iterateOn" set. This command will be ignored. {Locked="\"iterateOn\""} + + 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. + {Locked="\"colorScheme\"","\"name\"","\"schemes\""} + An optional command, with arguments, to be spawned in the new tab or pane diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 35ee46c0a13..2355814a6c8 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -287,6 +287,8 @@ void CascadiaSettings::_ValidateSettings() // This will also catch other keybinding warnings, like from GH#4239 _ValidateKeybindings(); + _ValidateColorSchemesInCommands(); + _ValidateNoGlobalsKey(); } @@ -714,6 +716,64 @@ void CascadiaSettings::_ValidateKeybindings() } } +// Method Description: +// - Ensures that every "setColorScheme" command has a valid "color scheme" set. +// Arguments: +// - +// Return Value: +// - +// - 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& nestedkv : command.NestedCommands()) + { + if (_HasInvalidColorScheme(nestedkv.Value())) + { + invalid = true; + break; + } + } + } + else if (const auto& actionAndArgs = command.Action()) + { + if (const auto& realArgs = actionAndArgs.Args().try_as()) + { + auto cmdImpl{ winrt::get_self(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 diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 45e1c0d8cdf..dd0472d2cb9 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -142,8 +142,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::DeserializationTests; friend class SettingsModelLocalTests::ProfileTests; friend class SettingsModelLocalTests::ColorSchemeTests; diff --git a/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl b/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl index aa5bf80f109..1446ff12ea4 100644 --- a/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl +++ b/src/cascadia/TerminalSettingsModel/TerminalWarnings.idl @@ -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. }; From bfba839bc356cd9568e6e897bc6ba2b7ee08a0e8 Mon Sep 17 00:00:00 2001 From: Marco Pelagatti <1140981+mpela81@users.noreply.github.com> Date: Wed, 4 Nov 2020 09:13:26 +0100 Subject: [PATCH 2/3] Spelling --- src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 2355814a6c8..b50310c1e30 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -747,9 +747,9 @@ bool CascadiaSettings::_HasInvalidColorScheme(const Model::Command& command) bool invalid{ false }; if (command.HasNestedCommands()) { - for (const auto& nestedkv : command.NestedCommands()) + for (const auto& nested : command.NestedCommands()) { - if (_HasInvalidColorScheme(nestedkv.Value())) + if (_HasInvalidColorScheme(nested.Value())) { invalid = true; break; From 6b78dad0cbe1a9f4a193b682f6f9dd71639f4a7b Mon Sep 17 00:00:00 2001 From: Marco Pelagatti <1140981+mpela81@users.noreply.github.com> Date: Fri, 20 Nov 2020 11:28:08 +0100 Subject: [PATCH 3/3] formatting --- src/cascadia/TerminalSettingsModel/CascadiaSettings.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index b22cb1a0ed2..bcc792eb665 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -147,7 +147,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _ValidateKeybindings(); void _ValidateColorSchemesInCommands(); void _ValidateNoGlobalsKey(); - + bool _HasInvalidColorScheme(const Model::Command& command); friend class SettingsModelLocalTests::SerializationTests;