Refactor TerminalInput to return strings #15611

lhecker · 2023-06-27T16:12:38Z

TerminalInput is configurable, but almost entirely state-less.
As such it isn't helpful that it emits its output via a callback.
It makes tracing the flow of data harder purely from reading the code
and also raises uncertainty about when TerminalInput may generate
output. This commit makes it more robust by having TerminalInput
simply return its data. Furthermore, it returns that data as a string
instead of converting back and forth between text and IInputEvent.

This change will help me make conhost's InputBuffer implementation
leaner and help me confidently make more difficult changes to it
with the goal to improve our Unicode support/correctness.

Validation Steps Performed

Windows Terminal produces correct results with showkey -a ✅

lhecker · 2023-06-27T16:13:17Z

src/cascadia/TerminalControl/ControlCore.h

@@ -24,8 +24,6 @@
 #include "../buffer/out/search.h"
 #include "../buffer/out/TextColor.h"

-#include <til/ticket_lock.h>


This PR is a little less bad if you suppress whitespace changes: https://github.com/microsoft/terminal/pull/15611/files?w=1

lhecker · 2023-06-27T16:14:29Z

src/terminal/input/terminalInput.hpp

 namespace Microsoft::Console::VirtualTerminal
 {
    class TerminalInput final
    {
    public:
-        TerminalInput(_In_ std::function<void(std::deque<std::unique_ptr<IInputEvent>>&)> pfn);
+        using StringType = std::wstring;


The choice of std::wstring is inoptimal because it'll heap-allocate on every single key press. I figured it doesn't really matter though, because we already do that and more.

you elected not to use memory_buffer? aw

wait, will it? std::wstring absolutely has support for storing short strings inside of it. Up to 16 bytes (so for a wstring, 8 chars)

BUF_SIZE is 16/sizeof(value_type) 😄

It's 7 chars due to the terminating null character, but the win32 key sequence is longer than that. (I mean it's also longer than 8 characters...)

I'm using std::wstring because memory_buffer is not comparable, which makes it harder to use for unit tests.

oh, win32 input sequences. bah.

can you add an operator== to the global namespace for memory_buffer?

is it comparable in fmt 9?

As far as I know it isn't. I'd probably just use til::small_vector instead, but it lacks an append(some_range) method. I think we should improve on this at some other time. It realistically doesn't make any difference in our overall application.

src/terminal/parser/ut_parser/InputEngineTest.cpp

DHowett · 2023-06-27T17:06:05Z

src/host/inputBuffer.cpp

+        if (const auto out = _termInput.HandleFocus(focused))
+        {
+            _HandleTerminalInputCallback(*out);


this pattern is different from the one in TerminalCore, since it takes *out instead of out. Should we converge?

Hmm... No, I don't think so. I feel like the _WriteBuffer loop makes more sense this way.

DHowett · 2023-06-27T18:47:01Z

src/terminal/input/terminalInput.hpp

 namespace Microsoft::Console::VirtualTerminal
 {
    class TerminalInput final
    {
    public:
-        TerminalInput(_In_ std::function<void(std::deque<std::unique_ptr<IInputEvent>>&)> pfn);
+        using StringType = std::wstring;


you elected not to use memory_buffer? aw

DHowett · 2023-06-27T18:52:04Z

src/terminal/input/terminalInput.hpp

 namespace Microsoft::Console::VirtualTerminal
 {
    class TerminalInput final
    {
    public:
-        TerminalInput(_In_ std::function<void(std::deque<std::unique_ptr<IInputEvent>>&)> pfn);
+        using StringType = std::wstring;


wait, will it? std::wstring absolutely has support for storing short strings inside of it. Up to 16 bytes (so for a wstring, 8 chars)

BUF_SIZE is 16/sizeof(value_type) 😄

DHowett · 2023-06-27T18:53:31Z

src/host/inputBuffer.cpp

+
+        for (const auto& wch : text)
+        {
+            _storage.push_back(std::make_unique<KeyEvent>(true, 1ui16, 0ui16, 0ui16, wch, 0));


now for THIS we are gonna heap alloc every time but WHATEVER I HATE IT TOO

src/terminal/input/mouseInput.cpp

src/terminal/input/terminalInput.cpp

DHowett · 2023-06-27T19:05:12Z

src/terminal/input/terminalInput.cpp

    if (til::is_leading_surrogate(ch))
    {
        if (_leadingSurrogate.has_value())
        {
-            // we already were storing a leading surrogate but we got another one. Go ahead and send the
-            // saved surrogate piece and save the new one
-            const auto formatted = wil::str_printf<std::wstring>(L"%I32u", _leadingSurrogate.value());


what in blazes? so we would literally just send the raw value of the leading surrogate as an integer?

this is a behavior change, but i can't see anyone relying on it.
the code on the right just sends the leading surrogate as a character instead.

noting of course that this is the broken case where we got a Lead + Lead

Yeah, I was quite confused as well. I looked up where it came from and it's been there ever since it got introduced with the WPF PR.

I made it so that it now discard invalid surrogate pairs entirely. It's rather unexpected that our input handling code would have to deal with unpaired surrogate pairs anyways. Discarding them is probably about as good as sending U+FFFD.

src/terminal/input/terminalInput.cpp

carlos-zamora · 2023-06-30T00:07:24Z

src/terminal/input/terminalInput.hpp

-        TerminalInput() = delete;
-        TerminalInput(const TerminalInput& old) = default;
-        TerminalInput(TerminalInput&& moved) = default;
+        static_assert(StringType{} == StringType{});


Why do we need this?

Ah woops that's a leftover from when I was testing which StringType would work with the unit tests. I needed a class that as a == operator. I'll remove it. 😅

carlos-zamora · 2023-06-30T00:20:17Z

src/terminal/input/terminalInput.cpp

-        return true;
+        // Note that we must return an empty string here to imply that we've handled
+        // the event, otherwise the key press can still end up being submitted.
+        return MakeOutput({});


To be clear MakeOutput({}) returns the empty string, whereas MakeUnhandled() returns OutputType{}, which is... what exactly? A null string?

Yeah kind of. It's like returning null vs. "" in JS. null in this case means "we didn't handle it", where "" means we did handle it and the result is simply an empty string.

carlos-zamora · 2023-06-30T00:56:14Z

src/terminal/input/mouseInput.cpp

-                if (success)
-                {
-                    _SendInputSequence(sequence);
-                    success = true;
-                }
-                if (_inputMode.any(Mode::ButtonEventMouseTracking, Mode::AnyEventMouseTracking))
-                {
-                    _mouseInputState.lastPos.x = position.x;
-                    _mouseInputState.lastPos.y = position.y;
-                    _mouseInputState.lastButton = button;


Didn't this get lost in the change?

This code change is a little (very) confusing tbh. This if condition has moved up in the scope, so that I could use early returns and remove the local success variable. It's fine to move the if condition up because none of the called functions depend on _mouseInputState (they're all 3 static functions).

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

lhecker commented Jun 27, 2023

View reviewed changes

lhecker force-pushed the dev/lhecker/8000-TerminalInput-cleanup branch from 625f121 to 376a993 Compare June 27, 2023 16:18

Refactor TerminalInput to return strings

f73fe75

lhecker force-pushed the dev/lhecker/8000-TerminalInput-cleanup branch from 376a993 to f73fe75 Compare June 27, 2023 16:25

DHowett reviewed Jun 27, 2023

View reviewed changes

Fix audit mode, Address feedback

fb7d745

This comment has been minimized.

Sign in to view

DHowett approved these changes Jun 27, 2023

View reviewed changes

Fix spelling

6f9bd30

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

microsoft-github-policy-service bot enabled auto-merge (squash) June 27, 2023 20:58

DHowett requested review from carlos-zamora and PankajBhojwani June 30, 2023 00:13

carlos-zamora reviewed Jun 30, 2023

View reviewed changes

Address feedback

a9eaa4d

carlos-zamora approved these changes Jun 30, 2023

View reviewed changes

microsoft-github-policy-service bot merged commit 030db09 into main Jun 30, 2023

microsoft-github-policy-service bot deleted the dev/lhecker/8000-TerminalInput-cleanup branch June 30, 2023 16:37

lhecker mentioned this pull request Mar 24, 2024

Implement grapheme clusters #16916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor TerminalInput to return strings #15611

Refactor TerminalInput to return strings #15611

lhecker commented Jun 27, 2023 •

edited

Loading

lhecker Jun 27, 2023

lhecker Jun 27, 2023

DHowett Jun 27, 2023

DHowett Jun 27, 2023

lhecker Jun 27, 2023 •

edited

Loading

DHowett Jun 27, 2023

DHowett Jun 27, 2023 •

edited

Loading

lhecker Jun 27, 2023

DHowett Jun 27, 2023

lhecker Jun 27, 2023

DHowett Jun 27, 2023

DHowett Jun 27, 2023

DHowett Jun 27, 2023

DHowett Jun 27, 2023

DHowett Jun 27, 2023

DHowett Jun 27, 2023

lhecker Jun 27, 2023

lhecker Jun 27, 2023

This comment has been minimized.

carlos-zamora Jun 30, 2023

lhecker Jun 30, 2023

carlos-zamora Jun 30, 2023

lhecker Jun 30, 2023

carlos-zamora Jun 30, 2023

lhecker Jun 30, 2023 •

edited

Loading

Refactor TerminalInput to return strings #15611

Refactor TerminalInput to return strings #15611

Conversation

lhecker commented Jun 27, 2023 • edited Loading

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 Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett Jun 27, 2023 • edited Loading

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

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

This comment has been minimized.

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 Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

lhecker commented Jun 27, 2023 •

edited

Loading

lhecker Jun 27, 2023 •

edited

Loading

DHowett Jun 27, 2023 •

edited

Loading

lhecker Jun 30, 2023 •

edited

Loading