-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Do not split runs on font scaling if the run is RTL #7149
Conversation
Doing something like this will break any application that is designed to work with RTL text. Applications need to know that if they output a character at a particular coordinate, then that is where the character will be displayed. There are escape sequences an application can use to change the directionality of text, but if the terminal just starts moving characters around of its own accord, it will be impossible to work with. |
@j4james are you critiquing my PR or the original behavior? My PR is about doing less reordering, not more. |
Wait, I'm not sure about this. What is actually causing it to display incorrectly? Is this just the I'm concerned about just opting-out of this codepath for RTL. That doesn't feel like the right answer here if it's still useful for LTR text. Can you give a sample file or some sample text to this including screenshots? |
It's possibly I'm just looking at it wrong, but my initial impression was that your PR was making things worse than they were before. If I output the character This is my test case:
And this is what that looks like in the preview build: However, with your PR merged, I'm now seeing those characters reordered as they are output, so That said, I don't think the current behaviour is correct either. More complicated examples seem to do some weird reordering too. But this PR doesn't help with that, at least as far as I can tell. |
@j4james that's not a bug, that's the feature! |
@miniksa so @j4james has a nice example actually, and here's another example: |
For reference, here's what it looks like in conhost, which is technically the correct order, i.e. 1234 5678 9-10-11-12 ... So while the middle version is clearly not right, it's probably the least broken of the three. At least some of the chunks are in the right order. For a Hebrew speaker, the left version may appear to be more correct, because it's displaying the chunks in their correct language order, but that's not something the terminal should be trying to do - that's the application's responsibility. |
No, ConHost is the wrong order since unlike ConHost WT aims (not at high priority, but aims) to support RTL. You can see in the linked issue. |
If you think this is the correct solution for RTL, I invite you to try editing a line of Hebrew text in vi, or even just the command line. For example, start by enter I'm not a Hebrew speaker (or any other bidirectional language for that matter), so this isn't going to effect me either way. But I've written enough bidirectional software to know that this approach will complete break any real bidi applications. And while it may sometimes give the appearance of "fixing" applications that aren't bidi-aware, it won't do that very well, and in the process will break a whole lot of other things. |
Under the PR, this specific use case with |
@miniksa I would formulate the case as follows: this codepath is generally beneficial since a single massive oversize character doesn't make everything else in the GlyphRun tiny. However, this is isn't worth it when splitting a GlyphRun has 'side effects', like it does with Bidi here. The scaling cases aren't worth the price of garbling the order, and any very unusually sized character like emoji is probably going to be a different BidiLevel anyway. It's really about the side effects that it has with the layout more than anything else. |
OK two thoughts here:
I think fixing either of those would also result in the same visual correction as your currently proposed fix while also getting deeper into the root of other related issues at the same time. |
A further thought on 2... if I were to have 7 RTL characters in a row in a stock |
For another example of software that will be broken by this PR, below are screenshots of VIM in Hebrew mode (i.e. The current Windows Terminal implementation isn't right either, but if you want to fix that, you need to match the conhost behaviour. |
So, this touches a bit on what it means to have host or guest-controlled BiDi, and it's going to make problems ever more apparent. There's a large body of work related to how to handle RTL in terminal emulators, but very little consensus on whose job it is to do so. It looks like vim's Hebrew mode (which I didn't know existed!) prefers to handle RTL itself... but detecting what an application's intent is when it puts RTL text in the buffer is going to be nigh-unto impossible. |
This is exactly what I've been trying to say. This is not a problem that the terminal can solve. And I don't believe the VTE proposal is any better. The best thing a terminal can do is stay out of the way, and let the applications handle the issue themselves (like VIM's Hebrew mode as one example). That said, there are cosmetic UI enhancements that a terminal could add to make things less painful when working with bidi-unaware applications. For example, a toggleable "mirror mode". which simply flips the entire screen. Or something like a bidi IME, that allows you to enter RTL text and have it pasted into the terminal in reverse order. I'd recommend looking at early Hebrew terminals and terminal emulators to see how they handled this sort of thing. |
No, staying out of the way is a cop-out. I care a million times more about having |
You've only given one example of something that doesn't work, and that's |
I wanna be clear. The ConHost behavior is unbearable. cat doesn't work. ls doesn't work. I actually use Hebrew. You can see from the original issue and the way people have commented on thread #538 (and all the closed dupe arabic threads) that nobody really wants the ConHost behavior in place. |
@miniksa that's a really good point. I guess we could essentially put in a for loop to reorder the thingy in _DrawGlyphRuns?
|
I understand the need to make Anyway, I've made my point. I'm not directly effected by this so I'll stay out of the discussion from now on. You guys can decide however you want to address the issue. |
I wonder if it might be most appropriate to simply offer a config flag? I hate that there's no way for us to detect that the guest wants to handle it itself right now, and we have to support both cases relatively "seamlessly". |
FWIW, most terminals that try to do something clever with RTL will have an option or escape sequence to turn that behaviour off. |
I mentioned this on the other PR, I agree that that's appropriate, and the behavior should be implemented by just marking every run as ltr if the flag is set. That'd cause the equivalent behavior. |
Consecutive RTL GlyphRuns are drawn from the last to the first. References #538, #7149, all those issues asking for RTL closed as dupes. As @miniksa suggested in a comment on #7149 -- handle the thingy on the render side. If we have GlyphRuns abcdEFGh, where EFG are RTL, we draw them now in order abcdGFEh. This has ransom-noting, because I didn't touch the font scaling at all. This should fix the majority of RTL issues, except it *doesn't* fix issues with colors, because those get split in the TextBuffer phase in the renderer I think, so they show up separately by the GlyphRun phase.
With the generalized fix that came in #7190, I think this PR is partially obsolete. I think it's a good idea to take this idea of the uniform scaling (especially?) in the case of font fallback, but there's no reason that it should be unique to RTL anymore. So I think it makes sense to close this PR (after completing some discussion items here) and open a ransomnoting specific PR, as well as one for the flag described.
|
Consecutive RTL GlyphRuns are drawn from the last to the first. References #538, #7149, all those issues asking for RTL closed as dupes. As @miniksa suggested in a comment on #7149 -- handle the thingy on the render side. If we have GlyphRuns abcdEFGh, where EFG are RTL, we draw them now in order abcdGFEh. This has ransom-noting, because I didn't touch the font scaling at all. This should fix the majority of RTL issues, except it *doesn't* fix issues with colors, because those get split in the TextBuffer phase in the renderer I think, so they show up separately by the GlyphRun phase. (cherry picked from commit 60b44c8)
Closing this, and I'll try and touch on the action items separately. |
Summary of the Pull Request
Right now if we have a glyph that needs scaling amidst some that don't, we split the GlyphRun and only apply scaling as necessary. This is generally fine, but it shouldn't be done in RTL since splitting the GlyphRun causes directionality issues.
References
#538
PR Checklist
Detailed Description of the Pull Request / Additional comments
In the process of working on this, I think I may have come up with the correct algorithmic idea for the Grand Fix for almost all RTL behavior: on the font fallback routine, if the run would be split by the index from the MapCharacters phase, try again and see if the fallback font can do the entire run. I have just been unable to create an object from the fallback font factory to run through the normal method with MapChars, so if you could point me in the right direction with DWrite that'd be great
Validation Steps Performed
Cat-ing. Generally speaking this means that RTL words will be intact though in reverse order when cat-ed, instead of just garbled arbitrarily.