Make Conpty emit wrapped lines as actually wrapped lines #4415

zadjii-msft · 2020-01-30T17:10:51Z

Summary of the Pull Request

Changes how conpty emits text to preserve line-wrap state, and additionally adds rudimentary support to the Windows Terminal for wrapped lines.

References

Does not fix (!) WSL Terminal Line Endings (the "exact wrap" bug) #3088, but that might be lower down in conhost. This makes wt behave like conhost, so at least there's that
Still needs a proper deferred EOL wrap implementation in Implement stream writing mechanism on buffer (a.k.a. WriteCharsLegacy2ElectricBoogaloo) #780, which is left as a todo
Scenario: ResizeWithReflow and related issues in Windows Terminal #4200 is the mega bucket with all this work
MSFT:16485846 was the first attempt at this task, which caused the regression MSFT:18123777 so we backed it out.
Move cursor in conpty correctly after a backspace when we've delayed an EOL wrap #4403 - I made sure this worked with that PR before I even sent Move cursor in conpty correctly after a backspace when we've delayed an EOL wrap #4403

PR Checklist

Closes Text Wrapping with ConPTY #405
Closes Make sure WT can copy wrapped lines as a single line #3367
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

I started with the following implementation:
When conpty is about to write the last column, note that we wrapped this line here. If the next character the vt renderer is told to paint get is supposed to be at the start of the following line, then we know that the previous line had wrapped, so we won't emit the usual \r\n here, and we'll just continue emitting text.

However, this isn't exactly right - if someone fills the row exactly with text, the information that's available to the vt renderer isn't enough to know for sure if this line broke or not. It is possible for the client to write a full line of text, with a \n at the end, to manually break the line. So, I had to also add the lineWrapped param to the IRenderEngine interface, which is about half the files in this changelist.

Validation Steps Performed

Ran tests
Checked how the Windows Terminal behaves with these changes
Made sure that conhost/inception and gnome-terminal both act as you'd expect with wrapped lines from conpty

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

src/renderer/vt/paint.cpp

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

…-wrapping-003

DHowett-MSFT

Okay, I think I understand this. Let's do it. It's easy to revert once we start tasting clay.

DHowett-MSFT · 2020-01-31T22:44:32Z

/azp run

zadjii-msft · 2020-01-31T22:44:40Z

/azp run

azure-pipelines · 2020-01-31T22:44:42Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2020-01-31T22:44:50Z

Azure Pipelines successfully started running 1 pipeline(s).

DHowett-MSFT · 2020-01-31T22:45:17Z

I edited the description from does not fix x to does not fix (!) x because fix x is a magic github keyword and it doesn't read your sentence to figure out that you were deceiving it

zadjii-msft · 2020-01-31T22:51:08Z

I edited the description from does not fix x to does not fix (!) x because fix x is a magic github keyword and it doesn't read your sentence to figure out that you were deceiving it

thanks :)

carlos-zamora · 2020-02-04T18:34:23Z

src/host/ut_host/VtRendererTests.cpp

+        VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters1.data(), clusters1.size() }, { 0, 0 }, false, false));
+        VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters2.data(), clusters2.size() }, { 0, 1 }, false, false));


If we're making that last param false for all of these tests, should it be a default param with a value of false unless stated otherwise?

DHowett-MSFT · 2020-02-04T21:48:24Z

@miniksa had comments on this -- I'd like to know whether he's cool before merging.

DHowett-MSFT · 2020-02-04T21:48:34Z

@msftbot make sure @miniksa signs off on this

ghost · 2020-02-04T21:48:39Z

Hello @DHowett-MSFT!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

…-wrapping-003 # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

…-wrapping-003 # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp # src/renderer/vt/XtermEngine.cpp # src/renderer/vt/vtrenderer.hpp

zadjii-msft · 2020-02-19T19:23:05Z

@miniksa this is the one

# Conflicts: # src/renderer/base/renderer.cpp

miniksa

Good enough. I'm sure there's still some rough edges that are going to show up (as is always) but doesn't look like there's anything here that I wouldn't have done.

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.

## Summary of the Pull Request When WSL vim prints the initial empty buffer (the one that's just a bunch of '\~'s), it prints this by doing the following: * Print '\~' followed by enough spaces to clear the line * Use CUP (`^[[H`) to move the cursor to the start of the next line * repeat until the buffer is full When we'd get the line of "\~ "... in conhost, we'd mark that line as wrapped. Logically, it doesn't really make any sense that when we follow that up by moving the cursor, the line is wrapped. However, this is just how conhost is right now. This wasn't ever a problem in just conhost before, because we really didn't care if lines in the alt buffer were "wrapped" or not. Plus, when vim would get resized, it would just reprint it's own buffer anyways. Nor was this a problem in conpty before this year (2020). We've only just recently added logic to conpty to try and preserve wrapped lines. Initially, I tried fixing this by breaking the line manually when the cursor was moved. This seemed to work great, except for the win32 vim.exe. Vim.exe doesn't emit a newline or a CUP to get to the next line. It just _goes for it_ and keeps printing. So there's _no way_ for us to know the line broke, because they're essentially just printing one long line, assuming we'll automatically move the cursor. So instead, I'm making sure to emit the proper number of spaces at the end of a line when the line is wrapped. We won't do any funny business in that scenario and try to optimize for them, we'll _just print the spaces_. ## References * #5181 - This change regressed this * #4415 - Actually implemented wrapped lines in conpty ## PR Checklist * [x] Closes #5291 * [x] I work here * [x] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed * Wrote a unittest first and foremost * Checked vtpipeterm to make sure vim still works * checked Terminal to make sure vim still works

zadjii-msft added 16 commits January 13, 2020 11:23

let's first move reflowing to the text buffer

b5c8c85

add a doc comment because I'm not a barbarian

9aec694

Try to wrap the line properly with conpty

1a2654d

This confusingly doesn't always work

This is some cleanup, almost ready for PR but I need to write tests w…

416be46

…hich are blocked on #4213

Merge branch 'master' into dev/migrie/f/conpty-wrapping-003

7f341a2

# Conflicts: # src/buffer/out/textBuffer.cpp # src/buffer/out/textBuffer.hpp # src/host/screenInfo.cpp

I think this is all I need to support wrapped lines in the Terminal

edeb346

(cherry picked from commit c21f740)

Let's pull all the test fixes into their own file

ce3138c

Merge branch 'dev/migrie/b/just-conpty-test-fixes' into dev/migrie/f/…

4a7f2e4

…conpty-wrapping-003

A simple test for wrapped lines

bfde821

add a roundtrip test

e000388

I've found a bug with the line wrapping, going to go update the Paint…

c040a82

…BufferLine interface

This is polished for PR, ready to go in after #4382

0755fd7

Add PaintCursor tracing

86623f5

remove some old TODO comments

b3de042

remove other dead code for PR

96642de

zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. labels Jan 30, 2020

zadjii-msft requested review from DHowett-MSFT, carlos-zamora, miniksa and leonMSFT January 30, 2020 21:16

zadjii-msft mentioned this pull request Jan 30, 2020

When the Terminal is resized, don't remove lines from scrollback #4354

Closed

3 tasks

hopefully fix the build

2298e9a

zadjii-msft commented Jan 31, 2020

View reviewed changes

src/renderer/vt/paint.cpp Outdated Show resolved Hide resolved

miniksa reviewed Jan 31, 2020

View reviewed changes

src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp Outdated Show resolved Hide resolved

zadjii-msft added 3 commits January 31, 2020 14:34

make the tests fail in the future

a0c2e11

Hopefully fix tests in CI

5dc4f52

This is the wrong way to use these, I'm sure

9c64d5c

zadjii-msft added 2 commits January 31, 2020 15:33

Merge remote-tracking branch 'origin/master' into dev/migrie/f/conpty…

f7faf8d

