When the Terminal is resized, don't remove lines from scrollback #4354
Conversation
This confusingly doesn't always work
The trick was that when a line that was exactly filled was followed by an empty line, we'd ECH when the cursor is virtually on the last char of the wrapped line. That was wrong. For a scenario like: |ABCDEF| | | We'd incorrectly render that as ["ABCDEF", "^[[K", "\r\n"]. That ECH in the middle there would erase the 'F', because the cursor was virtually still on the 'F' (because it had deferred wrapping to the next char). So, when we're about to ECH following a wrapped line, reset the _wrappedRow first, to make sure we correctly \r\n first.
…hich are blocked on #4213
…duplicated. I believe this is due to conpty painting too many lines. When we resize from Y to Y-1 lines, we'll emit Y-1 lines of text. If while we're rendering, the Terminal resize to Y-2, then that resize is going to sit in the buffer for the PtySignalThread until the paint is done, while the Paint buffers up Y-1 lines and sends them to the Terminal. So the Terminal has Y-2 lines in it's viewport at this point, but conpty just sent Y-1 lines of text, which means the top line is going to get scrolled off the top (and duplicated in the buffer)
…e resize operations. This actually made the problem a lot _worse_
… multiple resize operations." This reverts commit c8c794f.
…ure that this is better.
…esize-down # Conflicts: # src/cascadia/TerminalCore/Terminal.hpp
// TODO! Set the virtual top to the spot where we just inserted a
// bunch of blank lines.
// TODO! Something seems to still be invalidating the bottom line,
// when you increase the height of the buffer. I dunno how. I need
// to debug this.
…ned. This commit seems to work very well
* 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
…st the lines that were below wrapped lines?
…0-resizing-but-no-invalidateall-ing # Conflicts: # src/buffer/out/textBuffer.cpp # src/buffer/out/textBuffer.hpp # src/host/screenInfo.cpp
zadjii-msft
added
Product-Conpty
…esizing-but-no-invalidateall-ing
|
I might need to self block this - Looks like VtPipeTerm is aggressively worse in this scenario actually. Since this is a replication of how WSL interop in conhost is supposed to work, this is a bad sign. I'll test more. EDIT: I thought this was a bad heuristic originally, and I guess I'm glad that my gut was right about that. my repro is essentially print a line that doesn't wrap, resize width down to have it wrap, then resize the width up to have the line unwrap. Chaos ensues. |
… think that's the case after all. Gonna step back and try again.
## Summary of the Pull Request #4354 is a pretty complicated PR. It's got a bunch of conpty changes, but what it also has was some critical improvements to the roundtrip test suite. I'm working on some other bugfixes in the same area currently, and need these tests enhancements in those branches _now_. The rest of #4354 is complex enough that I don't trust it will get merged soon (if ever). However, these fixes _should_ be in regardless. ## PR Checklist * [x] Taken directly from #4354 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments This is four main changes: * Enable conpty to be fully enabled in unittests. Just setting up a VT renderer isn't enough to trick the host into being in conpty mode - it also needs to have some other flags set. * Some minor changes to `CommonState` to better configure the common test state for conpty * Move some of the verify helpers from `ConptyRoundtripTests` into their own helper class, to be shared in multiple tests * Add a `TerminalBufferTests` class, for testing the Terminal buffer directly (without conpty). This change is really easier than  would suggest, I promise.
| // If the final position in the buffer is on the bottom row of the new | ||
| // viewport, then we're going to need to move the top down. Otherwise, | ||
| // move the bottom up. | ||
| const COORD cOldCursorPos = _buffer->GetCursor().GetPosition(); |
| const bool beforeLastRow = maxRow < bufferSize.Y - 1; | ||
| const auto adjustment = beforeLastRow ? 0 : std::max(0, -dy); | ||
|
|
||
| auto proposedTop = oldTop + adjustment; |
There was a problem hiding this comment.
Is this math a candidate for the new base::ClampedAdd or base::CheckedAdd? Looks like it might be. If oldTop and adjustment are SHORT or you declare SHORT proposedTop, I think those could help you clamp/check it to be in bounds or set a reasonable default if it goes past and avoid the need to gsl::narrow_cast<SHORT> unchecked below.
|
|
||
| VERIFY_ARE_EQUAL(secondView.Top(), thirdView.Top()); | ||
| VERIFY_ARE_EQUAL(50 + dy, thirdView.BottomInclusive()); | ||
| // VERIFY_ARE_EQUAL(50 - thirdView.Height() + 1, thirdView.Top()); |
| // GH#3490 - In conpty mode, We want to invalidate all of the viewport that | ||
| // might have been below any wrapped lines, up until the last character of | ||
| // the buffer. Lines that were wrapped may have been re-wrapped during this | ||
| // resize, so we want them repainted to the terminal. We don't want to just | ||
| // invalidate everything though - if there were blank lines at the bottom, | ||
| // those can just be ignored. | ||
| if (isConpty && widthChanged) | ||
| { | ||
| // Loop through all the rows of the old buffer and reprint them into the new buffer | ||
| const auto bottom = std::max(_textBuffer->GetCursor().GetPosition().Y, | ||
| std::min(_viewport.BottomInclusive(), | ||
| _textBuffer->GetLastNonSpaceCharacter().Y)); | ||
| bool foundWrappedLine = false; | ||
| for (short y = _viewport.Top(); y <= bottom; y++) | ||
| { | ||
| // Fetch the row and its "right" which is the last printable character. | ||
| const ROW& row = _textBuffer->GetRowByOffset(y); | ||
| const CharRow& charRow = row.GetCharRow(); | ||
| if (foundWrappedLine || charRow.WasWrapForced()) | ||
| { | ||
| foundWrappedLine = true; | ||
| _renderTarget.TriggerRedraw(Viewport::FromDimensions({ 0, y }, _viewport.Width(), 1)); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nnnnnn. I don't really like this but I don't have another offering at hand so maybe it has to stick for the short term.
I can see this going where the Resize method on the TextBuffer becomes more responsible for providing appropriate viewport notifications since it seems like every caller needs to figure out something based on the Cursor.GetPosition() after the fact. And then to add some... intricate rendering details about the VtRenderer to this side of the equation feels dirty, design wise. I felt like determining what was different fit better in the renderer side.
But I don't know what to do about it right now. I'm guessing there just isn't enough data available to the VtRenderer to know that this happened? What specifically was missing? Isn't the width available to the VtRenderer via the RenderData interface? Or is it that it can't reach that specifically because RenderData is only in the base renderer and it can't query that?
There was a problem hiding this comment.
Okay so this is the part that actually might have messed up gnome-terminal and conhost/inception.
The real trick here is that we don't want to just invalidate the blank lines at the bottom of the buffer. However, I'm afarid now that this might never work quite right, if the terminal resized again before this frame is printed. I'm still investigating if this PR is the right solutions - it might not turn out that way.
|
|
||
| void ConptyRoundtripTests::TestResizeHeight() | ||
| { | ||
| // This test class is _60_ tests to ensure that resizing the terminal works |
There was a problem hiding this comment.
OK, well, with this level of coverage... maybe I'm not that mad about the PTY-specific stuff going into ResizeWithReflow because if we decide it's a bad place for it later, this test should cover it and make sure we didn't screw up the future refactor.
|
You know what, this PR is a mess, and I can fix the "decrease the height removes a row from the scrollback" (#3490) bug with a combination of:
In fact, all these tests even still pass, so that's good and makes me happy. I'm going to close this, and the new PR for #3490 will come after #4415 and #4403 get merged. |
Mike what are you doing These changes make conpty reprint less when the buffer resizes. This works largely quite well tbh. The biggest problem is that the input line seems to move, and that's no good. Presumably that's because the cursor position moved in conpty, but we didn't update the terminal appropriately. That can be fixed. This is also a horrible idea and I'm sure will come back to bite me, but hey lets try it. But seriously, this works really well for fast resizing (I mean, not including the cursor position thing). Didn't check maximize/restore yet but I bet that's _just wrong_ right now.
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.
zadjii-msft
deleted the
dev/migrie/b/3490-resizing-but-no-invalidateall-ing
branch
Summary of the Pull Request
This PR fixes a bug in the Windows Terminal, where decreasing the height of the window would cause some lines from the scrollback to be lost. While a seemingly trivial bug, this actually involved some elaborate manipulation of how conpty decides to render a resize operation.
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
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.
RtoR-1toR-2), once during the frame being emitted by conpty, the Terminal would try to process theR-1lines emitted by conpty when there are onlyR-2available rows in the terminal buffer. This would lead to duplicated lines.This involved a bunch of changes throughout the codebase to make sure we weren't just calling
TriggerRedrawon the entire viewport when we resize the buffer. Magically, conpty was pretty good about knowing what actually changed, though that did need some updating as well (inVtEngine::UpdateViewport).ScreenInfoto cause it to only invalidate wrapped lines during a ResizeWithReflow. Previously, we'd be invalidating the entire viewport, but that caused issues terminal-side. It's better to only tell the terminal what actually changed during the reflow.With those changes, the Terminal was changed to more intelligently stick to the right location in the buffer. When increasing the height of conpty, new lines always get inserted at the bottom of the viewport, so the terminal needs to stick to roughly the old top's position to re-create this stateWhen resizing down, empty lines are clipped from the conpty if there are any, then lines from the top are removed.
This is obviously all very complicated, so I've added some tests to validate all this.
ConptyRoundtripTests::TestResizeHeightis elaborate and it does the right thing here, validating the contents of both conpty and the Terminal after emitting various numbers of lines into the buffer and doing arbitrary resizes.As with any conpty change, I'm horrified that this will break in new and more horrifying ways. I'm hoping that a long bake time will help us identify bugs in this changelist (if there are any).
Validation Steps Performed
In VtPipeTerm and Windows Terminal:
wsl seq 100Additionally, resize quickly from the corner, and ensure ll the lines are right.
Aditionally, perform all the above with less than a full viewport of output.
Aditidionally, after the above tests, perform a
sleep 2 ; printf "\e[6n"and quickly hit esc to query the cursors position in conpty, and make sure it lines up with the Terminal's.Also play with resizing and PSReadline, and all the lovely things PSR does to the cursor.
This was the original PR body I had for this change. It's included here for future code archeologists. It's not really accurate, but it might guide people with the text that's in #3490 to help recreaate how I got here.
This PR encompasses a pair of fixes that are needed simultaneously to make the Windows Terminal work correctly.
RtoR-1toR-2), once during the frame being emitted by conpty, the Terminal would try to process theR-1lines emitted by conpty when there are onlyR-2available rows in the terminal buffer. This would tlead to duplicated lines.This involved a bunch of changes throughout the codebase to make sure we weren't just calling
TriggerRedrawon the entire viewport when we resize the buffer. Magically, conpty was pretty good about knowing what actually changed, though that did need some updating as well (inVtEngine::UpdateViewport).As with any conpty change, I'm horrified that this will break in new and more horrifying ways. I'm hoping that a long bake time will help us identify bugs in this changelist (if there are any).
Validation Steps Performed
In VtPipeTerm and Windows Terminal:
wsl seq 100