Use VT for COOKED_READ_DATA #17445

lhecker · 2024-06-19T00:59:03Z

By rewriting COOKED_READ_DATA to use VT for its output we make it
possible to pass this VT output 1:1 straight to the hosting terminal
if we're running under ConPTY. This is also possible with the current
console APIs it uses, but it's somewhat janky. In particular the
usage of ReadConsoleOutput to backup/restore the popup contents
could be considered bad faith "rules for thee, not for me",
given that we're telling people to move away from those APIs.

The new implementation contains a bare bones "pager" to fit even
very long prompt contents into the VT viewport.
I fully expect this initial PR to not be entirely bug free, because
writing a proper pager with line wrapping is a little bit complex.
This PR takes some significant shortcuts by leveraging the fact
that the prompt line is always left-to-right and always a series
of fully filled lines followed by one potentially semi-full line.
This allows us to skip using a front/back-buffer for diffing the
contents between two redisplay calls.

Part of #14000

Validation Steps Performed

ASCII input
Chinese input (中文維基百科) ✅
Surrogate pair input (🙂) ✅
In cmd.exe
- Create 2 files: "a😊b.txt" and "a😟b.txt"
- Press tab: Autocomplete to "a😊b.txt" ✅
- Navigate the cursor right past the "a"
- Press tab twice: Autocomplete to "a😟b.txt" ✅
Execute printf(" "); gets(buffer); in C (or equivalent)
- Press Tab, A, Ctrl+V, Tab, A ✅
- The prompt is " A^V A" ✅
- Cursor navigation works ✅
- Backspacing/Deleting random parts of it works ✅
- It never deletes the initial 4 spaces ✅
Backspace deletes preceding glyphs ✅
Ctrl+Backspace deletes preceding words ✅
Escape clears input ✅
Home navigates to start ✅
Ctrl+Home deletes text between cursor and start ✅
End navigates to end ✅
Ctrl+End deletes text between cursor and end ✅
Left navigates over previous code points ✅
Ctrl+Left navigates to previous word-starts ✅
Right and F1 navigate over next code points ✅
- Pressing right at the end of input copies characters
  from the previous command ✅
Ctrl+Right navigates to next word-ends ✅
Insert toggles overwrite mode ✅
Delete deletes next code point ✅
Up and F5 cycle through history ✅
- Doesn't crash with no history ✅
- Stops at first entry ✅
Down cycles through history ✅
- Doesn't crash with no history ✅
- Stops at last entry ✅
PageUp retrieves the oldest command ✅
PageDown retrieves the newest command ✅
F2 starts "copy to char" prompt ✅
- Escape dismisses prompt ✅
- Typing a character copies text from the previous command up
  until that character into the current buffer (acts identical
  to F3, but with automatic character search) ✅
F3 copies the previous command into the current buffer,
starting at the current cursor position,
for as many characters as possible ✅
- Doesn't erase trailing text if the current buffer
  is longer than the previous command ✅
- Puts the cursor at the end of the copied text ✅
F4 starts "copy from char" prompt ✅
- Escape dismisses prompt ✅
- Erases text between the current cursor position and the
  first instance of a given char (but not including it) ✅
F6 inserts Ctrl+Z ✅
F7 without modifiers starts "command list" prompt ✅
- Escape dismisses prompt ✅
- Entries wider than the window width are truncated ✅
- Height expands up to 20 rows with longer histories ✅
- F9 starts "command number" prompt ✅
- Left/Right replace the buffer with the given command ✅
  - And put cursor at the end of the buffer ✅
- Up/Down navigate selection through history ✅
  - Stops at start/end with <10 entries ✅
  - Stops at start/end with >20 entries ✅
  - Scrolls through the entries if there are too many ✅
- Shift+Up/Down moves history items around ✅
- Home navigates to first entry ✅
- End navigates to last entry ✅
- PageUp navigates by $height items at a time or to first ✅
- PageDown navigates by $height items at a time or to last ✅
Alt+F7 clears command history ✅
F8 cycles through commands that start with the same text as
the current buffer up until the current cursor position ✅
- Doesn't crash with no history ✅
F9 starts "command number" prompt ✅
- Escape dismisses prompt ✅
- Ignores non-ASCII-decimal characters ✅
- Allows entering between 1 and 5 digits ✅
- Pressing Enter fetches the given command from the history ✅
Alt+F10 clears doskey aliases ✅
In cmd.exe, with an empty prompt in an empty directory:
Pressing tab produces an audible bing and prints no text ✅
When Narrator is enabled, in cmd.exe:
- Typing individual characters announces only
  exactly each character that is being typed ✅
