Add support for "reflow"ing the Terminal buffer #4741
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
# Conflicts: # src/buffer/out/textBuffer.cpp # src/buffer/out/textBuffer.hpp # src/host/screenInfo.cpp
(cherry picked from commit c21f740)
…conpty-wrapping-003
…BufferLine interface
…ly, esp conhost v wt v gnome-terminal. Remove ambiguity - just hardcore move the cursor in this scenario.
…avior quite right
…inal behavior quite right" This reverts commit 0a98cce.
…-actually-did-it-this-time
…low-buffer-on-resize
…/migrie/f/reflow-buffer-on-resize # Conflicts: # src/renderer/vt/XtermEngine.cpp # src/renderer/vt/vtrenderer.hpp
…-buffer-on-resize
…-buffer-on-resize
…-buffer-on-resize # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
|
@miniksa I actually prefer the in-out-opt pointer version compared to the referenceboi. even if it's a |
Fine, leave it the pointer way then. I was mildly worried about that when asking for it, but generally I can't tell whether it's going to look "confusing" or not until I try it. |
DHowett-MSFT
approved these changes
Mar 12, 2020
There was a problem hiding this comment.
Okay, I'm happy with how this looks and feels. @miniksa If your only comment is about the optional, Mike's willing to fix it tomorrow as a codehealth fix and I do not want to block further on that. I'd prefer a pointer or an optional ref wrapper, not a ref to an optional, but i don't care enough here.
carlos-zamora
approved these changes
Mar 12, 2020
OK. Reviewing RIGHT NOW. |
miniksa
approved these changes
Mar 12, 2020
DHowett-MSFT
added
the
AutoMerge
|
Hello @DHowett-MSFT! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
IT'S HAPPENING itshappening.gif |
…-buffer-on-resize # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
This was referenced Mar 13, 2020
ghost
pushed a commit
that referenced
this pull request
Mar 16, 2020
## Summary of the Pull Request Currently, when the user resizes the Terminal, we'll snap the visible viewport back to the bottom of the buffer. This PR changes the visible viewport of the Terminal to instead remain in the same relative location it was before the resize. ## References Made possible by our sponsors at #4741, and listeners like you. ## PR Checklist * [x] Closes #3494 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments We already hated the `std::optional<short>&` thing I yeet'd into #4741 right at the end to replace a `short*`. So I was already going to change that to a `std::optional<std::reference_wrapper<short>>`, which is more idomatic. But then I was looking through the list of bugs and #3494 caught my eye. I realized it would be trivial to not only track the top of the `mutableViewport` during a resize, but we could use the same code path to track the _visible_ viewport's start as well. So basically I'm re-using that bit of code in `Reflow` to calculate the visible viewport's position too. ## Validation Steps Performed Gotta love just resizing things all day, errday
3 tasks
6 tasks
ghost
pushed a commit
that referenced
this pull request
Apr 9, 2020
Now that the Terminal is doing a better job of actually marking which lines were and were not wrapped, we're not always copying lines as "wrapped" when they should be. We're more correctly marking lines as not wrapped, when previously we'd leave them marked wrapped. The real problem is here in the `ScrollFrame` method - we'd manually newline the cursor to make the terminal's viewport shift down to a new line. If we had to scroll the viewport for a _wrapped_ line, this would cause the Terminal to mark that line as broken, because conpty would emit an extra `\n` that didn't actually exist. This more correctly implements `ScrollFrame`. Now, well move where we "thought" the cursor was, so when we get to the next `PaintBufferLine`, if the cursor needs to newline for the next line, it'll newline, but if we're in the middle of a wrapped line, we'll just keep printing the wrapped line. A couple follow up bugs were found to be caused by the same bad logic. See #5039 and #5161 for more details on the investigations there. ## References * #4741 RwR, which probably made this worse * #5122, which I branched off of * #1245, #357 - a pair of other conpty wrapped lines bugs * #5228 - A followup issue for this PR ## PR Checklist * [x] Closes #5113 * [x] Closes #5180 (by fixing DECRST 25) * [x] Closes #5039 * [x] Closes #5161 (by ensuring we only `removeSpaces` on the actual bottom line) * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed * Checked the cases from #1245, #357 to validate that they still work * Added more and more tests for these scenarios, and then I added MORE tests * The entire team played with this in selfhost builds
This was referenced May 16, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 20, 2024
## Summary of the Pull Request The dirty view calculation in the `XtermEngine::StartPaint` method was originally used to detect a full frame paint that would require a clear screen, but that code was removed as part of PR #4741, making this calculation obsolete. The `performedSoftWrap` variable in the `XtermEngine::_MoveCursor` method was assumedly a remanent of some WIP code that was mistakenly committed in PR #5181. The variable was never actually used. ## Validation Steps Performed All the unit tests still pass and nothing seems obviously broken in manual testing. ## PR Checklist - [x] Closes #17280
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for "Resize with Reflow" to the Terminal. In
conhost,
ResizeWithReflowis the function that's responsible forreflowing 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
this
PR Checklist
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
InvalidateAllwhen theterminal 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 newoutput 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.