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); }