Restrict DX run height adjustment to only relevant glyph AND Correct PTY rendering on trailing half of fullwidth glyphs #4668
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
…t portions of the screen are considered invalid.
…ies to an entire run, break the one glyph into its own run before applying scale down.
…counts to save mental effort.
…cters from getting struck over the PTY.
This reverts commit 22899a0.
…the run splitting function.
miniksa
requested review from
DHowett-MSFT,
zadjii-msft,
carlos-zamora and
leonMSFT
miniksa
added
Area-Rendering
zadjii-msft
approved these changes
Feb 20, 2020
DHowett-MSFT
approved these changes
Feb 20, 2020
|
I've kicked the x64 build to restart only that leg. |
miniksa
added
the
AutoMerge
|
Hello @miniksa! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 5 hours 18 minutes. No worries though, I will be back when the time is right! 😉 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 merge in -1 minutes |
|
Hello @miniksa! 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". |
ghost
merged commit 
This was referenced Nov 29, 2020
ghost
pushed a commit
that referenced
this pull request
Dec 4, 2020
When the renderer is called on to render part of a line starting halfway through a DBCS character (as can occur in conhost when the viewport is offset horizontally), it could result in the character not being displayed, and/or with following the characters drawn in the wrong place. This PR is an attempt to fix those problems. The original code for handling the trailing half of fullwidth glyphs was introduced in PR #4668 to fix issue #2191. When the content being rendered starts with the trailing half of a DBCS character, the renderer tries to move the `screenPoint` back a position, so it can instead render the full character, but instructing the render engine to trim off the left half of it. If the X position was already in column 0, though, it would instead move forward one position, intending to skip that character. At best this would mean the half character wouldn't be rendered, but since the iterator wasn't incremented correctly, it actually just ended up rendering the character in the wrong place. The fix for this was simply to drop the check for the X position being in column 0, and allow it go negative. The rendering engine would then just start rendering the character partially off screen, and only the second half of it would be displayed, which is exactly what is needed. The second problem was that the code incrementing the iterator was using the `columnCount` variable rather than the `it->Columns()` value for the current position. When dealing with the trailing half of a DBCS character, the `columnCount` is 2, but the `Columns()` value is 1. If you use the former rather than the later, then the renderer may skip the following character. ## Validation Steps Performed I've developed a more easily reproducible version of the test case described in #8390, and confirmed that the problem no longer occurs when this PR is applied. I've also manually confirmed that the problem described in #2191 that was fixed by PR #4668 is still working correctly now. Closes #8390
This pull request was closed.
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.
Summary of the Pull Request
renderer instead of applying to the entire run
character. The entire render base has this behavior restored now as
well.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Two issues:
the correction code would apply the shrunken size to the entire run, not
just the potentially individual glyph that needed to be reduced in size.
Unfortunately while adjusting the horizontal X width can be done for
each glyph in a run, the vertical Y height has to be adjusted for an
entire run. So the solution here was to split the individual glyph
needing shrinking out of the run into its own run so it can be shrunk.
completed to deal with a request to draw only the right half of a
two-column character. This meant that when encountering a request for
the right half only, we would transmit the entire full character to be
drawn, left and right halves, struck over the right half position. Now
we correct the cursor back a position (if space) and draw it out so the
right half is struck over where we believe the right half should be (and
the left half is updated as well as a consequence, which should be OK.)
The reason this happens right now is because despite VIM only updating
two cells in the buffer, the differential drawing calculation in the
ConPTY is very simplistic and intersects only rectangles. This means
from the top left most character drawn down to the row/col cursor count
indicator in vim's modeline are redrawn with each character typed. This
catches the line below the edited line in the typing and refreshes it.
But incorrectly.
We need to address making ConPTY smarter about what it draws
incrementally as it's clearly way too chatty. But I plan to do that with
some of the structures I will be creating to solve #778.
Validation Steps Performed
in runs from Microsoft community page