Re-add keybindings to new tab dropdown (#3618)

## Summary of the Pull Request With #3391, I almost certainly regressed the ability for the new tab dropdown to display the keybindings for each profile. This adds them back. ## PR Checklist * [x] Closes #3603 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments Now, we can lookup keybindings not only for `ShortcutAction`s, but also `ActionAndArgs`s, so we can look up the binding for an action with a particular set of arguments. --------------------------------------------- * fixes #3603 by searching for ActionAndArgs too * Apply suggestions from code review Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>
microsoft · Nov 21, 2019 · 71debc1 · 71debc1
1 parent b3ecb73
commit 71debc1
Show file tree

Hide file tree

Showing 7 changed files with 104 additions and 9 deletions.
diff --git a/src/cascadia/TerminalApp/ActionArgs.h b/src/cascadia/TerminalApp/ActionArgs.h
@@ -41,6 +41,15 @@ namespace winrt::TerminalApp::implementation
         static constexpr std::string_view TrimWhitespaceKey{ "trimWhitespace" };
 
     public:
+        bool Equals(const IActionArgs& other)
+        {
+            auto otherAsUs = other.try_as<CopyTextArgs>();
+            if (otherAsUs)
+            {
+                return otherAsUs->_TrimWhitespace == _TrimWhitespace;
+            }
+            return false;
+        };
         static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
@@ -61,6 +70,15 @@ namespace winrt::TerminalApp::implementation
         static constexpr std::string_view ProfileIndexKey{ "index" };
 
     public:
+        bool Equals(const IActionArgs& other)
+        {
+            auto otherAsUs = other.try_as<NewTabArgs>();
+            if (otherAsUs)
+            {
+                return otherAsUs->_ProfileIndex == _ProfileIndex;
+            }
+            return false;
+        };
         static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
@@ -81,6 +99,15 @@ namespace winrt::TerminalApp::implementation
         static constexpr std::string_view TabIndexKey{ "index" };
 
     public:
+        bool Equals(const IActionArgs& other)
+        {
+            auto otherAsUs = other.try_as<SwitchToTabArgs>();
+            if (otherAsUs)
+            {
+                return otherAsUs->_TabIndex == _TabIndex;
+            }
+            return false;
+        };
         static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
@@ -136,6 +163,15 @@ namespace winrt::TerminalApp::implementation
         static constexpr std::string_view DirectionKey{ "direction" };
 
     public:
+        bool Equals(const IActionArgs& other)
+        {
+            auto otherAsUs = other.try_as<ResizePaneArgs>();
+            if (otherAsUs)
+            {
+                return otherAsUs->_Direction == _Direction;
+            }
+            return false;
+        };
         static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
@@ -156,6 +192,15 @@ namespace winrt::TerminalApp::implementation
         static constexpr std::string_view DirectionKey{ "direction" };
 
     public:
+        bool Equals(const IActionArgs& other)
+        {
+            auto otherAsUs = other.try_as<MoveFocusArgs>();
+            if (otherAsUs)
+            {
+                return otherAsUs->_Direction == _Direction;
+            }
+            return false;
+        };
         static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!
@@ -176,6 +221,15 @@ namespace winrt::TerminalApp::implementation
         static constexpr std::string_view AdjustFontSizeDelta{ "delta" };
 
     public:
+        bool Equals(const IActionArgs& other)
+        {
+            auto otherAsUs = other.try_as<AdjustFontSizeArgs>();
+            if (otherAsUs)
+            {
+                return otherAsUs->_Delta == _Delta;
+            }
+            return false;
+        };
         static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
         {
             // LOAD BEARING: Not using make_self here _will_ break you in the future!

diff --git a/src/cascadia/TerminalApp/ActionArgs.idl b/src/cascadia/TerminalApp/ActionArgs.idl
@@ -3,9 +3,10 @@
 
 namespace TerminalApp
 {
-    // An empty interface must specify an explicit [uuid] to ensure uniqueness.
-    // We also manually have to specify a "version" attribute to make the compiler happy.
-    [uuid("191C2BDE-1A60-4BAB-9765-D850F0EF2CAC")][version(1)] interface IActionArgs{};
+    interface IActionArgs
+    {
+        Boolean Equals(IActionArgs other);
+    };
 
     interface IActionEventArgs
     {

diff --git a/src/cascadia/TerminalApp/AppKeyBindings.cpp b/src/cascadia/TerminalApp/AppKeyBindings.cpp
@@ -9,6 +9,7 @@
 
 using namespace winrt::Microsoft::Terminal;
 using namespace winrt::TerminalApp;
+using namespace winrt::Microsoft::Terminal::Settings;
 
 namespace winrt::TerminalApp::implementation
 {
@@ -29,7 +30,7 @@ namespace winrt::TerminalApp::implementation
         _keyShortcuts.erase(chord);
     }
 
-    Microsoft::Terminal::Settings::KeyChord AppKeyBindings::GetKeyBinding(TerminalApp::ShortcutAction const& action)
+    KeyChord AppKeyBindings::GetKeyBindingForAction(TerminalApp::ShortcutAction const& action)
     {
         for (auto& kv : _keyShortcuts)
         {
@@ -41,6 +42,28 @@ namespace winrt::TerminalApp::implementation
         return { nullptr };
     }
 
+    // Method Description:
+    // - Lookup the keychord bound to a particular combination of ShortcutAction
+    //   and IActionArgs. This enables searching no only for the binding of a
+    //   particular ShortcutAction, but also a particular set of values for
+    //   arguments to that action.
+    // Arguments:
+    // - actionAndArgs: The ActionAndArgs to lookup the keybinding for.
+    // Return Value:
+    // - The bound keychord, if this ActionAndArgs is bound to a key, otherwise nullptr.
+    KeyChord AppKeyBindings::GetKeyBindingForActionWithArgs(TerminalApp::ActionAndArgs const& actionAndArgs)
+    {
+        for (auto& kv : _keyShortcuts)
+        {
+            if (kv.second.Action() == actionAndArgs.Action() &&
+                kv.second.Args().Equals(actionAndArgs.Args()))
+            {
+                return kv.first;
+            }
+        }
+        return { nullptr };
+    }
+
     bool AppKeyBindings::TryKeyChord(const Settings::KeyChord& kc)
     {
         const auto keyIter = _keyShortcuts.find(kc);

diff --git a/src/cascadia/TerminalApp/AppKeyBindings.h b/src/cascadia/TerminalApp/AppKeyBindings.h
@@ -45,7 +45,8 @@ namespace winrt::TerminalApp::implementation
         void SetKeyBinding(TerminalApp::ActionAndArgs const& actionAndArgs,
                            winrt::Microsoft::Terminal::Settings::KeyChord const& chord);
         void ClearKeyBinding(winrt::Microsoft::Terminal::Settings::KeyChord const& chord);
-        Microsoft::Terminal::Settings::KeyChord GetKeyBinding(TerminalApp::ShortcutAction const& action);
+        Microsoft::Terminal::Settings::KeyChord GetKeyBindingForAction(TerminalApp::ShortcutAction const& action);
+        Microsoft::Terminal::Settings::KeyChord GetKeyBindingForActionWithArgs(TerminalApp::ActionAndArgs const& actionAndArgs);
 
         static Windows::System::VirtualKeyModifiers ConvertVKModifiers(winrt::Microsoft::Terminal::Settings::KeyModifiers modifiers);
 

diff --git a/src/cascadia/TerminalApp/AppKeyBindings.idl b/src/cascadia/TerminalApp/AppKeyBindings.idl
@@ -76,7 +76,8 @@ namespace TerminalApp
         void SetKeyBinding(ActionAndArgs actionAndArgs, Microsoft.Terminal.Settings.KeyChord chord);
         void ClearKeyBinding(Microsoft.Terminal.Settings.KeyChord chord);
 
-        Microsoft.Terminal.Settings.KeyChord GetKeyBinding(ShortcutAction action);
+        Microsoft.Terminal.Settings.KeyChord GetKeyBindingForAction(ShortcutAction action);
+        Microsoft.Terminal.Settings.KeyChord GetKeyBindingForActionWithArgs(ActionAndArgs actionAndArgs);
 
         event Windows.Foundation.TypedEventHandler<AppKeyBindings, ActionEventArgs> CopyText;
         event Windows.Foundation.TypedEventHandler<AppKeyBindings, ActionEventArgs> PasteText;

diff --git a/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp b/src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp
@@ -354,7 +354,7 @@ Json::Value winrt::TerminalApp::implementation::AppKeyBindings::ToJson()
         const auto searchedForName = actionName.first;
         const auto searchedForAction = actionName.second;
 
-        if (const auto chord{ GetKeyBinding(searchedForAction) })
+        if (const auto chord{ GetKeyBindingForAction(searchedForAction) })
         {
             if (const auto serialization{ _ShortcutAsJsonObject(chord, searchedForName) })
             {

diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -3,6 +3,7 @@
 
 #include "pch.h"
 #include "TerminalPage.h"
+#include "ActionAndArgs.h"
 #include "Utils.h"
 
 #include <LibraryResources.h>
@@ -253,7 +254,21 @@ namespace winrt::TerminalApp::implementation
             {
                 // enum value for ShortcutAction::NewTabProfileX; 0==NewTabProfile0
                 const auto action = static_cast<ShortcutAction>(profileIndex + static_cast<int>(ShortcutAction::NewTabProfile0));
-                auto profileKeyChord = keyBindings.GetKeyBinding(action);
+                // First, attempt to search for the keybinding for the simple
+                // NewTabProfile0-9 ShortcutActions.
+                auto profileKeyChord = keyBindings.GetKeyBindingForAction(action);
+                if (!profileKeyChord)
+                {
+                    // If NewTabProfileN didn't have a binding, look for a
+                    // keychord that is bound to the equivalent
+                    // NewTab(ProfileIndex=N) action
+                    auto actionAndArgs = winrt::make_self<winrt::TerminalApp::implementation::ActionAndArgs>();
+                    actionAndArgs->Action(ShortcutAction::NewTab);
+                    auto newTabArgs = winrt::make_self<winrt::TerminalApp::implementation::NewTabArgs>();
+                    newTabArgs->ProfileIndex(profileIndex);
+                    actionAndArgs->Args(*newTabArgs);
+                    profileKeyChord = keyBindings.GetKeyBindingForActionWithArgs(*actionAndArgs);
+                }
 
                 // make sure we find one to display
                 if (profileKeyChord)
@@ -306,7 +321,7 @@ namespace winrt::TerminalApp::implementation
             settingsItem.Click({ this, &TerminalPage::_SettingsButtonOnClick });
             newTabFlyout.Items().Append(settingsItem);
 
-            auto settingsKeyChord = keyBindings.GetKeyBinding(ShortcutAction::OpenSettings);
+            auto settingsKeyChord = keyBindings.GetKeyBindingForAction(ShortcutAction::OpenSettings);
             if (settingsKeyChord)
             {
                 _SetAcceleratorForMenuItem(settingsItem, settingsKeyChord);