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

Better support for unicode strings in TTR #78909

Closed
wants to merge 1 commit into from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 1, 2023

This string

Label *label = memnew(Label(vformat(TTR(U"Dimensions: %d × %d"), dimensions.x, dimensions.y)));

is not included in translation files, because U breaks the regex. The U is necessary to make the string parsed as unicode, because TTR converts it automatically to String.

I came up with as solution that involves the least changes - I just added an overload for TTR that takes char * and automatically interprets it as UTF-8. This probably means that all translated strings will now be parsed as unicode, not sure if brings any drawbacks (maybe it's slower than just making a String, idk).

@KoBeWi KoBeWi added this to the 4.2 milestone Jul 1, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner July 1, 2023 14:08
@akien-mga akien-mga requested review from timothyqiu, akien-mga and bruvzg and removed request for a team July 1, 2023 14:22
core/string/ustring.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

If we go with this solution, it likely also needs an equivalent for TTRN and RTR.

@Calinou
Copy link
Member

Calinou commented Jul 1, 2023

The U is necessary to make the string parsed as unicode

That reminds me, could we replace uses of String::utf8() in the codebase with U"string"? I've relied on the former exclusively so far.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 1, 2023

I think we could, at least for untranslated strings.

@bruvzg
Copy link
Member

bruvzg commented Jul 1, 2023

U"*" should be a bit faster since it's converted at the compile time.

I guess we can also add an extra case here to handle U instead - https://github.com/godotengine/godot-editor-l10n/blob/73d94c5089831177414c301bd671cf731694d89e/scripts/extract_editor.py#L52

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 1, 2023

Ok I pushed other methods and also simplified the implementation.

@timothyqiu
Copy link
Member

That reminds me, could we replace uses of String::utf8() in the codebase with U"string"? I've relied on the former exclusively so far.

Yeah, that's technically more portable. As U"string" is guaranteed to be UTF-32, while String::utf8() requires the source file to be UTF-8 encoded.

@akien-mga
Copy link
Member

godotengine/godot-editor-l10n#6 should be sufficient to fix this I believe.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 2, 2023

Yeah, closing.

@KoBeWi KoBeWi closed this Jul 2, 2023
@KoBeWi KoBeWi added the archived label Jul 2, 2023
@KoBeWi KoBeWi removed this from the 4.2 milestone Jul 2, 2023
@KoBeWi KoBeWi deleted the TTRU branch July 2, 2023 10:56
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.

5 participants