From b3b648496e92d14e88bbede1b2c0b63e9d56b599 Mon Sep 17 00:00:00 2001
From: Carlos Zamora <carlos.zamora@microsoft.com>
Date: Wed, 16 Jun 2021 15:43:29 -0700
Subject: [PATCH] Ensure equality when hashing default args and no args in
 actions (#10341)

## Summary of the Pull Request
#10297 found a bug in `ActionMap` where the `ToggleCommandPalette` key chord could not be found using `GetKeyBindingForAction`.

This was caused by the following:
- `AddAction`: when adding the action, the `ActionAndArgs` is basically added as `{ToggleCommandPalette, ToggleCommandLineArgs{}}` (Note the default ctor used for the action args)
- `GetKeyBindingForAction`: we're searching for an `ActionAndArgs` structured as `{ToggleCommandPalette, nullptr}`
- Since these are _technically_ two different actions, we are unable to find it.

This issue was fixed by making the `Hash(ActionAndArgs)` function smarter! If the `ActionAndArgs` has no args, but the `ShortcutAction` _supports_ args, generate the args using the default ctor.

By making `Hash()` smarter, everybody benefits from this logic! We can basically now enforce that `ActionAndArgs{ <X>, nullptr } == ActionAndArgs{ <X>, <default_ctor> }`.

## Validation Steps Performed
- Added a test.
- Tested this on #10297's branch and this does fix the bug
---
 .../KeyBindingsTests.cpp                      | 10 +++++++++
 .../TerminalSettingsModel/ActionArgs.h        |  9 ++++++++
 .../TerminalSettingsModel/ActionMap.cpp       | 22 +++++++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
index 35fde481bea..bc0a21f72d3 100644
--- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp
@@ -647,10 +647,12 @@ namespace SettingsModelLocalTests
         const std::string bindings0String{ R"([ { "command": "closeWindow", "keys": "ctrl+a" } ])" };
         const std::string bindings1String{ R"([ { "command": { "action": "copy", "singleLine": true }, "keys": "ctrl+b" } ])" };
         const std::string bindings2String{ R"([ { "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+c" } ])" };
+        const std::string bindings3String{ R"([ { "command": "commandPalette", "keys": "ctrl+shift+p" } ])" };
 
         const auto bindings0Json = VerifyParseSucceeded(bindings0String);
         const auto bindings1Json = VerifyParseSucceeded(bindings1String);
         const auto bindings2Json = VerifyParseSucceeded(bindings2String);
+        const auto bindings3Json = VerifyParseSucceeded(bindings3String);
 
         auto VerifyKeyChordEquality = [](const KeyChord& expected, const KeyChord& actual) {
             if (expected)
@@ -699,5 +701,13 @@ namespace SettingsModelLocalTests
             const auto& kbd{ actionMap->GetKeyBindingForAction(ShortcutAction::NewTab, *args) };
             VerifyKeyChordEquality({ KeyModifiers::Ctrl, static_cast<int32_t>('C') }, kbd);
         }
+        {
+            Log::Comment(L"command with hidden args");
+            actionMap->LayerJson(bindings3Json);
+            VERIFY_ARE_EQUAL(4u, actionMap->_KeyMap.size());
+
+            const auto& kbd{ actionMap->GetKeyBindingForAction(ShortcutAction::ToggleCommandPalette) };
+            VerifyKeyChordEquality({ KeyModifiers::Ctrl | KeyModifiers::Shift, static_cast<int32_t>('P') }, kbd);
+        }
     }
 }
diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.h b/src/cascadia/TerminalSettingsModel/ActionArgs.h
index 6cf7d8cb53a..280ac9ac101 100644
--- a/src/cascadia/TerminalSettingsModel/ActionArgs.h
+++ b/src/cascadia/TerminalSettingsModel/ActionArgs.h
@@ -68,6 +68,15 @@ constexpr size_t Microsoft::Terminal::Settings::Model::HashUtils::HashProperty(c
     return gsl::narrow_cast<size_t>(args.Hash());
 }
 
+// Retrieves the hash value for an empty-constructed object.
+template<typename T>
+static size_t EmptyHash()
+{
+    // cache the value of the empty hash
+    static const size_t cachedHash = winrt::make_self<T>()->Hash();
+    return cachedHash;
+}
+
 namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 {
     using namespace ::Microsoft::Terminal::Settings::Model;
diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp
index a9ab094e770..5decbab3d76 100644
--- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp
+++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp
@@ -2,6 +2,7 @@
 // Licensed under the MIT license.
 
 #include "pch.h"
+#include "AllShortcutActions.h"
 #include "ActionMap.h"
 
 #include "ActionMap.g.cpp"
@@ -18,12 +19,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         size_t hashedArgs{};
         if (const auto& args{ actionAndArgs.Args() })
         {
+            // Args are defined, so hash them
             hashedArgs = gsl::narrow_cast<size_t>(args.Hash());
         }
         else
         {
-            std::hash<IActionArgs> argsHash;
-            hashedArgs = argsHash(nullptr);
+            // Args are not defined.
+            // Check if the ShortcutAction supports args.
+            switch (actionAndArgs.Action())
+            {
+#define ON_ALL_ACTIONS_WITH_ARGS(action)                        \
+    case ShortcutAction::action:                                \
+        /* If it does, hash the default values for the args.*/  \
+        hashedArgs = EmptyHash<implementation::action##Args>(); \
+        break;
+                ALL_SHORTCUT_ACTIONS_WITH_ARGS
+#undef ON_ALL_ACTIONS_WITH_ARGS
+            default:
+            {
+                // Otherwise, hash nullptr.
+                std::hash<IActionArgs> argsHash;
+                hashedArgs = argsHash(nullptr);
+            }
+            }
         }
         return hashedAction ^ hashedArgs;
     }