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.

Sign up for GitHub

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

Jump to bottom

Replace gettext with tinygettext #6411

Closed
wants to merge 53 commits into from
Closed

Replace gettext with tinygettext #6411

wants to merge 53 commits into from

Conversation

bunnybot
Copy link

@bunnybot bunnybot commented Mar 14, 2024 •
edited

NordfrieseMirrored from Codeberg
Created on Thu Mar 14 13:41:47 CET 2024 by Benedikt Straub (Nordfriese)

Type of change
Refactoring

Issue(s) closed
Fixes #4978
Fixes #6450

How it works
Embed tinygettext to replace gettext.

Language changes are now applied immediately. We do not mess with any environment variables, except to read LANG and/or LANGUAGE once at startup to find out the platform's system language.

This removes the dependencies on gettext and libintl. We still need xgettext to build the catalogues in the automated workflow and that's it.

Tinygettext reads the PO files directly, so I have moved them all into the data/i18n directory. I hope I updated all occurrences and that builds will still work.

Tinygettext can not use MO files. Therefore, add-on translations are broken until I make a branch for the server to serve us the raw PO files instead of precompiled MO files. (Which I will do after this branch has been merged.)

Since translations no longer need to be compiled, I removed the -DOPTION_BUILD_TRANSLATIONS option.

Since the diff is a bit cluttered by the many renamings and the new module, I recommend reviewing this commit by commit.

Possible regressions
Strings not being translated correctly
The translation maintenance scripts

@bunnybot bunnybot added this to the v1.3 milestone Mar 14, 2024
@bunnybot bunnybot self-assigned this Mar 14, 2024
@bunnybot
Copy link
Author

Assigned to Nordfriese

@bunnybot bunnybot added bug Something isn't working internationalization Translation system, string fixes, RTL support cleanup & refactoring Improving our code quality labels Mar 14, 2024
@bunnybot
Copy link
Author

tothxaMirrored from Codeberg
On Thu Mar 14 16:03:31 CET 2024, Tóth András (tothxa) wrote:

I think it would be better handled similar to asio when not packaged, ie. have a script to check a tested commit out into auto_dependencies instead of bundling it in src/third_party. It can also patch it (it's a strange choice that they hardcode the language list), but we should also try to get the additions upstream, or maintain our own fork. Merging upstream changes would still be easier than in third party, wouldn't it?

Why all the new gettext_noop()-s? Not all of them can be verified from the diffs alone.

@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Thu Mar 14 17:37:03 CET 2024, Benedikt Straub (Nordfriese) wrote:

It doesn't seem to get a lot of commits:
https://github.com/tinygettext/tinygettext/commits/master/
https://github.com/SuperTux/tinygettext/commits/master/
(I actually bundled SuperTux's fork of tinygettext, which removes a lot of unnecessary additional stuff.)
Unlike Asio which is packaged for almost all supported distros, tinygettext seems to be packaged only for one system so I think embedding it is easier than a separate download.
So IMO it would be easiest to treat it like Eris.

The gettext_noops are used during static initialization. Calling _() before the textdomains are set up simply returns the source string in gettext, but with these C++ classes it instead segfaults on startup, so we must now enforce not to call a translation function before static init is complete. These strings are all translated again before they are actually used.

@bunnybot bunnybot added review:approve The pull request has been approved. ci:success CI checks succeeded and removed review:approve The pull request has been approved. labels May 17, 2024
@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Fri May 17 11:30:35 CEST 2024, Benedikt Straub (Nordfriese) wrote:

Thanks for the review :)

<@>bunnybot merge

@bunnybot bunnybot added review:approve The pull request has been approved. and removed review:approve The pull request has been approved. labels May 17, 2024
@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Fri May 17 11:39:36 CEST 2024, Benedikt Straub (Nordfriese) wrote:

Yes, bunnbot is clearly a little confused about the review state here…

<@>bunnybot merge force

@bunnybot bunnybot closed this May 17, 2024
@bunnybot bunnybot deleted the mirror/Nordfriese/widelands/tinygettext branch May 17, 2024 09:44
Noordfrees pushed a commit to Noordfrees/widelands that referenced this pull request May 17, 2024
Replace gettext with tinygettext (CB widelands#4772 / GH widelands#6411)
524a53c 
Co-authored-by: Benedikt Straub <[email protected]>
Co-committed-by: Benedikt Straub <[email protected]>
This was referenced May 17, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tothxa tothxa tothxa left review comments

Assignees

@bunnybot bunnybot

Labels
bug Something isn't working ci:success CI checks succeeded cleanup & refactoring Improving our code quality internationalization Translation system, string fixes, RTL support review:approve The pull request has been approved.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

Addons: Date shows sign for german umlauts Set 'System Language' not applied immediately
3 participants
@bunnybot @tothxa @Noordfrees