Clean up InputBuffer coalescing and preprocessing #15671

lhecker · 2023-07-07T12:50:29Z

This reverts a number of changes to input handling to how it used to be
in conhost v1. It merges the input event coalescing logic into a single
function and inlines the console suspension event handling, because
soon these functions will receive std::span arguments which cannot
be preprocessed anymore, unlike a std::deque.

It also adds back support for Ctrl-S being an alias for VK_PAUSE
which was lost in commit fccc741 in 2018.

Closes #809

Validation Steps Performed

Unit and feature tests are ✅
Ctrl-S pauses output 🎉

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

src/host/ut_host/InputBufferTests.cpp

@@ -611,6 +611,7 @@ class InputBufferTests
        auto inputEvent2 = IInputEvent::Create(record2);
        // write another event to a non-empty buffer
        waitEvent = false;
+        storage.clear();


Starting with this PR, _WriteBuffer moves its items out of the argument, without emptying the argument. This results in this test failing. In a follow-up PR this will not be a problem anymore, because I'll remove IInputEvent.

DHowett · 2023-07-07T18:18:39Z

src/host/inputBuffer.cpp

+                UnblockWriteConsole(CONSOLE_OUTPUT_SUSPENDED);
+                continue;
+            }
+            // intercept control-s


hey uh, does this fix #809?

Does Ctrl+Q un-suspend it?

wait, ^S is not VK_PAUSe...

this is the problem with the old comments you found...

Ah, you're right. The old code looked like this:

// intercept control-s if ((g_ciConsoleInformation.pInputBuffer->InputMode & ENABLE_LINE_INPUT) && (InputEvent->Event.KeyEvent.wVirtualKeyCode == VK_PAUSE || IsPauseKey(&InputEvent->Event.KeyEvent))) {

I'll add that back. It fits into the PR.

DHowett · 2023-07-07T18:21:33Z

src/host/inputBuffer.cpp

