Skip to content

COOKED_READ_DATA: Fix scrolling under ConPTY - #15930

Merged
Dustin L. Howett (DHowett) merged 1 commit into
mainfrom
dev/lhecker/15899-cooked-read-fixup
Sep 8, 2023
Merged

Dustin L. Howett (DHowett) merged 1 commit into
mainfrom
dev/lhecker/15899-cooked-read-fixup

Conversation

@lhecker

@lhecker Leonard Hecker (lhecker) commented Sep 5, 2023 •

Copy link
Copy Markdown
Member

This commit fixes 3 bugs:

  • COOKED_READ_DATA failed to initialize its _distanceCursor and
    _distanceEnd members. I took this as an opportunity to make them
    ptrdiff_t, to reduce the likelihood of overflows in the future.
  • COOKED_READ_DATA::_writeChars added scrollY to the written
    distance, even though WriteCharsLegacy writes a negative value into
    that out parameter. This was fixed by changing WriteCharsLegacy to
    write positive values and by adding a debug assertion.
  • StreamScrollRegion calls IncrementCircularBuffer which causes a
    synchronous (!) ConPTY flush to the output pipe (side note: this is
    the primary reason why newlines are so slow in ConPTY).
    Since cooked reads are supposed to behave like a pager and not write
    into the scrollback, we temporarily mark the buffer as inactive
    which prevents TextBuffer from snitching about it to VtEngine.

Even after this change, there's still some weird behavior left:

  • You cannot move your cursor back beyond (0,0), because this isn't a
    real pager-like implementation. That might be a neat future extension.
  • Writing a lot of text and pressing Ctrl+C doesn't properly place the
    cursor and scroll the buffer, unless the cursor is at the end.
    That might also be worth investigating in the future (minor issue).
  • When the viewport is full, backspacing more than 1 line of text
    (using Ctrl+Backspace) doesn't erase all of the affected lines,
    because COOKED_READ_DATA::_erase uses the same WriteCharsLegacy
    function to write whitespace to erase that text. It's only gone
    after typing one more character.
    I've written the code to mostly fix this, but decided against it
    as I considered the problem to be too niche to warrant extra code.

Closes #15899

Validation Steps Performed

  • Generate some text to paste in PowerShell:
    "" + (0..512 | % { "word" + $_.ToString().PadLeft(4, "0") })
  • Launch cmd.exe and paste that text
  • No flickering ✅
  • No writing into the scrollback ✅
  • No weird behavior when backspacing ✅

@microsoft-github-policy-service microsoft-github-policy-service Bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-CookedRead The cmd.exe COOKED_READ handling Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Sep 5, 2023
@lhecker
Leonard Hecker (lhecker) force-pushed the dev/lhecker/15899-cooked-read-fixup branch 2 times, most recently from 1bc9a02 to 9a9cd5c Compare September 5, 2023 14:51
@lhecker
Leonard Hecker (lhecker) force-pushed the dev/lhecker/15899-cooked-read-fixup branch from 9a9cd5c to 0e20dcd Compare September 6, 2023 15:24
else if (writingMethod == PrintWithWriteCharsLegacy)
{
WriteCharsLegacy(si, str, true, nullptr);
WriteCharsLegacy(si, str, false, nullptr);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This makes the tests pass. I don't think they ever relied on the interactive behavior of WriteCharsLegacy.

const auto distanceAfterCursor = _writeChars(view.substr(_bufferCursor));
const auto distanceEnd = distanceBeforeCursor + distanceAfterCursor;
const auto eraseDistance = std::max(0, _distanceEnd - distanceEnd);
const auto eraseDistance = std::max<ptrdiff_t>(0, _distanceEnd - distanceEnd);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After a lot of back and forth, I went with ptrdiff_t after all. That way we can have bigger buffers in the future and make full use of our int32_t coordinates.

const auto finalCursorPos = cursor.GetPosition();
return (finalCursorPos.y - initialCursorPos.y + scrollY) * width + finalCursorPos.x - initialCursorPos.x;
const auto distance = (finalCursorPos.y - initialCursorPos.y + scrollY) * width + finalCursorPos.x - initialCursorPos.x;
assert(distance >= 0);

@lhecker Leonard Hecker (lhecker) Sep 6, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This helped me find the 2nd bug.

Comment on lines +143 to +144
ptrdiff_t _distanceCursor = 0;
ptrdiff_t _distanceEnd = 0;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Whoops. Forgot to 0 initialize.

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 Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

COOKED_READ: Writing more text than fit into the ConPTY viewport behaves weirdly

3 participants