Replace IInputEvent with INPUT_RECORD #15673

lhecker · 2023-07-07T13:17:56Z

IInputEvent makes adding Unicode support to InputBuffer more
difficult than necessary as the abstract class makes downcasting
as well as copying quite verbose. I found that using INPUT_RECORDs
directly leads to a significantly simplified implementation.

In addition, this commit fixes at least one bug: The previous approach
to detect the null key via DoActiveModifierKeysMatch didn't work.
As it compared the modifier keys as a bitset with == it failed to
match whenever the numpad key was set, which it usually is.

Validation Steps Performed

Unit and feature tests are ✅

lhecker · 2023-07-07T13:23:21Z

src/host/ft_host/Message_KeyPressTests.cpp

@@ -86,7 +86,6 @@ class KeyPressTests
        expectedRecord.Event.KeyEvent.uChar.UnicodeChar = 0x0;
        expectedRecord.Event.KeyEvent.bKeyDown = true;
        expectedRecord.Event.KeyEvent.dwControlKeyState = ENHANCED_KEY;
-        expectedRecord.Event.KeyEvent.dwControlKeyState |= (GetKeyState(VK_NUMLOCK) & KEY_STATE_TOGGLED) ? NUMLOCK_ON : 0;


This was quite puzzling. On line 78 above we call TurnOffModifierKeys(hwnd) and then assume that conhost still thinks that the numlock is set? This test should've always failed on every keyboard where numlock was on.

lhecker · 2023-07-07T13:26:12Z

src/host/stream.cpp

+                if (LOBYTE(zeroKey) == Event.Event.KeyEvent.wVirtualKeyCode &&
+                    WI_IsAnyFlagSet(Event.Event.KeyEvent.dwControlKeyState, ALT_PRESSED) == WI_IsFlagSet(zeroKey, 0x400) &&
+                    WI_IsAnyFlagSet(Event.Event.KeyEvent.dwControlKeyState, CTRL_PRESSED) == WI_IsFlagSet(zeroKey, 0x200) &&
+                    WI_IsAnyFlagSet(Event.Event.KeyEvent.dwControlKeyState, SHIFT_PRESSED) == WI_IsFlagSet(zeroKey, 0x100))


This is what replaces the std::unordered_set code. I've decided against using clever bitmasking or against converting dwControlKeyState into a Win32 compatible bitset first, because I felt like writing it this way is easier to understand (and review). The constants at the end are kind of magic, but it should be easy to see how they correspond to the ALT_PRESSED, CTRL_PRESSED, SHIFT_PRESSED constants.

lhecker · 2023-07-07T13:27:37Z

src/inc/til/small_vector.h

+        [[nodiscard]] friend constexpr small_vector_iterator operator+(const difference_type off, small_vector_iterator next) noexcept
+        {
+            next += off;
+            return next;
+        }


This was missing. It's required for til::small_vector to fulfill the Range concept and be implicitly convertible to a std::span.

lhecker · 2023-07-07T13:28:35Z

src/interactivity/base/EventSynthesis.cpp

@@ -166,64 +120,35 @@ std::deque<std::unique_ptr<KeyEvent>> Microsoft::Console::Interactivity::Synthes
 // alt + numpad
 // Note:
 // - will throw exception on error
-std::deque<std::unique_ptr<KeyEvent>> Microsoft::Console::Interactivity::SynthesizeNumpadEvents(const wchar_t wch, const unsigned int codepage)
+void Microsoft::Console::Interactivity::SynthesizeNumpadEvents(const wchar_t wch, const unsigned int codepage, InputEventQueue& keyEvents)


An out-parameter allows us to directly append to an existing buffer instead of returning a buffer and then appending that buffer to another buffer. This function is only called from a single place too (the clipboard handler).

DHowett · 2023-07-12T17:29:51Z

tools/ConsoleTypes.natvis

@@ -45,9 +45,27 @@
        </Expand>
    </Type>

