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 option to add built-in strings in the POT generation #86222

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented Dec 16, 2023

This PR adds the option to add the built-in strings that can appear inside user projects (such as FileDialog node and the menu from LineEdit) into the POT generation. This is made possible with the new ETR/ETRN() functions, which from now on should be used on those occasions. Here's the sister PR on the "l10n" repo: godotengine/godot-editor-l10n#13

As a bonus, this PR also makes so that the POT generator stops putting location comments into strings that don't have one.

Bugsquad edit: Closes godotengine/godot-proposals#3827 Supersedes #81180
Production edit: Closes godotengine/godot-roadmap#33

@Calinou
Copy link
Member

Calinou commented Dec 16, 2023

Does this pave the way to godotengine/godot-proposals#7334? As I understand it, it should, but I'm asking to make sure nonetheless.

@YeldhamDev
Copy link
Member Author

@Calinou It's a first step. For, it just populates the POT, but the plan is to add translations to the PO files in the future.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 16, 2023

Generate POT is disabled when you have only built-in strings included (no scenes). Not sure how useful is that, but technically it could work.

EDIT:
Also, even though I enabled built-in strings, they are not included for some reason.

@YeldhamDev
Copy link
Member Author

@KoBeWi As stated above, this PR won't work until its sister PR is merged (godotengine/godot-editor-l10n#13).

@YeldhamDev
Copy link
Member Author

Alright, I made an important change on how ETR/ETRN() works: now, they won't return translated strings at all, but instead the exact string it was given to them. They will now work purely to tag strings to be extract for translation.

This change was made so they could work better with auto_translate, because now the original string is preserved, and the controls themselves can handle a translation change with atr() internally. I think this technically supersedes #81180 for what it was intended to do (WDYT @KoBeWi?).

auto_translated isn't working as best as it could however, since it only affects the control itself, and not its children, which is bad for how some UI nodes in Godot work. I plan to open a proposal on how to tackle that.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 14, 2024

ETR won't work for formatted text. Auto-translation only has effect on exact strings.
My PR adds ATR, which is like RTR, but respects auto_translate. You need a similar method for dynamic strings. I see you do atr(ETR( in some places, but you missed some strings.

@YeldhamDev
Copy link
Member Author

My PR adds ATR, which is like RTR, but respects auto_translate.

I'm aware, but that method has a flaw: it overrides the original string if the locale is not set to "en" at the start. That means if a control has a lot of other controls (e. g. FileDialog), it would need to updated all of them manually if auto_translated is changed. But if the original string is preserved, it can let the controls themselves update the string.

I see you do atr(ETR( in some places, but you missed some strings.

I see. I will update those them.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 14, 2024

I only used ATR for dynamic texts. It's basically the same as your atr(ETR(.

@YeldhamDev
Copy link
Member Author

I only used ATR for dynamic texts. It's basically the same as your atr(ETR(.

Not really, the ColorPicker and GraphEdit constructors have them, for example. We're starting to get off-topic now.

@YeldhamDev
Copy link
Member Author

@KoBeWi I've added more cases, give me a heads-up if I forgot anything else.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 14, 2024

This change is missing if you were to supersede my PR:
https://github.com/godotengine/godot/pull/81180/files#diff-f7fb6c2c4c5d4fce35e389297048f54be3d4b08621edfc55a5586b437031ff10R51-R59
You already added TRANSLATION_CHANGED to ColorPicker, so you could handle this as well.

btw ATR in constructor was a mistake.

core/object/object.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks fine on GUI side.

@akien-mga akien-mga self-requested a review January 15, 2024 11:39
@YeldhamDev
Copy link
Member Author

Here's the proposal I was talking about: godotengine/godot-proposals#8897

@YuriSizov YuriSizov modified the milestones: 4.x, 4.3 Jan 17, 2024
@Kiisu-Master
Copy link
Contributor

Would this also enable translating built in strings with CSV or is that not possible?
I'm translating my app with CSV and have been searching for ways to also translate some built-in context menus, but I haven't found any info. Even for Gettext.

@YeldhamDev
Copy link
Member Author

@Kiisu-Master Built-in nodes should already be using translations when available.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good overall, though I'm a bit confused about when we should use atr(ETR(...)) and when it should only be ETR(...).

core/object/object.cpp Show resolved Hide resolved
Comment on lines 39 to +42
void load_editor_translations(const String &p_locale);
void load_doc_translations(const String &p_locale);
void load_property_translations(const String &p_locale);
void load_doc_translations(const String &p_locale);
void load_extractable_translations(const String &p_locale);
Copy link
Member

Choose a reason for hiding this comment

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

This is starting to look like something that could be refactored in a single method, using an enum parameter to define whether it's editor, property, doc, or extractable.

But that can be for future rework.

@@ -689,7 +705,7 @@ void ColorPicker::_text_type_toggled() {
text_type->set_icon(nullptr);

c_text->set_editable(true);
c_text->set_tooltip_text(RTR("Enter a hex code (\"#ff0000\") or named color (\"red\")."));
c_text->set_tooltip_text(ETR("Enter a hex code (\"#ff0000\") or named color (\"red\")."));
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional that some of the tooltips use atr(ETR()) and others just ETR()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tooltips obey auto translation, so if they are a static string, they don't need atr(). However, if the string has a dynamic component, it needs to be translated directly.

@akien-mga akien-mga merged commit 846428e into godotengine:master Feb 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@YeldhamDev YeldhamDev deleted the give_me_those_strings_baby branch February 29, 2024 13:42
akien-mga added a commit that referenced this pull request Mar 4, 2024
Adds 'extractable' strings after #86222.
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.

Add a way to get list of strings used in GUI nodes
7 participants