Fix layering of sc() keybindings with vk() ones (#10917)

The quake mode keybinding is bound to a scancode. This made it impossible to override it with a vkey-based one like "win+\`". This commit fixes the issue by making sure that a `KeyChord` always has a vkey, and leveraging this fact inside ActionMap, which now ignores the scan-code. ## PR Checklist * [x] Closes #10875 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * quake mode and other keybinding still work ✔️ * Repro settings from #10875 work correctly ✔️
2021-08-12 01:09:25 +02:00 · 2021-08-12 01:09:25 +02:00 · d465a47bc5
parent 9c858cd5b8
commit d465a47bc5
8 changed files with 99 additions and 74 deletions
--- 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
--- 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());
        }
    }

--- 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
--- 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<KeyChord, Model::Command, KeyChordHash, KeyChordEquality> keyBindingsMap;
-            std::unordered_set<KeyChord, KeyChordHash, KeyChordEquality> unboundKeys;
+            std::unordered_set<ActionMapKeyChord> unboundKeys;
            _PopulateKeyBindingMapWithStandardCommands(keyBindingsMap, unboundKeys);

            _KeyBindingMapCache = single_threaded_map<KeyChord, Model::Command>(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<KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<Control::KeyChord, KeyChordHash, KeyChordEquality>& unboundKeys) const
+    void ActionMap::_PopulateKeyBindingMapWithStandardCommands(std::unordered_map<KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<ActionMapKeyChord>& 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<Model::Command> ActionMap::_GetActionByKeyChordInternal(Control::KeyChord const& keys) const
+    std::optional<Model::Command> 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())
        {
--- 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<uint8_t>(keys.Modifiers())),
+            vkey(gsl::narrow_cast<uint8_t>(keys.Vkey()))
+        {
+        }
+
+        constexpr bool operator==(ActionMapKeyChord other) const noexcept
+        {
+            return value == other.value;
+        }
+    };
+}
+
+template<>
+struct std::hash<winrt::Microsoft::Terminal::Settings::Model::implementation::ActionMapKeyChord>
+{
+    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<uint16_t>{}(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<Model::Command> _GetActionByID(const InternalActionID actionID) const;
-        std::optional<Model::Command> _GetActionByKeyChordInternal(const Control::KeyChord& keys) const;
+        std::optional<Model::Command> _GetActionByKeyChordInternal(const ActionMapKeyChord keys) const;

        void _PopulateAvailableActionsWithStandardCommands(std::unordered_map<hstring, Model::ActionAndArgs>& availableActions, std::unordered_set<InternalActionID>& visitedActionIDs) const;
        void _PopulateNameMapWithSpecialCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
        void _PopulateNameMapWithStandardCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
-        void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<Control::KeyChord, KeyChordHash, KeyChordEquality>& unboundKeys) const;
+        void _PopulateKeyBindingMapWithStandardCommands(std::unordered_map<Control::KeyChord, Model::Command, KeyChordHash, KeyChordEquality>& keyBindingsMap, std::unordered_set<ActionMapKeyChord>& unboundKeys) const;
        std::vector<Model::Command> _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<winrt::hstring, Model::Command> _NestedCommands;
        std::vector<Model::Command> _IterableCommands;
-        std::unordered_map<Control::KeyChord, InternalActionID, KeyChordHash, KeyChordEquality> _KeyMap;
+        std::unordered_map<ActionMapKeyChord, InternalActionID> _KeyMap;
        std::unordered_map<InternalActionID, Model::Command> _ActionMap;

        // Masking Actions:
--- a/src/cascadia/WindowsTerminal/IslandWindow.cpp
+++ b/src/cascadia/WindowsTerminal/IslandWindow.cpp
@ -1091,16 +1091,7 @@ void IslandWindow::UnregisterHotKey(const int index) noexcept
 // - <none>
 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();
--- 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")
        {
--- a/tools/tests.xml
+++ b/tools/tests.xml
@ -4,8 +4,8 @@
  <test name="textBuffer" type="unit" binary="TextBuffer.Unit.Tests.dll" />
  <test name="terminalCore" type="unit" binary="UnitTests_TerminalCore\Terminal.Core.Unit.Tests.dll" />
  <test name="terminalApp" type="unit" binary="UnitTests_TerminalApp\Terminal.App.Unit.Tests.dll" />
-  <test name="localTerminalApp" type="unit" runInHostApp="true" binary="TerminalApp.LocalTests.dll" />
-  <test name="localSettingsModel" type="unit" runInHostApp="true" binary="SettingsModel.LocalTests.dll" />
+  <test name="localTerminalApp" type="unit" binary="TestHostApp\TerminalApp.LocalTests.dll" />
+  <test name="localSettingsModel" type="unit" binary="TestHostApp\SettingsModel.LocalTests.dll" />
  <test name="unitRemoting" type="unit" binary="UnitTests_Remoting\Remoting.Unit.Tests.dll" />
  <test name="unitControl" type="unit" binary="UnitTests_Control\Control.Unit.Tests.dll" />
  <test name="interactivityWin32" type="unit" binary="Conhost.Interactivity.Win32.Unit.Tests.dll" />