Skip to content

Conversation

@Aufricer
Copy link
Contributor

@Aufricer Aufricer commented Nov 28, 2021

This pull request is solving the problem with displaying Right to Left texts in JME. fixes #1158 and would also make the additional file for testcase close #1159 unnecessary.

More info could be found in the forum or in the discussion for issue #1158.

What changed:
Beside repairing the behaviour of RtL texts, a new parameter in BitMapfont allowing to set the textdirection in the font and in Bitmaptext. Before that was not possible in the font itself.
An interface "GlyphParser" now allowing the user of Persian, Arabic or other RtL oriented fonts to use individual parsers for the correct position of letters in a text (that may differ depending on letter before or after that) something that is not needed for Left to Rights texts.
Also a new discovered bug that brings different results for linewidth from BitmapText.getLineWidth() and BitmapFont.getLineWidth() for textinput with multilines had been fixed.

We have tested it and feel the PR is ready for review.

Edit: Of course it is Right to Left texts (at least I wrote it wrong on each occasion in the text)

Aufricer and others added 14 commits October 17, 2021 13:47
…nt. Not sure if that will break the import from assetmanager.loadFont
If the boolean RtL should not be set via Bitmaptext, te whole class could be unchanged
…for RtL text. (it was deducted twice before)

Still not sure if it is correct that way.
If the boolean RtL should not be set via Bitmaptext, te whole class could be unchanged
…o have the same result for LtR texts as it is given by BitMapFont.getLineWidth.
…dth() and BitmapFont.getLineWidth() will now have the same results.

A few open ToDo and tests have been solvd/closed.
@pspeed42
Copy link
Contributor

Did you mean "Right to Left" in the above?

@stephengold stephengold added this to the v3.5.0 milestone Nov 28, 2021
@stephengold
Copy link
Member

I appreciate the hard work that went into this PR. Thank you for contributing to the project!

@Aufricer Aufricer changed the title Enabling Left to Right Fonts Issue #1158 Enabling Right to Left Fonts - Issue #1158 Nov 28, 2021
@Aufricer
Copy link
Contributor Author

@pspeed42 - Yes of course. Right to Left texts...

@stephengold
Copy link
Member

Why was the "local.properties" file deleted?

@pspeed42 Do you have any desire to review this PR?

@pspeed42
Copy link
Contributor

pspeed42 commented Dec 2, 2021

I should probably give it a glance at least.

@Aufricer
Copy link
Contributor Author

Aufricer commented Dec 5, 2021

Why was the "local.properties" file deleted?

I realized it was added by my JDK. It contained a warning saying remove before publishing and contained system info of the computer I had used.

@stephengold
Copy link
Member

I should probably give it a glance at least.

Will you have time to do so during the next 10 days?

@stephengold
Copy link
Member

Why was the "local.properties" file deleted?

I realized it was added by my JDK. It contained a warning saying remove before publishing and contained system info of the computer I had used.

"local.properties" has been part of the Engine since 2015. I'm unsure what it does, but if it doesn't relate to this PR, please restore it.

@Aufricer
Copy link
Contributor Author

Aufricer commented Dec 7, 2021

I (hope) I have just restored the local.properties

Copy link
Member

@stephengold stephengold left a comment

Choose a reason for hiding this comment

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

I want to get this PR integrated before beta, and I don't know when @pspeed42 is going to get a chance to look at it, so did a quick pass. If anything's unclear, please discuss.

@Ali-RS
Copy link
Member

Ali-RS commented Dec 9, 2021

I fixed some of the change requests.

@Ali-RS
Copy link
Member

Ali-RS commented Dec 9, 2021

I have implemented GlyphParser for Persian text, should we include it in the repo as well?

@stephengold
Copy link
Member

I have implemented GlyphParser for Persian text, should we include it in the repo as well?

Language-specific features seem like the sort of thing that should be in https://github.com/jMonkeyEngine-Contributions or at the JmonkeyStore, not jme3-core.

@Ali-RS
Copy link
Member

Ali-RS commented Dec 9, 2021

Added a commit to serialize "rightToLeft" field in BitmapFont.

Should "glyphParser" field in the BitmapFont be serialized as well? (GlyphParser interface must extend com.jme3.export.Savable?)

@stephengold
Copy link
Member

Should "glyphParser" field in the BitmapFont be serialized as well?

I think if the font got saved for some reason, you'd want to serialize the parser as well. So yes.

It might be okay not to clone the glyphParser, however, at least if you document that detail.

@Ali-RS
Copy link
Member

Ali-RS commented Dec 9, 2021

Want to mention one more thing, that's about using numbers inside RTL text (bidirectional text).
The numbers will be shown in reverse (e.g 123 will be displayed 321).

Swing uses Unicode Bidirectional Algorithm to handle RTL mixed with LTR text. In the case of JME, I managed to fix this issue inside the glyph parser simply by reversing the numbers.

@stephengold
Copy link
Member

Seems worth documenting in a comment somewhere. Probably in GlyphParser.

@Ali-RS
Copy link
Member

Ali-RS commented Dec 10, 2021

I think if the font got saved for some reason, you'd want to serialize the parser as well. So yes.

Done.

Seems worth documenting in a comment somewhere. Probably in GlyphParser.

I will document this inside the PersianGlyphParser implementation, which I will add it to jMonkeyEngine-Contributions page later. Then I will add a link to it inside GlyphParser interface.

Like this: "For an example implementation see [link to PersianGlyphParser]."

What do you think?

@stephengold
Copy link
Member

stephengold commented Dec 10, 2021

Sorry, I can't follow that link. Retry?

Edit: Sorry I misunderstood. Yes, including a URL in the GlyphParser javadoc would be great.

@Ali-RS
Copy link
Member

Ali-RS commented Dec 11, 2021

Deprecated the Deprecate BitmapText(BitmapFont font, boolean rightToLeft).

@Ali-RS
Copy link
Member

Ali-RS commented Dec 11, 2021

I noticed setting text horizontal alignment to the Align.Right does not work when vertical alignment is set to anything other than VAlign.Top. I will submit a patch to fix it and other change requests.

… alignment was set to anything other than VAlign.Top also made some code cleanup.
@Ali-RS
Copy link
Member

Ali-RS commented Dec 11, 2021

And this is the GlyphParser implementation for Persian text:

https://github.com/Ali-RS/JME-PersianGlyphParser/blob/8f705a291277f744a2abf6ec73b7071e3f31be61/src/main/java/com/jme3/font/plugins/PersianGlyphParser.java#L47

@Ali-RS
Copy link
Member

Ali-RS commented Dec 12, 2021

Then I will add a link to it inside GlyphParser interface.
Like this: "For an example implementation see [link to PersianGlyphParser]."

Done

@stephengold
Copy link
Member

I believe this is ready for integration.

Unless there's further discussion, I plan to fix a few stylistic issues and integrate this in about 24 hours.

@stephengold
Copy link
Member

The "local.properties" file will soon be addressed by PR #1708.

@stephengold stephengold changed the title Enabling Right to Left Fonts - Issue #1158 solve issue #1158 (enabling Right-to-Left fonts) Dec 15, 2021
@stephengold stephengold merged commit 118b6c8 into jMonkeyEngine:master Dec 15, 2021
@Ali-RS
Copy link
Member

Ali-RS commented Dec 15, 2021

Thanks, @stephengold :)

@stephengold
Copy link
Member

Thanks, @Aufricer and @Ali-RS!

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.

BitmapText right-to-left line wrapping not working

4 participants