From 45dc3452b13146d2e92d6c1abf24fbed34267ca1 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 28 Feb 2020 15:41:42 -0600 Subject: [PATCH] Display these two warning cases * When the keybindings are missing required args (#3522) * When more than one "keys" aws specified (#4239) --- src/cascadia/TerminalApp/ActionArgs.h | 9 +++++++- src/cascadia/TerminalApp/AppKeyBindings.h | 2 +- .../AppKeyBindingsSerialization.cpp | 22 +++++++++++++++--- src/cascadia/TerminalApp/AppLogic.cpp | 9 +++++--- src/cascadia/TerminalApp/CascadiaSettings.cpp | 23 +++++++++++++++++++ src/cascadia/TerminalApp/CascadiaSettings.h | 1 + .../TerminalApp/GlobalAppSettings.cpp | 9 +++++++- src/cascadia/TerminalApp/GlobalAppSettings.h | 3 +++ .../Resources/Resources.language-en.resw | 14 ++++++++++- src/cascadia/TerminalApp/TerminalWarnings.h | 9 ++++++-- 10 files changed, 89 insertions(+), 12 deletions(-) diff --git a/src/cascadia/TerminalApp/ActionArgs.h b/src/cascadia/TerminalApp/ActionArgs.h index 4e744280d15..abb0b7c71dc 100644 --- a/src/cascadia/TerminalApp/ActionArgs.h +++ b/src/cascadia/TerminalApp/ActionArgs.h @@ -261,7 +261,14 @@ namespace winrt::TerminalApp::implementation { args->_Direction = ParseDirection(directionString.asString()); } - return { *args, {} }; + if (args->_Direction == TerminalApp::Direction::None) + { + return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } }; + } + else + { + return { *args, {} }; + } } }; diff --git a/src/cascadia/TerminalApp/AppKeyBindings.h b/src/cascadia/TerminalApp/AppKeyBindings.h index a12b92b51d2..63df542a75a 100644 --- a/src/cascadia/TerminalApp/AppKeyBindings.h +++ b/src/cascadia/TerminalApp/AppKeyBindings.h @@ -53,7 +53,7 @@ namespace winrt::TerminalApp::implementation static Windows::System::VirtualKeyModifiers ConvertVKModifiers(winrt::Microsoft::Terminal::Settings::KeyModifiers modifiers); // Defined in AppKeyBindingsSerialization.cpp - void LayerJson(const Json::Value& json); + std::vector<::TerminalApp::SettingsLoadWarnings> LayerJson(const Json::Value& json); Json::Value ToJson(); void SetDispatch(const winrt::TerminalApp::ShortcutActionDispatch& dispatch); diff --git a/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp b/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp index 9656b3e0f4a..99bbcf07825 100644 --- a/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp +++ b/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp @@ -431,8 +431,10 @@ static ShortcutAction GetActionFromString(const std::string_view actionString) // `"unbound"`, then we'll clear the keybinding from the existing keybindings. // Arguments: // - json: and array of JsonObject's to deserialize into our _keyShortcuts mapping. -void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::Value& json) +std::vector<::TerminalApp::SettingsLoadWarnings> winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::Value& json) { + std::vector<::TerminalApp::SettingsLoadWarnings> warnings; + for (const auto& value : json) { if (!value.isObject()) @@ -447,6 +449,12 @@ void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::V { const auto validString = keys.isString(); const auto validArray = keys.isArray() && keys.size() == 1; + + if (keys.isArray() && keys.size() > 1) + { + warnings.push_back(::TerminalApp::SettingsLoadWarnings::TooManyKeysForChord); + } + if (!validString && !validArray) { continue; @@ -490,16 +498,22 @@ void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::V // does, we'll try to deserialize any "args" that were provided with // the binding. IActionArgs args{ nullptr }; - std::vector<::TerminalApp::SettingsLoadWarnings> warnings; + std::vector<::TerminalApp::SettingsLoadWarnings> parseWarnings; const auto deserializersIter = argParsers.find(action); if (deserializersIter != argParsers.end()) { auto pfn = deserializersIter->second; if (pfn) { - std::tie(args, warnings) = pfn(argsVal); + std::tie(args, parseWarnings) = pfn(argsVal); } } + warnings.insert(warnings.end(), parseWarnings.begin(), parseWarnings.end()); + + if (args == nullptr) + { + continue; + } // Try parsing the chord try @@ -528,4 +542,6 @@ void winrt::TerminalApp::implementation::AppKeyBindings::LayerJson(const Json::V } } } + + return warnings; } diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index bd9288450bc..0ee565cda0e 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -27,14 +27,17 @@ namespace winrt // !!! IMPORTANT !!! // Make sure that these keys are in the same order as the // SettingsLoadWarnings/Errors enum is! -static const std::array settingsLoadWarningsLabels { +static const std::array(SettingsLoadWarnings::WARNINGS_SIZE)> settingsLoadWarningsLabels { USES_RESOURCE(L"MissingDefaultProfileText"), USES_RESOURCE(L"DuplicateProfileText"), USES_RESOURCE(L"UnknownColorSchemeText"), USES_RESOURCE(L"InvalidBackgroundImage"), - USES_RESOURCE(L"InvalidIcon") + USES_RESOURCE(L"InvalidIcon"), + USES_RESOURCE(L"AtLeastOneKeybindingWarning"), + USES_RESOURCE(L"TooManyKeysForChord"), + USES_RESOURCE(L"MissingRequiredParameter") }; -static const std::array settingsLoadErrorsLabels { +static const std::array(SettingsLoadErrors::ERRORS_SIZE)> settingsLoadErrorsLabels { USES_RESOURCE(L"NoProfilesText"), USES_RESOURCE(L"AllProfilesHiddenText") }; diff --git a/src/cascadia/TerminalApp/CascadiaSettings.cpp b/src/cascadia/TerminalApp/CascadiaSettings.cpp index 73300016547..56c46369f39 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.cpp +++ b/src/cascadia/TerminalApp/CascadiaSettings.cpp @@ -209,6 +209,7 @@ void CascadiaSettings::_ValidateSettings() // TODO:GH#3522 With variable args to keybindings, it's possible that a user // set a keybinding without all the required args for an action. Display a // warning if an action didn't have a required arg. + _ValidateKeybindings(); } // Method Description: @@ -651,3 +652,25 @@ GUID CascadiaSettings::_GetProfileForIndex(std::optional index) const } return profileGuid; } + +// Method Description: +// - Ensures that all specified images resources (icons and background images) are valid URIs. +// This does not verify that the icon or background image files are encoded as an image. +// Arguments: +// - +// Return Value: +// - +// - Appends a SettingsLoadWarnings::InvalidBackgroundImage to our list of warnings if +// we find any invalid background images. +// - Appends a SettingsLoadWarnings::InvalidIconImage to our list of warnings if +// we find any invalid icon images. +void CascadiaSettings::_ValidateKeybindings() +{ + auto keybindingWarnings = _globals.GetKeybindingsWarnings(); + + if (!keybindingWarnings.empty()) + { + _warnings.push_back(::TerminalApp::SettingsLoadWarnings::AtLeastOneKeybindingWarning); + _warnings.insert(_warnings.end(), keybindingWarnings.begin(), keybindingWarnings.end()); + } +} diff --git a/src/cascadia/TerminalApp/CascadiaSettings.h b/src/cascadia/TerminalApp/CascadiaSettings.h index 50fa261a187..063a8ba8000 100644 --- a/src/cascadia/TerminalApp/CascadiaSettings.h +++ b/src/cascadia/TerminalApp/CascadiaSettings.h @@ -116,6 +116,7 @@ class TerminalApp::CascadiaSettings final void _RemoveHiddenProfiles(); void _ValidateAllSchemesExist(); void _ValidateMediaResources(); + void _ValidateKeybindings(); friend class TerminalAppLocalTests::SettingsTests; friend class TerminalAppLocalTests::ProfileTests; diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.cpp b/src/cascadia/TerminalApp/GlobalAppSettings.cpp index 2007655c5ca..30b31b725e9 100644 --- a/src/cascadia/TerminalApp/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalApp/GlobalAppSettings.cpp @@ -43,6 +43,7 @@ static constexpr std::wstring_view SystemThemeValue{ L"system" }; GlobalAppSettings::GlobalAppSettings() : _keybindings{ winrt::make_self() }, + _keybindingsWarnings{}, _colorSchemes{}, _defaultProfile{}, _alwaysShowTabs{ true }, @@ -330,7 +331,8 @@ void GlobalAppSettings::LayerJson(const Json::Value& json) if (auto keybindings{ json[JsonKey(KeybindingsKey)] }) { - _keybindings->LayerJson(keybindings); + auto warnings = _keybindings->LayerJson(keybindings); + _keybindingsWarnings.insert(_keybindingsWarnings.end(), warnings.begin(), warnings.end()); } if (auto snapToGridOnResize{ json[JsonKey(SnapToGridOnResizeKey)] }) @@ -537,3 +539,8 @@ void GlobalAppSettings::AddColorScheme(ColorScheme scheme) std::wstring name{ scheme.GetName() }; _colorSchemes[name] = std::move(scheme); } + +std::vector GlobalAppSettings::GetKeybindingsWarnings() const +{ + return _keybindingsWarnings; +} diff --git a/src/cascadia/TerminalApp/GlobalAppSettings.h b/src/cascadia/TerminalApp/GlobalAppSettings.h index 738aace24d8..0905202d497 100644 --- a/src/cascadia/TerminalApp/GlobalAppSettings.h +++ b/src/cascadia/TerminalApp/GlobalAppSettings.h @@ -83,11 +83,14 @@ class TerminalApp::GlobalAppSettings final void ApplyToSettings(winrt::Microsoft::Terminal::Settings::TerminalSettings& settings) const noexcept; + std::vector GetKeybindingsWarnings() const; + GETSET_PROPERTY(bool, SnapToGridOnResize, true); private: GUID _defaultProfile; winrt::com_ptr _keybindings; + std::vector<::TerminalApp::SettingsLoadWarnings> _keybindingsWarnings; std::unordered_map _colorSchemes; diff --git a/src/cascadia/TerminalApp/Resources/Resources.language-en.resw b/src/cascadia/TerminalApp/Resources/Resources.language-en.resw index 7034489e757..5b0f86f23d1 100644 --- a/src/cascadia/TerminalApp/Resources/Resources.language-en.resw +++ b/src/cascadia/TerminalApp/Resources/Resources.language-en.resw @@ -223,6 +223,18 @@ Temporarily using the Windows Terminal default settings. Found a profile with an invalid "icon". Defaulting that profile to have no icon. Make sure that when setting an "icon", the value is a valid file path to an image. + + Warnings were found while parsing your keybindings: + + + + Found a keybinding with too many strings for the "keys" array. There should only be one string value in the "keys" array. + + + + Found a keybinding that was missing a required parameter value. This keybinding will be ignored. + + An optional command, with arguments, to be spawned in the new tab or pane @@ -256,4 +268,4 @@ Temporarily using the Windows Terminal default settings. Open in the given directory instead of the profile's set startingDirectory - \ No newline at end of file + diff --git a/src/cascadia/TerminalApp/TerminalWarnings.h b/src/cascadia/TerminalApp/TerminalWarnings.h index 35db814d999..f8b13c88522 100644 --- a/src/cascadia/TerminalApp/TerminalWarnings.h +++ b/src/cascadia/TerminalApp/TerminalWarnings.h @@ -25,7 +25,11 @@ namespace TerminalApp DuplicateProfile = 1, UnknownColorScheme = 2, InvalidBackgroundImage = 3, - InvalidIcon = 4 + InvalidIcon = 4, + AtLeastOneKeybindingWarning = 5, + TooManyKeysForChord = 6, + MissingRequiredParameter = 7, + WARNINGS_SIZE // IMPORTANT: This MUST be the last value in this enum. It's an unused placeholder. }; // SettingsLoadWarnings are scenarios where the settings had invalid state @@ -33,7 +37,8 @@ namespace TerminalApp enum class SettingsLoadErrors : uint32_t { NoProfiles = 0, - AllProfilesHidden = 1 + AllProfilesHidden = 1, + ERRORS_SIZE // IMPORTANT: This MUST be the last value in this enum. It's an unused placeholder. }; // This is a helper class to wrap up a SettingsLoadErrors into a proper