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

Validate environment locale settings #6455

Closed
wants to merge 5 commits into from
Closed

Validate environment locale settings #6455

wants to merge 5 commits into from

Conversation

bunnybot
Copy link

NordfrieseMirrored from Codeberg
Created on Fri May 17 18:41:31 CEST 2024 by Benedikt Straub (Nordfriese)

Type of change
Bugfix

Issue(s) closed
Fixes #6454

New behavior

  • Split preference lists at the colon and consider each entry separately
  • Skip languages not understood by tinygettext
  • C locale aliases to en
  • LC_ALL=C takes top precedence

Possible regressions
System language detection ^^

Screenshots
If applicable, add screenshots to help explain the feature.

Additional context
Setting a locale understood by tinygettext but not used by Widelands (e.g. cy Welsh) is accepted and treated like a language with 0% completion. I don't think it's worth the effort making this extra distinction though.

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

Assigned to Nordfriese

@bunnybot bunnybot added bug Something isn't working internationalization Translation system, string fixes, RTL support regression This used to work... labels May 17, 2024
@bunnybot
Copy link
Author

frankystoneMirrored from Codeberg
On Fri May 17 19:08:39 CEST 2024, frankystone wrote:

Works!

And LC_ALL=C works also!

Thanks.

@bunnybot bunnybot added the ci:success CI checks succeeded label May 17, 2024
bunnybot
bunnybot commented May 17, 2024
View reviewed changes
Copy link
Author

@bunnybot bunnybot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tothxaMirrored from Codeberg
On Fri May 17 21:53:38 CEST 2024, Tóth András (tothxa) commented.

src/base/i18n.cc
/* Check environment variables in order of precedence:
* "LANGUAGE", "LC_ALL", "LC_MESSAGES", "LANG"
* Each environment variable, if set and not empty, may be a preference list of any number of
* languages separated by colons. We must skip entries that are not known to the translations
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tothxaMirrored from Codeberg
On Fri May 17 21:53:38 CEST 2024, Tóth András (tothxa) wrote:

Isn't a colon separated list only valid for LANGUAGE?

@bunnybot
Copy link
Author

tothxaMirrored from Codeberg
On Fri May 17 22:01:01 CEST 2024, Tóth András (tothxa) wrote:

While we're at it, the testsuite should be changed to use plain en (or C) instead of en_US. I'd also patch tinygettext to not log fallbacks at all. Then tinygettext log_info() can be routed to WL plain log_info() instead of verb_log_info(). (I wanted to do these, but can only do it sometime next week.)

@bunnybot
Copy link
Author

NordfrieseMirrored from Codeberg
On Sat May 18 09:44:46 CEST 2024, Benedikt Straub (Nordfriese) wrote:

Then tinygettext log_info() can be routed to WL plain log_info() instead of verb_log_info().

This still results in masses of irrelevant log output about strings that can't be translated in the main menu alone. There's a lot of background scanning for maps and savegames that doesn't bother with correct textdomains, and installed add-ons without translations, and for languages that are not 100% complete it would be much worse…

Isn't a colon separated list only valid for LANGUAGE?

IMO it doesn't hurt to perform the split for all variables. Since environment variables are freely modifiable I wouldn't say there's any "invalid" content possible at all, just unexpected usages :)

@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 18, 2024
@bunnybot bunnybot added ci:success CI checks succeeded and removed ci:success CI checks succeeded labels May 23, 2024
@bunnybot
Copy link
Author

tothxaMirrored from Codeberg
On Sat May 25 11:25:28 CEST 2024, Tóth András (tothxa) wrote:

IIRC you've mentioned somewhere to use --verbose-i18n for reporting untranslated strings. Here's a patch (suggestion) implementing that, that I use when testing #6412. Could you please review it and throw it in here if you agree with the approach?

bunnybot
bunnybot commented May 25, 2024
View reviewed changes
Copy link
Author

@bunnybot bunnybot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tothxaMirrored from Codeberg
On Sat May 25 11:33:34 CEST 2024, Tóth András (tothxa) approved this pull request:

Thank you!

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

NordfrieseMirrored from Codeberg
On Sat May 25 18:28:21 CEST 2024, Benedikt Straub (Nordfriese) wrote:

<@>bunnybot merge

@bunnybot bunnybot closed this May 25, 2024
@bunnybot bunnybot deleted the mirror/Nordfriese/widelands/4816-validate-env-locale branch May 25, 2024 16:31
Noordfrees added a commit to Noordfrees/widelands that referenced this pull request May 25, 2024
@Noordfrees
Validate environment locale settings (CB widelands#4817 / GH wideland…
b85b8b9 
…s#6455)

Co-authored-by: Benedikt Straub <benedikt-straub@web.de>
Co-committed-by: Benedikt Straub <benedikt-straub@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bunnybot bunnybot

Labels
bug Something isn't working ci:success CI checks succeeded internationalization Translation system, string fixes, RTL support regression This used to work... review:approve The pull request has been approved.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

Assertion `language' failed in debug builds
2 participants
@bunnybot @Noordfrees