Fix rendering of DBCS characters when partially off screen

[j4james]

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

[zadjii-msft]

This seems sensible to me, but let's make sure @miniksa agrees

[miniksa]

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.

Oof. I'm 80% sure that the Columns() count for the latter half was returning 2 at some point in time and was probably adjusted to 1 or 0 and then 1 again to compensate for other bugs. Turns out column counting is hard.

[miniksa]

Yeah, this seems fine. I'm pretty sure the restriction was some part of me comparing what conhostv1 looked like to try to prevent it from striking the left most column weirdly.

But the longer I go for anything DBCS, the more I realize conhostv1 wasn't correct ever. So it's not really a model to stick to.

So given that the manual tests for this prove it's better and maintains the previous fix, I'm good with it.

[ghost]

Hello @DHowett!

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.

[ghost]

🎉Windows Terminal Preview v1.6.10272.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download