-
-
Notifications
You must be signed in to change notification settings - Fork 416
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
Arabic ligature + Better RTL #2614
base: develop
Are you sure you want to change the base?
Conversation
I'm using this new PR to make it cleaner. Compared to the old one #2551
|
Niceee! Might be nice to display what works and what's not in this pr with tasks, for example:
Should display like:
|
html5 target already has partial rtl support. |
Indeed an openfl bug, I remember encountering this from time to time... |
Came across a small bug with this on desktop targets, when the text of a textfield starts with a new line (e.g. package;
import openfl.text.TextFormat;
import openfl.events.MouseEvent;
import openfl.text.TextField;
import openfl.display.Sprite;
class Main extends Sprite
{
var textField:TextField;
public function new()
{
super();
textField = new TextField();
textField.text = "\nאוטובוס";
final format = new TextFormat("arialuni.ttf");
textField.defaultTextFormat = format;
addChild(textField);
textField.addEventListener(MouseEvent.MOUSE_OVER, mouseOver);
}
function mouseOver(_) {
trace(textField.textDirection); // leftToRight
}
} |
Just interested - does this problem still accur when you replace the newline with every other weakly-directionaled chars like - or +? Or is it purely newline related? |
I've just tested it now with - and +, those work as intended:
|
Actually curious, what's the problem with that one? Is the problem here a missing implementation of line-based auto alignment? If not, where's the issue? |
If I remember well it's not right aligning, it's right margins that didn't seem to work correctly. Like if you put the right margin to 20for example. I was doing some tests with textfields with background colour, to see if left margins worked, and I noticed the right margins didn't seem to work, like I thought it should work. |
@joshtynjala Is there any chance this pull request to be merged in the next release ? |
__setRenderDirty(); | ||
} | ||
|
||
if (LocaleID.RTL_LANGUAGES.contains(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "contains" method call is causing compilation failures on older versions of Haxe.
Here is a fork of the branch with the current merge conflicts resolved: https://github.com/tobil4sk/openfl/tree/TempTextField |
|
||
// This has to do with a weird bg in Guess mode where the first textLayout repeats itself | ||
// instead of "ال صبا صباح" it writes "ال ال صبا صباح " | ||
if (positions.length > 0) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for positions.length > 0
to be true here? positions
is set to an empty array on line 103.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the answer is yes, this is because of the direction
setter which changes dirty
to true. Therefore, everywhere else in this function where positions
is accessed, its getter does another call to __position
. This is a bit messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If __direction
is set directly instead of via the setter, this issue can be avoided, then maybe this check is no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 'positions.length > 0`
I only put it in case of the "weird bug" I'm not sure I even had this problem in other non arabic text.
For _direction. I think it was necessary Maybe it's for when you have only special characters like commas where you can go both directions As the script would be invalid in this case, the buffer direction would be ltr, when in reality it should be the direction of the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only put it in case of the "weird bug" I'm not sure I even had this problem in other non arabic text.
I think this "weird bug" was happening because direction =
accesses the set_direction
setter, which has a side effect of setting dirty = true
:
__dirty = true; |
This means that all further accesses of the positions
member variable (e.g. on line 206) in the __position()
method, which pass through the get_positions
getter, now call __position()
again:
openfl/src/openfl/text/_internal/TextLayout.hx
Lines 218 to 222 in 88a12d2
if (__dirty) | |
{ | |
__dirty = false; | |
__position(); | |
} |
So you end up with a __position()
call in the middle of another __position()
call, which is how it is possible for the positions
array to no longer be empty even though it was emptied at the start of the function:
positions = []; |
By using __direction =
directly instead of direction =
equals, this side effect can be avoided, and we don't end up with nested __position()
calls:
tobil4sk@e4bf395
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't testing this be as easy as just adding two underlines before where we set the direction?
I might be able to test with Hebrew, just not in the next 12 hours (not at home rn)
Shouldn't that be done automatically though? If we use harfbuzz for deciding text direction automatically, it should automatically assign the current line direction for punctuation/signs. There's also no problem with numbers, as they're generally written ltr (even Arabic numerals, and Arabic is written rtl) |
I know I'm very late to the party, but after revisiting this i remembered that most text fields decide text direction and alignment in a line-by-line basis. Can that be the case here, that we overlook and assume that the direction of the first line is the direction of the text, and therefore it defaults to LTR? genuinely curious here though, I'm not sure how harfbuzz/this PR does RTL support, im just used to working with both LTR and RTL text |
Here's a pull request to start a discussion on rtl/ltr . There still are a few things I would like to correct before an openfl release.
Improvements ( only on non js targets)
Arabic ligatures
rTL alignment
Automatic rTL detection
The automatic detection is done by harfbuzz. It detects from the first few characters
For the detection to work, the script and direction must be invalid ones
I've enabled by default, by setting the language, the script, and direction to invalid ones
I created a new text script GUESS which is in fact just the harfbuzz invalid one
I've expose language, script and text direction to the textField
I've done this before coding the detection part. I'm not sure it has so many uses now the detection is coded
TextDirection uses openfl.text._internal.TextLayout.TextDirection. If we keep exposing the textdirection in textField, maybe it's better that TextDIrection has own hx file not in _internal
You can use TextFormat to control rtl /ltr
You can use htmlText and use TextFormat tags to control rtl/ltr
For example you can do
textfield.htmlText = "1234 <textformat dir='rtl'> 567 12 34</textformat>"
Weird Bugs
textfield.htmlText = "12345<textformat dir='rtl'>12345</textformat>"
If you have the exact same word in rtl and ltr problem, it's because of caches. It's not really important IRL, because you usually use different scripts in rtl and ltr
It may be just an openfl bug and not this patch bug, just noticed it here
What is left to do
_ [ ] I've focused more on harfbuzz.
What I don't plan to do