diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 7af541cec..44f9b0d60 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -788,6 +788,7 @@ flyout fmodern fmtarg fmtid +FNV FOLDERID FONTCHANGE fontdlg @@ -1469,6 +1470,7 @@ Mul multiline munged munges +murmurhash mutex mutexes muxes diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp index 4154cf3d4..cbdf82fda 100644 --- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp @@ -109,24 +109,23 @@ namespace SettingsModelLocalTests }, }; - // Use the KeyChordHash and KeyChordEquality to compare if two - // KeyChord's are the same. If you try to just directly compare them - // with VERIFY_ARE_EQUAL, that will always fail. It'll revert to the - // default winrt equality operator, which will compare if they point to - // literally the same object (they won't). - implementation::KeyChordHash hash; - implementation::KeyChordEquality equals; for (const auto& tc : testCases) { Log::Comment(NoThrowString().Format(L"Testing case:\"%s\"", tc.expected.data())); - KeyChord expectedKeyChord{ tc.modifiers, tc.vkey, tc.scanCode }; - const auto actualString = KeyChordSerialization::ToString(expectedKeyChord); + const auto actualString = KeyChordSerialization::ToString({ tc.modifiers, tc.vkey, tc.scanCode }); VERIFY_ARE_EQUAL(tc.expected, actualString); + auto expectedVkey = tc.vkey; + if (!expectedVkey) + { + expectedVkey = MapVirtualKeyW(tc.scanCode, MAPVK_VSC_TO_VK_EX); + } + const auto actualKeyChord = KeyChordSerialization::FromString(actualString); - VERIFY_ARE_EQUAL(hash(expectedKeyChord), hash(actualKeyChord)); - VERIFY_IS_TRUE(equals(expectedKeyChord, actualKeyChord)); + VERIFY_ARE_EQUAL(tc.modifiers, actualKeyChord.Modifiers()); + VERIFY_ARE_EQUAL(expectedVkey, actualKeyChord.Vkey()); + VERIFY_ARE_EQUAL(tc.scanCode, actualKeyChord.ScanCode()); } } diff --git a/src/cascadia/TerminalControl/KeyChord.cpp b/src/cascadia/TerminalControl/KeyChord.cpp index 4dd61d6fa..04a3af358 100644 --- a/src/cascadia/TerminalControl/KeyChord.cpp +++ b/src/cascadia/TerminalControl/KeyChord.cpp @@ -21,9 +21,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } KeyChord::KeyChord(bool ctrl, bool alt, bool shift, bool win, int32_t vkey, int32_t scanCode) noexcept : - _modifiers{ modifiersFromBooleans(ctrl, alt, shift, win) }, - _vkey{ vkey }, - _scanCode{ scanCode } + KeyChord(modifiersFromBooleans(ctrl, alt, shift, win), vkey, scanCode) { } @@ -32,6 +30,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation _vkey{ vkey }, _scanCode{ scanCode } { + // 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. + // We can help ActionMap with this by ensuring that Vkey() is always valid. + if (!_vkey) + { + _vkey = MapVirtualKeyW(scanCode, MAPVK_VSC_TO_VK_EX); + } } VirtualKeyModifiers KeyChord::Modifiers() noexcept diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 9b18354c0..a8f1de792 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -302,7 +302,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { // populate _KeyBindingMapCache std::unordered_map keyBindingsMap; - std::unordered_set unboundKeys; + std::unordered_set unboundKeys; _PopulateKeyBindingMapWithStandardCommands(keyBindingsMap, unboundKeys); _KeyBindingMapCache = single_threaded_map(std::move(keyBindingsMap)); @@ -317,7 +317,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) @@ -683,21 +683,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - false if either the keychord is bound, or not bound at all bool ActionMap::IsKeyChordExplicitlyUnbound(Control::KeyChord const& keys) const { - const auto modifiers = keys.Modifiers(); - - // The "keys" given to us can contain both a Vkey, as well as a ScanCode. - // For instance our UI code fills out a KeyChord with all available information. - // But our _KeyMap only contains KeyChords that contain _either_ a Vkey or ScanCode. - // Due to this we'll have to call _GetActionByKeyChordInternal twice. - if (auto vkey = keys.Vkey()) - { - // We use the fact that the ..Internal call returns nullptr for explicitly unbound - // key chords, and nullopt for keychord that are not bound - it allows us to distinguish - // between unbound and lack of binding. - return nullptr == _GetActionByKeyChordInternal({ modifiers, vkey, 0 }); - } - - return nullptr == _GetActionByKeyChordInternal({ modifiers, 0, keys.ScanCode() }); + // We use the fact that the ..Internal call returns nullptr for explicitly unbound + // key chords, and nullopt for keychord that are not bound - it allows us to distinguish + // between unbound and lack of binding. + return _GetActionByKeyChordInternal(keys) == nullptr; } // Method Description: @@ -709,21 +698,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - nullptr if the key chord doesn't exist Model::Command ActionMap::GetActionByKeyChord(Control::KeyChord const& keys) const { - const auto modifiers = keys.Modifiers(); - - // The "keys" given to us can contain both a Vkey, as well as a ScanCode. - // For instance our UI code fills out a KeyChord with all available information. - // But our _KeyMap only contains KeyChords that contain _either_ a Vkey or ScanCode. - // Due to this we'll have to call _GetActionByKeyChordInternal twice. - if (auto vkey = keys.Vkey()) - { - if (auto command = _GetActionByKeyChordInternal({ modifiers, vkey, 0 })) - { - return *command; - } - } - - return _GetActionByKeyChordInternal({ modifiers, 0, keys.ScanCode() }).value_or(nullptr); + return _GetActionByKeyChordInternal(keys).value_or(nullptr); } // Method Description: @@ -735,8 +710,15 @@ 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(Control::KeyChord const& keys) const + std::optional ActionMap::_GetActionByKeyChordInternal(const ActionMapKeyChord 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 d98d5fce5..51e96963a 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -28,6 +28,60 @@ 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; @@ -36,7 +90,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { std::size_t operator()(const Control::KeyChord& key) const { - return ::Microsoft::Terminal::Settings::Model::HashUtils::HashProperty(key.Modifiers(), key.Vkey(), key.ScanCode()); + return ::Microsoft::Terminal::Settings::Model::HashUtils::HashProperty(key.Modifiers(), key.Vkey()); } }; @@ -44,7 +98,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { bool operator()(const Control::KeyChord& lhs, const Control::KeyChord& rhs) const { - return lhs.Modifiers() == rhs.Modifiers() && lhs.Vkey() == rhs.Vkey() && lhs.ScanCode() == rhs.ScanCode(); + return lhs.Modifiers() == rhs.Modifiers() && lhs.Vkey() == rhs.Vkey(); } }; @@ -78,12 +132,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: std::optional _GetActionByID(const InternalActionID actionID) const; - std::optional _GetActionByKeyChordInternal(const Control::KeyChord& keys) const; + std::optional _GetActionByKeyChordInternal(const ActionMapKeyChord keys) const; 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); @@ -97,7 +151,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/WindowsTerminal/IslandWindow.cpp b/src/cascadia/WindowsTerminal/IslandWindow.cpp index 92dc184c5..731dfa168 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.cpp +++ b/src/cascadia/WindowsTerminal/IslandWindow.cpp @@ -1091,16 +1091,7 @@ void IslandWindow::UnregisterHotKey(const int index) noexcept // - bool IslandWindow::RegisterHotKey(const int index, const winrt::Microsoft::Terminal::Control::KeyChord& hotkey) noexcept { - auto vkey = hotkey.Vkey(); - if (!vkey) - { - vkey = MapVirtualKeyW(hotkey.ScanCode(), MAPVK_VSC_TO_VK); - } - if (!vkey) - { - return false; - } - + const auto vkey = hotkey.Vkey(); auto hotkeyFlags = MOD_NOREPEAT; { const auto modifiers = hotkey.Modifiers(); diff --git a/tools/OpenConsole.psm1 b/tools/OpenConsole.psm1 index 84b85c2dc..bad0afb0d 100644 --- a/tools/OpenConsole.psm1 +++ b/tools/OpenConsole.psm1 @@ -193,11 +193,9 @@ function Invoke-OpenConsoleTests() return } $OpenConsolePlatform = $Platform - $TestHostAppPath = "$root\bin\$OpenConsolePlatform\$Configuration\TestHostApp" if ($Platform -eq 'x86') { $OpenConsolePlatform = 'Win32' - $TestHostAppPath = "$root\$Configuration\TestHostApp" } $OpenConsolePath = "$env:OpenConsoleroot\bin\$OpenConsolePlatform\$Configuration\OpenConsole.exe" $TaefExePath = "$root\packages\Microsoft.Taef.10.60.210621002\build\Binaries\$Platform\te.exe" @@ -236,14 +234,7 @@ function Invoke-OpenConsoleTests() { if ($t.type -eq "unit") { - if ($t.runInHostApp -eq "true") - { - & $TaefExePath "$TestHostAppPath\$($t.binary)" $TaefArgs - } - else - { - & $TaefExePath "$BinDir\$($t.binary)" $TaefArgs - } + & $TaefExePath "$BinDir\$($t.binary)" $TaefArgs } elseif ($t.type -eq "ft") { diff --git a/tools/tests.xml b/tools/tests.xml index fd6444662..c6085b0b4 100644 --- a/tools/tests.xml +++ b/tools/tests.xml @@ -4,8 +4,8 @@ - - + +