…-wrapping-003

safemath this line

8bc6bff

DHowett-MSFT approved these changes Jan 31, 2020

View reviewed changes

carlos-zamora approved these changes Feb 4, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 4, 2020

zadjii-msft added 2 commits February 11, 2020 11:01

Merge remote-tracking branch 'origin/master' into dev/migrie/f/conpty…

a788db2

…-wrapping-003 # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

Merge remote-tracking branch 'origin/master' into dev/migrie/f/conpty…

d716308

…-wrapping-003 # Conflicts: # src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp # src/renderer/vt/XtermEngine.cpp # src/renderer/vt/vtrenderer.hpp

Merge branch 'master' into dev/migrie/f/conpty-wrapping-003

52ccccf

# Conflicts: # src/renderer/base/renderer.cpp

miniksa approved these changes Feb 27, 2020

View reviewed changes

ghost merged commit e5182fb into master Feb 27, 2020

ghost deleted the dev/migrie/f/conpty-wrapping-003 branch February 27, 2020 16:40

zadjii-msft mentioned this pull request Feb 28, 2020

Add support for "reflow"ing the Terminal buffer #4741

Merged

4 tasks

angelog0 mentioned this pull request Mar 17, 2020

Make sure WT can copy wrapped lines as a single line #3367

Closed

j4james mentioned this pull request Mar 31, 2020

Option to disable ConPty wrapping/reflow functionality #5192

Closed

kchibisov mentioned this pull request Mar 31, 2020

Links that overflow the line lose their tails on Windows. alacritty/alacritty#3361

Closed

zadjii-msft mentioned this pull request Apr 9, 2020

Emit lines wrapped due to spaces at the end correctly #5294

Merged

3 tasks

wesley800 mentioned this pull request Nov 25, 2022

Text Wrapping with ConPTY #405

Closed

zadjii-msft mentioned this pull request Dec 6, 2022

When Copying, Line Wrapped Text Is Inconsistently Broken Into Multiple Lines #6901

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Conpty emit wrapped lines as actually wrapped lines #4415

Make Conpty emit wrapped lines as actually wrapped lines #4415

zadjii-msft commented Jan 30, 2020 •

edited by DHowett-MSFT

Loading

DHowett-MSFT left a comment

DHowett-MSFT commented Jan 31, 2020

zadjii-msft commented Jan 31, 2020

azure-pipelines bot commented Jan 31, 2020

azure-pipelines bot commented Jan 31, 2020

DHowett-MSFT commented Jan 31, 2020

zadjii-msft commented Jan 31, 2020

carlos-zamora Feb 4, 2020

DHowett-MSFT commented Feb 4, 2020

DHowett-MSFT commented Feb 4, 2020

ghost commented Feb 4, 2020

zadjii-msft commented Feb 19, 2020

miniksa left a comment

		VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters1.data(), clusters1.size() }, { 0, 0 }, false, false));
		VERIFY_SUCCEEDED(engine->PaintBufferLine({ clusters2.data(), clusters2.size() }, { 0, 1 }, false, false));

Make Conpty emit wrapped lines as actually wrapped lines #4415

Make Conpty emit wrapped lines as actually wrapped lines #4415

Conversation

zadjii-msft commented Jan 30, 2020 • edited by DHowett-MSFT Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Jan 31, 2020

zadjii-msft commented Jan 31, 2020

azure-pipelines bot commented Jan 31, 2020

azure-pipelines bot commented Jan 31, 2020

DHowett-MSFT commented Jan 31, 2020

zadjii-msft commented Jan 31, 2020

carlos-zamora Feb 4, 2020

Choose a reason for hiding this comment

DHowett-MSFT commented Feb 4, 2020

DHowett-MSFT commented Feb 4, 2020

ghost commented Feb 4, 2020

zadjii-msft commented Feb 19, 2020

miniksa left a comment

Choose a reason for hiding this comment

zadjii-msft commented Jan 30, 2020 •

edited by DHowett-MSFT

Loading