NOTIFICATION_TRANSLATION_CHANGED gets sent at wrong times

[Kiisu-Master]

Tested versions

• Reproducible in 4.3.dev4, 4.3.dev5
• Not reproducible in 4.3.dev3

Likely due to #87530 (@YeldhamDev)

System information

Windows 10

Issue description

The notification NOTIFICATION_TRANSLATION_CHANGED now gets emit when creating nodes nodes enter the tree.

It happens before the node is ready, which leads to crashes in one of the most common use case: you hook something like update_ui() to it. Generally the function to update UI will set a bunch of text on other nodes, but as the node just got created and its children aren't ready, it will lead to crashes. This also creates duplicate calls to the update_ui, because in most uses _ready would already take care of initial setup call.

Steps to reproduce

• Create a scene with node that listens on _notification for NOTIFICATION_TRANSLATION_CHANGED. (print something or put a breakpoint)
• The notification will happen when running project, even when locale did not change.

Minimal reproduction project (MRP)

Translation changed bug MRP.zip

[AThousandShips]

This is by design, and it matters, the translation isn't known until it enters the tree so having this emitted is necessary, how else would you know what the configured translation for the tree is?

This should maybe be documented better though

The main issue here is simply that you aren't adjusting to how the engine works

but as the node just got created and its children aren't ready

The node isn't ready until its children are, they have already had _ready fired on them when their parent fires it

This also creates duplicate calls to the update_ui, because in most uses _ready would already take care of initial setup call.

The solution to that is to just do those on the translation notification though, right?

Edit: The issue here (which isn't clear from the description) seems to be that it's fired on entering the tree, not in _ready, that is something to discuss, but that's something to adjust to I'd say, it's not impossible to deal with this, and this improvement is important and good

We could theoretically move it to some other point in the initialization, but that's a complex question, and depends on various use cases

[Kiisu-Master]

This is by design, and it matters, the translation isn't known until it enters the tree so having this emitted is necessary, how else would you know what the configured translation for the tree is?

• The notification is sent when node enters tree. That is incorrect time to send notification, as the translation didn't change.
• The node doesn't need to know that it entered tree by receiving the NOTIFICATION_TRANSLATION_CHANGED.
• The node does not receive info about the locale from the notification, and it doesn't need to. It would in most cases just use tr() to retranslate all text after translation changed.

This also creates duplicate calls to the update_ui, because in most uses _ready would already take care of initial setup call.

The solution to that is to just do those on the translation notification though, right?

Using the translation notification instead of _ready for initialization seems crazy to me.

In my opinion the notification should only be sent when setting the locale, not when node enters tree. That would be how it worked before 4.3.dev4.

[AThousandShips]

The notification is sent when node enters tree. That is incorrect time to send notification, as the translation didn't change.

How would you know? What if it's configured for a different locale than the current one?

Using the translation notification instead of _ready for initialization seems crazy to me.

Well that's your opinion, and it's different from the people developing this feature :)

[Kiisu-Master]

The notification is sent when node enters tree. That is incorrect time to send notification, as the translation didn't change.

How would you know?

Well, as i am the one who wrote the code of the MRP , i know it will never change the translation, nor does it even have any translations in the project.

Using the translation notification instead of _ready for initialization seems crazy to me.

Well that's your opinion, and it's different from the people developing this feature :)

Uhhh

[AThousandShips]

Well, as i am the one who wrote the code of the MRP , i know it will never change the translation, nor does it even have any translations in the project.

So you don't care about this notification... So your use case isn't relevant, why are you even using the notification if you're not using translation?

Bur you're missing my point:
How would you know that your node is configured for the current locale?

[Kiisu-Master]

Well, as i am the one who wrote the code of the MRP , i know it will never change the translation, nor does it even have any translations in the project.

So you don't care about this notification... So your use case isn't relevant, why are you even using the notification if you're not using translation?

No i just made a MRP. I discovered the problem in GodSVG when opening in 4.3 dev. It will currently crash due to this when you click on the settings button. This doesn't happen in 4.2 or 4.3.dev3.

How would you know that your node is configured for the current locale?

It just entered the tree and used the currently set locale to translate everything. The notification is generally only used to update nodes when they are already in the tree.

[AThousandShips]

It just entered the tree and used the currently set locale to translate everything. The notification is generally only used to update nodes when they are already in the tree.

That's inefficient though, why not just use the single notification, that's much more efficient, you're only considering this an issue because you're used to one method, it makes a lot of sense and improves a lot of code, avoids duplication of code, etc.

You really don't see the value of this?

But as I said already, you need to adjust your code to how the engine works, that's what happens with compatibility breakage :) so the crash isn't a bug with this, it's because your code isn't fixed to adjust to how the engine works, please try updating your code so it works

[MewPurPur]

That's inefficient though, why not just use the single notification

I don't understand how that's supposed to happen more efficiently. How would the code of the MRP need to change? I can only see this working by adding an is_node_ready().

[AThousandShips]

To adjust to translation specially, handle it in a safe way, I don't see why you change the translation of another node, it should do that itself? It should not rely on @onready

As I already said we can evaluate and discuss if this should be moved after _ready, but it doesn't make sense to make another control translate IMO, it should do it itself

Note that in 4.2 you'd still error if you, for example, called set_auto_translate on the node when it's not ready

[KoBeWi]

NOTIFICATION_TRANSLATION_CHANGED now fires consistently with NOTIFICATION_THEME_CHANGED - when node enters tree. This is the first occasion when a translation/theme can be consistently fetched, hence you are supposed to use it to update your theming and strings that don't use auto-translation.

Note however that these notifications are mostly used internally, where nodes are created from code instead of being part of an instantiated scene. Hence there is discrepancy between core usage and how would users typically use it. You should use these notifications only on the current nodes, nodes created from script inside _init(), and if you want to use it on child nodes created in the editor, you need to wait until ready signal.

We should document this if it isn't mentioned already.

EDIT:
Also the reason why this was changed is that translation is now propagated from parent nodes (see auto_translate_mode).