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

Flattening the window loses scrollback lines #3490

Closed
egmontkob opened this issue Nov 8, 2019 · 2 comments · Fixed by #4741
Closed

Flattening the window loses scrollback lines #3490

egmontkob opened this issue Nov 8, 2019 · 2 comments · Fixed by #4741
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) 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-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@egmontkob
Copy link

Environment

Windows build number: 10.0.18362.0
Windows Terminal version (if applicable): 0.6.2951.0

Steps to reproduce

  • ssh to a Linux box
  • seq 100
  • shrink the window vertically (fewer lines)
  • scroll back

Expected behavior

See the numbers from 1 to 100

Actual behavior

Lines get permanently lost as the window is made smaller, e.g. the sequence goes like ... 70 71 72 73 78 79 80 81 ...

@ghost ghost added 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 Nov 8, 2019
@zadjii-msft zadjii-msft added Product-Conpty For console issues specifically related to conpty Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Nov 11, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 11, 2019
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Nov 11, 2019
@zadjii-msft zadjii-msft added the Priority-1 A description (P1) label Jan 9, 2020
@zadjii-msft zadjii-msft self-assigned this Jan 16, 2020
@zadjii-msft
Copy link
Member

Wow this might be crazy hard to fix with conpty. I've put together a prototype branch to fix this, and it just keeps getting more and more ridiculous. Closer and closer to the expected behavior, but the edge cases might not be solvable here.

From a PR description that I typed up before realizing my fixes weren't quite right:

  • Conpty was not emitting the correct thing when you'd decrease the height of the viewport. When you increase the height of the window, we'd both invalidate the entire buffer, and insert new lines to the bottom of the buffer. What this meant is that on every height-only resize, we'd re-emit the entire viewport.
    • If you resized down twice (From R to R-1 to R-2), once during the frame being emitted by conpty, the Terminal would try to process the R-1 lines emitted by conpty when there are only R-2 available rows in the terminal buffer. This would tlead to duplicated lines.
  • This buggy conpty behavior was causing us to correctly believe that the terminal viewport should remain "stuck" to the top position. That was what was causing us to apparently erase lines from the scrollback as we'd resize down. So this also changes the terminal to stick to the bottom when the viewport height changes.

What I've gathered from other terminal is that when you increase the height of the buffer, one of two things happens:

  1. If there are still lines in the scrollback, those scrollback lines get pushed back into the viewport.
  2. If there aren't, then blank lines are inserted at the bottom.

The trick for us here is that conpty isn't a dumb pipe - there's fundamentally a console buffer that we're trying to sync with the state of the terminal connected to it. Additionally, conpty doesn't have a scrollback at all, and it doesn't know anything about the terminal. It doesn't know if the terminal has a scrollback at all, or how many lines are in the terminal's scrollback.

I had a good amount of success with "padding the top" of the conpty buffer when we increase in height, trying to cover case 1. By "padding the top", conpty can create space in it's buffer where it thinks that lines from the terminal will be. However, things break down at that case 2 scenario. Conpty doesn't know when there aren't more lines in the scrollback. Conpty can know how many lines it's theoretically moved to scrollback, but it doesn't actually know if all of those were kept by the terminal - consider emitting 100 lines to something with only 10 lines of scrollback. Conpty would mistakenly think that it would need to "pad the top" 90 more times than the terminal actually would.

Peculiarly enough, this all works well enough until a client application tries to get the cursor position. Until then, the text we're emitting from conpty just so happens to work well enough relative to previous output that the terminal has the expected text in it. As soon as a client app requests the cursor position, that's when things start to break down. The case I'm thinking of particularly is powershell with PSReadline. PSReadline will get the cursor position, and re-move the cursor to that position while typing at the prompt. When PSR tries this after we've incorrectly padded the top, then the cursor in the terminal seems to "jump down" to where the conpty thinks the cursor should be.

I could maybe solve this by having conpty immediately ask the terminal for its cursor position on a ResizePseudoconsole (with a DSR). With that response, we could know where the terminal thinks the cursor is - if it's on the same line, then the terminal inserted blank lines to the bottom of the buffer. If the cursor has moved down, then lines were moved from scrollback.

However, the problem would be that the response would come in asynchronously to that thread. The ResizePseudoconsole handler is on a separate thread from the input handler. The input handler might already be blocked on a ReadFile from the input handle, so if the PtySignalThread tried to also ReadFile the input handle, there'd be no way to know who would handle the read first. The PtySignalThread would need to release the global lock before the read, but one of the input thread or signal thread would complete the read, but maybe after the renderer thread has another go at it. Having the signal thread do the read is a bad idea. But there would be no way to ensure the input thread is the next thread scheduled. We might paint a bad frame to the terminal after the terminal requested a resize, but before we've actually handled the resize.

This last paragraph altogether feels hacky to me anyways. I'll have to keep thinking about this, and maybe prototyping more.

zadjii-msft added a commit that referenced this issue Jan 23, 2020
  * don't padTop on resize
  * always pin to top
    - caveat: sometimes, move the viewport down a bit, when the buffer was full before the resize

  This seems to work better for the cases that weren't working before. It doesn't behave like gnome-terminal, but it works.

  Perf seems _really_ bad, but maybe I've just got a rogue sihost.exe.

  Also hey let's link this branch to the issue #3490
@ghost ghost added the In-PR This issue has a related PR label Jan 24, 2020
@zadjii-msft
Copy link
Member

I'm getting the sinking suspicion that #4354 is wrong, but without more time to complete it, I'm leaving a note for here, so that I'll remember.