@@ -779,40 +796,12 @@ void InputBuffer::_WriteBuffer(_Inout_ std::deque<std::unique_ptr<IInputEvent>>&
        // record at a time because this is the original behavior of
        // the input buffer. Changing this behavior may break stuff
        // that was depending on it.
-        if (initialInEventsSize == 1 && !_storage.empty())
+        if (initialInEventsSize == 1 && !_storage.empty() && _CoalesceEvent(inEvents[0]))


I find it strange that we're doing a size check and a [0] inside a range-for-loop. Is that the clearest way to represent this concept?

Not at all! 😄
The old code did this check once, at the start of the function and outside of the loop. But I felt like this is a good tradeoff between making the code simpler while not changing too much.

DHowett · 2023-07-07T18:22:23Z

src/host/inputBuffer.cpp

@@ -905,76 +826,44 @@ bool InputBuffer::_CanCoalesce(const KeyEvent& a, const KeyEvent& b) const noexc
 // the buffer with updated values from an incoming event, instead of
 // storing the incoming event (which would make the original one
 // redundant/out of date with the most current state).
-bool InputBuffer::_CoalesceRepeatedKeyPressEvents(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& inEvents)
+bool InputBuffer::_CoalesceEvent(const std::unique_ptr<IInputEvent>& inEvent) const noexcept


How can this be const? It modifies lastEvent which is in _storage

Because std::unique_ptr is a pointer and const in C++ has about as much meaning as me saying that I won't eat that second pudding today: https://godbolt.org/z/38334rzjc

(The const will go away once I use INPUT_RECORD, because then there's no std::unique_ptr anymore.)

DHowett · 2023-07-07T18:24:01Z

src/host/inputBuffer.cpp

+            (lastKey.GetVirtualScanCode() == inKey.GetVirtualScanCode() || WI_IsFlagSet(inKey.GetActiveModifierKeys(), NLS_IME_CONVERSION)) &&
+            lastKey.GetCharData() == inKey.GetCharData() &&
+            lastKey.GetActiveModifierKeys() == inKey.GetActiveModifierKeys() &&
+            // TODO: This behavior is an import from old conhost v1 and has been broken for decades.


track with issue#?

If you don't mind, I don't want to track that separately in my backlog. After all, this is a bug that is directly standing in my path of removing IsGlyphFullWidth everywhere that is outside of TextBuffer (and ROW) and thus would get addressed by me anyways in the near term.

Hell I mean, you could just say TODO:GH#8000 since that's the megathread tracking your crusade, but idgaf

Oh, right I could've done that... 😮
But... Eh, I don't want to wait 30min for the CI. I'll just cross my fingers to not die within the next few months. 😄

zadjii-msft

Wow it's kinda horrifying looking at that old code nowadays

zadjii-msft · 2023-07-12T17:00:21Z

src/host/inputBuffer.cpp

+            (lastKey.GetVirtualScanCode() == inKey.GetVirtualScanCode() || WI_IsFlagSet(inKey.GetActiveModifierKeys(), NLS_IME_CONVERSION)) &&
+            lastKey.GetCharData() == inKey.GetCharData() &&
+            lastKey.GetActiveModifierKeys() == inKey.GetActiveModifierKeys() &&
+            // TODO: This behavior is an import from old conhost v1 and has been broken for decades.


Hell I mean, you could just say TODO:GH#8000 since that's the megathread tracking your crusade, but idgaf

Gemesys · 2024-03-08T04:25:40Z

Ok, it's March 2024. Can I have Ctrl-s actually pause my running program now? Like I could do for many years, on various Windows platforms? I have a newish Windows-11 HP laptop, it updates itself regularly, and when I am running a program that outputs diagnostics to Cmd session window, I still cannot ctrl-s pause the running prgm (like I can on all my Linux and MacOS machines...) Do I need to edit the registry or something? It's in my left-hand's muscle-memory - rest left thumb on CTRL key, press "s" key, have a look, press "q" to continue. Works on Linux and MacOS. How do I re-enable this on my Window-11 machine?

oising · 2024-03-08T16:14:34Z

@DHowett-MSFT you guys need to re-open #809 -- the bot closed it.

Clean up InputBuffer coalescing and preprocessing

fd3774d

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

Fix comment, unit tests

d5480bb

lhecker commented Jul 7, 2023

View reviewed changes

DHowett reviewed Jul 7, 2023

View reviewed changes

Fix Ctrl-S support for pausing

3659c4f

lhecker added Product-Conhost For issues in the Console codebase Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Area-Input Related to input processing (key presses, mouse, etc.) labels Jul 7, 2023

zadjii-msft approved these changes Jul 12, 2023

View reviewed changes

DHowett approved these changes Jul 12, 2023

View reviewed changes

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

microsoft-github-policy-service bot merged commit e1ab64e into main Jul 12, 2023
15 checks passed

microsoft-github-policy-service bot deleted the dev/lhecker/8000-InputBuffer-Write branch July 12, 2023 17:18

oising mentioned this pull request Mar 8, 2024

Re-investigate Ctrl+S pausing #809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up InputBuffer coalescing and preprocessing #15671

Clean up InputBuffer coalescing and preprocessing #15671

lhecker commented Jul 7, 2023 •

edited

lhecker Jul 7, 2023

DHowett Jul 7, 2023

DHowett Jul 7, 2023

DHowett Jul 7, 2023

lhecker Jul 7, 2023 •

edited

DHowett Jul 7, 2023

lhecker Jul 7, 2023

DHowett Jul 7, 2023

lhecker Jul 7, 2023

lhecker Jul 7, 2023 •

edited

DHowett Jul 7, 2023

lhecker Jul 7, 2023

zadjii-msft Jul 12, 2023

lhecker Jul 12, 2023

zadjii-msft left a comment

zadjii-msft Jul 12, 2023

Gemesys commented Mar 8, 2024

oising commented Mar 8, 2024

Clean up InputBuffer coalescing and preprocessing #15671

Clean up InputBuffer coalescing and preprocessing #15671

Conversation

lhecker commented Jul 7, 2023 • edited

Validation Steps Performed

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 Jul 7, 2023 • edited

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 Jul 7, 2023 • edited

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

Gemesys commented Mar 8, 2024

oising commented Mar 8, 2024

lhecker commented Jul 7, 2023 •

edited

lhecker Jul 7, 2023 •

edited

lhecker Jul 7, 2023 •

edited