Skip to content

Commit

Permalink
Rewrite COOKED_READ_DATA (microsoft#15783)
Browse files Browse the repository at this point in the history
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 microsoft#1377
Closes microsoft#1503
Closes microsoft#4628
Closes microsoft#4975
Closes microsoft#5033
Closes microsoft#8008

This commit is required to fix microsoft#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 ✅
  • Loading branch information
lhecker committed Aug 25, 2023
1 parent 2fb4a7f commit 821ae3a
Show file tree
Hide file tree
Showing 59 changed files with 1,410 additions and 7,058 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/alphabet.txt
Expand Up @@ -21,6 +21,7 @@ BBBBCCCCC
BBGGRR
efg
EFG
efgh
EFGh
KLMNOQQQQQQQQQQ
QQQQQQQQQQABCDEFGHIJ
Expand Down
13 changes: 3 additions & 10 deletions .github/actions/spelling/expect/expect.txt
Expand Up @@ -17,7 +17,6 @@ ADDALIAS
ADDREF
ADDSTRING
ADDTOOL
AEnd
AFew
AFill
AFX
Expand Down Expand Up @@ -679,7 +678,7 @@ FSINFOCLASS
fte
Ftm
Fullscreens
fullwidth
Fullwidth
FUNCTIONCALL
fuzzer
fuzzmain
Expand Down Expand Up @@ -923,7 +922,6 @@ itermcolors
ITerminal
itf
Ith
itoa
IUI
IUnknown
ivalid
Expand Down Expand Up @@ -1102,6 +1100,7 @@ MDs
MEASUREITEM
megamix
memallocator
meme
MENUCHAR
MENUCONTROL
MENUDROPALIGNMENT
Expand Down Expand Up @@ -1588,7 +1587,6 @@ rgrc
rgs
rgui
rgw
rgwch
RIGHTALIGN
RIGHTBUTTON
riid
Expand Down Expand Up @@ -1783,7 +1781,6 @@ STDMETHODCALLTYPE
STDMETHODIMP
STGM
stl
stoutapot
Stri
Stringable
STRINGTABLE
Expand Down Expand Up @@ -1838,7 +1835,6 @@ TBM
tchar
TCHFORMAT
TCI
tcome
tcommandline
tcommands
Tdd
Expand All @@ -1865,8 +1861,7 @@ testname
TESTNULL
testpass
testpasses
testtestabc
testtesttesttesttest
testtimeout
TEXCOORD
texel
TExpected
Expand Down Expand Up @@ -2083,7 +2078,6 @@ vtseq
vtterm
vttest
VWX
waaay
waitable
WANSUNG
WANTARROWS
Expand Down Expand Up @@ -2201,7 +2195,6 @@ wprp
wprpi
wregex
writeback
writechar
WRITECONSOLE
WRITECONSOLEINPUT
WRITECONSOLEOUTPUT
Expand Down
58 changes: 58 additions & 0 deletions src/buffer/out/textBuffer.cpp
Expand Up @@ -416,6 +416,64 @@ size_t TextBuffer::GraphemePrev(const std::wstring_view& chars, size_t position)
return til::utf16_iterate_prev(chars, position);
}

// 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) const
{
const til::CoordType maxX = _width - 1;
const til::CoordType maxY = _height - 1;
auto x = std::clamp(position.x, 0, maxX);
auto y = std::clamp(position.y, 0, maxY);
auto row = &GetRowByOffset(y);

if (distance < 0)
{
do
{
if (x > 0)
{
x = row->NavigateToPrevious(x);
}
else if (y <= 0)
{
break;
}
else
{
--y;
row = &GetRowByOffset(y);
x = row->GetReadableColumnCount() - 1;
}
} while (++distance != 0);
}
else if (distance > 0)
{
auto rowWidth = row->GetReadableColumnCount();

do
{
if (x < rowWidth)
{
x = row->NavigateToNext(x);
}
else if (y >= maxY)
{
break;
}
else
{
++y;
row = &GetRowByOffset(y);
rowWidth = row->GetReadableColumnCount();
x = 0;
}
} while (--distance != 0);
}

return { x, y };
}

// This function is intended for writing regular "lines" of text as it'll set the wrap flag on the given row.
// You can continue calling the function on the same row as long as state.columnEnd < state.columnLimit.
void TextBuffer::Write(til::CoordType row, const TextAttribute& attributes, RowWriteState& state)
Expand Down
2 changes: 2 additions & 0 deletions src/buffer/out/textBuffer.hpp
Expand Up @@ -138,6 +138,8 @@ class TextBuffer final
static size_t GraphemeNext(const std::wstring_view& chars, size_t position) noexcept;
static size_t GraphemePrev(const std::wstring_view& chars, size_t position) noexcept;

til::point NavigateCursor(til::point position, til::CoordType distance) const;

// Text insertion functions
void Write(til::CoordType row, const TextAttribute& attributes, RowWriteState& state);
void FillRect(const til::rect& rect, const std::wstring_view& fill, const TextAttribute& attributes);
Expand Down
24 changes: 2 additions & 22 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
Expand Up @@ -21,7 +21,6 @@
#include "../host/readDataCooked.hpp"
#include "../host/output.h"
#include "../host/_stream.h" // For WriteCharsLegacy
#include "../host/cmdline.h" // For WC_INTERACTIVE
#include "test/CommonState.hpp"

#include "../cascadia/TerminalCore/Terminal.hpp"
Expand Down Expand Up @@ -3165,20 +3164,6 @@ void ConptyRoundtripTests::NewLinesAtBottomWithBackground()
verifyBuffer(*termTb, term->_mutableViewport.ToExclusive());
}

void doWriteCharsLegacy(SCREEN_INFORMATION& screenInfo, const std::wstring_view string, DWORD flags = 0)
{
auto dwNumBytes = string.size() * sizeof(wchar_t);
VERIFY_NT_SUCCESS(WriteCharsLegacy(screenInfo,
string.data(),
string.data(),
string.data(),
&dwNumBytes,
nullptr,
screenInfo.GetTextBuffer().GetCursor().GetPosition().x,
flags,
nullptr));
}

void ConptyRoundtripTests::WrapNewLineAtBottom()
{
// The actual bug case is
Expand Down Expand Up @@ -3220,11 +3205,6 @@ void ConptyRoundtripTests::WrapNewLineAtBottom()
return;
}

// I've tested this with 0x0, 0x4, 0x80, 0x84, and 0-8, and none of these
// flags seem to make a difference. So we're just assuming 0 here, so we
// don't test a bunch of redundant cases.
const auto writeCharsLegacyMode = 0;

// This test was originally written for
// https://github.com/microsoft/terminal/issues/5691
//
Expand Down Expand Up @@ -3263,7 +3243,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottom()
}
else if (writingMethod == PrintWithWriteCharsLegacy)
{
doWriteCharsLegacy(si, str, writeCharsLegacyMode);
WriteCharsLegacy(si, str, false, nullptr);
}
};

Expand Down Expand Up @@ -3421,7 +3401,7 @@ void ConptyRoundtripTests::WrapNewLineAtBottomLikeMSYS()
}
else if (writingMethod == PrintWithWriteCharsLegacy)
{
doWriteCharsLegacy(si, str, WC_INTERACTIVE);
WriteCharsLegacy(si, str, true, nullptr);
}
};

Expand Down

0 comments on commit 821ae3a

Please sign in to comment.