There's probably benefit in improving the conpty invalidating story - I've long thought that just InvalidateAlling is the wrong solution. I'm not sure 4354 is right either.

Maybe it's possible that just the viewport moving code is sufficient to get this right. Maybe pull the rest and just move the viewport a little better. I came at this a really strange angle, from first thinking the terminal was doing it wrong, because other terminal behaved a certain way, then changing conpty to try and act like other terminals, realizing it really really doesn't, then changing Terminal to try and behave in accordance with conpty, without reverting everything and starting fresh. I made too many wrong assumptions in a row, and I think I maybe accumulated some of those bad assumptions when I shouldn't have. I'm thinking now that I should scope this smaller - resizing down quickly might still be broken, resizing diagonally might still be broken, but it'll be no worse than it currently is in both those cases

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Mar 13, 2020
DHowett-MSFT pushed a commit that referenced this issue Mar 13, 2020
This PR adds support for "Resize with Reflow" to the Terminal. In
conhost, `ResizeWithReflow` is the function that's responsible for
reflowing wrapped lines of text as the buffer gets resized. Now that
#4415 has merged, we can also implement this in the Terminal. Now, when
the Terminal is resized, it will reflow the lines of it's buffer in the
same way that conhost does. This means, the terminal will no longer chop
off the ends of lines as the buffer is too small to represent them. 

As a happy side effect of this PR, it also fixed #3490. This was a bug
that plagued me during the investigation into this functionality. The
original #3490 PR, #4354, tried to fix this bug with some heavy conpty
changes. Turns out, that only made things worse, and far more
complicated. When I really got to thinking about it, I realized "conhost
can handle this right, why can't the Terminal?". Turns out, by adding
resize with reflow, I was also able to fix this at the same time.
Conhost does a little bit of math after reflowing to attempt to keep the
viewport in the same relative place after a reflow. By re-using that
logic in the Terminal, I was able to fix #3490.

I also included that big ole test from #3490, because everyone likes
adding 60 test cases in a PR.

## References
* #4200 - this scenario
* #405/#4415 - conpty emits wrapped lines, which was needed for this PR
* #4403 - delayed EOL wrapping via conpty, which was also needed for
  this
* #4354 - we don't speak of this PR anymore

## PR Checklist
* [x] Closes #1465
* [x] Closes #3490
* [x] Closes #4771
* [x] Tests added/passed

## EDIT: Changes to this PR on 5 March 2020

I learned more since my original version of this PR. I wrote that in
January, and despite my notes that say it was totally working, it
_really_ wasn't.

Part of the hard problem, as mentioned in #3490, is that the Terminal
might request a resize to (W, H-1), and while conpty is preparing that
frame, or before the terminal has received that frame, the Terminal
resizes to (W, H-2). Now, there aren't enough lines in the terminal
buffer to catch all the lines that conpty is about to emit. When that
happens, lines get duplicated in the buffer. From a UX perspective, this
certainly looks a lot worse than a couple lost lines. It looks like
utter chaos.

So I've introduced a new mode to conpty to try and counteract this
behavior. This behavior I'm calling "quirky resize". The **TL;DR** of
quirky resize mode is that conpty won't emit the entire buffer on a
resize, and will trust that the terminal is prepared to reflow it's
buffer on it's own.

This will enable the quirky resize behavior for applications that are
prepared for it. The "quirky resize" is "don't `InvalidateAll` when the
terminal resizes". This is added as a quirk as to not regress other
terminal applications that aren't prepared for this behavior
(gnome-terminal, conhost in particular). For those kinds of terminals,
when the buffer is resized, it's just going to lose lines. That's what
currently happens for them.  

When the quirk is enabled, conpty won't repaint the entire buffer. This
gets around the "duplicated lines" issue that requesting multiple
resizes in a row can cause. However, for these terminals that are
unprepared, the conpty cursor might end up in the wrong position after a
quirky resize.

The case in point is maximizing the terminal. For maximizing
(height->50) from a buffer that's 30 lines tall, with the cursor on
y=30, this is what happens: 

  * With the quirk disabled, conpty reprints the entire buffer. This is
    60 lines that get printed. This ends up blowing away about 20 lines
    of scrollback history, as the terminal app would have tried to keep
    the text pinned to the bottom of the window. The term. app moved the
    viewport up 20 lines, and then the 50 lines of conpty output (30
    lines of text, and 20 blank lines at the bottom) overwrote the lines
    from the scrollback. This is bad, but not immediately obvious, and
    is **what currently happens**. 


  * With the quirk enabled, conpty doesn't emit any lines, but the
    actual content of the window is still only in the top 30 lines.
    However, the terminal app has still moved 20 lines down from the
    scrollback back into the viewport. So the terminal's cursor is at
    y=50 now, but conpty's is at 30. This means that the terminal and
    conpty are out of sync, and there's not a good way of re-syncing
    these. It's very possible (trivial in `powershell`) that the new
    output will jump up to y=30 override the existing output in the
    terminal buffer. 

The Windows Terminal is already prepared for this quirky behavior, so it
doesn't keep the output at the bottom of the window. It shifts it's
viewport down to match what conpty things the buffer looks like.

What happens when we have passthrough mode and WT is like "I would like
quirky resize"? I guess things will just work fine, cause there won't be
a buffer behind the passthrough app that the terminal cares about. Sure,
in the passthrough case the Terminal could _not_ quirky resize, but the
quirky resize won't be wrong.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) 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-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
4 participants