- Backspacing at the end of a prompt announces
  only exactly each deleted character ✅

src/host/readDataCooked.cpp

DHowett · 2024-06-19T18:33:55Z

qq: x offset?

DHowett

Well holy shit

src/host/VtIo.cpp

src/host/readDataCooked.cpp

src/host/readDataCooked.hpp

DHowett

Currently talking about a bug with Leonard - hold tight

…cooked-read

DHowett

Tested, broke, fixed, tested, love it, ship it

DHowett · 2024-06-24T17:49:59Z

One question! The old popup style allows you to see the F7 command list behind the F9 prompt, when you are using both:

I wonder if this helps you enter a number - you can see which command it lines up with.

The new one does not allow for that:

DHowett · 2024-06-24T18:59:17Z

Ah, another horror from beyond the known universe

this arrow character is in the ambiguous width section, which is quite fun for users of the font NSimSun

lhecker · 2024-06-25T15:56:18Z

Should be much better now!

src/host/readDataCooked.cpp

DHowett · 2024-06-25T17:14:09Z

Tab completing something that doesn't exist if you have a control character in your input line does a funny thing too.

…cooked-read

DHowett · 2024-06-25T22:44:47Z

/azp run

carlos-zamora · 2024-07-03T18:31:38Z

src/host/screenInfo.cpp

-    const auto cookedReadRestore = wil::scope_exit([]() {
-        auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-        if (gci.HasPendingCookedRead())
-        {
-            gci.CookedReadData().RedrawAfterResize();
-        }
-    });


Curious, why don't we need this anymore?

The RedrawAfterResize call moved upwards in this file. The problem with screenInfo is that it calls RedrawAfterResize right after resizing the buffer but before it actually updated the _viewport. This causes the VT parser to think that the addressable range is smaller/larger than it actually is which results in issues.
My solution was to delay the RedrawAfterResize call until the inevitable call to _InternalSetViewportSize later on.

src/host/readDataCooked.cpp

carlos-zamora · 2024-07-03T23:21:10Z

src/host/readDataCooked.cpp

        }

-        if (measureOnly)
+        // Render the popups, if there are any.
+        if (!_popups.empty())


I'm not really a CMD user so I'm not really familiar with how to get popups to appear. BUT if you tell me how to get popups to appear, I'd be happy to test this out manually to get some extra coverage :)

