[Editor] Fix parsing translations in EditorTranslationParserPlugin

[AThousandShips]

Method used passed arguments as return data which does not work with extensions, switched to using a Dictionary return. This might work in extensions after fixing typed array copying, but unsure, and I think this return format is safer in any case to use, and is the standard in most of the virtual methods like this (also any binding for this will have to use const_cast as the resulting methods will pass as const TypedArray<String> &)

Follow-up to:

• POT Generator: Add support for TRANSLATORS: and NO_TRANSLATE comments #98099

See also:

• Fix some arguments being passed as references in virtual functions #92175

[AThousandShips]

To illustrate the issue, this is the signature of the related _parse_file method, which is being fixed in the related PR:

        virtual void _parse_file(const String &p_path, const TypedArray<String> &p_msgids, const TypedArray<Array> &p_msgids_context_plural);

Haven't generated the headers for the current state but _get_comments will have the same signature and you need to const_cast those arguments, which is very unintuitive and fragile

[dalexeev]

I'm not familiar with the GDExtension issue, but it looks good to me as long as _get_comments() is consistent with _parse_file() (so I would prefer #92175 to be merged or at least approved first). However, I have a question:

1. If we decide to break _parse_file() compatibility, maybe it makes sense to combine _parse_file() and _get_comments() into a single method that returns a table (Array[PackedStringArray], Array[Array], or Array[Dictionary]) with the columns msgid, msgctxt, msgid_plural, and comment? The required column would be msgid, the others can be left blank. I don't think it makes sense to separate msgids and msgids_context_plural as it is now, because it's all a single table under the hood anyway.
2. If we want to maintain maximum compatibility, we could add a _parse_file_ext() or _parse_file_2() method and deprecate _parse_file(). This is more related to Fix some arguments being passed as references in virtual functions #92175, as _get_comments() did not appear in any stable release.

[AThousandShips]

That makes sense, will test some alternatives for deprecation and update as needed

I'm not familiar enough with the code here to say if merging the two methods into one is feasible, but if it is might merging the two, dropping _get_comments and deprecating _parse_file, work?

The new method could be _parse_file_and_comments or something, would be less obscure to have as a replacement maybe

That way we could possibly detect if _parse_file_and_comments or what we call it is bound, and otherwise call the outdated one

Would depend on if we consider deprecation worth it, unsure what happens in other languages but it is possible to use it in godot-cpp but requires casting, I don't know how it works in C# though so confirming how the current methods work or don't work for plugins made in C# would be good

It does seem that these cases do work with plugins in GDScript so that's a reason to deprecate over entirely replacing, but will test with GDScript soon to confirm

[raulsntos]

I don't know how it works in C# though so confirming how the current methods work or don't work for plugins made in C# would be good

I haven't tested personally but since Arrays in C# are reference types I imagine they may work. #99195 uses the method so, assuming the PR works, it would prove that the method works in C#.

Also, how is the GDExtension validation not complaining about breaking compatibility? Does it not check virtual methods?

[dalexeev]

Also, how is the GDExtension validation not complaining about breaking compatibility? Does it not check virtual methods?

EditorTranslationParserPlugin._get_comments() is not in 4.3 stable. Only #92175 breaks compatibility, this PR can be considered as a non-breaking part of #92175.

[dalexeev]

I'm not familiar enough with the code here to say if merging the two methods into one is feasible, but if it is might merging the two, dropping _get_comments and deprecating _parse_file, work?

The new method could be _parse_file_and_comments or something, would be less obscure to have as a replacement maybe

That way we could possibly detect if _parse_file_and_comments or what we call it is bound, and otherwise call the outdated one

I guess it depends on how we handle other cases in #92175. Yes, the fact that it seems to work in GDScript and C# is an argument for deprecation instead of removal/replacement. However, I don't think EditorTranslationParserPlugin is used very often, so breaking compatibility there is more acceptable than with core APIs like Object, Node, etc.

[AThousandShips]

I'll see about some options today for this and #92175

[AThousandShips]

Will try investigate this properly this week

Edit: haven't been able to find the time to investigate this yet but will try next week

[akien-mga]

Bump :) This seems to be important to merge before 4.4, as IIUC it's fixing the return type of a newly exposed function.

[AThousandShips]

Agreed, will try get to this before the end of the week!

[AThousandShips]

Working on this and testing out making a new parse_file with the new compatibility system, will report back with results

[AThousandShips]

Gotten it to work, and works correctly with existing code and plugins, just tested the plugin in GDScript so couldn't test the deprecation

The one question is if we keep get_comments as a deprecated method in tandem, for now I've simply dropped it, pushing soon

Edit: Will update the documentation after we've decided on a solution

[KoBeWi]

The return value will need explanation.

[AThousandShips]

Agreed, see above:

Will update the documentation after we've decided on a solution

[akien-mga]

WDYT about this solution @dalexeev? This builds upon your previous PR, changing the API, so if wanted it would be good to get this merged for 4.4.

[AThousandShips]

Ping @dalexeev

[AThousandShips]

Pushed a new solution, the order is different from the one suggested in the example above, see:
test-translation-parser.zip

This is to match the old method order, with [message, context, plural message, comment], can adjust it desired but I think keeping the old format is cleaner, it also matches the internals of POTGenerator

Will write the documentation and update the example if this new solution is approved

The above test project also contains some examples of the engine systems for the same to confirm it works correctly when parsing scenes and GDScript

[dalexeev]

Looks good to me, just a few minor notes and incomplete documentation.

[dalexeev]

Not directly related to this PR, but I would replace Vector<String> with a 3-string struct. This code is safe, but relies on the implicit assumption that the vector is always 3-element:

godot/modules/gdscript/editor/gdscript_translation_parser_plugin.cpp

Lines 351 to 352 in e567f49

	Vector<String> id_ctx_plural;
	id_ctx_plural.resize(3);

[AThousandShips]

Documentation added and updated, need help to confirm the C# part of the examples as I'm unfamiliar with C# and how to handle packed arrays and similar there

[akien-mga]

Thanks!

[AThousandShips]

Thank you!