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

Rework the auto translation system #87530

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented Jan 24, 2024

Closes godotengine/godot-proposals#8897.

This PR adds the auto_translate_mode member to Node, deprecating auto_translate in Control and Window. This not only removes the need for duplicate code between those two, but also makes it easier to add it to other node types in the future as well (such as Label3D). atr() was also moved to Node for the same reasons above.

Here's a list of the available modes:

  • AUTO_TRANSLATE_MODE_ALWAYS - Makes auto_translate work exactly as it does now.
  • AUTO_TRANSLATE_MODE_INHERIT - The new default, inherits the auto_translate value of its parent.
  • AUTO_TRANSLATE_MODE_DISABLED - The same as if auto_translate was toggled off.

The new default slight breaks compatibility, but I think it's for the better.

scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.h Outdated Show resolved Hide resolved
scene/gui/control.h Outdated Show resolved Hide resolved
scene/main/window.h Outdated Show resolved Hide resolved
@YeldhamDev
Copy link
Member Author

@AThousandShips Changes made.

@Mickeon
Copy link
Contributor

Mickeon commented Jan 24, 2024

Where's the "new default" in the current version of this PR? I don't think I see it.

Edit: Ok, I understand now.

Although, I do have a feeling that there could be a way to remove auto_translate and still maintain compatibility by manipulating _set() and _get() like some other classes do (such as RectangleShape2D's extents).

@timothyqiu
Copy link
Member

Can we deprecate auto_translate, and make auto_translate_mode one of DISABLED, INHERIT, and ALWAYS?

To keep things compatible, make auto_translate backed by auto_translate_mode under the hood.

  • If existing code sets auto_translate, set auto_translate_mode to DISABLED / ALWAYS accordingly.
  • Hide auto_translate in the Inspector and don't save it when saving the scene.
    • If exsiting scene enabled it, auto_translate_mode will be updated when opening the scene.

@akien-mga
Copy link
Member

Can we deprecate auto_translate, and make auto_translate_mode one of DISABLED, INHERIT, and ALWAYS?

To keep things compatible, make auto_translate backed by auto_translate_mode under the hood.

* If existing code sets `auto_translate`, set `auto_translate_mode` to `DISABLED` / `ALWAYS` accordingly.

* Hide `auto_translate` in the Inspector and don't save it when saving the scene.
  
  * If exsiting scene enabled it, `auto_translate_mode` will be updated when opening the scene.

Yes, this can be done by handling auto_translate is _set/_get, it doesn't even need a property registered formally (so there's no need to hide it).

scene/gui/control.h Outdated Show resolved Hide resolved
@YeldhamDev
Copy link
Member Author

@timothyqiu @akien-mga The methods themselves would still need to be registered, right? To maintain GDNative compatibility I mean.

@akien-mga
Copy link
Member

@timothyqiu @akien-mga The methods themselves would still need to be registered, right? To maintain GDNative compatibility I mean.

Yeah, but they can be registered as compat bindings. See e.g. scene/2d/tile_map.compat.inc for an example of how it's done.

Alternatively, we can keep the methods registered as normal methods and the property registered too, and mark them deprecated. Both options should be fine.

@YeldhamDev
Copy link
Member Author

Alright, with the increasing number of nodes that need auto translation that don't inherit Control, I think moving the auto translation properties back to Node is the only sane way forward.

@YeldhamDev YeldhamDev requested review from a team as code owners January 29, 2024 13:49
@YeldhamDev
Copy link
Member Author

The PR has been heavily reworked, please see the updated OP.

editor/plugins/packed_scene_translation_parser_plugin.cpp Outdated Show resolved Hide resolved
scene/main/node.cpp Outdated Show resolved Hide resolved
scene/main/node.cpp Outdated Show resolved Hide resolved
@YeldhamDev YeldhamDev changed the title Make auto translation inheritable Rework the auto translation system Jan 29, 2024
@YeldhamDev
Copy link
Member Author

This PR is ready.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 14, 2024

I don't have anything against the documentation. I'm still questioning the usefulness of atr and atr_n. I'm in the opinion that a method called can_auto_translate() would be more generally useful and less confusing, but I'm not going to cry in my sleep about it.

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.

Quick code review. Looks good!
Still have to actually test.

doc/classes/Node.xml Outdated Show resolved Hide resolved
doc/classes/Node.xml Outdated Show resolved Hide resolved
@@ -3558,9 +3550,12 @@ void Control::_bind_methods() {
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "size_flags_stretch_ratio", PROPERTY_HINT_RANGE, "0,20,0.01,or_greater"), "set_stretch_ratio", "get_stretch_ratio");

ADD_GROUP("Localization", "");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "auto_translate"), "set_auto_translate", "is_auto_translating");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "localize_numeral_system"), "set_localize_numeral_system", "is_localizing_numeral_system");
Copy link
Member

Choose a reason for hiding this comment

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

Just food for thought - is this something that could be streamlined together with the added auto_translate_mode, maybe adding enum values that enable both translation + numbers localization?

May not be a good idea, but just leaving this here for consideration.

Copy link
Member Author

Choose a reason for hiding this comment

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

As much as I like the idea, how many extra options that would add? I feel like it would make it a little convoluted.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I don't really see how it would work to be honest :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Clamping it all together in a BitField of some sort sounds interesting...

scene/main/node.cpp Show resolved Hide resolved
scene/main/node.h Show resolved Hide resolved
scene/main/window.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

I tested it with this scene setup:

image

With Auto Translate disabled on FileDialog, and after setting the project test locale to fr and importing editor/translations/editor/fr.po as translations, it seems like the internal nodes of the dialog still get translated automatically. Only the dialog title seems to stay in English (and gets translated with Auto Translate enabled).

With Inherit/Always:
image

With Disabled:
image

MRP: autotranslate_filedialog.zip

@YeldhamDev
Copy link
Member Author

@akien-mga

With Auto Translate disabled on FileDialog, and after setting the project test locale to fr and importing editor/translations/editor/fr.po as translations, it seems like the internal nodes of the dialog still get translated automatically. Only the dialog title seems to stay in English (and gets translated with Auto Translate enabled).

I fixed the part that this PR affects, however, this will be only fixed completely with #86222, since this bug occurs because those strings are translated before being given to the button.

doc/classes/Window.xml Outdated Show resolved Hide resolved
doc/classes/Control.xml Outdated Show resolved Hide resolved
@YeldhamDev
Copy link
Member Author

Changes made.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 17, 2024
@akien-mga akien-mga merged commit 2c5fa95 into godotengine:master Feb 17, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@YeldhamDev YeldhamDev deleted the atr_inheritance branch February 17, 2024 15:05
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.

Make auto_translate state inheritable
8 participants