DBCS characters rendered incorrectly when scrolling horizontally in conhost

[j4james]

Environment

Windows build number: Version 10.0.18363.1198
Windows Terminal version (if applicable): Commit 1fbcf34

Steps to reproduce

1. Build OpenConsole.
2. Run Host.EXE with a bash shell.
3. Open the console properties and turn off Wrap text output on resize.
4. Also make sure the Screen Buffer Size is wider than the Window Size so there's a horizontal scrollbar.
5. Enter some DBCS characters at the prompt (the example below uses 子小尒尢).
6. Wiggle the horizontal scroll bar backwards and forwards a few times, so those characters are being scrolled in and out of view.

I should add that I just picked these characters at random from charmap, so my apologies if they mean anything offensive.

Expected behavior

The characters should always be rendered correctly.

Actual behavior

Sometimes the characters get chopped in half, or rendered in the wrong place so they're overlapping other characters.

I can't reproduce this in the conhost that comes with my current version of Windows, so it looks like it might be a regression. But I also checked out the initial commit of OpenConsole and it seems to have already been broken then. Is it possible my version of Windows is so old that it's prior to the first open source release?

Anyway, I think I know what the problem is - a couple of problems actually. I'll try and write up an explanation later with my proposed fix.

[j4james]

As far as I can tell, the problem is in the Renderer::_PaintBufferOutputHelper method. When the content starts with the trailing half of a DBCS character, it tries to move the screenPoint back a position, so it can safely render the full character. See below:

terminal/src/renderer/base/renderer.cpp

Lines 767 to 783 in 26ca73b

	// 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;
	}

But as you can see, if the X position is already in column 0, it instead moves forward one position and "skips" that character. At least that's the intention, but the code doesn't actually increment the iterator, so it just ends up rendering the character in the wrong place. And even if it worked as intended, at best you're going to end up with half a character not rendered.

My suggested fix for this is just to drop that check. If the X position is already in column 0, let it go negative. It should then paint the character exactly as expected, with half of it off screen. At least that seems to work for the GDI engine.

But that brings us to the next problem. When we're dealing with the second half of a DBCS char, the it->Columns() value is 1, so we set the columnCount to it->Columns() + 1 to allow for the fact that it's actually going to occupy two cells. However, that columnCount variable is also used to increment the iterator, so it ends up incrementing too much, and any subsequent characters will be rendered in the wrong place, or possibly skipped altogether. See below:

terminal/src/renderer/base/renderer.cpp

Lines 797 to 799 in 26ca73b

	// Advance the cluster and column counts.
	it += columnCount > 0 ? columnCount : 1; // prevent infinite loop for no visible columns
	cols += columnCount;

My suggested fix is to use the it->Columns() value itself for the increment, since that should always be correct. So something like this:

it += std::max<size_t>(it->Columns(), 1);

I'm not sure if we still need to make sure the increment is non-zero, but it probably wouldn't harm.

[DHowett]

I believe @miniksa changed something in here recently (after 1836x, as you've observed) that fixed an issue with ConPTY; blame might be enlightening here. 😄

Otherwise, this looks like the right fix. I hope we have test coverage for the ConPTY thing!

[j4james]

@DHowett I think the PR you're thinking of is #4668, but that didn't introduce this bug. It was an attempt to fix a related issue with fullwidth glyphs (#2191), but didn't correctly handle this particular case. I've now created a PR with my proposed changes, and confirmed that the test cases described in #2191 are still working correctly with those changes.

However, I should note that the #2191 issue is no longer reproducible in Terminal even with this whole section commented out (i.e. the fix introduced in #4668). I suspect that might be because the DX engine now always invalidates a full line at a time. I could reproduce the problem with the GDI engine in conhost, though, so could confirm that the fix does still work there.

[ghost]

🎉This issue was addressed in #8438, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

• Release Notes
• Store Download