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

Fix copying wrapped lines by implementing better scrolling #5181

Merged
51 commits merged into from
Apr 9, 2020

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 30, 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

PR Checklist

Validation Steps Performed

miniksa and others added 17 commits March 25, 2020 14:29
…idation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled.
…g to ETW. Change the scroll member variable to a til::point and math that directly (and add the operators and related tests to til::point).
(cherry picked from commit d972b5e)
…migrie/b/5113-with-miniksas-fix

# Conflicts:
#	src/renderer/vt/XtermEngine.cpp
#	src/renderer/vt/invalidate.cpp
  The test fails validating the second wrapped terminal line.

  I think the problem comes from us manually placing the cursor on the last line
  of the buffer. When we write the next 20 'A's, the first one gets written on
  the last cell of the first row, and that's bad.

  I'm going to see if I can't get rid of that call (the MoveCursor in
  PaintCursor) for this case. That seems like the cause of all this mess.
    TODO: This proves that the scrolling during a frame with other text
    breaks this scenario. We're going to try and special-case the scrolling
    thing to _only_ when
    * the invalid area is the bottom line, and
    * the line wrapped
    So in that case, we'll be sure that the next text will cause us to move
    the viewport down a line appropriately
    I've got a crazy theory that rendering bottom-up _might_ fix this
# Conflicts:
#	src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
#	src/host/ut_host/ConptyOutputTests.cpp
#	src/renderer/vt/XtermEngine.cpp
#	src/renderer/vt/math.cpp
  It was unhappy with the exact line lengths and this change
@zadjii-msft zadjii-msft added the Product-Conpty For console issues specifically related to conpty label Mar 30, 2020
@@ -105,7 +105,7 @@ XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe,
// by prepending a cursor off.
if (_lastCursorIsVisible)
{
_buffer.insert(0, "\x1b[25l");
_buffer.insert(0, "\x1b[?25l");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. lol, how long has this been like this, and does it map to any bugs in our repo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and is there a constant val we can use somewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mentioned in #3093, and I had a branch that fixed this before (dev/migrie/b/be-better-at-hiding), but I have no memory of why that was never fixed

src/renderer/vt/tracing.cpp Outdated Show resolved Hide resolved
// position we think we left the cursor.
//
// See GH#5113
_lastText.Y += dy;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the newlining gone, how do we make sure that the pty host knows how many lines got pushed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit in the comment that was removed, too, is the pushing-into-scrollback behavior

Copy link
Member Author

@zadjii-msft zadjii-msft Mar 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait could you clarify? maybe it's just 5 o'clock calling, but I can't make sense of this comment 🙃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. This function used to print a set of newlines commensurate with the scroll distance, which would make the connected terminal push the scrolled-off lines into its scrollback. The newlines are now dead, and I can't determine whether that broke anything.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They seemingly didn't break anything.

Conpty always emits a frame immediately before every new line, due to TriggerCircling. Basically we'll finish printing the frame before the newline, then we'll additionally paint another frame with scrollDelta={0, -1}, and in that second frame, since the cursor is now one line higher, we'll newline to move the cursor down, into the new line in the terminal.

Since we're emitting a frame every newline, I'm not sure this function can actually get called with a delta != 1

@DHowett
Copy link
Member

DHowett commented Apr 1, 2020

Testing a selfhost build with this in it, I am seeing terminal not responding to ?25h. I don't know if this is caused by this PR :) I just haven't done a bisect and wanted to make sure I didn't lose it

@zadjii-msft
Copy link
Member Author

Testing a selfhost build with this in it, I am seeing terminal not responding to ?25h. I don't know if this is caused by this PR :) I just haven't done a bisect and wanted to make sure I didn't lose it

Can I get an exact repro to investigate? If what you're seeing is in 0.11.912.0, I'm not seeing that with a simple

printf '\e[?25l'
printf '\e[?25h'

in bash

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Apr 1, 2020

OKAY, here's your repro.

^[[?25l^[[H^[[115CXXXXX^[[4;9H^[[?25h
  • terminal size 120-wide (required)
  • cursor off
  • home the cursor
  • write up against the right edge of the screen (X=115, W=120, string="XXXXX")
  • move the cursor again
  • cursor back on

If this arrives in one packet, the cursor will remain hidden.

@DHowett-MSFT
Copy link
Contributor

What comes out of the debug tap:

␛[?25l␛[1;76HXXXXX␛[?25h

it seems like the cursor becomes visible, in delayed-EOL position off the edge. conpty doesn't render out the next cursor movement because no text comes with it. Here it says 76 because my terminal was 80, not 120.

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sane and reasonable to me. The newest fixes haven't invalidated the earlier ones, right?

@@ -558,7 +555,7 @@ using namespace Microsoft::Console::Types;
{
_deferredCursorPos = { _lastText.X + sNumSpaces, _lastText.Y };
}
else if (numSpaces > 0)
else if (removeSpaces && numSpaces > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're removing spaces, and there were spaces, print them? I am confus

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man so am I, I have no idea why that fixed the test and didn't break any of the other ones. It was literally the first thing I guessed, looks like I didn't actually add a comment

@@ -538,15 +547,15 @@ using namespace Microsoft::Console::Types;
RETURN_IF_FAILED(_EraseLine());
}
}
else if (_newBottomLine)
else if (_newBottomLine && coord.Y == _lastViewport.BottomInclusive())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it was the same issue as before! should we pull this out into a local, dirtyingNewBottomLine? That'll make sure we don't use _newBottomLine as much in the body here..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh probably

@DHowett-MSFT
Copy link
Contributor

@msftbot merge this in 5 minutes

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 9, 2020
@ghost
Copy link

ghost commented Apr 9, 2020

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 won't merge this pull request until after the UTC date Thu, 09 Apr 2020 00:06:03 GMT, which is in 5 minutes

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".

@ghost ghost merged commit 6fabc4a into master Apr 9, 2020
@ghost ghost deleted the dev/migrie/b/5113-with-miniksas-fix branch April 9, 2020 00:06
ghost pushed a commit that referenced this pull request Apr 15, 2020
## 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
DHowett-MSFT pushed a commit that referenced this pull request Apr 21, 2020
Improve wide glyph support in UIA (GH-4946)
Add enhanced key support for ConPty (GH-5021)
Set DxRenderer non-text alias mode (GH-5149)
Reduce CursorChanged Events for Accessibility (GH-5196)
Add more object ID tracing for Accessibility (GH-5215)
Add SS3 cursor key encoding to ConPty (GH-5383)
UIA: Prevent crash from invalid UTR endpoint comparison (GH-5399)
Make CodepointWidthDetector::GetWidth faster (CC-3727)
add til::math, use it for float conversions to point, size (GH-5150)
Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI (GH-5353)
Fix a deadlock and a bounding rects issue in UIA (GH-5385)
Don't duplicate spaces from potentially-wrapped EOL-deferred lines (GH-5398)
Reimplement the VT tab stop functionality (CC-5173)
Clamp parameter values to a maximum of 32767. (CC-5200)
Prevent the cursor type being reset when changing the visibility (CC-5251)
Make RIS switch back to the main buffer (CC-5248)
Add support for the DSR-OS operating status report (CC-5300)
Update the virtual bottom location if the cursor moves below it (CC-5317)
ci: run spell check in CI, fix remaining issues (CC-4799) (CC-5352)
Set Cascadia Code as default font (GH-5121)
Show a double width cursor for double width characters (GH-5319)
Delegate all character input to the character event handler (CC-4192)
Update til::bitmap to use dynamic_bitset<> + libpopcnt (GH-5092)
Merged PR 4465022: [Git2Git] Merged PR 4464559: Console: Ingest OSS changes up to e055079
Correct scrolling invalidation region for tmux in pty w/ bitmap (GH-5122)
Render row-by-row instead of invalidating entire screen (GH-5185)
Make conechokey use ReadConsoleInputW by default (GH-5148)
Manually pass mouse wheel messages to TermControls (GH-5131)
This fixes C-M-space for WSL but not for Win32, but I'm not sure there's a problem in Win32 quite yet. (GH-5208)
Fix copying wrapped lines by implementing better scrolling (GH-5181)
Emit lines wrapped due to spaces at the end correctly (GH-5294)
Remove unneeded whitespace (CC-5162)
@ghost
Copy link

ghost commented Apr 22, 2020

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

@zadjii-msft zadjii-msft mentioned this pull request May 12, 2020
31 tasks
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
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Conpty For console issues specifically related to conpty
Projects
None yet
6 participants