-    <Type Name="KeyEvent">
-        <DisplayString Condition="_keyDown">{{↓ wch:{_charData} mod:{_activeModifierKeys} repeat:{_repeatCount} vk:{_virtualKeyCode} vsc:{_virtualScanCode}}</DisplayString>


i did think the arrows were clever... :P

I'll restore the arrows before this merges. 🙂

lhecker · 2023-07-27T15:38:23Z

@DHowett @zadjii-msft The PR is now ready. Despite the size, I feel like it's actually a surprisingly light read. 🙂

DHowett

24/54, checkpoint

DHowett · 2023-07-27T19:25:29Z

src/terminal/input/mouseInput.cpp

@@ -4,6 +4,7 @@
 #include "precomp.h"
 #include "terminalInput.hpp"

+#include "../types/inc/IInputEvent.hpp"


IInputEvent lives?

Sort of... It now contains helper functions to generate INPUT_RECORDs. They all start with "Synthesize".

DHowett · 2023-07-27T19:27:17Z

src/terminal/input/terminalInput.cpp

    {
        return MakeUnhandled();
    }

-    auto keyEvent = *static_cast<const KeyEvent* const>(pInEvent);
+    auto keyEvent = event.Event.KeyEvent;


hmm would we benefit from an auto&?

We modify the keyEvent unfortunately to decoalesce the repeated key events.

zadjii-msft

Yea, I think I'm cool with this. I don't think any of my comments are blocking

src/host/ft_host/Message_KeyPressTests.cpp

zadjii-msft · 2023-07-12T17:32:40Z

src/terminal/input/terminalInput.cpp

                                                        const bool ansiMode,
                                                        const bool cursorApplicationMode,
                                                        const bool keypadApplicationMode) noexcept
 {
    if (ansiMode)
    {
-        if (keyEvent.IsCursorKey())
+        if (keyEvent.wVirtualKeyCode >= VK_END && keyEvent.wVirtualKeyCode <= VK_DOWN)


This could probably use a comment - IsCursorKey was definitely more informative

Hmm... I felt like it was fine given that the code inside the if condition mentions s_cursorKeysApplicationMapping and s_cursorKeysNormalMapping. I've got an idea how to improve it though! 🙂

BTW I thought a little bit about how to construct the function "optimally" and I came up with this:

static std::span<const TermKeyMap> _getKeyMapping(const KEY_EVENT_RECORD& keyEvent, const bool ansiMode, const bool cursorApplicationMode, const bool keypadApplicationMode) noexcept static constexpr uint8_t stage1[2][2][2][2]{ // ansiMode | cursorApplicationMode | keypadApplicationMode 0, 1, // f | f | f 0, 1, // f | f | true 0, 1, // f | true | f 0, 1, // f | true | true 2, 4, // true | f | f 3, 4, // true | f | true 2, 5, // true | true | f 3, 5, // true | true | true }; static constexpr std::span<const TermKeyMap> stage2[]{ s_keypadVt52Mapping, s_cursorKeysVt52Mapping, s_keypadNumericMapping, s_keypadApplicationMapping, s_cursorKeysNormalMapping, s_cursorKeysApplicationMapping, }; // Cursor keys: VK_END, VK_HOME, VK_LEFT, VK_UP, VK_RIGHT, VK_DOWN const auto isCursorKey = keyEvent.wVirtualKeyCode >= VK_END && keyEvent.wVirtualKeyCode <= VK_DOWN; const auto idx = stage1[ansiMode][cursorApplicationMode][keypadApplicationMode][isCursorKey]; return stage2[idx]; }

Would that be too much / too confusing? I personally do like seeing that 2-3, 2-3 and 4-4, 5-5 symmetry, but that's a pretty strong matter of taste. 😅

I TOTALLY MISSED THIS

I LOVE IT

src/terminal/input/terminalInput.cpp

…utEvent-exorcism

DHowett

Let's do this. I'm so excited to not be making individual 8-byte allocations ALL OVER THE PLACE and PASSING POINTERS AROUND.

Thank you for this necessary cleanup :D

DHowett · 2023-08-09T20:52:42Z

src/host/ut_host/Host.UnitTests.vcxproj.filters

@@ -63,9 +63,6 @@
    <ClCompile Include="VtRendererTests.cpp">
      <Filter>Source Files</Filter>
    </ClCompile>
-    <Clcompile Include="..\..\types\IInputEventStreams.cpp">


YIKES including a cpp file right out of another project lol

DHowett · 2023-08-09T20:54:16Z

src/interactivity/base/EventSynthesis.cpp

-    }
-
-    return CodepointWidth::Narrow;
+    return (0x1100 <= wch && wch <= 0x115f) || // From Unicode 9.0, Hangul Choseong is wide


cute, this reformatting is much better

DHowett · 2023-08-09T20:54:56Z

src/interactivity/base/EventSynthesis.cpp

@@ -16,35 +12,29 @@ static constexpr WORD leftShiftScanCode = 0x2A;
 // Routine Description:
 // - naively determines the width of a UCS2 encoded wchar (with caveats noted above)
 #pragma warning(suppress : 4505) // this function will be deleted if numpad events are disabled
-static CodepointWidth GetQuickCharWidthLegacyForNumpadEventSynthesis(const wchar_t wch) noexcept
+static bool GetQuickCharWidthLegacyForNumpadEventSynthesis(const wchar_t wch) noexcept


i might recommend renaming this since it doesn't get a width any longer

DHowett · 2023-08-09T20:56:14Z

src/interactivity/base/EventSynthesis.cpp

+    const auto shiftSet = WI_IsFlagSet(modifierState, 1);
+    const auto ctrlSet = WI_IsFlagSet(modifierState, 2);
+    const auto altSet = WI_IsFlagSet(modifierState, 4);
+    const auto altGrSet = WI_AreAllFlagsSet(modifierState, 4 | 2);


huh this doesn't care about left ctrl or right alt specifically. good to know!

DHowett · 2023-08-09T20:56:42Z

src/interactivity/base/EventSynthesis.cpp


-    // add modifier key up event
+    // handle yucky alt-gr keys


…utEvent-exorcism

lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Jul 7, 2023

lhecker changed the title ~~Clean up InputBuffer coalescing and preprocessing~~ Replace IInputEvent with INPUT_RECORD Jul 7, 2023

lhecker commented Jul 7, 2023

View reviewed changes

lhecker force-pushed the dev/lhecker/8000-WriteConsoleInputA branch from 80fcebc to 42ac9b7 Compare July 12, 2023 17:21

lhecker force-pushed the dev/lhecker/8000-IInputEvent-exorcism branch from 1caa29f to 12038ca Compare July 12, 2023 17:24

DHowett reviewed Jul 12, 2023

View reviewed changes

lhecker marked this pull request as draft July 13, 2023 18:34

Base automatically changed from dev/lhecker/8000-WriteConsoleInputA to main July 18, 2023 18:16

Replace IInputEvent with INPUT_RECORD

fcbbe08

lhecker force-pushed the dev/lhecker/8000-IInputEvent-exorcism branch from 12038ca to fcbbe08 Compare July 27, 2023 15:31

Address feedback

cd37514

lhecker marked this pull request as ready for review July 27, 2023 15:38

Fix bad rebase

77cb0e3

DHowett reviewed Jul 27, 2023

View reviewed changes

Fix another bad rebase

18b13ce

lhecker added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jul 28, 2023

zadjii-msft approved these changes Aug 1, 2023

View reviewed changes

Address feedback

d28fa9c

lhecker requested a review from DHowett August 7, 2023 12:20

lhecker added 2 commits August 7, 2023 14:22

Merge remote-tracking branch 'origin/main' into dev/lhecker/8000-IInp…

fc091a8

…utEvent-exorcism

Move GetChar functions next to GetChar

2683f09

zadjii-msft added the zBugBash-Consider label Aug 8, 2023

zadjii-msft assigned DHowett Aug 9, 2023

DHowett approved these changes Aug 9, 2023

View reviewed changes

lhecker added 2 commits August 10, 2023 21:26

Address feedback

73f545e

Merge remote-tracking branch 'origin/main' into dev/lhecker/8000-IInp…

4a619db

…utEvent-exorcism

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 10, 2023

microsoft-github-policy-service bot enabled auto-merge (squash) August 10, 2023 19:34

Fix cmdline vkey filter

b1e2810

Merge remote-tracking branch 'origin/main' into dev/lhecker/8000-IInp…

793d290

…utEvent-exorcism

microsoft-github-policy-service bot merged commit 5b44476 into main Aug 11, 2023

microsoft-github-policy-service bot deleted the dev/lhecker/8000-IInputEvent-exorcism branch August 11, 2023 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace IInputEvent with INPUT_RECORD #15673

Replace IInputEvent with INPUT_RECORD #15673

lhecker commented Jul 7, 2023 •

edited

Loading

lhecker Jul 7, 2023

lhecker Jul 7, 2023 •

edited

Loading

lhecker Jul 7, 2023

lhecker Jul 7, 2023 •

edited

Loading

DHowett Jul 12, 2023

lhecker Jul 13, 2023

lhecker commented Jul 27, 2023

DHowett left a comment

DHowett Jul 27, 2023

lhecker Jul 27, 2023

DHowett Jul 27, 2023

lhecker Jul 27, 2023

zadjii-msft left a comment

zadjii-msft Jul 12, 2023

lhecker Aug 2, 2023

lhecker Aug 2, 2023 •

edited

Loading

DHowett Aug 11, 2023

DHowett left a comment

DHowett Aug 9, 2023

DHowett Aug 9, 2023

DHowett Aug 9, 2023

DHowett Aug 9, 2023

DHowett Aug 9, 2023

Replace IInputEvent with INPUT_RECORD #15673

Replace IInputEvent with INPUT_RECORD #15673

Conversation

lhecker commented Jul 7, 2023 • edited Loading

Validation Steps Performed

Choose a reason for hiding this comment

lhecker Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Jul 27, 2023

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Jul 7, 2023 •

edited

Loading

lhecker Jul 7, 2023 •

edited

Loading

lhecker Jul 7, 2023 •

edited

Loading

lhecker Aug 2, 2023 •

edited

Loading