There are 3 "question" popups with F2, F4, and F9, and there's one interactive popup via F7. When you're in the F7 prompt, you can press F9 to select one of the entries immediately. You can read about what the 4 popups are supposed to do here (they're in the order F2, F4, F9, F7): https://github.com/microsoft/terminal/blob/dev/lhecker/14000-vt-cooked-read/src/host/readDataCooked.hpp#L54-L73

I should mention the F-keys that open them in that comment. 🤔

Alright, I did a little bit of testing on the popups. Tested against v1.20.11781. Here's a short report of my findings:

F2

The UI does look different between the two. I'm guessing that's expected (and I'm guessing it's ok for us to do this). Curious, why though? I'm guessing it's a limitation on switching over to just using VT?

🐛BUG:

run echo hello world

F2->w

expected: echo hello

actual: the characters are copied over to the input buffer, but they aren't rendered

✅ the input buffer is properly updated regardless of the initial content of the input buffer

i.e. start with abcdefghijklmnop then use F2 --> input buffer is updated appropriately (data-wise)

F4

✅ input buffer properly updated

test: abcdefghijklmnopqrstuvwxyz (cursor just after the f) + F4 + x --> abcdefxyz

F9

✅ correctly retrieves the right command in the history and throws it in the input buffer

test: fill history with echo 1 up to echo 5 + F9 + 1 --> echo 2 (it's 0-indexed!)

F7

Again, UI looks pretty different. This one is probably going to be the most noticeable one since it's widely used.

✅ given the new UI, if we don't have enough space at the bottom, we shift everything over

✅ if the retrieved command doesn't fit on the screen, we truncate it

Looks like there's just one bug (and it's probably fixed by a missed call to the renderer somewhere). Nice work!

Curious, why though? I'm guessing it's a limitation on switching over to just using VT?

Yes, but also not quite. We could implement the same popup interface as we had before, but it always had the problem that it reads the buffer contents. When you dismiss that old popup UI, it needs to restore the text that was behind it after all. It's possible to make this Unicode correct, but it's also very annoying to do so.
However, there's another problem: When you restart a session ("press Enter to restart" in WT), it recreates a new ConPTY instance which does not have the previous buffer contents. When the popup reads the buffer contents it'll read whitespace and when you dismiss it, it'll destroy your previous buffer contents.
Both issues can be avoided by simply modernizing the popup UI a little, and by making sure that the "popup" is always below the prompt and never on top of it.

The last 2 commits address the issue you found. You can pretty much ignore the "Fix buffer invalidation" one though. 😅 I've decided to go with a way simpler solution for now until we improve our grapheme cluster support even further at some point.

Just on the subject of the popups restoring the text behind them, technically we should be able to achieve that now with VT pages, without having to read the buffer contents. The catch is that it's unlikely to work on most other terminals, so we'd still have to have a simpler fallback UI that doesn't rely on popups, and some form of feature detection at startup. I would have loved for us to do that, but I know that's asking a bit much.

I really wouldn't mind adding something like that in the future! It sounds pretty cool and we could show off our more advanced VT features that way.

carlos-zamora · 2024-07-03T23:49:13Z

src/host/readDataCooked.cpp

-    case PopupKind::CopyToChar:
-        _popupDrawPrompt(popup, ID_CONSOLE_MSGCMDLINEF2);
-        break;
-    case PopupKind::CopyFromChar:
-        _popupDrawPrompt(popup, ID_CONSOLE_MSGCMDLINEF4);
-        break;


Why no more CopyToChar and CopyFromChar?

All of the drawing logic moved over to _redisplay(). I did this because with VT we can't directly manipulate the buffer anymore, so we need to be smart and correctly calculate the positions of the text and the popups before we even write them out. That only works of course, if we have all of the necessary information at hand simultaneously within a single function.

src/host/screenInfo.cpp

…cooked-read

lhecker · 2024-07-09T13:29:48Z

src/host/screenInfo.cpp


            // If the bottom of the window when adjusted would be
            // above the final line of valid text...
-            if (srNewViewport.bottom + DeltaY < coordValidEnd.y)
+            if (sBottomProposed < coordValidEnd.y)


The GetValidAreaBoundaries will not have been updated to reflect the new prompt position at this point, so we can't use that anymore to determine the viewport position. As an alternative I'm now using the cursor position. That will work a little worse for very long prompts, but at least it works. We can improve this later on.

carlos-zamora

Blocking for that one bug. Other than that, looks great! Nice work!

link: #17445 (comment)

DHowett · 2024-07-02T17:27:38Z

src/host/screenInfo.cpp

@@ -1037,6 +1037,11 @@ void SCREEN_INFORMATION::_InternalSetViewportSize(const til::size* const pcoordS
    const auto DeltaY = pcoordSize->height - _viewport.Height();
    const auto coordScreenBufferSize = GetBufferSize().Dimensions();

+    if (DeltaX == 0 && DeltaY == 0)
+    {
+        return;


is this going to suppress some sort of detectable input event that a client app could have been depending on?

The only thing this function does (apart from changing the viewport) is to make the current cursor visible if a cooked read is active. So, I'd say "no".

src/host/VtIo.cpp

lhecker · 2024-07-09T23:01:14Z

I find it remarkable that the ARM64 tests keep failing in the exact tests that I had to patch for #17510, but in this PR, I didn't change any of that code yet, so why is it failing? And why does it work in the x64 tests?? I think it's just a fluke. In any case, it'll get fixed by #17510.

carlos-zamora

Thanks for doing this 😊 Fantastic work!

zadjii-msft

I did not get through all this, but I won't block over what I've seen so far

zadjii-msft · 2024-07-10T15:19:24Z

src/host/VtIo.cpp

+        out += 2;
+    }
+
+    // 7 bytes (";97").


zadjii-msft · 2024-07-10T15:48:15Z

src/host/readDataCooked.cpp

+            // Hilarious bit-trickery that no one can read. But it works. Check it out in a debugger.
+            // The idea is to use bits 1:31 as the value and bit 0 as a trigger to bit-flip the value.
+            // A bit-flipped positive number is negative, but offset by 1, so we add 1. Fun!
+            const auto offset = ((i / 2) ^ ((i & 1) - 1)) + 1;


zadjii-msft · 2024-07-10T15:56:06Z

src/host/readDataCooked.cpp

+
+        // Do a brute force search for the best starting position that ends at the current cursor position.
+        // The search is centered around `bestGuessColumn` with offsets 0, 1, -1, 2, -2, 3, -3, ...
+        for (til::CoordType i = 0, attempts = 2 * size.width; i <= attempts; i++)


this seems... really wacky? Why can't we just layout the initialData once, then measure the total number of columns that all takes, then go back that many columns and start there? I don't think I get why we instead are trying a bunch of different starting places till one just works...?

Oh right, I forgot to mention in the comment why we need to do that! I'll update it.

We need to do this, because of wide glyphs. When a wide glyph intersects the end of a row, we insert a padding space. We're given an "end position" and a string and we need to find its starting position. The problem is that multiple starting positions can result in the same end position depending on what wide glyphs intersect the end of their row where. Because of that it's not trivially possible to layout those lines backwards starting from the end position.

zadjii-msft · 2024-07-10T22:00:53Z

src/host/readDataCooked.cpp

-//
-// NOTE: Don't call _flushBuffer() after appending newlines to the buffer! See _handlePostCharInputLoop for more information.
-void COOKED_READ_DATA::_flushBuffer()
+til::point COOKED_READ_DATA::_getViewportCursorPosition() const noexcept


📝: this thru _setCursorPosition are pretty verbatim moved from BufferState at the top of the file

zadjii-msft

Yea I feel comfortable with this merging while I'm OOF. I didn't read all of it, so I don't want to be the second, but what I have read is inoffensive

zadjii-msft · 2024-07-11T11:45:51Z

src/host/readDataCooked.cpp


+    // FYI: This loop does not loop. It exists because goto is considered evil


well, it might loop once. which we can't know at the top of this, for the same "wide chars wrap weird" reason

well, it might loop once.

To me it felt like writing a duff's device. Sure, it does wrap around once, but like...
It literally has a break; at the tail end of the for-loop. 😅 For all intents and purposes, I basically wrote a goto but I'm not calling it as such. I just didn't want to write another pragma suppress.

zadjii-msft · 2024-07-11T11:56:30Z

src/host/readDataCooked.cpp

 }

-til::CoordType COOKED_READ_DATA::_getColumnAtRelativeCursorPosition(ptrdiff_t distance) const
+COOKED_READ_DATA::LayoutResult COOKED_READ_DATA::_layoutLine(std::wstring& output, const std::wstring_view& input, const size_t inputOffset, const til::CoordType columnBegin, const til::CoordType columnLimit) const


🔖 i didn't read this method yet

lhecker added 2 commits June 19, 2024 02:28

Move colorbrewer into its own proper header

bbbe20f

Use VT for COOKED_READ_DATA

b0cc432

github-advanced-security bot found potential problems Jun 19, 2024

View reviewed changes

src/host/readDataCooked.cpp Fixed Show fixed Hide fixed

src/host/readDataCooked.cpp Fixed Show fixed Hide fixed

src/host/readDataCooked.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

lhecker mentioned this pull request Jun 19, 2024

Use the full buffer width for the VT viewport #17446

Closed

lhecker added Product-Conhost For issues in the Console codebase Area-VT Virtual Terminal sequence support Area-CookedRead The cmd.exe COOKED_READ handling labels Jun 19, 2024

lhecker force-pushed the dev/lhecker/colorbrewer branch from 1f4219f to 4f35067 Compare June 19, 2024 01:54

lhecker added 2 commits June 20, 2024 15:13

Fix GetVirtualViewport, Fix popup alignment

e08ebbe

Address feedback

88ef5f4

Base automatically changed from dev/lhecker/colorbrewer to main June 20, 2024 17:45

DHowett approved these changes Jun 20, 2024

View reviewed changes

src/host/VtIo.cpp Outdated Show resolved Hide resolved

src/host/readDataCooked.cpp Show resolved Hide resolved

src/host/readDataCooked.hpp Outdated Show resolved Hide resolved

DHowett requested changes Jun 20, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 20, 2024

DHowett removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 20, 2024

lhecker added 2 commits June 21, 2024 02:11

Address feedback, Fix scrolling bug

413991e

Merge remote-tracking branch 'origin/main' into dev/lhecker/14000-vt-…

8688a50

…cooked-read

DHowett approved these changes Jun 24, 2024

View reviewed changes

lhecker added 2 commits June 25, 2024 14:41

Fix several issues with control character visualizers

32a1249

Fix ambiguous chars, Improve F7+F9 UX

4471e98

DHowett reviewed Jun 25, 2024

View reviewed changes

src/host/readDataCooked.cpp Show resolved Hide resolved

src/host/readDataCooked.cpp Show resolved Hide resolved

lhecker added 2 commits June 25, 2024 23:27

Merge remote-tracking branch 'origin/main' into dev/lhecker/14000-vt-…

0e63bd0

…cooked-read

Overkill initialData impl, but hopefully not kill

8f39b49

zadjii-msft added this to the Terminal v1.22 milestone Jul 2, 2024

lhecker mentioned this pull request Jul 3, 2024

A minor ConPTY refactoring: Goodbye VtEngine Edition #17510

Merged

carlos-zamora reviewed Jul 3, 2024

View reviewed changes

lhecker commented Jul 8, 2024

View reviewed changes

src/host/screenInfo.cpp Show resolved Hide resolved

lhecker added 2 commits July 9, 2024 14:44

Merge remote-tracking branch 'origin/main' into dev/lhecker/14000-vt-…

2fe302c

…cooked-read

Address feedback, Add comments, Update formatAttributes

a63b806

lhecker commented Jul 9, 2024

View reviewed changes

carlos-zamora requested changes Jul 9, 2024

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 9, 2024

lhecker added 3 commits July 9, 2024 22:33

Address j4james' feedback from the other PR

8116d5b

Fix buffer invalidation

1f5dc90

Oh, right, we support graphemes

8c7cf58

DHowett requested changes Jul 9, 2024

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 9, 2024

lhecker added 2 commits July 9, 2024 22:59

Fix formatAttributes

b3f11d5

Forgot to finish this

08f3bac

carlos-zamora approved these changes Jul 10, 2024

View reviewed changes

zadjii-msft reviewed Jul 10, 2024

View reviewed changes

zadjii-msft reviewed Jul 11, 2024

View reviewed changes

Improve comments, resize_and_overwrite is terrible

dc8ac1d

DHowett approved these changes Jul 11, 2024

View reviewed changes

DHowett added this pull request to the merge queue Jul 11, 2024

Merged via the queue into main with commit ac5b4f5 Jul 11, 2024
20 checks passed

DHowett deleted the dev/lhecker/14000-vt-cooked-read branch July 11, 2024 20:47

j4james mentioned this pull request Jul 12, 2024

Backspace in wrapped cooked read erases char on line above #17554

Closed

DHowett mentioned this pull request Jul 31, 2024

History window partially invisible when switching from bash to cmd.exe #3678

Closed


		// FYI: This loop does not loop. It exists because goto is considered evil

Use VT for COOKED_READ_DATA #17445

Use VT for COOKED_READ_DATA #17445

Conversation

lhecker commented Jun 19, 2024 • edited Loading

Validation Steps Performed

This comment has been minimized.

DHowett commented Jun 19, 2024

DHowett left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Jun 24, 2024

DHowett commented Jun 24, 2024

lhecker commented Jun 25, 2024

DHowett commented Jun 25, 2024

DHowett commented Jun 25, 2024

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

F2

F4

F9

F7

lhecker Jul 9, 2024 • 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

lhecker Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment • 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 9, 2024

carlos-zamora left a 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

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

Choose a reason for hiding this comment

lhecker commented Jun 19, 2024 •

edited

Loading

lhecker Jul 9, 2024 •

edited

Loading

lhecker Jul 4, 2024 •

edited

Loading

carlos-zamora left a comment •

edited

Loading