Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid splitting surrogate pairs when breaking runs for scaling #4731

Merged
1 commit merged into from
Feb 26, 2020
Merged

Avoid splitting surrogate pairs when breaking runs for scaling #4731

1 commit merged into from
Feb 26, 2020

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Feb 26, 2020

Summary of the Pull Request

  • Surrogate pairs are being split in half with the run splitting check.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • The adjustment of the run heights in the correction function reports back a text index and a scaling factor. However, I didn't remember at the time that the text is being stored as UTF-16. So the index given can be pointing to the high surrogate of a pair. Thus adding 1 to split "after" the text character, then backing up by 1 isn't valid in if the index given was for a high surrogate.

The quick fix is to advance by two if it's a high surrogate and one otherwise.

Validation Steps Performed

@miniksa miniksa added the Needs-Second It's a PR that needs another sign-off label Feb 26, 2020
@ghost ghost requested a review from zadjii-msft February 26, 2020 21:16
@miniksa miniksa removed the Needs-Second It's a PR that needs another sign-off label Feb 26, 2020
@miniksa miniksa self-assigned this Feb 26, 2020
DHowett-MSFT
DHowett-MSFT approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

zadjii-msft
zadjii-msft approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean there should be a test for this right?

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Product-Terminal The new Windows Terminal. labels Feb 26, 2020
@DHowett-MSFT
Copy link
Contributor

@msftbot merge this in 1 minute

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 26, 2020
@ghost
Copy link

ghost commented Feb 26, 2020

Hello @DHowett-MSFT!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Wed, 26 Feb 2020 21:54:40 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit d7ea526 into master Feb 26, 2020
@ghost ghost deleted the dev/miniksa/broken_layout branch February 26, 2020 21:55
DHowett-MSFT pushed a commit that referenced this pull request Feb 28, 2020
@DHowett
Revert "Don't split surrogate pairs when breaking runs for scaling. A…
8fb90dc 
…ffects emoji rendering. #4704 (#4731)"

This reverts commit d7ea526.
@miniksa miniksa mentioned this pull request Feb 28, 2020
Merged
10 tasks
ghost pushed a commit that referenced this pull request Mar 2, 2020
@miniksa
Improve glyph scaling correction (#4747)
7f43b40 
## Summary of the Pull Request
- Improves the correction of the scaling and spacing that is applied to
  glyphs if they are too large or too small for the number of columns that
  the text buffer is expecting

## References
- Supersedes #4438 
Co-authored-by: Mili (Yi) Zhang <milizhang@gmail.com>

- Related to #4704 (#4731)

## PR Checklist
* [x] Closes #696 
* [x] Closes #4375 
* [x] Closes #4708 
* [x] Closes a crash that @DHowett-MSFT complained about with
  `"x" * ($Host.UI.RawUI.BufferSize.Width - 1) + "`u{241b}"`
* [x] Eliminates an exception getting thrown with the U+1F3E0 emoji in
  `_CorrectGlyphRun`
* [x] Corrects several graphical issues that occurred after #4731 was
  merged to master (weird repeats and splits of runs)
* [x] I work here.
* [x] Tested manually versus given scenarios.
* [x] Documentation written into comments in the code.
* [x] I'm a core contributor.

## Detailed Description of the Pull Request / Additional comments
- The `_CorrectGlyphRun` function now walks through and uses the
  `_glyphClusters` map to determine the text span and glyph span for each
  cluster so it can be considered as a single unit for scaling.
- The total number of columns expected across the entire cluster
  text/glyph unit is considered for the available spacing for drawing
- The total glyph advances are summed to see how much space they will
  take
- If more space than necessary to draw, all glyphs in the cluster are
  offset into the center and the extra space is padded onto the advance of
  the last glyph in the range.
- If less space than necessary to draw, the entire cluster is marked for
  shrinking as a single unit by providing the initial text index and
  length (that is back-mapped out of the glyph run) up to the parent
  function so it can use the `_SetCurrentRun` and `_SplitCurrentRun`
  existing functions (which operate on text) to split the run into pieces
  and only scale the one glyph cluster, not things next to it as well.
- The scale factor chosen for shrinking is now based on the proportion
  of the advances instead of going through some font math wizardry
- The parent that calls the run splitting functions now checks to not
  attempt to split off text after the cluster if it's already at the end.
  This was @DHowett-MSFT's crash.
- The split run function has been corrected to fix the `glyphStart`
  position of the back half (it failed to `+=` instead of `=` which
  resulted in duplicated text, sometimes).
- Surrogate pair emoji were not allocating an appropriate number of
  `_textClusterColumns`. The constructor has been updated such that the
  trailing half of surrogate pairs gets a 0 column width (as the lead is
  marked appropriately by the `GetColumns()` function). This was the
  exception thrown.
- The `_glyphScaleCorrections` array stored up over the calls to
  `_CorrectGlyphRun` now uses a struct `ScaleCorrection` as we're up to 3
  values.
- The `ScaleCorrection` values are named to clearly indicate they're in
  relation to the original text span, not the glyph spans.
- The values that are used to construct `ScaleCorrection`s within
  `_CorrectGlyphRun` have been double checked and corrected to not
  accidentally use glyph index/counts when text index/counts are what's
  required.

## Validation Steps Performed
- Tested the utf82.txt file from one of the linked bugs. Looked
  specifically at Burmese through Thai to ensure restoration (for the most
  part) of the behavior
- Ensured that U+1f3e0 emoji (🏠) continues to draw correctly
- Checked Fixedsys Excelsior font to ensure it's not shrinking the line
  with its ligatures
- Checked ligatureness of Cascadia Code font 
- Checked combining characters U+0300-U+0304 with a capital A
@miniksa miniksa mentioned this pull request Apr 22, 2020
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

Assignees

@miniksa miniksa

Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some kind of unicode regression
4 participants
@miniksa @DHowett-MSFT @carlos-zamora @zadjii-msft