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

Add attributes and properties for language and direction #563

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Add attributes and properties for language and direction #563

merged 3 commits into from
Jan 9, 2024

Conversation

TriangulumDesire
Copy link
Contributor

@TriangulumDesire TriangulumDesire commented Jan 5, 2024

This pull request adds more localisation support to RmlUi, based on the discussion in #553.

To summarise, it adds the following:

  • Two new internal properties (language and direction) that directly cannot be set with RCSS.
  • Two new attributes for elements:
    • lang: represents the language of the element's text and sets the language property.
    • dir: represents the text-flow direction of the element's text and sets the direction property.
  • A text-shaping context structure containing the language, direction, and letter-spacing, and it is used by the font engine interface.
  • Unit tests for the new localisation properties.

I have created a new sample for text-shaping to show these properties in action, but I will save that for another pull request (after this one is done). Below are some images of this sample.

Font rendering without text shaping.
(English–Arabic–Hindi)
Font rendering demonstration without text shaping.

Font rendering with text shaping.
(English–Arabic–Hindi)
Font rendering demonstration with text shaping.

@TriangulumDesire TriangulumDesire changed the title Add attributes and properties for language and direction. Add attributes and properties for language and direction Jan 5, 2024
@mikke89 mikke89 added enhancement New feature or request internationalization labels Jan 6, 2024
Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Very cool, those screenshots in particular look excellent! Nice work.

I'm happy to see this in the library, most of these are minor and stylistic comments.

There is one performance consideration that I believe is quite important to avoid any degradation. It would be nice if you could run the Benchmarks before and after the changes in this PR, to see if we have any differences.

@TriangulumDesire
Copy link
Contributor Author

Thanks for the feedback! I'll apply the suggestions and push a new commit sometime soon.

@TriangulumDesire
Copy link
Contributor Author

Alright, I've applied your suggestions in another commit.

I've run the benchmarks, and it looks like there isn't much of a difference. I've attached the outputs to this comment if you would like to look at them:
before_localisation.txt
after_localisation.txt

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

Great work! Just a couple of minor comments, then this is all good to merge in my view.

Thanks for running benchmarks too, looks good, if there are any changes then they are below the noise level.

@TriangulumDesire
Copy link
Contributor Author

TriangulumDesire commented Jan 9, 2024

Done! I went with Get<String>. This will have a small performance penalty since it copies the string, but lang's value should be short, so copying it won't be too expensive most of the time (especially with short-string optimisation). We can always change it to retrieving a reference (with a safeguard) in the future if needed.

@mikke89 mikke89 merged commit f9b8f6b into mikke89:master Jan 9, 2024
17 checks passed
@mikke89
Copy link
Owner

mikke89 commented Jan 9, 2024

Perfect, thanks a lot for the contribution! Looking forward to the follow-up sample :)

@TriangulumDesire
Copy link
Contributor Author

Awesome! There's a few more tweaks I have to make to it, but it should be done within the next few days.

@TriangulumDesire TriangulumDesire deleted the add-l10n-attributes branch January 13, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants