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.

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

Fix RTL regression (fixes #4779) #5734

Merged
3 commits merged into from
May 4, 2020
Merged

Fix RTL regression (fixes #4779) #5734

3 commits merged into from
May 4, 2020

Conversation

schorrm
Copy link
Contributor

@schorrm schorrm commented May 4, 2020

Summary of the Pull Request

This fixes the RTL regression caused in #4747. We create the rectangle taking the direction (through the BiDi Level) into account, and then the rendering works again. The GlyphRun shaping could still probably use some work to be a polished thingy, and there are still issues with RTL getting chopped up a lot when there's font fallback going on, but this fixes the regression, and it's now functional again.

References

#4779 #4747

PR Checklist

  • Closes RTL is worse after #4747 #4779
  • CLA signed. If not, go over here 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

Detailed Description of the Pull Request / Additional comments

The baseline is actually direction dependent. So when it was being initialized, the unconditional baseline as left broke it, setting the box off to right of the text. We just check if the GlyphRun->bidiLevel is set, and if so, we adjust it so that the baseline lines up with the right, not with the left.

Validation Steps Performed

image

@ghost ghost added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels May 4, 2020
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.

This seems totally reasonable to me, but I want to make sure this gets the renderer-guru's approval

src/renderer/dx/CustomTextRenderer.cpp Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextRenderer.cpp Outdated Show resolved Hide resolved
@zadjii-msft
Copy link
Member

@msftbot make sure @miniksa signs off on this

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

ghost commented May 4, 2020

Hello @zadjii-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'll only merge this pull request if it's approved by @miniksa

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".

simple codeformatting nits
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.

uhg github, trim the whitespace plz

src/renderer/dx/CustomTextRenderer.cpp Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextRenderer.cpp Outdated Show resolved Hide resolved
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Huh. Alright then. Looks good to me. Thank you.

@DHowett-MSFT
Copy link
Contributor

@msftbot merge this in 3 minutes

@ghost
Copy link

ghost commented May 4, 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'll only merge this pull request if it's approved by @miniksa
  • I won't merge this pull request until after the UTC date Mon, 04 May 2020 16:14:08 GMT, which is in 3 minutes

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 9c52920 into microsoft:master May 4, 2020
@DHowett-MSFT
Copy link
Contributor

Thank you, @schorrm!

@ghost
Copy link

ghost commented May 5, 2020

🎉Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1) has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request May 5, 2020
@DHowett
Copy link
Member

DHowett commented Jul 2, 2020

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 20161.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTL is worse after #4747
5 participants