From 56d48a2b3cd8a559f46fdd8c43497e965fcc5ce1 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 4 Jun 2021 15:48:30 -0700 Subject: [PATCH 1/5] Ensure equality when hashing default args and no args in actions --- .../KeyBindingsTests.cpp | 10 ++++++++ .../TerminalSettingsModel/ActionMap.cpp | 24 +++++++++++++++++-- 2 files changed, 32 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('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('P') }, kbd); + } } } diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index a9ab094e770..f0c1842d04c 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" @@ -9,6 +10,12 @@ using namespace winrt::Microsoft::Terminal::Settings::Model; using namespace winrt::Microsoft::Terminal::Control; +#define CHECK_IF_ACTION_WITH_ARG(myAction, actionWithArg, result) \ + if (myAction == ShortcutAction::actionWithArg) \ + { \ + result = winrt::make(); \ + } + namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static InternalActionID Hash(const Model::ActionAndArgs& actionAndArgs) @@ -22,8 +29,21 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } else { - std::hash argsHash; - hashedArgs = argsHash(nullptr); + // Check if this action is supposed to have args. + Model::IActionArgs hiddenActionArgs{ nullptr }; +#define ON_ALL_ACTIONS_WITH_ARGS(action) CHECK_IF_ACTION_WITH_ARG(actionAndArgs.Action(), action, hiddenActionArgs) + ALL_SHORTCUT_ACTIONS_WITH_ARGS +#undef ON_ALL_ACTIONS_WITH_ARGS + + if (hiddenActionArgs) + { + hashedArgs = gsl::narrow_cast(hiddenActionArgs.Hash()); + } + else + { + std::hash argsHash; + hashedArgs = argsHash(hiddenActionArgs); + } } return hashedAction ^ hashedArgs; } From 1b6846ef84a86eff6731be8a4e6a109426c6504c Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 7 Jun 2021 11:30:13 -0700 Subject: [PATCH 2/5] use switch-case instead --- .../TerminalSettingsModel/ActionMap.cpp | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index f0c1842d04c..31e4568479b 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -10,12 +10,6 @@ using namespace winrt::Microsoft::Terminal::Settings::Model; using namespace winrt::Microsoft::Terminal::Control; -#define CHECK_IF_ACTION_WITH_ARG(myAction, actionWithArg, result) \ - if (myAction == ShortcutAction::actionWithArg) \ - { \ - result = winrt::make(); \ - } - namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static InternalActionID Hash(const Model::ActionAndArgs& actionAndArgs) @@ -25,24 +19,30 @@ 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(args.Hash()); } else { - // Check if this action is supposed to have args. - Model::IActionArgs hiddenActionArgs{ nullptr }; -#define ON_ALL_ACTIONS_WITH_ARGS(action) CHECK_IF_ACTION_WITH_ARG(actionAndArgs.Action(), action, hiddenActionArgs) - ALL_SHORTCUT_ACTIONS_WITH_ARGS -#undef ON_ALL_ACTIONS_WITH_ARGS - - if (hiddenActionArgs) + // Args are not defined. + // Check if the ShortcutAction supports args. + switch (actionAndArgs.Action()) { - hashedArgs = gsl::narrow_cast(hiddenActionArgs.Hash()); - } - else +#define ON_ALL_ACTIONS_WITH_ARGS(action) \ + case ShortcutAction::action: \ + { \ + /* If it does, hash the default values for the args.*/ \ + hashedArgs = gsl::narrow_cast(make().Hash()); \ + break; \ + } + ALL_SHORTCUT_ACTIONS_WITH_ARGS +#undef ON_ALL_ACTIONS_WITH_ARGS + default: { + // Otherwise, hash nullptr. std::hash argsHash; - hashedArgs = argsHash(hiddenActionArgs); + hashedArgs = argsHash(nullptr); + } } } return hashedAction ^ hashedArgs; From 96a08ba1348ea1436c088f405df2d4af144c211c Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Mon, 7 Jun 2021 12:53:38 -0700 Subject: [PATCH 3/5] Remove unnecessary braces from cases --- src/cascadia/TerminalSettingsModel/ActionMap.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 31e4568479b..15dcf46e1ae 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -30,11 +30,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { #define ON_ALL_ACTIONS_WITH_ARGS(action) \ case ShortcutAction::action: \ - { \ /* If it does, hash the default values for the args.*/ \ hashedArgs = gsl::narrow_cast(make().Hash()); \ - break; \ - } + break; ALL_SHORTCUT_ACTIONS_WITH_ARGS #undef ON_ALL_ACTIONS_WITH_ARGS default: From d83a764899d308f3028756081581d942d9badfce Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 10 Jun 2021 13:37:44 -0700 Subject: [PATCH 4/5] cache the special hash value --- src/cascadia/TerminalSettingsModel/ActionArgs.h | 13 +++++++++++++ src/cascadia/TerminalSettingsModel/ActionMap.cpp | 8 ++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.h b/src/cascadia/TerminalSettingsModel/ActionArgs.h index 6cf7d8cb53a..40d178b7df1 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.h +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.h @@ -68,6 +68,19 @@ constexpr size_t Microsoft::Terminal::Settings::Model::HashUtils::HashProperty(c return gsl::narrow_cast(args.Hash()); } +// Retrieves the hash value for an empty-constructed object. +template +static size_t EmptyHash() +{ + // cache the value of the empty hash + static size_t cachedHash{ 0 }; + if (!cachedHash) + { + cachedHash = { winrt::make_self()->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 15dcf46e1ae..5decbab3d76 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -28,10 +28,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // 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 = gsl::narrow_cast(make().Hash()); \ +#define ON_ALL_ACTIONS_WITH_ARGS(action) \ + case ShortcutAction::action: \ + /* If it does, hash the default values for the args.*/ \ + hashedArgs = EmptyHash(); \ break; ALL_SHORTCUT_ACTIONS_WITH_ARGS #undef ON_ALL_ACTIONS_WITH_ARGS From 160b17ab7574693ae17bc8a7622526a08ea45394 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 10 Jun 2021 19:00:45 -0400 Subject: [PATCH 5/5] Use magic static for EmptyHash Co-authored-by: Dustin L. Howett --- src/cascadia/TerminalSettingsModel/ActionArgs.h | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.h b/src/cascadia/TerminalSettingsModel/ActionArgs.h index 40d178b7df1..280ac9ac101 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.h +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.h @@ -73,11 +73,7 @@ template static size_t EmptyHash() { // cache the value of the empty hash - static size_t cachedHash{ 0 }; - if (!cachedHash) - { - cachedHash = { winrt::make_self()->Hash() }; - } + static const size_t cachedHash = winrt::make_self()->Hash(); return cachedHash; }