Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite COOKED_READ_DATA #15783

Merged
merged 14 commits into from
Aug 25, 2023
Merged

Rewrite COOKED_READ_DATA #15783

merged 14 commits into from
Aug 25, 2023

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 31, 2023

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
Copy link
Member

DHowett commented Jul 31, 2023

its_happening.gif

@lhecker 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 lhecker marked this pull request as ready for review August 1, 2023 23:00
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CookedRead The cmd.exe COOKED_READ handling Area-Input Related to input processing (key presses, mouse, etc.) Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Priority-1 A description (P1) Priority-2 A description (P2) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Impact-Compatibility Sure don't work like it used to. labels Aug 2, 2023
@lhecker
Copy link
Member Author

lhecker commented Aug 11, 2023

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

@github-actions

This comment has been minimized.

Comment on lines -1031 to -1034
// 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);
Copy link
Member Author

@lhecker lhecker Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const auto pCommandLine = &CommandLine::Instance();

pCommandLine->Hide(FALSE);

LOG_IF_FAILED(ScreenInfo.ResizeScreenBuffer(coordBuffer, TRUE));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +1517 to +1521
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
if (gci.HasPendingCookedRead())
{
gci.CookedReadData().RedrawAfterResize();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/buffer/out/textBuffer.cpp Show resolved Hide resolved
src/host/readDataCooked.cpp Outdated Show resolved Hide resolved
// or a write routine. Both of these callers grab the current console
// lock.

// MSFT:13994975 This is REALLY weird.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

auto sourceCopy = sourceText;

// Trim trailing \r\n off of sourceCopy if it has one.
s_TrimTrailingCrLf(sourceCopy);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@lhecker lhecker Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay this went to COOKED_READ_DATA::_handleChar


if (wch != UNICODE_BACKSPACE || _bufPtr != _backupLimit)
if (hasPopup)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: This is about where I was 🔖

@DHowett
Copy link
Member

DHowett commented Aug 23, 2023

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

omae wa mou shindeiru

dying

Comment on lines -658 to -662
// 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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat! this is no longer a concern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, see here: #15783 (comment)

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
src/host/server.h Show resolved Hide resolved
src/host/readDataCooked.cpp Outdated Show resolved Hide resolved
src/host/readDataCooked.cpp Outdated Show resolved Hide resolved
src/host/readDataCooked.cpp Outdated Show resolved Hide resolved
src/host/readDataCooked.cpp Outdated Show resolved Hide resolved
@DHowett
Copy link
Member

DHowett commented Aug 23, 2023

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
Copy link
Member Author

lhecker commented Aug 24, 2023

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:

// 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
Copy link
Member

#15701 fixes it.

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

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zadjii-msft
Copy link
Member

@DHowett
Copy link
Member

DHowett commented Aug 24, 2023

Now you just need to not conflict yourself! :D

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this one require mutability?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, yeah, good point.

@microsoft-github-policy-service 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 DHowett enabled auto-merge (squash) August 25, 2023 17:58
@DHowett DHowett merged commit 821ae3a into main Aug 25, 2023
15 of 17 checks passed
@DHowett DHowett deleted the dev/lhecker/8000-cmdline branch August 25, 2023 18:25
DHowett pushed a commit that referenced this pull request Nov 7, 2023
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 ✅
DHowett pushed a commit that referenced this pull request Nov 7, 2023
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
radu-cernatescu pushed a commit to radu-cernatescu/terminal that referenced this pull request Nov 8, 2023
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 ✅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CookedRead The cmd.exe COOKED_READ handling Area-Input Related to input processing (key presses, mouse, etc.) Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Impact-Compatibility Sure don't work like it used to. Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Priority-2 A description (P2) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase zBugBash-Consider
Projects
None yet
3 participants