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.

Sign up for GitHub

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

Jump to bottom

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

Closed
lhecker opened this issue Aug 29, 2023 · 0 comments · Fixed by #15930
Closed

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

lhecker opened this issue Aug 29, 2023 · 0 comments · Fixed by #15930
Assignees
@lhecker
Labels
Area-CookedRead The cmd.exe COOKED_READ handling In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v1.19

Comments

@lhecker
Copy link
Member

lhecker commented Aug 29, 2023 •
edited
Loading

Steps to reproduce

  • Make Windows Terminal small
  • Launch cmd.exe
  • Write lots of text, more than fit into the viewport

Expected Behavior

No flickering and everything works as if there's infinite scrollback.

Actual Behavior

To bring my feelings across I wrote a haiku:

The window flickering was hell.
The scrolling drained me of life's happiness.
Cooked read drives me nuts.
@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. Priority-1 A description (P1) Area-CookedRead The cmd.exe COOKED_READ handling labels Aug 29, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 29, 2023
@lhecker lhecker self-assigned this Aug 30, 2023
@lhecker lhecker added this to the Terminal v1.19 milestone Aug 30, 2023
@lhecker lhecker removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 30, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Aug 30, 2023
@lhecker lhecker mentioned this issue Sep 5, 2023
Merged
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Sep 5, 2023
DHowett pushed a commit that referenced this issue Sep 8, 2023
@lhecker
COOKED_READ_DATA: Fix scrolling under ConPTY (#15930)
4ddfc3e 
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:
  ```pwsh
  "" + (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 ✅
@lhecker lhecker mentioned this issue Sep 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-CookedRead The cmd.exe COOKED_READ handling In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements 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
Milestone
Terminal v1.19
Development

Successfully merging a pull request may close this issue.

COOKED_READ_DATA: Fix scrolling under ConPTY microsoft/terminal
1 participant
@lhecker