Add warning messages for bad keybindings (#4746)

## Summary of the Pull Request Adds warning messages for a pair of keybindings-related scenarios. This covers the following two bugs: * #4239 - If the user has supplied more than one key chord in their `"keys"` array. * #3522 - If a keybinding has a _required_ argument, then we'll display a message to the user - currently, the only required parameter is the `direction` parameter for both `resizePane` and `moveFocus` ## References When we get to #1334, we'll want to remove the `TooManyKeysForChord` warning. ## PR Checklist * [x] Closes #4239 * [x] Closes #3522 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ![image](https://user-images.githubusercontent.com/18356694/75593132-f18ec700-5a49-11ea-9d26-6acd0d28b0b7.png) ## Validation Steps Performed Tested manually, added tests.
microsoft · Mar 5, 2020 · 2cba4c6 · 2cba4c6
1 parent a3d68d2
commit 2cba4c6
Show file tree

Hide file tree

Showing 11 changed files with 204 additions and 46 deletions.
diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
@@ -70,6 +70,8 @@ namespace TerminalAppLocalTests
 
         TEST_METHOD(TestTerminalArgsForBinding);
 
+        TEST_METHOD(ValidateKeybindingsWarnings);
+
         TEST_CLASS_SETUP(ClassSetup)
         {
             InitializeJsonReader();
@@ -2089,4 +2091,48 @@ namespace TerminalAppLocalTests
             VERIFY_ARE_EQUAL(2, termSettings.HistorySize());
         }
     }
+
+    void SettingsTests::ValidateKeybindingsWarnings()
+    {
+        const std::string badSettings{ R"(
+        {
+            "globals": {
+                "defaultProfile": "{6239a42c-2222-49a3-80bd-e8fdd045185c}"
+            },
+            "profiles": [
+                {
+                    "name" : "profile0",
+                    "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}"
+                },
+                {
+                    "name" : "profile1",
+                    "guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}"
+                }
+            ],
+            "keybindings": [
+                { "command": { "action": "splitPane", "split":"auto" }, "keys": [ "ctrl+alt+t", "ctrl+a" ] },
+                { "command": { "action": "moveFocus" }, "keys": [ "ctrl+a" ] },
+                { "command": { "action": "resizePane" }, "keys": [ "ctrl+b" ] }
+            ]
+        })" };
+
+        const auto settingsObject = VerifyParseSucceeded(badSettings);
+        auto settings = CascadiaSettings::FromJson(settingsObject);
+
+        VERIFY_ARE_EQUAL(0u, settings->_globals._keybindings->_keyShortcuts.size());
+
+        VERIFY_ARE_EQUAL(3u, settings->_globals._keybindingsWarnings.size());
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::TooManyKeysForChord, settings->_globals._keybindingsWarnings.at(0));
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter, settings->_globals._keybindingsWarnings.at(1));
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter, settings->_globals._keybindingsWarnings.at(2));
+
+        settings->_ValidateKeybindings();
+
+        VERIFY_ARE_EQUAL(4u, settings->_warnings.size());
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::AtLeastOneKeybindingWarning, settings->_warnings.at(0));
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::TooManyKeysForChord, settings->_warnings.at(1));
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter, settings->_warnings.at(2));
+        VERIFY_ARE_EQUAL(::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter, settings->_warnings.at(3));
+    }
+
 }
diff --git a/src/cascadia/TerminalApp/ActionArgs.h b/src/cascadia/TerminalApp/ActionArgs.h
@@ -17,6 +17,7 @@
 
 #include "../../cascadia/inc/cppwinrt_utils.h"
 #include "Utils.h"
+#include "TerminalWarnings.h"
 
 // Notes on defining ActionArgs and ActionEventArgs:
 // * All properties specific to an action should be defined as an ActionArgs
@@ -26,6 +27,8 @@
 
 namespace winrt::TerminalApp::implementation
 {
+    using FromJsonResult = std::tuple<winrt::TerminalApp::IActionArgs, std::vector<::TerminalApp::SettingsLoadWarnings>>;
+
     struct ActionEventArgs : public ActionEventArgsT<ActionEventArgs>
     {
         ActionEventArgs() = default;
@@ -104,15 +107,15 @@ namespace winrt::TerminalApp::implementation
             }
             return false;
         };
-        static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
+        static FromJsonResult FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
             auto args = winrt::make_self<CopyTextArgs>();
             if (auto trimWhitespace{ json[JsonKey(TrimWhitespaceKey)] })
             {
                 args->_TrimWhitespace = trimWhitespace.asBool();
             }
-            return *args;
+            return { *args, {} };
         }
     };
 
@@ -131,12 +134,12 @@ namespace winrt::TerminalApp::implementation
             }
             return false;
         };
-        static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
+        static FromJsonResult FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
             auto args = winrt::make_self<NewTabArgs>();
             args->_TerminalArgs = NewTerminalArgs::FromJson(json);
-            return *args;
+            return { *args, {} };
         }
     };
 
@@ -157,15 +160,15 @@ namespace winrt::TerminalApp::implementation
             }
             return false;
         };
-        static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
+        static FromJsonResult FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
             auto args = winrt::make_self<SwitchToTabArgs>();
             if (auto tabIndex{ json[JsonKey(TabIndexKey)] })
             {
                 args->_TabIndex = tabIndex.asUInt();
             }
-            return *args;
+            return { *args, {} };
         }
     };
 
@@ -221,15 +224,22 @@ namespace winrt::TerminalApp::implementation
             }
             return false;
         };
-        static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
+        static FromJsonResult FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
             auto args = winrt::make_self<ResizePaneArgs>();
             if (auto directionString{ json[JsonKey(DirectionKey)] })
             {
                 args->_Direction = ParseDirection(directionString.asString());
             }
-            return *args;
+            if (args->_Direction == TerminalApp::Direction::None)
+            {
+                return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
+            }
+            else
+            {
+                return { *args, {} };
+            }
         }
     };
 
@@ -250,15 +260,22 @@ namespace winrt::TerminalApp::implementation
             }
             return false;
         };
-        static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
+        static FromJsonResult FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
             auto args = winrt::make_self<MoveFocusArgs>();
             if (auto directionString{ json[JsonKey(DirectionKey)] })
             {
                 args->_Direction = ParseDirection(directionString.asString());
             }
-            return *args;
+            if (args->_Direction == TerminalApp::Direction::None)
+            {
+                return { nullptr, { ::TerminalApp::SettingsLoadWarnings::MissingRequiredParameter } };
+            }
+            else
+            {
+                return { *args, {} };
+            }
         }
     };
 
@@ -279,15 +296,15 @@ namespace winrt::TerminalApp::implementation
             }
             return false;
         };
-        static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
+        static FromJsonResult FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
             auto args = winrt::make_self<AdjustFontSizeArgs>();
             if (auto jsonDelta{ json[JsonKey(AdjustFontSizeDelta)] })
             {
                 args->_Delta = jsonDelta.asInt();
             }
-            return *args;
+            return { *args, {} };
         }
     };
 
@@ -333,7 +350,7 @@ namespace winrt::TerminalApp::implementation
             }
             return false;
         };
-        static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
+        static FromJsonResult FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
             auto args = winrt::make_self<SplitPaneArgs>();
@@ -342,7 +359,7 @@ namespace winrt::TerminalApp::implementation
             {
                 args->_SplitStyle = ParseSplitState(jsonStyle.asString());
             }
-            return *args;
+            return { *args, {} };
         }
     };
 }

diff --git 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);