Introduce vk() and sc() key chord specifiers by lhecker · Pull Request #10666 · microsoft/terminal

lhecker · 2021-07-15T01:14:59Z

This commit introduces an alternative to specifying key bindings as a combination of key modifiers and a character. It allows you to specify an explicit virtual key as vk(nnn).
Additionally this commit makes it possible to bind actions to scan codes. As scan code 41 appears to be the button below the Escape key on virtually all keyboards, we'll be able to bind the quake mode hotkey to win+sc(41) and have it work consistently across most if not all keyboard layouts.

PR Checklist

Closes Default bound key combinations are not working as expected on international keyboard layouts #7539, Closes Make Quake mode work with a German keyboard layout #10203
I work here
Tests added/passed

Validation Steps Performed

The following was tested both on US and DE keyboard layouts:

Ctrl+, opens settings ✔️
Win+` opens quake mode window ✔️
Ctrl+plus/minus increase/decrease font size ✔️

lhecker · 2021-07-15T01:16:28Z

src/cascadia/TerminalControl/KeyChord.idl

+        KeyChord(Windows.System.VirtualKeyModifiers modifiers, Int32 vkey, Int32 scanCode);
+        KeyChord(Boolean ctrl, Boolean alt, Boolean shift, Boolean win, Int32 vkey, Int32 scanCode);


I didn't want to add a 5th constructor, so I removed 1 instead.

lhecker · 2021-07-15T01:18:24Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp


-        // This key chord is not explicitly bound
-        return nullptr;
+        return _GetActionByKeyChordInternal({ modifiers, 0, keys.ScanCode() }).value_or(nullptr);


We'll now have to run _GetActionByKeyChordInternal twice, since the given keys might have both a vkey as well as a scan code set, but the internal hashmaps have only key bindings loaded that contain either of the two.

I used to have a comment explaining this here, but I must've lost it at some point. I'll re-add one.

src/cascadia/TerminalSettingsModel/KeyChordSerialization.cpp

lhecker · 2021-07-15T01:20:07Z

src/cascadia/TerminalSettingsModel/defaults-universal.json

+        { "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+plus" },
+        { "command": { "action": "adjustFontSize", "delta": -1 }, "keys": "ctrl+minus" },


This fixes zooming for non-US keyboards.

lhecker · 2021-07-15T01:20:13Z

src/cascadia/TerminalSettingsModel/defaults.json

        { "command": "identifyWindow" },
        { "command": "openWindowRenamer" },
-        { "command": "quakeMode", "keys":"win+`" },
+        { "command": "quakeMode", "keys":"win+sc(41)" },


This fixes quake mode for non-US keyboards.

lhecker · 2021-07-15T01:21:04Z

src/cascadia/WindowsTerminal/AppHost.cpp

