Skip to content

Commit

Permalink
Fix line drawing during IME operations. (#6223)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request
Restores proper line drawing during IME operations in `conhost`

## PR Checklist
* [x] Closes #803
* [x] I work here.
* [x] Tested manually.
* [x] Check the performance of this and see if it's worse-enough to merit a more confusing algorithm. It was worse for the majority case so I scoped it.
* [x] No doc, it should have worked this way.
* [x] Am core contributor.

## Detailed Description of the Pull Request / Additional comments
- Changed `ConsoleImeInfo::s_ConvertToCells` to be less confusing. It's doing about the same thing, but it's way easier to read now and the compiler/linker/optimizer should just be the same.
- Edited `Renderer::_PaintBufferOutputHelper` to check each attribute for line drawing characters as the right half of a two-col character might have different line drawing characters than the left-half.

## Validation Steps Performed
- [x] Manual operation of IME in conhost with Japanese IME.
- [x] Manual operation of IME in conhost with Chinese IME.
- [x] Manual operation of IME in conhost with Chinese (Traditional) IME.
- [x] Manual operation of IME in conhost with and Korean IME. - @leonMSFT says Korean doesn't work this way. But Korean is broken worse in that it's not showing suggestions at all. Filing new bug. #6227 
- [x] Validated against API-filling calls through `SetConsoleTextAttribute` per @j4james's sample code
  • Loading branch information
miniksa authored Jun 3, 2020
1 parent 413b658 commit 9b075e9
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 5 deletions.
11 changes: 8 additions & 3 deletions src/host/conimeinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ std::vector<OutputCell> ConsoleImeInfo::s_ConvertToCells(const std::wstring_view
if (IsGlyphFullWidth(glyph))
{
auto leftHalfAttr = drawingAttr;
auto rightHalfAttr = drawingAttr;

// Don't draw lines in the middle of full width glyphs.
// If we need a right vertical, don't apply it to the left side of the character
Expand All @@ -271,12 +272,16 @@ std::vector<OutputCell> ConsoleImeInfo::s_ConvertToCells(const std::wstring_view
dbcsAttr.SetTrailing();

// If we need a left vertical, don't apply it to the right side of the character
if (drawingAttr.IsLeftVerticalDisplayed())
if (rightHalfAttr.IsLeftVerticalDisplayed())
{
drawingAttr.SetLeftVerticalDisplayed(false);
rightHalfAttr.SetLeftVerticalDisplayed(false);
}
cells.emplace_back(glyph, dbcsAttr, rightHalfAttr);
}
else
{
cells.emplace_back(glyph, dbcsAttr, drawingAttr);
}
cells.emplace_back(glyph, dbcsAttr, drawingAttr);
}

return cells;
Expand Down
44 changes: 42 additions & 2 deletions src/renderer/base/renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,11 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
screenPoint.X += gsl::narrow<SHORT>(cols);
cols = 0;

// Hold onto the start of this run iterator and the target location where we started
// in case we need to do some special work to paint the line drawing characters.
const auto currentRunItStart = it;
const auto currentRunTargetStart = screenPoint;

// Ensure that our cluster vector is clear.
clusters.clear();

Expand All @@ -713,6 +718,9 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
// as the first item in our run.
bool trimLeft = false;

// Run contains wide character (>1 columns)
bool containsWideCharacter = false;

// This inner loop will accumulate clusters until the color changes.
// When the color changes, it will save the new color off and break.
do
Expand Down Expand Up @@ -762,6 +770,11 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
clusters.emplace_back(it->Chars(), columnCount);
}

if (columnCount > 1)
{
containsWideCharacter = true;
}

// Advance the cluster and column counts.
it += columnCount > 0 ? columnCount : 1; // prevent infinite loop for no visible columns
cols += columnCount;
Expand All @@ -772,10 +785,37 @@ void Renderer::_PaintBufferOutputHelper(_In_ IRenderEngine* const pEngine,
THROW_IF_FAILED(pEngine->PaintBufferLine({ clusters.data(), clusters.size() }, screenPoint, trimLeft, lineWrapped));

// If we're allowed to do grid drawing, draw that now too (since it will be coupled with the color data)
// We're only allowed to draw the grid lines under certain circumstances.
if (_pData->IsGridLineDrawingAllowed())
{
// We're only allowed to draw the grid lines under certain circumstances.
_PaintBufferOutputGridLineHelper(pEngine, currentRunColor, cols, screenPoint);
// See GH: 803
// If we found a wide character while we looped above, it's possible we skipped over the right half
// attribute that could have contained different line information than the left half.
if (containsWideCharacter)
{
// Start from the original position in this run.
auto lineIt = currentRunItStart;
// Start from the original target in this run.
auto lineTarget = currentRunTargetStart;

// We need to go through the iterators again to ensure we get the lines associated with each
// exact column. The code above will condense two-column characters into one, but it is possible
// (like with the IME) that the line drawing characters will vary from the left to right half
// of a wider character.
// We could theoretically pre-pass for this in the loop above to be more efficient about walking
// the iterator, but I fear it would make the code even more confusing than it already is.
// Do that in the future if some WPR trace points you to this spot as super bad.
for (auto colsPainted = 0; colsPainted < cols; ++colsPainted, ++lineIt, ++lineTarget.X)
{
auto lines = lineIt->TextAttr();
_PaintBufferOutputGridLineHelper(pEngine, lines, 1, lineTarget);
}
}
else
{
// If nothing exciting is going on, draw the lines in bulk.
_PaintBufferOutputGridLineHelper(pEngine, currentRunColor, cols, screenPoint);
}
}
}
}
Expand Down

0 comments on commit 9b075e9

Please sign in to comment.