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

Make is_layout_rtl() translation domain aware #97918

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

timothyqiu
Copy link
Member

Fixes #97893

This PR is mostly extracted from my #96921:

  • The edited scene root is now using the main translation domain.
  • Locale can be overridden per TranslationDomain.
  • In the editor, main translation domain's locale is overriden to use the fallback locale (en).
    • For now, if users do want to use RTL in the edited scene, they can set project settings like force_right_to_left_layout_direction, root_node_layout_direction, or the fallback locale.
  • is_layout_rtl() is now translation domain aware.
    • The translation domain's locale of current node is used instead of global TranslationServer.get_tool_locale().
    • When using inherited layout direction, the search stops at the last parent sharing the same translation domain.

Currently, this locale override behavior is available to engine code only, and is only used by the edited scene root. So it's relatively safe. API exposing and proper runtime switching will be implemented by a follow-up #96921.


Some further changes needed related to this PR:

It seems that TranslationServer.get_tool_locale() should be avoided in most places now because locale is potentially node-dependent. I did not touch those usages in TextServer because I don't know what's the best way to handle this change. For example:

unsigned int text_size = hb_ot_name_get_utf32(hb_face, lbl_id, hb_language_from_string(TranslationServer::get_singleton()->get_tool_locale().ascii().get_data(), -1), nullptr, nullptr) + 1;

CC @bruvzg

@timothyqiu timothyqiu added this to the 4.4 milestone Oct 7, 2024
@timothyqiu timothyqiu requested review from a team as code owners October 7, 2024 11:29
@bruvzg
Copy link
Member

bruvzg commented Oct 7, 2024

It seems that TranslationServer.get_tool_locale() should be avoided in most places now because locale is potentially node-dependent. I did not touch those usages in TextServer because I don't know what's the best way to handle this change.

There are two cases:

  • get_tool_locale used as fallback if locale is not specified when setting up text, in this case controls can properly set it (currently most of them do not).
  • It's used to read font feature names when loading font (names are used by the editor), not sure what's the best way to fix it. I guess TextServer can read all available languages, and editor UI select one it wants to use.

`TextServer` has a bunch of APIs that uses `get_tool_locale()` when an
empty language parameter is passed. This behavior can't be changed, so
the best I can do is to avoid passing empty strings for these
parameters.
@timothyqiu
Copy link
Member Author

timothyqiu commented Oct 7, 2024

I converted as much of the get_tool_locale() in the first case as possible to the actual locale.

TextServer has a bunch of APIs that uses get_tool_locale() when an empty language parameter is passed. This behavior can't be changed, so the best I can do is to avoid passing empty strings for these parameters.

It's submitted as a separate commit for easier review.

@timothyqiu timothyqiu marked this pull request as draft October 8, 2024 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants