Prevent the VT engine painting unnecessarily #17194
Merged
Conversation
This file contains 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
lhecker
approved these changes
May 6, 2024
lhecker
reviewed
May 6, 2024
| if (_usingLineRenditions && !_quickReturn) | ||
| // when we're writing out a single character, which should preclude there | ||
| // being a rendition switch. | ||
| if (_usingLineRenditions && !_invalidMap.one()) |
There was a problem hiding this comment.
I'm not sure if .one() returns true for a single invalid wide char, but on the other hand... meh?
There was a problem hiding this comment.
Yeah, this was very much a "meh" solution, even more so when it was using quickReturn. I just wanted something that was easy to test, and not risk false positives by overcomplicating it. Most people will never be impacted by this anyway.
carlos-zamora
approved these changes
May 6, 2024
DHowett
approved these changes
May 6, 2024
DHowett
pushed a commit
that referenced
this pull request
May 6, 2024
When the VT render engine starts a paint operation, it first checks to see whether there is actually something to do, and if not it can end the frame early. However, the result of that check was being ignored, which could sometimes result in an unwanted `SGR` reset being written to the conpty pipe. This was particular concerning when passing through `DCS` sequences, because an unexpected `SGR` in the middle of the `DCS` string would cause it to abort early. This PR addresses the problem by making sure the `VtEngine::StartPaint` return value is appropriately handled in the `XtermEngine` class. ## Detailed Description of the Pull Request / Additional comments To make this work, I also needed to correct the `_cursorMoved` flag, because that is one of things that determines whether a paint is needed or not, but it was being set in the `InvalidateCursor` method at the start of ever frame, regardless of whether the cursor had actually moved. I also took this opportunity to get rid of the `_WillWriteSingleChar` method and the `_quickReturn` flag, which have been mostly obsolete for a long time now. The only place the flag was still used was to optimize single char writes when line renditions are active. But that could more easily be handled by testing the `_invalidMap` directly. ## Validation Steps Performed I've confirmed that the test case in issue #17117 is no longer aborting the `DCS` color table sequence early. Closes #17117 (cherry picked from commit 432dfcc) Service-Card-Id: 92501130 Service-Version: 1.21
This was referenced May 9, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 10, 2024
When the VT render engine checks whether the cursor has moved in the `InvalidateCursor` method, it does so by comparing the origin of the given cursor region with the last text output coordinates. But these two values are actually from different coordinate systems, and when on a double-width line, the x text coordinate is half of the corresponding screen coordinate. As a result, the movement detection is sometimes incorrect. This PR fixes the issue by adding another field to track the last cursor origin in screen coordinates, so we have a meaningful value to compare against. ## References and Relevant Issues The previous cursor movement detection was added in PR #17194 to fix issue #17117. ## Validation Steps Performed I've confirmed that the test case from issue #17232 is now fixed, and the test case from issue #17117 is still working as expected. ## PR Checklist - [x] Closes #17232
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.
When the VT render engine starts a paint operation, it first checks to
see whether there is actually something to do, and if not it can end the
frame early. However, the result of that check was being ignored, which
could sometimes result in an unwanted
SGRreset being written to theconpty pipe.
This was particular concerning when passing through
DCSsequences,because an unexpected
SGRin the middle of theDCSstring wouldcause it to abort early.
This PR addresses the problem by making sure the
VtEngine::StartPaintreturn value is appropriately handled in the
XtermEngineclass.Detailed Description of the Pull Request / Additional comments
To make this work, I also needed to correct the
_cursorMovedflag,because that is one of things that determines whether a paint is needed
or not, but it was being set in the
InvalidateCursormethod at thestart of ever frame, regardless of whether the cursor had actually
moved.
I also took this opportunity to get rid of the
_WillWriteSingleCharmethod and the
_quickReturnflag, which have been mostly obsolete fora long time now. The only place the flag was still used was to optimize
single char writes when line renditions are active. But that could more
easily be handled by testing the
_invalidMapdirectly.Validation Steps Performed
I've confirmed that the test case in issue #17117 is no longer aborting
the
DCScolor table sequence early.Closes #17117