Emit lines wrapped due to spaces at the end correctly #5294
Conversation
|
Can we be certain that this will only happen for a cursor move immediately after we deferred, and only from the line that deferred? Is there any other state transition that might set/unset deferred that we aren't aware of? Do we un-wrap when we get a |
|
No idea.
Almost certainly. I don't really know.
I'd presume, but I don't know. Wouldn't be hard to modify this test to test that as well. |
|
I think CRLF is the more common name for CRNL |
zadjii-msft
added
Area-Output
|
@msftbot make sure @miniksa signs off on this |
ghost
added
the
AutoMerge
|
Hello @zadjii-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:
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". |
|
Hmmmm. This is a conundrum. So the way I thought of wrap in the first place was from a stream write. If you had nothing special happen as you passed the end of the line, it got marked as wrap. If literally any other operation happened, it stayed unmarked and wasn't a wrap. Is what's currently implemented between PTY/Terminal that if you get into a deferred cursor newline (you've written enough that the cursor is just sitting there waiting off the right boundary for the next character) that you also set the wrap at the same time? Should we not be waiting to set the wrap status until one more character arrives and it comes off the deferred newline onto the next line? My concern here is that if it is implemented the way I'm describing (entering the deferred newline state sets wrap) that there are an unknowable set of circumstances where it may still be broken. You probably caught the major ones here with the cursor movements, but you're right to fear a long tail. If there's a strong reason why wrap marking happens on ENTER defer newline instead of on EXIT defer newline with the follow up character.... then I'm fine with this as I think you've caught the major scenarios the best you can and we'll keep improving over time. Just like everything else with our PTY. However, if there's not a good reason... perhaps investigate the alternative wrap marking scenario instead? |
|
@zadjii-msft idly wondering if fixing Niksa's concerns also helps exact-wrap? |
Okay I don't have a good reason, so I'll investigate this tomorrow. This is a tad bit scary, since that's a bit more mucking with |
|
@miniksa is there a world where we merge this because it fixes the significant regression in PTY wrapping and continue the investigation tomorrow? |
A world with a p0 follow on filed and linked, sure. |
|
vim.exe still doesn't work with this change.
it's also picked up a status line bug
also, when it exits (!) and reprints the entire screen, it reprints the entire screen wrapped now
same symptom in the debug tap: no newlines between ~[K ~[K ~[K lines |
|
I managed to get a duplicate status line in vim over ssh:
|
|
Does the pull request description here need updating before merge? @miniksa's concern no longer applies as we're not changing the wrap state in the host. |
Certainly does, on it. |
zadjii-msft
changed the title
| // | ||
| // GH#5291: DON'T remove spaces when the row wrapped. We might need those | ||
| // spaces to preserve the wrap state of this line, or the cursor position. | ||
| // For example, vim.exe uses "~ "... to clear the line, and then leaves |
There was a problem hiding this comment.
We used to used to do something like "80X 80C (delete 80, move 80 right)" or "K 80C", right? Would that still work here? After all, we want the spaces to appear on the output but the wrap state to be maintained
is the real problem that we're not putting the cursor at the end of the line, and the spaces make up for that? and suppressing the spaces would mean we move the cursor wrong?
There was a problem hiding this comment.
(I'm worried about the accidental deoptimization in not using ECH/EL, i guess.)
There was a problem hiding this comment.
if we just did the ^[[80X^[[80C, then the line won't actually get wrapped. We need to emit an actual space there. It's a combo of both needing the spaces to trigger the wrapping logic, and needing the spaces to move the cursor.
Theoretically we could do ^[[79X^[[79C (with a space at the end), but that seems dirtier.
There was a problem hiding this comment.
I'm concerned about the efficiency of it, though. We realized a lot of gains recently simply by emitting fewer things between the PTY and the Terminal. It's a little dirtier, but it's 69 fewer bytes to send, parse, and react to. That doesn't sound like much for one line, but it could be easily magnified over multiple lines and redraws.
There was a problem hiding this comment.
So here's my thought - the client app was already emitting all these spaces intentionally. This was actually an optimization that conpty was doing for them, and an optimization that we were doing wrong (in this scenario).
We'll still be sending the optimized sequences if the app does something like "Y X", or if the app uses ^[[K to clear the line (like a sane application should)
There was a problem hiding this comment.
Alright, then I'm fine with it. Thanks.
| // In GH#5291, we experimented with manually breaking the line on all cursor | ||
| // movements here. As we print lines into the buffer, we mark lines as | ||
| // wrapped when we print the last cell of the row, not the first cell of the | ||
| // subsequent row (the row the first line wrapped onto). | ||
| // |
ghost
merged commit 
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)
|
🎉 Handy links: |
ghost
mentioned this pull request
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:
^[[H) to move the cursor to the start of the next lineWhen 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
PR Checklist
Validation Steps Performed