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

[3.x] i18n: Make property paths and categories translatable #58634

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

timothyqiu
Copy link
Member

@timothyqiu timothyqiu commented Feb 28, 2022

3.x l10n is more complete than master, thus easier to test, so this PR targets 3.x. I'll do a master version after this is discussed :)

I did some Ad-hoc localization for testing locally. When interface/editor/translate_properties is on, which is the default, the editor looks like this:

ksnip_20220301-153147

  • As you can see from the screenshot, VRAM and GLES2 are correctly capitalized. This makes msgids in the POT file easier to read. Capitalization logic is from (and thus supersedes) Improve editor property capitalization #32734.
    • Translation happens after capitalization.
    • Translation only happen when the new editor setting interface/editor/translate_properties is on.
  • Tooltips are added to section headers.
    • When translate_properties is on, shows the original name.
    • When translate_properties is off, shows the translated name.
  • In extract.py:
    • Replaced character by character line parsing with regular expression matching.
      • Extracting property paths are as easy as adding additional expressions.
      • I tested extraction without the property path patterns, the result is the same as the original approach. Except one string, which is fixed in [3.x] Make string inside TTR() single-line #58627.
    • Capitalization data are also extracted from C++ code with regular expression.

Comment on lines +83 to +88
# fmt: off
capitalized = " ".join(
part.title()
for part in capitalize_re.sub("_", name).replace("_", " ").split()
)
# fmt: on
Copy link
Member Author

@timothyqiu timothyqiu Feb 28, 2022

Choose a reason for hiding this comment

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

I turned off black for this block. Because it wants me to put these on a single line.

Keeping these in multiple lines is much more readable I think.

@akien-mga akien-mga added this to the 3.5 milestone Mar 1, 2022
@akien-mga akien-mga self-requested a review March 1, 2022 00:56
@timothyqiu
Copy link
Member Author

Updated the regular expression for GLOBAL_DEF so that overrides like audio/mix_rate.web won't be matched.

@timothyqiu
Copy link
Member Author

Updated how section tooltip works, it now looks like this:

translate_properties on translate_properties off
ksnip_20220301-152903 ksnip_20220301-152927

@timothyqiu timothyqiu force-pushed the property-i18n-3.x branch 2 times, most recently from 3d4f9b0 to dfd7785 Compare March 2, 2022 08:11
@timothyqiu
Copy link
Member Author

Added property name capitalization & translation to Editor Feature Profile dialog.

translate_properties on translate_properties off
ksnip_20220302-160759 ksnip_20220302-160827

Comment on lines 38 to 43
String capitalized_string = p_name.capitalize();

// Fix the casing of a few strings commonly found in editor property/setting names
for (Map<String, String>::Element *E = capitalize_string_remaps.front(); E; E = E->next()) {
capitalized_string = capitalized_string.replace(E->key(), E->value());
}
Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit hacky IMO to do replacements based on the wrongly capitalized output instead of handling the input directly (i.e. map the input s3tc and not the output S 3 T C to the correct form S3TC), but I don't see how to do it without making things less efficient and more cumbersome, so I guess it's fine as is :)

@akien-mga
Copy link
Member

Looks pretty good to me overall! Added some review comments, mostly nitpicks. I do think the general approach is good :)

As a side note for the tooltips, it could be useful to have the full property paths (e.g. application/config for the "Config" section under "Application", or application/config/name for the property). But that's beyond the scope of this PR IMO, and maybe something to think about more for the general UX of these editors (e.g. to make sure that it's easy to copy a property name to clipboard, instead of just having it in a tooltip that would need to be typed manually).

@timothyqiu
Copy link
Member Author

Updated according to review and added some necessary remapping found during testing.

From To
Gi Probe GI Probe
Bvh BVH
Cpu CPU
Ik IK
Png PNG
Sdk SDK
Vector 2 Vector2

Checked the extracted messages, there are no false positives.

@Calinou
Copy link
Member

Calinou commented Mar 3, 2022

@timothyqiu You can find more English remappings that would be useful to add here: #32734

@timothyqiu
Copy link
Member Author

@Calinou Yeah, those are what I used as the initial remappings. I saw that PR is several months old and some changes are required, so I did the changes here and listed you as the co-author :)

@akien-mga akien-mga merged commit 153a068 into godotengine:3.x Mar 10, 2022
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the property-i18n-3.x branch March 10, 2022 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants