Rewrite COOKED_READ_DATA #15783

lhecker · 2023-07-31T20:05:25Z

This massive refactoring has two goals:

Enable us to go beyond UCS-2 support for input editing
Bring clarity into COOKED_READ_DATA's inner workings

Unfortunately, over time, knowledge about its exact operation was lost.
While the new code is still complex it reduces the amount of code by 4x
which will make preserving knowledge hopefully significantly easier.

The new implementation is simpler and slower than the old one in a way,
because every time the input line is modified it's rewritten to the text
buffer from scratch. This however massively simplifies the underlying
algorithm and the amount of state that needs to be tracked and results
in a significant reduction in code size. It also makes it more robust,
because there's less code now that can be incorrect.

This "optimization laziness" can be afforded due the recent >10x
improvements to TextBuffer's text ingestion performance.
For short inputs (<1000 characters) I still expect this implementation
to outperform the conhost from the past.
It has received one optimization already however: While reading text
from the InputBuffer we'll now defer writing into the TextBuffer
until we've stopped reading. This improves the overhead of pasting text
from O(n^2) to O(n), which is immediately noticeable for inputs >100kB.

Resizing the text buffer still ends up corrupting the input line
however, which unfortunately cannot be fixed in COOKED_READ_DATA.
The issue occurs due to bugs in TextBuffer::Reflow itself, as it
misplaces the cursor if the prompt is on the last line of the buffer.

Closes #1377
Closes #1503
Closes #4628
Closes #4975
Closes #5033
Closes #8008

This commit is required to fix #797

Validation Steps Performed

ASCII input ✅
Chinese input (中文維基百科) ❔
- Resizing the window properly wraps/unwraps wide glyphs ❌
  Broken due to TextBuffer::Reflow bugs
Surrogate pair input (🙂) ❔
- Resizing the window properly wraps/unwraps surrogate pairs ❌
  Broken due to TextBuffer::Reflow bugs
In cmd.exe
- Create 2 file: "a😊b.txt" and "a😟b.txt"
- Press tab: Autocompletes "a😊b.txt" ✅
- Navigate the cursor right past the "a"
- Press tab twice: Autocompletes "a😟b.txt" ✅
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 ✅
- Minimum size of 40x10 characters ✅
- Width expands to fit the widest history command ✅
- Height expands up to 20 rows with longer histories ✅
- F9 starts "command number" prompt ✅
- Left/Right paste 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 ✅
  - Wide text rendering during pagination with >20 entries ✅
- Shift+Up/Down moves history items around ✅
- Home navigates to first entry ✅
- End navigates to last entry ✅
- PageUp navigates by 20 items at a time or to first ✅
- PageDown navigates by 20 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 ✅

DHowett · 2023-07-31T23:32:13Z

its_happening.gif

lhecker · 2023-08-11T13:26:25Z

The latest commit (or rather rebase) should address all the issues we found. 🙂

lhecker · 2023-08-11T13:45:57Z

src/host/screenInfo.cpp

-        // MSFT:19976291 Don't re-show the commandline here. We need to wait for
-        //      the viewport to also get resized before we can re-show the commandline.
-        //      ProcessResizeWindow will call commandline.Show() for us.
-        _textBuffer->GetCursor().SetIsVisible(savedCursorVisibility);


The reason the old code has these commandLine.Hide(); and Show() calls everywhere is because of this one exception here where it explicitly doesn't Show() it again immediately.

But I read into MSFT:19976291 (summary: COOKED_READ_DATA could crash conhost when you resize the window quickly & randomly) and then tried to test it with my reimplementation and it doesn't seem to reproduce. I don't think the issue can happen anymore, because the EraseBeforeResize() and RedrawAfterResize() implementation is a bit more robust now.

As such all of these are gone and moved a couple lines below in this diff into SCREEN_INFORMATION::ResizeScreenBuffer.

zadjii-msft

leaving notes as I go. This might take a while.

zadjii-msft · 2023-08-09T20:37:53Z

src/interactivity/win32/menu.cpp

-            const auto pCommandLine = &CommandLine::Instance();
-
-            pCommandLine->Hide(FALSE);
-
            LOG_IF_FAILED(ScreenInfo.ResizeScreenBuffer(coordBuffer, TRUE));


note to self: does resizing redraw the prompt appropriately? I'd be shocked if it didn't

answer: yes, this is in SCREEN_INFORMATION::ResizeScreenBuffer in a scope_exit

zadjii-msft · 2023-08-09T20:41:58Z

src/host/screenInfo.cpp

+        auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+        if (gci.HasPendingCookedRead())
+        {
+            gci.CookedReadData().RedrawAfterResize();
+        }


nit: could we just do this in the same scope_exit as the above one (endResize)?

zadjii-msft · 2023-08-15T21:09:32Z

src/buffer/out/textBuffer.cpp

+// Pretend as if `position` is a regular cursor in the TextBuffer.
+// This function will then pretend as if you pressed the left/right arrow
+// keys `distance` amount of times (negative = left, positive = right).
+til::point TextBuffer::NavigateCursor(til::point position, til::CoordType distance)


oh lmao so it's like the opposite of the GetCellDistance thing I did

Yep exactly! 😅 I use it to implement the whacky initialData argument for cooked read.

src/buffer/out/textBuffer.cpp

src/host/readDataCooked.cpp

zadjii-msft · 2023-08-17T16:00:17Z

src/host/readDataCooked.cpp

-    // or a write routine. Both of these callers grab the current console
-    // lock.
-
-    // MSFT:13994975 This is REALLY weird.


Is this scary comment no longer relevant? I remember this as one of the wackiest bits of COOKED_READ we ever debugged.

Seems like it's okay because we're not stashing _pdwNumBytes anywhere... though, are we ever writing to pNumBytes now?

I don't think this is relevant anymore. The existing cooked read instance now owns the popups and will directly feed them with input.

zadjii-msft · 2023-08-17T16:19:37Z

src/host/alias.cpp

-    auto sourceCopy = sourceText;
-
-    // Trim trailing \r\n off of sourceCopy if it has one.
-    s_TrimTrailingCrLf(sourceCopy);


Will the source text no longer have trailing crlf's?

I moved the removal of trailing newlines into the cooked read class. Technically we can also continue to remove the CRLF in alias.cpp - it doesn't really matter. I removed this code, because cooked read also depends on the newlines being removed and so this was redundant.

src/host/readDataCooked.cpp

zadjii-msft · 2023-08-17T16:55:05Z

src/host/readDataCooked.cpp


-    if (_ctrlWakeupMask != 0 && wch < L' ' && (_ctrlWakeupMask & (1 << wch)))


okay this went to COOKED_READ_DATA::_handleChar

zadjii-msft · 2023-08-17T16:57:27Z

src/host/readDataCooked.cpp


-        if (wch != UNICODE_BACKSPACE || _bufPtr != _backupLimit)
+        if (hasPopup)


note to self: This is about where I was 🔖

DHowett · 2023-08-23T18:23:09Z

I'm sure there'll be more feedback from whomever the second on this is.

dying

DHowett · 2023-08-23T19:46:45Z

src/host/getset.cpp

-            // GH#1856 - make sure to hide the commandline _before_ we execute
-            // the resize, and the re-display it after the resize. If we leave
-            // it displayed, we'll crash during the resize when we try to figure
-            // out if the bounds of the old commandline fit within the new
-            // window (it might not).


neat! this is no longer a concern?

Yeah, see here: #15783 (comment)

DHowett

You know what? I reviewed roughly the whole thing and only came up with 6 comments. I'm ready to sign off, but I have a couple comments first.

src/host/readDataCooked.hpp

src/host/server.h

src/host/readDataCooked.cpp

DHowett · 2023-08-23T21:19:06Z

Chinese input (中文維基百科) ❔
Resizing the window properly wraps/unwraps wide glyphs ❌
Broken due to TextBuffer::Reflow bugs
Surrogate pair input (🙂) ❔
Resizing the window properly wraps/unwraps surrogate pairs ❌
Broken due to TextBuffer::Reflow bugs

Why are these broken? You clear the commandline before resizing, and redraw it completely at the new size. Where does Reflow come in?

lhecker · 2023-08-24T12:42:36Z

Why are these broken? You clear the commandline before resizing, and redraw it completely at the new size. Where does Reflow come in?

This code doesn't work:

terminal/src/buffer/out/textBuffer.cpp

Lines 2654 to 2684 in e10b7e4

    
           // Advance the cursor to the same offset as before 
        
           // get the number of newlines and spaces between the old end of text and the old cursor, 
        
           //   then advance that many newlines and chars 
        
           auto iNewlines = cOldCursorPos.y - cOldLastChar.y; 
        
           const auto iIncrements = cOldCursorPos.x - cOldLastChar.x; 
        
           const auto cNewLastChar = newBuffer.GetLastNonSpaceCharacter(); 
        
           // If the last row of the new buffer wrapped, there's going to be one less newline needed, 
        
           //   because the cursor is already on the next line 
        
           if (newBuffer.GetRowByOffset(cNewLastChar.y).WasWrapForced()) 
        
           { 
        
               iNewlines = std::max(iNewlines - 1, 0); 
        
           } 
        
           else 
        
           { 
        
               // if this buffer didn't wrap, but the old one DID, then the d(columns) of the 
        
               //   old buffer will be one more than in this buffer, so new need one LESS. 
        
               if (oldBuffer.GetRowByOffset(cOldLastChar.y).WasWrapForced()) 
        
               { 
        
                   iNewlines = std::max(iNewlines - 1, 0); 
        
               } 
        
           } 
        
           for (auto r = 0; r < iNewlines; r++) 
        
           { 
        
               newBuffer.NewlineCursor(); 
        
           } 
        
           for (auto c = 0; c < iIncrements - 1; c++) 
        
           { 
        
               newBuffer.IncrementCursor(); 
        
           }

#15701 fixes it.

zadjii-msft · 2023-08-24T13:26:13Z

#15701 fixes it.

Psh that PR's only a ~800 LOC delta. That's easy compared to the last couple here

DHowett

I accept that we are going to break something, but that breaking something is necessary to improve [[gestures broadly at everything]]

zadjii-msft · 2023-08-24T22:47:29Z

DHowett · 2023-08-24T23:13:03Z

Now you just need to not conflict yourself! :D

DHowett · 2023-08-25T17:19:27Z

src/host/_stream.cpp

@@ -206,7 +206,7 @@ void WriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view& t
                // Otherwise handling backspacing tabs/whitespace can turn up complex and bug-prone.
                assert(!interactive);
                auto pos = cursor.GetPosition();
-                pos.x = textBuffer.GetRowByOffset(pos.y).NavigateToPrevious(pos.x);
+                pos.x = textBuffer.GetMutableRowByOffset(pos.y).NavigateToPrevious(pos.x);


Why did this one require mutability?

Huh, yeah, good point.

A late change in #16105 wrapped `_buffer` into a class to better track its dirty state, but I failed to notice that in this one instance we intentionally manipulated `_buffer` without marking it as dirty. This fixes the issue by adding a call to `MarkAsClean()`. This changeset also adds the test instructions from #15783 as a document to this repository. I've extended the list with two bugs we've found in the implementation since then. ## Validation Steps Performed * In cmd.exe, with an empty prompt in an empty directory: Pressing tab produces an audible bing and prints no text ✅

A late change in #16105 wrapped `_buffer` into a class to better track its dirty state, but I failed to notice that in this one instance we intentionally manipulated `_buffer` without marking it as dirty. This fixes the issue by adding a call to `MarkAsClean()`. This changeset also adds the test instructions from #15783 as a document to this repository. I've extended the list with two bugs we've found in the implementation since then. ## Validation Steps Performed * In cmd.exe, with an empty prompt in an empty directory: Pressing tab produces an audible bing and prints no text ✅ (cherry picked from commit 7a8dd90) Service-Card-Id: 91033502 Service-Version: 1.19

A late change in microsoft#16105 wrapped `_buffer` into a class to better track its dirty state, but I failed to notice that in this one instance we intentionally manipulated `_buffer` without marking it as dirty. This fixes the issue by adding a call to `MarkAsClean()`. This changeset also adds the test instructions from microsoft#15783 as a document to this repository. I've extended the list with two bugs we've found in the implementation since then. ## Validation Steps Performed * In cmd.exe, with an empty prompt in an empty directory: Pressing tab produces an audible bing and prints no text ✅

lhecker force-pushed the dev/lhecker/8000-cmdline branch from 7e79ab5 to dfc0896 Compare July 31, 2023 20:06

lhecker mentioned this pull request Aug 1, 2023

[Epic] COOKED READ and character conversion fun with all our different Convert* functions #7777

Closed

lhecker force-pushed the dev/lhecker/8000-cmdline-prep4 branch from 31f7961 to aa4fdda Compare August 1, 2023 19:38

Base automatically changed from dev/lhecker/8000-cmdline-prep4 to main August 1, 2023 22:46

lhecker force-pushed the dev/lhecker/8000-cmdline branch from 78fd2f4 to 20415e8 Compare August 1, 2023 23:00

lhecker marked this pull request as ready for review August 1, 2023 23:00

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

Rewrite COOKED_READ_DATA

b47784d

lhecker force-pushed the dev/lhecker/8000-cmdline branch from 004546d to b47784d Compare August 11, 2023 13:25

This comment has been minimized.

Sign in to view

What's wrong with would've?

5f683cf

lhecker commented Aug 11, 2023

View reviewed changes

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

4b8e5e2

zadjii-msft reviewed Aug 17, 2023

View reviewed changes

zadjii-msft mentioned this pull request Aug 17, 2023

doskey macros broken for inputs with >1 consecutive whitespace #15736

Closed

lhecker added 2 commits August 21, 2023 19:53

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

46f8ee2

Address feedback, Fix ENABLE_PROCESSED_INPUT

80fa72d

DHowett reviewed Aug 23, 2023

View reviewed changes

Address feedback

4f09dab

lhecker added 4 commits August 24, 2023 18:03

Remove loop around _popupsDone

3ca867c

Avoid unnecessary redraws

69532f2

Get GetReadableColumnCount from that other PR

2b467fd

Fix incorrect cursor position with git add -p

1f5504f

DHowett approved these changes Aug 24, 2023

View reviewed changes

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

b9302d7

DHowett approved these changes Aug 25, 2023

View reviewed changes

DHowett reviewed Aug 25, 2023

View reviewed changes

Address feedback

cd3a749

microsoft-github-policy-service bot added the Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) label Aug 25, 2023

DHowett approved these changes Aug 25, 2023

View reviewed changes

DHowett enabled auto-merge (squash) August 25, 2023 17:58

DHowett merged commit 821ae3a into main Aug 25, 2023
15 of 17 checks passed

DHowett deleted the dev/lhecker/8000-cmdline branch August 25, 2023 18:25

lhecker mentioned this pull request Sep 25, 2023

ReadConsole does not work with utf-8 codepage #16020

Closed

j4james mentioned this pull request Oct 12, 2023

Crash when toggling alt buffer in cmd shell #16158

Closed

lhecker mentioned this pull request Nov 7, 2023

Fix tabs being printed in cmd.exe prompts #16273

Merged

zadjii-msft mentioned this pull request Nov 13, 2023

interaction of _read on fd 0 in terminal #16299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite COOKED_READ_DATA #15783

Rewrite COOKED_READ_DATA #15783

lhecker commented Jul 31, 2023 •

edited

DHowett commented Jul 31, 2023

lhecker commented Aug 11, 2023

This comment has been minimized.

lhecker Aug 11, 2023 •

edited

zadjii-msft left a comment

zadjii-msft Aug 9, 2023

zadjii-msft Aug 9, 2023

zadjii-msft Aug 9, 2023

zadjii-msft Aug 15, 2023

lhecker Aug 21, 2023

zadjii-msft Aug 17, 2023

lhecker Aug 21, 2023

zadjii-msft Aug 17, 2023

lhecker Aug 21, 2023 •

edited

zadjii-msft Aug 17, 2023

zadjii-msft Aug 17, 2023

DHowett commented Aug 23, 2023

DHowett Aug 23, 2023

lhecker Aug 23, 2023

DHowett left a comment

DHowett commented Aug 23, 2023

lhecker commented Aug 24, 2023

zadjii-msft commented Aug 24, 2023

DHowett left a comment

zadjii-msft commented Aug 24, 2023

DHowett commented Aug 24, 2023

DHowett Aug 25, 2023

lhecker Aug 25, 2023


		if (_ctrlWakeupMask != 0 && wch < L' ' && (_ctrlWakeupMask & (1 << wch)))


		if (wch != UNICODE_BACKSPACE \|\| _bufPtr != _backupLimit)
		if (hasPopup)

Rewrite COOKED_READ_DATA #15783

Rewrite COOKED_READ_DATA #15783

Conversation

lhecker commented Jul 31, 2023 • edited

Validation Steps Performed

DHowett commented Jul 31, 2023

lhecker commented Aug 11, 2023

This comment has been minimized.

lhecker Aug 11, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Aug 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Aug 23, 2023

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

DHowett commented Aug 23, 2023

lhecker commented Aug 24, 2023

zadjii-msft commented Aug 24, 2023

DHowett left a comment

Choose a reason for hiding this comment

zadjii-msft commented Aug 24, 2023

DHowett commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Jul 31, 2023 •

edited

lhecker Aug 11, 2023 •

edited

lhecker Aug 21, 2023 •

edited