Correct scrolling invalidation region for tmux in pty w/ bitmap#5122
Merged
7 commits merged intomasterfrom Mar 27, 2020
Merged
Conversation
…idation within AdjustCursorPosition routine in the subsection about scrolling down at the bottom with a set of margins enabled.
Member
Author
|
Still needs some sort of automated test and for me to finish the description... |
zadjii-msft
reviewed
Mar 25, 2020
Member
zadjii-msft
left a comment
There was a problem hiding this comment.
I'm waiting for tests, but sure sounds fine to me
Member
Author
|
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).
DHowett-MSFT
approved these changes
Mar 25, 2020
zadjii-msft
reviewed
Mar 26, 2020
Member
zadjii-msft
left a comment
There was a problem hiding this comment.
I like everything here, but I'll patiently wait for the tests Friday :)
zadjii-msft
added a commit
that referenced
this pull request
Mar 27, 2020
Member
Author
Future notes that may need work items (needs discussion):
|
DHowett-MSFT
approved these changes
Mar 27, 2020
…. Add the NO that Dustin called out.
|
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 (
|
zadjii-msft
added a commit
that referenced
this pull request
Mar 30, 2020
It was unhappy with the exact line lengths and this change
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
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)
|
🎉 Handy links: |
This pull request was closed.
This file contains hidden or 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
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.
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
Detailed Description of the Pull Request / Additional comments
newline operation is performed to attempt to spin the contents of just
the scroll region. This is a frequent behavior of tmux.
operation, so what happens here generally speaking is that the PTY in
the ConHost will repaint everything when this happens.
AdjustCursorPositionwith a scroll regionrestriction would do the following things:
advantage of rotating the circular buffer. (This would force a
repaint in PTY as the PTY always forces repaint when the buffer
circles.)
supposed to go.
be the newline.
(this part was wrong)
unnecessary, but OK)
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
ConptyRoundtripTests::ScrollWithMargins.Closes #5104