From 70d44c84c84ec22f2c56fce72c617fea7f9816f4 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 20 Aug 2021 02:21:33 +0200 Subject: [PATCH] Make ActionMap compatible with ScanCode-only KeyChords (#10945) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit partially reverts d465a47 and introduces an alternative approach by adding Hash and Equals methods to the KeyChords class. Those methods will now favor any existing Vkeys over ScanCodes. ## PR Checklist * [x] Closes #10933 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Added a new test, which is ✔️ * Various standard commands still work ✔️ * Hash() returns the same value for all KeyChords that are Equals() ✔️ --- .../KeyBindingsTests.cpp | 12 ++++ src/cascadia/TerminalControl/KeyChord.cpp | 56 +++++++++++++-- src/cascadia/TerminalControl/KeyChord.h | 11 +-- src/cascadia/TerminalControl/KeyChord.idl | 3 + .../TerminalSettingsModel/ActionMap.cpp | 56 +++++++-------- .../TerminalSettingsModel/ActionMap.h | 69 +++---------------- src/cascadia/ut_app/JsonTests.cpp | 13 ++-- 7 files changed, 117 insertions(+), 103 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp index cbdf82fda..946eda0f1 100644 --- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp @@ -58,6 +58,7 @@ namespace SettingsModelLocalTests TEST_METHOD(TestMoveTabArgs); TEST_METHOD(TestGetKeyBindingForAction); + TEST_METHOD(KeybindingsWithoutVkey); TEST_CLASS_SETUP(ClassSetup) { @@ -788,4 +789,15 @@ namespace SettingsModelLocalTests actionMap->LayerJson(bindings2Json); VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); } + + void KeyBindingsTests::KeybindingsWithoutVkey() + { + const auto json = VerifyParseSucceeded(R"!([{"command": "quakeMode", "keys":"shift+sc(255)"}])!"); + + const auto actionMap = winrt::make_self(); + actionMap->LayerJson(json); + + const auto action = actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Shift, 0, 255 }); + VERIFY_IS_NOT_NULL(action); + } } diff --git a/src/cascadia/TerminalControl/KeyChord.cpp b/src/cascadia/TerminalControl/KeyChord.cpp index 04a3af358..9144726e7 100644 --- a/src/cascadia/TerminalControl/KeyChord.cpp +++ b/src/cascadia/TerminalControl/KeyChord.cpp @@ -38,19 +38,67 @@ namespace winrt::Microsoft::Terminal::Control::implementation { _vkey = MapVirtualKeyW(scanCode, MAPVK_VSC_TO_VK_EX); } + + assert(_vkey || _scanCode); } - VirtualKeyModifiers KeyChord::Modifiers() noexcept + uint64_t KeyChord::Hash() const noexcept + { + // Two KeyChords are equal if they have the same modifiers and either identical + // Vkey or ScanCode, with Vkey being preferred. See KeyChord::Equals(). + // This forces us to _either_ hash _vkey or _scanCode. + // + // Additionally the hash value with _vkey==123 and _scanCode==123 must be different. + // --> Taint hashes of KeyChord without _vkey. + auto h = static_cast(_modifiers) << 32; + h |= _vkey ? _vkey : (_scanCode | 0xBABE0000); + + // I didn't like how std::hash uses the byte-wise FNV1a for integers. + // So I built my own std::hash with murmurhash3. + h ^= h >> 33; + h *= UINT64_C(0xff51afd7ed558ccd); + h ^= h >> 33; + h *= UINT64_C(0xc4ceb9fe1a85ec53); + h ^= h >> 33; + + return h; + } + + bool KeyChord::Equals(const Control::KeyChord& other) const noexcept + { + // Two KeyChords are equal if they have the same modifiers and either identical + // Vkey or ScanCode, with Vkey being preferred. Vkey is preferred because: + // ActionMap needs to identify KeyChords which should "layer" (overwrite) each other. + // For instance win+sc(41) and win+` both specify the same KeyChord on an US keyboard layout + // from the perspective of a user. Either of the two should correctly overwrite the other. + // + // Two problems exist here: + // * Since a value of 0 indicates that the Vkey/ScanCode isn't set, we cannot use == to compare them. + // Otherwise we return true, even if the Vkey/ScanCode isn't set on both sides. + // * Whenever Equals() returns true, the Hash() value _must_ be identical. + // For instance the code below ensures the preference of Vkey over ScanCode by: + // this->_vkey || other->_vkey ? ...vkey... : ...scanCode... + // We cannot use "&&", even if it would be technically more correct, as this would mean the + // return value of this function would be dependent on the existence of a Vkey in "other". + // But Hash() has no "other" argument to consider when deciding if its Vkey or ScanCode should be hashed. + // + // Bitwise operators are used because MSVC doesn't support compiling + // boolean operators into bitwise ones at the time of writing. + const auto otherSelf = winrt::get_self(other); + return _modifiers == otherSelf->_modifiers && ((_vkey | otherSelf->_vkey) ? _vkey == otherSelf->_vkey : _scanCode == otherSelf->_scanCode); + } + + VirtualKeyModifiers KeyChord::Modifiers() const noexcept { return _modifiers; } - void KeyChord::Modifiers(VirtualKeyModifiers const& value) noexcept + void KeyChord::Modifiers(const VirtualKeyModifiers value) noexcept { _modifiers = value; } - int32_t KeyChord::Vkey() noexcept + int32_t KeyChord::Vkey() const noexcept { return _vkey; } @@ -60,7 +108,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation _vkey = value; } - int32_t KeyChord::ScanCode() noexcept + int32_t KeyChord::ScanCode() const noexcept { return _scanCode; } diff --git a/src/cascadia/TerminalControl/KeyChord.h b/src/cascadia/TerminalControl/KeyChord.h index 23d42462d..c8348c2fe 100644 --- a/src/cascadia/TerminalControl/KeyChord.h +++ b/src/cascadia/TerminalControl/KeyChord.h @@ -13,11 +13,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation KeyChord(const winrt::Windows::System::VirtualKeyModifiers modifiers, int32_t vkey, int32_t scanCode) noexcept; KeyChord(bool ctrl, bool alt, bool shift, bool win, int32_t vkey, int32_t scanCode) noexcept; - winrt::Windows::System::VirtualKeyModifiers Modifiers() noexcept; - void Modifiers(winrt::Windows::System::VirtualKeyModifiers const& value) noexcept; - int32_t Vkey() noexcept; + uint64_t Hash() const noexcept; + bool Equals(const Control::KeyChord& other) const noexcept; + + winrt::Windows::System::VirtualKeyModifiers Modifiers() const noexcept; + void Modifiers(const winrt::Windows::System::VirtualKeyModifiers value) noexcept; + int32_t Vkey() const noexcept; void Vkey(int32_t value) noexcept; - int32_t ScanCode() noexcept; + int32_t ScanCode() const noexcept; void ScanCode(int32_t value) noexcept; private: diff --git a/src/cascadia/TerminalControl/KeyChord.idl b/src/cascadia/TerminalControl/KeyChord.idl index fb2325ad8..3b8e662ed 100644 --- a/src/cascadia/TerminalControl/KeyChord.idl +++ b/src/cascadia/TerminalControl/KeyChord.idl @@ -10,6 +10,9 @@ namespace Microsoft.Terminal.Control KeyChord(Windows.System.VirtualKeyModifiers modifiers, Int32 vkey, Int32 scanCode); KeyChord(Boolean ctrl, Boolean alt, Boolean shift, Boolean win, Int32 vkey, Int32 scanCode); + UInt64 Hash(); + Boolean Equals(KeyChord other); + Windows.System.VirtualKeyModifiers Modifiers; Int32 Vkey; Int32 ScanCode; diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index a8f1de792..6472245aa 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -124,7 +124,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation ALL_SHORTCUT_ACTIONS #undef ON_ALL_ACTIONS - _AvailableActionsCache = single_threaded_map(std::move(availableActions)); + _AvailableActionsCache = single_threaded_map(std::move(availableActions)); } return _AvailableActionsCache.GetView(); } @@ -173,7 +173,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _PopulateNameMapWithSpecialCommands(nameMap); _PopulateNameMapWithStandardCommands(nameMap); - _NameMapCache = single_threaded_map(std::move(nameMap)); + _NameMapCache = single_threaded_map(std::move(nameMap)); } return _NameMapCache.GetView(); } @@ -281,17 +281,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { if (!_GlobalHotkeysCache) { - std::unordered_map globalHotkeys; - for (const auto& [keys, cmd] : KeyBindings()) - { - // Only populate GlobalHotkeys with actions whose - // ShortcutAction is GlobalSummon or QuakeMode - if (cmd.ActionAndArgs().Action() == ShortcutAction::GlobalSummon || cmd.ActionAndArgs().Action() == ShortcutAction::QuakeMode) - { - globalHotkeys.emplace(keys, cmd); - } - } - _GlobalHotkeysCache = single_threaded_map(std::move(globalHotkeys)); + _RefreshKeyBindingCaches(); } return _GlobalHotkeysCache.GetView(); } @@ -300,16 +290,33 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { if (!_KeyBindingMapCache) { - // populate _KeyBindingMapCache - std::unordered_map keyBindingsMap; - std::unordered_set unboundKeys; - _PopulateKeyBindingMapWithStandardCommands(keyBindingsMap, unboundKeys); - - _KeyBindingMapCache = single_threaded_map(std::move(keyBindingsMap)); + _RefreshKeyBindingCaches(); } return _KeyBindingMapCache.GetView(); } + void ActionMap::_RefreshKeyBindingCaches() + { + std::unordered_map keyBindingsMap; + std::unordered_map globalHotkeys; + std::unordered_set unboundKeys; + + _PopulateKeyBindingMapWithStandardCommands(keyBindingsMap, unboundKeys); + + for (const auto& [keys, cmd] : keyBindingsMap) + { + // Only populate GlobalHotkeys with actions whose + // ShortcutAction is GlobalSummon or QuakeMode + if (cmd.ActionAndArgs().Action() == ShortcutAction::GlobalSummon || cmd.ActionAndArgs().Action() == ShortcutAction::QuakeMode) + { + globalHotkeys.emplace(keys, cmd); + } + } + + _KeyBindingMapCache = single_threaded_map(std::move(keyBindingsMap)); + _GlobalHotkeysCache = single_threaded_map(std::move(globalHotkeys)); + } + // Method Description: // - Populates the provided keyBindingsMap with all of our actions and our parents actions // while omitting the key bindings that were already added before. @@ -317,7 +324,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Arguments: // - keyBindingsMap: the keyBindingsMap we're populating. This maps the key chord of a command to the command itself. // - unboundKeys: a set of keys that are explicitly unbound - void ActionMap::_PopulateKeyBindingMapWithStandardCommands(std::unordered_map& keyBindingsMap, std::unordered_set& unboundKeys) const + void ActionMap::_PopulateKeyBindingMapWithStandardCommands(std::unordered_map& keyBindingsMap, std::unordered_set& unboundKeys) const { // Update KeyBindingsMap with our current layer for (const auto& [keys, actionID] : _KeyMap) @@ -710,15 +717,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - the command with the given key chord // - nullptr if the key chord is explicitly unbound // - nullopt if it was not bound in this layer - std::optional ActionMap::_GetActionByKeyChordInternal(const ActionMapKeyChord keys) const + std::optional ActionMap::_GetActionByKeyChordInternal(const Control::KeyChord& keys) const { - // KeyChord's constructor ensures that Modifiers() & Vkey() is a valid value at a minimum. - // This allows ActionMap to identify KeyChords which should "layer" (overwrite) each other. - // For instance win+sc(41) and win+` both specify the same KeyChord on an US keyboard layout - // from the perspective of a user. Either of the two should correctly overwrite the other. - // As such we need to pretend as if ScanCode doesn't exist. - assert(keys.vkey != 0); - // Check the current layer if (const auto actionIDPair = _KeyMap.find(keys); actionIDPair != _KeyMap.end()) { diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index 51e96963a..8cc314241 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -28,77 +28,23 @@ namespace SettingsModelLocalTests class TerminalSettingsTests; } -namespace winrt::Microsoft::Terminal::Settings::Model::implementation -{ - union ActionMapKeyChord - { - uint16_t value = 0; - struct - { - uint8_t modifiers; - uint8_t vkey; - }; - - constexpr ActionMapKeyChord() = default; - ActionMapKeyChord(const Control::KeyChord& keys) : - modifiers(gsl::narrow_cast(keys.Modifiers())), - vkey(gsl::narrow_cast(keys.Vkey())) - { - } - - constexpr bool operator==(ActionMapKeyChord other) const noexcept - { - return value == other.value; - } - }; -} - -template<> -struct std::hash -{ - constexpr size_t operator()(winrt::Microsoft::Terminal::Settings::Model::implementation::ActionMapKeyChord keys) const noexcept - { - // I didn't like how std::hash uses the byte-wise FNV1a for integers. - // So I built my own std::hash with murmurhash3. -#if SIZE_MAX == UINT32_MAX - size_t h = keys.value; - h ^= h >> 16; - h *= UINT32_C(0x85ebca6b); - h ^= h >> 13; - h *= UINT32_C(0xc2b2ae35); - h ^= h >> 16; - return h; -#elif SIZE_MAX == UINT64_MAX - size_t h = keys.value; - h ^= h >> 33; - h *= UINT64_C(0xff51afd7ed558ccd); - h ^= h >> 33; - h *= UINT64_C(0xc4ceb9fe1a85ec53); - h ^= h >> 33; - return h; -#else - return std::hash{}(keys.value); -#endif - } -}; - namespace winrt::Microsoft::Terminal::Settings::Model::implementation { using InternalActionID = size_t; struct KeyChordHash { - std::size_t operator()(const Control::KeyChord& key) const + inline std::size_t operator()(const Control::KeyChord& key) const { - return ::Microsoft::Terminal::Settings::Model::HashUtils::HashProperty(key.Modifiers(), key.Vkey()); + return static_cast(key.Hash()); } }; struct KeyChordEquality { - bool operator()(const Control::KeyChord& lhs, const Control::KeyChord& rhs) const + inline bool operator()(const Control::KeyChord& lhs, const Control::KeyChord& rhs) const { - return lhs.Modifiers() == rhs.Modifiers() && lhs.Vkey() == rhs.Vkey(); + return lhs.Equals(rhs); } }; @@ -132,12 +78,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: std::optional _GetActionByID(const InternalActionID actionID) const; - std::optional _GetActionByKeyChordInternal(const ActionMapKeyChord keys) const; + std::optional _GetActionByKeyChordInternal(const Control::KeyChord& keys) const; + void _RefreshKeyBindingCaches(); void _PopulateAvailableActionsWithStandardCommands(std::unordered_map& availableActions, std::unordered_set& visitedActionIDs) const; void _PopulateNameMapWithSpecialCommands(std::unordered_map& nameMap) const; void _PopulateNameMapWithStandardCommands(std::unordered_map& nameMap) const; - void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map& keyBindingsMap, std::unordered_set& unboundKeys) const; + void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map& keyBindingsMap, std::unordered_set& unboundKeys) const; std::vector _GetCumulativeActions() const noexcept; void _TryUpdateActionMap(const Model::Command& cmd, Model::Command& oldCmd, Model::Command& consolidatedCmd); @@ -151,7 +98,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::unordered_map _NestedCommands; std::vector _IterableCommands; - std::unordered_map _KeyMap; + std::unordered_map _KeyMap; std::unordered_map _ActionMap; // Masking Actions: diff --git a/src/cascadia/ut_app/JsonTests.cpp b/src/cascadia/ut_app/JsonTests.cpp index f9bc61f6b..0d673ad2d 100644 --- a/src/cascadia/ut_app/JsonTests.cpp +++ b/src/cascadia/ut_app/JsonTests.cpp @@ -36,26 +36,27 @@ namespace TerminalAppUnitTests return true; } - Json::Value VerifyParseSucceeded(std::string content); - void VerifyParseFailed(std::string content); + Json::Value VerifyParseSucceeded(std::string_view content); + void VerifyParseFailed(std::string_view content); private: Json::StreamWriterBuilder _builder; }; - Json::Value JsonTests::VerifyParseSucceeded(std::string content) + Json::Value JsonTests::VerifyParseSucceeded(std::string_view content) { Json::Value root; std::string errs; - const bool parseResult = _reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs); + const bool parseResult = _reader->parse(content.data(), content.data() + content.size(), &root, &errs); VERIFY_IS_TRUE(parseResult, winrt::to_hstring(errs).c_str()); return root; } - void JsonTests::VerifyParseFailed(std::string content) + + void JsonTests::VerifyParseFailed(std::string_view content) { Json::Value root; std::string errs; - const bool parseResult = _reader->parse(content.c_str(), content.c_str() + content.size(), &root, &errs); + const bool parseResult = _reader->parse(content.data(), content.data() + content.size(), &root, &errs); VERIFY_IS_FALSE(parseResult); }