Skip to content

Commit

Permalink
Fix column count issues with certain ligature. (#4081)
Browse files Browse the repository at this point in the history
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
This change tries to fix column size calculation when shaping return glyphs that represents multiple characters (e.g. ligature).

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

This should fix #696.

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #xxx
* [X] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

Currently, it seems like CustomTextLayout::_CorrectGlyphRun generally assumes that glyphs and characters have a 1:1 mapping relationship - which holds true for most trivial scenarios with basic western scripts, and also many, but unfortunately not all, monospace "programming" fonts with programming ligatures.

This change makes terminal correctly processes glyphs that represents multiple characters, by properly accumulating the column counts of all these characters together (which I believe is more close to what this code originally intended to do).

There are still many issues existing in both CustomTextLayout as well as the TextBuffer, and the correct solution to them will likely demand large-scale changes, at least at the scale of #3578. I wish small changes like this can serve as a stop gap solution while we take our time to work on the long-term right thing.

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

Builds and runs. Manual testing confirmed that it solves #696 with both LigConsalata and Fixedsys Excelsior.
  • Loading branch information
milizhang authored and msftbot[bot] committed Jan 21, 2020
1 parent 69f3070 commit 027f122
Showing 1 changed file with 24 additions and 2 deletions.
26 changes: 24 additions & 2 deletions src/renderer/dx/CustomTextLayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -404,8 +404,30 @@ CustomTextLayout::CustomTextLayout(gsl::not_null<IDWriteFactory1*> const factory
// Offsets is how far to move the origin (in pixels) from where it is
auto& offset = _glyphOffsets.at(i);

// Get how many columns we expected the glyph to have and mutiply into pixels.
const auto columns = _textClusterColumns.at(i);
// Get how many columns we expected the glyph to have and multiply into pixels.
UINT16 columns = 0;
{
// Because of typographic features such as ligatures, it is well possible for a glyph to represent
// multiple code points. Previous calls to IDWriteTextAnalyzer::GetGlyphs stores the mapping
// information between code points and glyphs in _glyphClusters.
// To properly allocate the columns for such glyphs, we need to find all characters that this glyph
// is representing and add column counts for all the characters together.

// Find the range for current glyph run in _glyphClusters.
const auto runStartIterator = _glyphClusters.begin() + run.textStart;
const auto runEndIterator = _glyphClusters.begin() + run.textStart + run.textLength;

// Find the range of characters that the current glyph is representing.
const auto firstIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart);
const auto lastIterator = std::find(runStartIterator, runEndIterator, i - run.glyphStart + 1);

// Add all allocated column counts together.
for (auto j = firstIterator; j < lastIterator; j++)
{
const auto charIndex = std::distance(_glyphClusters.begin(), j);
columns += _textClusterColumns.at(charIndex);
}
}
const auto advanceExpected = static_cast<float>(columns * _width);

// If what we expect is bigger than what we have... pad it out.
Expand Down

0 comments on commit 027f122

Please sign in to comment.