Add automatic translation of items to ItemList

[DennisManaa]

What this PR does

While using the ItemList-Node, I noticed that items were not automatically translated when switching between locales. To rectify this issue, I am submitting this pull request (PR). The prior implementation lacked this functionality entirely, so this PR introduces automatic item translation to the ItemList-Node. (Notably, it's worth mentioning that many other nodes, like MenuButton or OptionButton, already include automatic translation. Initially, I presumed the issue addressed in this PR might be a bug.)

How it was before

Bildschirmaufzeichnung.vom.2023-10-18.21-18-34.webm

How it is now

Bildschirmaufzeichnung.vom.2023-10-18.21-23-23.webm

Minimal Test Project

Here you can download the project I used in the videos above.
test-project.zip

[DennisManaa]

To provide further clarity regarding the nature of this PR, I'd like to argue that it isn't precisely a "feature proposal.". However, I am open to any classification that helps the process. :)

As demonstrated in the accompanying video for two other nodes, it's worth noting that many other nodes already demonstrate the same behavior I've implemented in this PR for the ItemList. (Even the button that is used for switching the locale, uses the automatic translation feature to switch its text in the video). The absence of automatic translations in the ItemList-Node is more of an exception. So for an end user, the absence of this feature in the ItemList feels more like a bug, because virtually all/most other GUI-Nodes support it currently.

Technically, I absolutely agree that it qualifies as an enhancement, and technically, it isn't a bug fix since automatic translation was never implemented for the ItemList. However, I'm just uncertain if it should be categorized as a "feature proposal" when all or at least the majority of other GUI-Nodes already exhibit the exact behavior that I have implemented here.

Bildschirmaufzeichnung.vom.2023-10-18.23-23-38.webm

[KoBeWi]

While the PR uses already-existing functionality, it changes the node's behavior by adding a new feature. It doesn't really matter, we had similar PRs for other nodes (like PopupMenu), so it probably doesn't need a proposal.

That said, you should check ItemList usage in the editor and call set_auto_translate(false) where necessary, as this change will likely cause words getting translated randomly when they shouldn't.

[DennisManaa]

Thank you for the clarification; I get the point. :)

Regarding the use of ItemList in the editor, you're absolutely right—I hadn't considered that aspect. I will investigate it as soon as possible, hopefully by tomorrow or the day after. Thanks for mentioning this 👍

[DennisManaa]

@KoBeWi, I've examined the usage of ItemList in the editor, and there are a total of 28 occurrences. The majority of these ItemLists are used for things such as file names, method names, or contributor names in the "About Godot" window. These are all elements that should remain unaltered in translation.

There are only a few instances where the text within the editor truly requires translation. For example, in the Export Window, the text within parentheses is translated. However, it's worth noting that this functionality worked properly in the past (and as far as I can tell when autotranslation is disabled) due to an alternate implementation (e. g. in the screenshots, where the text "Runnable" inside the parentheses is translated).

For now, I've disabled autotranslation for all instances of ItemList in the editor. It would also be a good idea to remove the individual attempts at automatic translation in places where this is currently happening (replacing them with the new feature provided by the ItemList, if possible). Probably it would be a good idea to do this in individual pull requests, where it can be tested in an isolated manner, as such changes might carry the risk of introducing new bugs. What do you think about this matter/this approach?

[QbieShay]

Approving on behalf of shaders (changes are trivial), but still needs approval from editor maintainers.

[AThousandShips]

Havent tested but looks straightforward

[KoBeWi]

The implementation looks ok.

That said, I wonder how much this feature is useful. Unlike other controls with list auto-translation, like MenuButton or OptionButton, ItemList is intended for dynamic content that most often shouldn't be translated. It's similar to Tree in that regard, which can't even be edited in the inspector. Maybe the auto_translate should be disabled by default for ItemList? It's doable by calling set_auto_translate(false) in the constructor. (this would make your other changes unnecessary 🙃) Up to discussion.

[DennisManaa]

Thanks for Reviewing :)

The question regarding the usefulness of this feature is indeed a good one. I'll try to explain my specific use case for this feature and why I think that it is valuable, but of course it's up to discussion if it is valuable for all users of the engine.

Currently, I utilize ItemLists on a settings screen within my game in a manner similar to OptionButtons. Nonetheless, there exists a fundamental distinction between these nodes: ItemLists always display all available options to the user, thereby enhancing usability, particularly in cases where the options are limited in number. (my opinion/design decision, so feel free to disagree)

To illustrate, the settings screen closely resembles the one shown in the following video. There are only 2 ItemLists and the MSAA-ItemList is a very bad example, but it should be good enough to explain the usecase:

Bildschirmaufzeichnung.vom.2023-10-28.01-55-05.webm

In this specific scenario, the text that requires translation remains static and never changes. Therefore, it seems logical to include it in the translation file. However, to achieve this, I currently rely on the following script:

extends ItemList

var _initial_option_texts: Array[String] = [];

func _ready() -> void:
    var item_num: int = 0;
    while(item_num < item_count):
        _initial_option_texts.append(get_item_text(item_num));
        item_num += 1
    
    _translate();
    SettingsManager.language_changed.connect(_translate);

func _translate() -> void:
    var item_num: int = 0;
    while(item_num < item_count):
        set_item_text(item_num, tr(_initial_option_texts[item_num]));
        item_num += 1;

In my personal view, this approach is suboptimal and counterintuitive, as most/all other elements support automatic translation. Initially, I even suspected a bug, when working with ItemLists, because it worked with all of the other Nodes I used.

Furthermore, I am open to discussing the possibility of disabling the auto_translate setting for ItemLists by default. Both options are fine to me, but I believe it's important to consider how other elements handle this to maintain consistency. Introducing an inconsistency, for instance, if all other elements enable auto translation by default, should be weighed carefully, because it might confuse users of the engine as the absence of auto-translation for ItemLists once confused me. 🙃

So I'm open to change this PR in any way that fits best :)

[KoBeWi]

Furthermore, I am open to discussing the possibility of disabling the auto_translate setting for ItemLists by default. Both options are fine to me, but I believe it's important to consider how other elements handle this to maintain consistency. Introducing an inconsistency, for instance, if all other elements enable auto translation by default, should be weighed carefully, because it might confuse users of the engine as the absence of auto-translation for ItemLists once confused me. 🙃

We do have some nodes that change the default values. Such overrides are listed in the documentation. We could add a more prominent note in the description of the class.

[DennisManaa]

We do have some nodes that change the default values. Such overrides are listed in the documentation. We could add a more prominent note in the description of the class.

In this case, I'd be happy to go ahead with your suggestion to disable auto_translate by default. I also like the idea to add a note in the description of the class. Do you think we need another opinion on this, or should I just start implementing it? :)

[KoBeWi]

There is no rush, we could wait.
Or you can make this change in a separate commit for now.

[YeldhamDev]

I think the auto-translation should be enabled by default, as it's how it works for all other nodes, so it would be odd for this one to go against the grain. While this would technically be a change in behavior, I really don't see it breaking anyone's project.

[akien-mga]

Thanks!

[CsloudX]

Tree & GraphNode need fix also. ^V^ #80549