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

Fixed DirectX RTL text issue where it'd be over other text / offscreen #1873

Merged
merged 8 commits into from
Jul 10, 2019
Merged

Conversation

schorrm
Copy link
Contributor

@schorrm schorrm commented Jul 8, 2019

This is a partial fix of #538 . This does not change the Console RTL behavior, it does however fix an issue in the rendering. Basically, DirectX expects the origin to be on the right if it is going to draw RTL text. This PR is a simple fix for that. Rather than draw with the left point and then move the origin rightwards, we check if it's RTL, if so, we move the origin rightwards immediately, and then draw. LTR rendering is unchanged.
This doesn't fix underlying questions of RTL handling in the console. It's just a render bugfix. However, this render bugfix should still be a big help and solve the low-hanging issues.

PR Checklist

  • Closes #xxx
  • 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

Validation Steps Performed

Behavior was tested. No changes were made to underlying console.
Three sample cases:

  1. RTL text input
    Before:
    image
    After:
    image
  2. Hebrew Output
    Before (the Hebrew text is all being drawn to the left of the screen, hence the phantom text):
    image
    After:
    image
  3. Mixed Output
    So, this is where this is partial. Due to inherent stuff with RTL behavior, it doesn't look perfect. But the rendering itself is no longer at fault.
    Before:
    image
    After:
    image

@schorrm
Copy link
Contributor Author

schorrm commented Jul 8, 2019

Just a note: the boxes in the prompt aren't a problem, they're just because Courier New doesn't support powerline.

@schorrm schorrm marked this pull request as ready for review July 8, 2019 14:55
@schorrm schorrm changed the title Fixed DirectX RTL text issue where it would appear to left, potential… Fixed DirectX RTL text issue where it would appear at left, over existing text, or off the term Jul 9, 2019
@schorrm schorrm changed the title Fixed DirectX RTL text issue where it would appear at left, over existing text, or off the term Fixed DirectX RTL text issue where it'd be over other text / offscreen Jul 9, 2019
@zadjii-msft zadjii-msft requested a review from miniksa July 10, 2019 13:15
@zadjii-msft
Copy link
Member

Honestly, this fix seems reasonable to me, with the caveat that RTL definitely still won't work right just better. We'll need a comprehensive design on the right way to handle RTL in the future, but this seems to me like it'll make cating RTL files work in the meantime.

@schorrm
Copy link
Contributor Author

schorrm commented Jul 10, 2019

Pretty much, it'll make basic input / output with RTL in general work tolerably. It's not just cating files, any RTL input or text runs into this problem big-time. This isn't just for cat.
More generally, yes, this isn't the grand unified solution to RTL directionality. But the bugfix is valuable nonetheless. It takes Windows Terminal RTL support from useless to tolerable, and that's not nothing.
And I would find it hard to imagine a long-term implementation that won't take into account the central thing that this code does: account for the fact that DWrite takes the right x-point for the writing call of an RTL sequence, not the left one.

src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved
src/renderer/dx/CustomTextLayout.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 10, 2019
@miniksa
Copy link
Member

miniksa commented Jul 10, 2019

Pretty much, it'll make basic input / output with RTL in general work tolerably. It's not just cating files, any RTL input or text runs into this problem big-time. This isn't just for cat.
More generally, yes, this isn't the grand unified solution to RTL directionality. But the bugfix is valuable nonetheless. It takes Windows Terminal RTL support from useless to tolerable, and that's not nothing.
And I would find it hard to imagine a long-term implementation that won't take into account the central thing that this code does: account for the fact that DWrite takes the right x-point for the writing call of an RTL sequence, not the left one.

Yeah, this is righteous for that reason. Let's get it cleaned up and merged!

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 10, 2019
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.

Excellent, thank you. Just get the code formatter check happy and I think this is good to go.

@schorrm
Copy link
Contributor Author

schorrm commented Jul 10, 2019

Format check is now pleased, praised be the Clang-Format VStudio add-on.

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.

This is straightforward! Thanks for working on it.

@DHowett-MSFT DHowett-MSFT merged commit b970356 into microsoft:master Jul 10, 2019
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019
microsoft#1873)

This is a partial fix of microsoft#538 . This does *not* change the Console RTL behavior, it does however fix an issue in the rendering. Basically, DirectX expects the origin to be on the right if it is going to draw RTL text. This PR is a simple fix for that. Rather than draw with the left point and then move the origin rightwards, we check if it's RTL, if so, we move the origin rightwards immediately, and then draw. LTR rendering is unchanged.
This doesn't fix underlying questions of RTL handling in the console. It's just a render bugfix. However, this render bugfix should still be a big help and solve the low-hanging issues.

## Validation Steps Performed
Behavior was tested. No changes were made to underlying console.
Three sample cases:
1. RTL text input
Before:
![image](https://user-images.githubusercontent.com/16987694/60816422-6737e100-a1a2-11e9-9e14-c62323fd5b02.png)
After:
![image](https://user-images.githubusercontent.com/16987694/60816395-5ab38880-a1a2-11e9-9f0a-17b03f8268ce.png)
2. Hebrew Output
Before (the Hebrew text is all being drawn to the left of the screen, hence the phantom text):
![image](https://user-images.githubusercontent.com/16987694/60816527-93ebf880-a1a2-11e9-9ba3-d3ebb46cc404.png)
After:
![image](https://user-images.githubusercontent.com/16987694/60816456-77e85700-a1a2-11e9-9783-9e69849f026d.png)
3. Mixed Output
So, this is where this is partial. Due to inherent stuff with RTL behavior, it doesn't look perfect. But the rendering itself is no longer at fault.
Before:
![image](https://user-images.githubusercontent.com/16987694/60816593-b5e57b00-a1a2-11e9-82be-0fcabb80f7d4.png)
After:
![image](https://user-images.githubusercontent.com/16987694/60816607-bb42c580-a1a2-11e9-849a-12846ec4d5c0.png)
@ghost
Copy link

ghost commented Aug 3, 2019

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

@donno2048
Copy link
Contributor

...
Behavior was tested. No changes were made to underlying console. Three sample cases:

  1. RTL text input
    Before:
    image
    After:
    image
    ...

"ריץ טארנר הוא ממש אחלה גבר" 💀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants