From 3ccd831a3ba59595b7612ab02e2603ad3eab138c Mon Sep 17 00:00:00 2001
From: James Holderness <j4_james@hotmail.com>
Date: Fri, 4 Dec 2020 00:39:23 +0000
Subject: [PATCH] Fix rendering of DBCS characters when partially off screen
 (#8438)

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
---
 src/renderer/base/renderer.cpp | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp
index 058ddab9a..14cf39910 100644
--- a/src/renderer/base/renderer.cpp
+++ b/src/renderer/base/renderer.cpp
@@ -764,23 +764,13 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
                 // (a.k.a. the right half of a two column character), then we need some special handling.
                 if (_clusterBuffer.empty() && it->DbcsAttr().IsTrailing())
                 {
-                    // If we have room to move to the left to start drawing...
-                    if (screenPoint.X > 0)
-                    {
-                        // Move left to the one so the whole character can be struck correctly.
-                        --screenPoint.X;
-                        // And tell the next function to trim off the left half of it.
-                        trimLeft = true;
-                        // And add one to the number of columns we expect it to take as we insert it.
-                        columnCount = it->Columns() + 1;
-                        _clusterBuffer.emplace_back(it->Chars(), columnCount);
-                    }
-                    else
-                    {
-                        // If we didn't have room, move to the right one and just skip this one.
-                        screenPoint.X++;
-                        continue;
-                    }
+                    // Move left to the one so the whole character can be struck correctly.
+                    --screenPoint.X;
+                    // And tell the next function to trim off the left half of it.
+                    trimLeft = true;
+                    // And add one to the number of columns we expect it to take as we insert it.
+                    columnCount = it->Columns() + 1;
+                    _clusterBuffer.emplace_back(it->Chars(), columnCount);
                 }
                 // Otherwise if it's not a special case, just insert it as is.
                 else
@@ -795,7 +785,7 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
                 }
 
                 // Advance the cluster and column counts.
-                it += columnCount > 0 ? columnCount : 1; // prevent infinite loop for no visible columns
+                it += std::max<size_t>(it->Columns(), 1); // prevent infinite loop for no visible columns
                 cols += columnCount;
 
             } while (it);