Correct scrolling invalidation region for tmux in pty w/ bitmap #5122

miniksa · 2020-03-25T21:32:53Z

Correct scrolling invalidation region for tmux in pty w/ bitmap

Add tracing for circling and scrolling operations. Fix improper
invalidation within AdjustCursorPosition routine in the subsection about
scrolling down at the bottom with a set of margins enabled.

References

Introduced with Move ConPTY to use til::bitmap #5024

Detailed Description of the Pull Request / Additional comments

This occurs when there is a scroll region restriction applied and a
newline operation is performed to attempt to spin the contents of just
the scroll region. This is a frequent behavior of tmux.
Right now, the Terminal doesn't support any sort of "scroll content"
operation, so what happens here generally speaking is that the PTY in
the ConHost will repaint everything when this happens.
The PTY when doing AdjustCursorPosition with a scroll region
restriction would do the following things:

Slide literally everything in the direction it needed to go to take
advantage of rotating the circular buffer. (This would force a
repaint in PTY as the PTY always forces repaint when the buffer
circles.)
Copy the lines that weren't supposed to move back to where they were
supposed to go.
Backfill the "revealed" region that encompasses what was supposed to
be the newline.

The invalidations for the three operations above were:

Invalidate the number of rows of the delta at the top of the buffer
(this part was wrong)
Invalidate the lines that got copied back into position (probably
unnecessary, but OK)
Invalidate the revealed/filled-with-spaces line (this is good).

When we were using a simple single rectangle for invalidation, the
union of the top row of the buffer from 1 and the bottom row of the
buffer from 2 (and 3 was irrelevant as it was already unioned it)
resulted in repainting the entire buffer and all was good.
When we switched to a bitmap, it dutifully only repainted the top line
and the bottom two lines as the middle ones weren't a consequence of
intersect.
The logic was wrong. We shouldn't be invalidating rows-from-the-top
for the amount of the delta. The 1 part should be invalidating
everything BUT the lines that were invalidated in parts 2 and 3.
(Arguably part 2 shouldn't be happening at all, but I'm not optimizing
for that right now.)
So this solves it by restoring an entire screen repaint for this sort
of slide data operation by giving the correct number of invalidated
lines to the bitmap.

Validation Steps Performed

Manual validation with the steps described in Screen content does not update in tmux #5104
Automatic test ConptyRoundtripTests::ScrollWithMargins.

Closes #5104

…idation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled.

miniksa · 2020-03-25T21:33:06Z

Still needs some sort of automated test and for me to finish the description...

src/renderer/vt/tracing.cpp

zadjii-msft

I'm waiting for tests, but sure sounds fine to me

src/renderer/vt/tracing.cpp

src/renderer/vt/XtermEngine.cpp

miniksa · 2020-03-25T23:18:36Z

Tests on Friday. Gotta be done for the day.

…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).

zadjii-msft

I like everything here, but I'll patiently wait for the tests Friday :)

…the test.

miniksa · 2020-03-27T21:26:07Z

Future notes that may need work items (needs discussion):

We could implement a scrolling region communication between the PTY and the Terminal so we wouldn't have to invalidate all of this and re-communicate it all.
We don't really need to be invalidating the modeline. It's just a consequence of "move everything up and then some things back down." This would mean improving the algorithm in AdjustCursorPosition to maybe just send all operations that it wants to do directly to a scroll method and let it handle the optimization if it should circle things itself.
We probably shouldn't be having the PTY emit a command for turning on the cursor for no good reason during some of this output.
There's likely an extraneous cursor positioning message from the PTY as well.
All of this will become less relevant as we focus more and more on letting pass-through occur.

zadjii-msft

tests ❤️

src/renderer/vt/math.cpp

…. Add the NO that Dustin called out.

ghost · 2020-03-27T22:37:20Z

Hello @DHowett-MSFT!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

It was unhappy with the exact line lengths and this change

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

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 · 2020-04-22T17:12:33Z

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

Handy links:

Add tracing for circling and scrolling operations. Fix improper inval…

4bbb63d

…idation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled.

DHowett-MSFT reviewed Mar 25, 2020

View reviewed changes

src/renderer/vt/tracing.cpp Show resolved Hide resolved

zadjii-msft reviewed Mar 25, 2020

View reviewed changes

src/renderer/vt/tracing.cpp Show resolved Hide resolved

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

zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Conpty For console issues specifically related to conpty labels Mar 25, 2020

PR feedback applied. Don't bother making string if no one is listenin…

a543b1f

…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).

DHowett-MSFT approved these changes Mar 25, 2020

View reviewed changes

DHowett-MSFT mentioned this pull request Mar 26, 2020

Screen content does not update in tmux #5104

Closed

zadjii-msft reviewed Mar 26, 2020

View reviewed changes

zadjii-msft added a commit that referenced this pull request Mar 27, 2020

cleanup for review, but I have to wait for #5122 to merge first

51f060e

miniksa added 2 commits March 27, 2020 11:46

Part 1: Set up conpty round trip test pattern that we will use to do …

6061cae

…the test.

Finish the test. It validates the movement and repaint.

85da64d

miniksa marked this pull request as ready for review March 27, 2020 21:26

miniksa added 2 commits March 27, 2020 14:27

code format pass.

e42965c

Merge branch 'master' into dev/miniksa/tmux_draw

8560b2c

zadjii-msft approved these changes Mar 27, 2020

View reviewed changes

DHowett-MSFT reviewed Mar 27, 2020

View reviewed changes

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

DHowett-MSFT approved these changes Mar 27, 2020

View reviewed changes

Update spell whitelists, rename a few things to make spelling happier…

3973998

…. Add the NO that Dustin called out.

DHowett-MSFT changed the title ~~Correct invalidation region for tmux in ConPTY under new bitmap invalidation pattern~~ Correct scrolling invalidation region for tmux in pty w/ bitmap Mar 27, 2020

DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 27, 2020

ghost merged commit ef80f66 into master Mar 27, 2020

ghost deleted the dev/miniksa/tmux_draw branch March 27, 2020 22:37

zadjii-msft added a commit that referenced this pull request Mar 30, 2020

Fix the test @miniksa wrote for #5122

436335f

It was unhappy with the exact line lengths and this change

zadjii-msft mentioned this pull request Mar 30, 2020

Fix copying wrapped lines by implementing better scrolling #5181

Merged

6 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct scrolling invalidation region for tmux in pty w/ bitmap #5122

Correct scrolling invalidation region for tmux in pty w/ bitmap #5122

miniksa commented Mar 25, 2020 •

edited by DHowett-MSFT

Loading

miniksa commented Mar 25, 2020

zadjii-msft left a comment

miniksa commented Mar 25, 2020

zadjii-msft left a comment

miniksa commented Mar 27, 2020 •

edited

Loading

zadjii-msft left a comment

ghost commented Mar 27, 2020

ghost commented Apr 22, 2020

Correct scrolling invalidation region for tmux in pty w/ bitmap #5122

Correct scrolling invalidation region for tmux in pty w/ bitmap #5122

Conversation

miniksa commented Mar 25, 2020 • edited by DHowett-MSFT Loading

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

miniksa commented Mar 25, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

miniksa commented Mar 25, 2020

zadjii-msft left a comment

Choose a reason for hiding this comment

miniksa commented Mar 27, 2020 • edited Loading

Future notes that may need work items (needs discussion):

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Mar 27, 2020

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Apr 22, 2020

miniksa commented Mar 25, 2020 •

edited by DHowett-MSFT

Loading

miniksa commented Mar 27, 2020 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.