+    for (const auto& [keyChord, cmd] : _logic.GlobalHotkeys())
    {
-        if (k != nullptr)
+        if (auto summonArgs = cmd.ActionAndArgs().Args().try_as<Settings::Model::GlobalSummonArgs>())


By casting the cmd prematurely, we won't have to cast it again in AppHost::_GlobalHotkeyPressed.
Additionally we won't have to hold onto the _hotkeyActions map.

lhecker · 2021-07-15T01:24:00Z

src/cascadia/WindowsTerminal/AppHost.cpp

-            args.SummonBehavior().MoveToCurrentDesktop(summonArgs.Desktop() == Settings::Model::DesktopBehavior::ToCurrent);
-            args.SummonBehavior().ToggleVisibility(summonArgs.ToggleVisibility());
-            args.SummonBehavior().DropdownDuration(summonArgs.DropdownDuration());
+    const auto& summonArgs = til::at(_hotkeys, hotkeyIndex);


As you can see here, a lot of casting is gone without the _hotkeyActions map.
Otherwise this code is still the same - just less indented.
(Suppressing white-space changes in GitHub makes this PR less scary.)

lhecker · 2021-07-15T01:25:00Z

src/cascadia/WindowsTerminal/AppHost.h

    winrt::com_ptr<IVirtualDesktopManager> _desktopManager{ nullptr };

+    bool _shouldCreateWindow{ false };
+    bool _useNonClientArea{ false };


_useNonClientArea wasn't initialized during construction. I moved them down so this class uses less padding.

lhecker · 2021-07-15T01:31:32Z

tools/GenerateHeaderForJson.ps1

+    "constexpr std::string_view $($VariableName){",
+    ($jsonData | ForEach-Object { "R`"#($_`n)#`"" }),
+    "};"
+) | Out-File -FilePath $OutPath -Encoding utf8


I had to add support for "( and )" escaping (now it uses R"#(...)#" instead), and I thought I could clean this file up a bit. 👍

lhecker · 2021-07-15T13:01:06Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

 // Return Value:
 // - <none>
-void IslandWindow::SetGlobalHotkeys(const std::vector<winrt::Microsoft::Terminal::Control::KeyChord>& hotkeyList)
+void IslandWindow::RegisterHotKey(const int index, const winrt::Microsoft::Terminal::Control::KeyChord& hotkey) noexcept


By passing the hotkey not as a vector but rather one by one, we can get rid of the std::vector of key chords in the AppHost.

zadjii-msft

meh, these are all nits, but the PR is still in draft, so I'll hold off ✔️-ing until the final version

src/cascadia/WindowsTerminal/IslandWindow.h

src/cascadia/TerminalSettingsModel/ActionMap.cpp

zadjii-msft · 2021-07-15T14:29:41Z

src/cascadia/TerminalSettingsModel/ActionMap.cpp

-        if (cmd.has_value())
+        const auto modifiers = keys.Modifiers();
+
+        if (auto vkey = keys.Vkey())


would it be tremendously dumb to oneline this as

return _GetActionByKeyChordInternal({ modifiers, keys.Vkey(), 0 }).value_or( _GetActionByKeyChordInternal({ modifiers, 0, keys.ScanCode() }).value_or(nullptr));

It would certainly be nice! But it would also eagerly check both variants, before returning either, and I'm not sure if that's a good idea...

KalleOlaviNiemitalo · 2021-07-15T15:27:15Z

Should this be added to profiles.schema.json too?

lhecker · 2021-07-15T15:49:00Z

@KalleOlaviNiemitalo Yeah I can do that. 👍
The space in the description field is rather limited though, so I've planned to add the actual, exhaustive, description to the online docs only.

KalleOlaviNiemitalo · 2021-07-15T21:00:17Z

Rather than the description, I meant this regular expression

terminal/doc/cascadia/profiles.schema.json

Line 7 in 84e30bc

    
           "pattern": "^(?<modifier>(?<mod1>ctrl|alt|shift|win)(?:\\+(?<mod2>ctrl|alt|shift|win)(?<!\\k<mod1>))?(?:\\+(?<mod3>ctrl|alt|shift|win)(?<!\\k<mod1>|\\k<mod2>))?(?:\\+(?<mod4>ctrl|alt|shift|win)(?<!\\k<mod1>|\\k<mod2>|\\k<mod3>))?\\+)?(?<key>[^\\s+]|app|menu|backspace|tab|enter|esc|escape|space|pgup|pageup|pgdn|pagedown|end|home|left|up|right|down|insert|delete|(?<!shift.+)(?:numpad_?[0-9]|numpad_(?:period|decimal))|numpad_(?:multiply|plus|add|minus|subtract|divide)|f[1-9]|f1[0-9]|f2[0-4]|plus)$",

lhecker · 2021-07-15T22:15:54Z

@KalleOlaviNiemitalo Oh wow that's a rather... comprehensive regex. 😄
I'm not sure if it's worth keeping it that complex, since it would get out of sync every time the VKEY_NAME_PAIRS list is changed in the code. (And it uses backtracking...)

For now I'll just remove the backtracking from the regex and update it with the 5 new key chord types.

lhecker

~~Remaining TODOs:~~

~~Add a comment to ActionMap::GetActionByKeyChord~~ ✔️
~~Add a unit test for vk(nnn) / sc(nnn) parsing~~ I can't run LocalTests_SettingsModel tests. 😟

The remaining implementation in this PR is "ready" from my side though.

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp

src/cascadia/TerminalSettingsModel/ActionMap.cpp

zadjii-msft · 2021-07-19T14:54:02Z

doc/cascadia/profiles.schema.json

  "title": "Microsoft's Windows Terminal Settings Profile Schema",
  "definitions": {
    "KeyChordSegment": {
-      "pattern": "^(?<modifier>(?<mod1>ctrl|alt|shift|win)(?:\\+(?<mod2>ctrl|alt|shift|win)(?<!\\k<mod1>))?(?:\\+(?<mod3>ctrl|alt|shift|win)(?<!\\k<mod1>|\\k<mod2>))?(?:\\+(?<mod4>ctrl|alt|shift|win)(?<!\\k<mod1>|\\k<mod2>|\\k<mod3>))?\\+)?(?<key>[^\\s+]|app|menu|backspace|tab|enter|esc|escape|space|pgup|pageup|pgdn|pagedown|end|home|left|up|right|down|insert|delete|(?<!shift.+)(?:numpad_?[0-9]|numpad_(?:period|decimal))|numpad_(?:multiply|plus|add|minus|subtract|divide)|f[1-9]|f1[0-9]|f2[0-4]|plus)$",


If there's an actually correct regex for all the things this accepts, I won't be shocked, but I don't think it's valuable to try and optimize this more past this point IMO.

Yeah I made it less correct than it was, because the previous regexp was really really hard to understand. 😅

zadjii-msft · 2021-07-19T14:59:00Z

src/til/ut_til/string.cpp

+    // The below function was used to test from_wchars for unsafety and conformance with clang's strtoul.
+    // The test was run as:
+    //   clang++ -fsanitize=address,undefined,fuzzer -std=c++17 file.cpp
+    // and was run for 20min across 16 jobs in parallel.


We may want to file a backlog follow-up to add this test to onefuzz after #10431 merges

…bindings-impl

src/cascadia/TerminalSettingsModel/KeyChordSerialization.cpp

DHowett · 2021-07-20T21:31:24Z

src/cascadia/WindowsTerminal/AppHost.h

-    std::vector<winrt::Microsoft::Terminal::Control::KeyChord> _hotkeys{};
-    winrt::Windows::Foundation::Collections::IMapView<winrt::Microsoft::Terminal::Control::KeyChord, winrt::Microsoft::Terminal::Settings::Model::Command> _hotkeyActions{ nullptr };
-
+    std::vector<winrt::Microsoft::Terminal::Settings::Model::GlobalSummonArgs> _hotkeys;


this does somewhat violate the design where any action could have been promoted to be "global" :/

if @zadjii-msft is okay with this ("YAGNI"), then okay

I don't think we'll need it, at least not for a while. Methinks the event horizon for "slam arbitrary actions into the tray flyout" is decently far away.

I just changed it because I found the code hard to read and I applied the YAGNI principle to it. I find it easier to understand now, as less casts/checks are necessary.
But of course I can't know how others about it. If you feel like I should revert it I'll gladly do so!
I'll wait a bit before I merge it. :)

Of course we can also convert the type of this vector to be std::vector<Command> in the future and retain the same advantages (well I mean the advantages as I perceived them). 🙂

ghost · 2021-07-20T22:29:02Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This commit introduces an alternative to specifying key bindings as a combination of key modifiers and a character. It allows you to specify an explicit virtual key as `vk(nnn)`. Additionally this commit makes it possible to bind actions to scan codes. As scan code 41 appears to be the button below the Escape key on virtually all keyboards, we'll be able to bind the quake mode hotkey to `win+sc(41)` and have it work consistently across most if not all keyboard layouts. ## PR Checklist * [x] Closes microsoft#7539, Closes microsoft#10203 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed The following was tested both on US and DE keyboard layouts: * Ctrl+, opens settings ✔️ * Win+` opens quake mode window ✔️ * Ctrl+plus/minus increase/decrease font size ✔️

`VkKeyScanW` as well as `MapVirtualKeyW` are used throughout the project, but are input method sensitive functions. Since #10666 `win+sc(41)` is used as the quake mode keybinding, which is then mapped to a virtual key in order to call `RegisterHotKey`. This mapping is highly dependent on the input method and the quake mode key binding will fail to work once the input method was changed.  ## PR Checklist * [x] Closes #10729 * [x] I work here * [ ] Tests added/passed ## Validation Steps Performed * win+` opens quake window before & after changing keyboard layout ✔️ * keyboard layout changes while WT is minimized trigger reloaded ✔️

@lhecker

## Summary of the Pull Request This isn't a fix for #10875, but it is logging that help identify the root cause here. The logging may additionally be helpful for some of the other issues we're seeing elsewhere in the repo, namely #10340. @lhecker is actually working on the fix for #10875, so hopefully this test will help validate. ## References * Regressed in #10666. * logging for #8888 ## PR Checklist * [x] Closes nothing * [x] I work here * [x] Tests added, and they absolutely fail, but they're localtests, so ¯\\\_(ツ)_/¯ * [n/a] Requires documentation to be updated ## details While I was here, I noticed that `KeyBindingsTests::KeyChords` has been broken for some time now. So I fixed that too.

ghost · 2021-08-31T17:13:39Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

DarthJahus · 2021-08-31T18:14:30Z

👏 This is a huge win for key bindings in general. I wish all video games switched to scan codes instead of being layout dependent.

MyloCyrus · 2023-01-21T19:36:02Z

vk(nnn)

vk(nnn)

This comment has been minimized.

Sign in to view

lhecker commented Jul 15, 2021

View reviewed changes

Introduce vk() and sc() key chord specifiers

0bb6e34

lhecker force-pushed the dev/lhecker/7539-key-bindings-impl branch from 09aa4f4 to 0bb6e34 Compare July 15, 2021 13:00

lhecker commented Jul 15, 2021

View reviewed changes

zadjii-msft reviewed Jul 15, 2021

View reviewed changes

lhecker added 3 commits July 15, 2021 17:37

Improve til::from_wchars

561d40a

Address reviewer comments

7297b41

Fix unit tests

b8d6f68

This comment has been minimized.

Sign in to view

lhecker added 2 commits July 15, 2021 17:50

Fix formatting

9fb176b

Fix spelling

877acab

This comment has been minimized.

Sign in to view

Fix audit checks

f901604

lhecker added 2 commits July 16, 2021 01:10

Update profiles.schema.json

098aff1

Fix x86 build

bb6e64d

lhecker marked this pull request as ready for review July 15, 2021 23:28

lhecker commented Jul 15, 2021

View reviewed changes

src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp Show resolved Hide resolved

Improve documentation and til::from_wchars

253072f

zadjii-msft approved these changes Jul 19, 2021

View reviewed changes

lhecker added 4 commits July 19, 2021 17:55

Add some basic KeyChordSerialization unit tests

c3cc1fc

Remove unnecessary comment

a38e415

Merge remote-tracking branch 'origin/main' into dev/lhecker/7539-key-…

9c54d77

…bindings-impl

Fix build error after merging main

d1bef1e

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Jul 20, 2021

ghost requested review from DHowett, PankajBhojwani, carlos-zamora, leonMSFT and miniksa July 20, 2021 14:17

lhecker mentioned this pull request Jul 20, 2021

Reload locale specific data when the input language or method changes #10729

Closed

DHowett approved these changes Jul 20, 2021

View reviewed changes

Address reviewer comments

a8b29ed

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 20, 2021

ghost merged commit 10b12ac into main Jul 20, 2021

ghost deleted the dev/lhecker/7539-key-bindings-impl branch July 20, 2021 22:35

lhecker mentioned this pull request Jul 27, 2021

Can't bind backtick with Colemak keyboard layout #10776

Closed

zadjii-msft mentioned this pull request Aug 4, 2021

[1.11 blocking] Overloading win+` doesn't work anymore #10875

Closed

lhecker mentioned this pull request Aug 5, 2021

Reload settings when the input method changes #10876

Merged

3 tasks

zadjii-msft mentioned this pull request Aug 9, 2021

Add logging, test for #10875 #10907

Merged

3 tasks

This was referenced Aug 31, 2021

Make Quake mode work with a German keyboard layout #10203

Closed

Default bound key combinations are not working as expected on international keyboard layouts #7539

Closed

This pull request was closed.

		KeyChord(Windows.System.VirtualKeyModifiers modifiers, Int32 vkey, Int32 scanCode);
		KeyChord(Boolean ctrl, Boolean alt, Boolean shift, Boolean win, Int32 vkey, Int32 scanCode);

		{ "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+plus" },
		{ "command": { "action": "adjustFontSize", "delta": -1 }, "keys": "ctrl+minus" },

Conversation

lhecker commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Validation Steps Performed

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KalleOlaviNiemitalo commented Jul 15, 2021

Uh oh!

lhecker commented Jul 15, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

KalleOlaviNiemitalo commented Jul 15, 2021

Uh oh!

lhecker commented Jul 15, 2021

Uh oh!

lhecker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 20, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

lhecker commented Jul 15, 2021 •

edited

Loading

lhecker Jul 15, 2021 •

edited

Loading

lhecker Jul 15, 2021 •

edited

Loading

lhecker left a comment •

edited

Loading

lhecker Jul 19, 2021 •

edited

Loading

lhecker Jul 20, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.