Skip to content

fix: properly work with languages with multiple scripts #18167

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

Merged
merged 2 commits into from
May 9, 2025

Conversation

LPkkjHD
Copy link
Contributor

@LPkkjHD LPkkjHD commented May 8, 2025

Description

The pointers by @zlewe in the Issue have been very helpful. This is literally just the implementation.

Fixes #17965

How Has This Been Tested?

This has been manually tested with the following steps

  • Open App
  • Choose {zh_TW|zh_CN|sr_Latn|sr_Cyrl}
  • Reopen App
  • Observe the chosen language to still be selected and applied

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if applicable
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

@alextran1502 alextran1502 enabled auto-merge (squash) May 8, 2025 20:32
@alextran1502
Copy link
Contributor

Please fix the formatting in the file. Thanks

auto-merge was automatically disabled May 9, 2025 06:37

Head branch was pushed to by a user without write access

@LPkkjHD
Copy link
Contributor Author

LPkkjHD commented May 9, 2025

@alextran1502 Done!

@zlewe
Copy link
Contributor

zlewe commented May 9, 2025

Hi @LPkkjHD , thanks for the mention.

The i18n files should be renamed too right? I'm assuming it would default to English if it doesn't find the files.

@LPkkjHD
Copy link
Contributor Author

LPkkjHD commented May 9, 2025

The i18n files should be renamed too right? I'm assuming it would default to English if it doesn't find the files.

Not quite. The i18n files are named correct (at least that's what I gathered from testing this change).

My read on why this bug occurred is that Locale(String _languageCode, [String? _countryCode]) as per documentation does not check for invalid countryCode tags (invalid in the sense, that they don't exist in the standard) and therefore ignores it and only sets the default locale for the given languageCode.

@alextran1502 alextran1502 merged commit 2ffcfe0 into immich-app:main May 9, 2025
41 checks passed
@zlewe
Copy link
Contributor

zlewe commented May 10, 2025

The i18n files should be renamed too right? I'm assuming it would default to English if it doesn't find the files.

Not quite. The i18n files are named correct (at least that's what I gathered from testing this change).

My read on why this bug occurred is that Locale(String _languageCode, [String? _countryCode]) as per documentation does not check for invalid countryCode tags (invalid in the sense, that they don't exist in the standard) and therefore ignores it and only sets the default locale for the given languageCode.

I agree with your read on the root cause of the original bug – that the Locale constructor wasn't properly handling the non-standard country codes like SIMPLIFIED and Hant, causing the locale setting not to stick correctly.

However, I think the change might have introduced a new issue. By changing Locale('zh', 'SIMPLIFIED') to Locale('zh', 'CN') and Locale('zh', 'Hant') to Locale('zh', 'TW'), the localization package now seems to fail to find the corresponding translation files based on my testing.

So, while the setting itself seems to stick correctly with your change, the UI language doesn't actually update and remains in English because the correct translation file isn't being loaded.

Perhaps the i18n file names do need to be updated to match the new locale identifiers? Or is there something else I might be missing in my test setup?

@LPkkjHD
Copy link
Contributor Author

LPkkjHD commented May 10, 2025

My Testing was starting the app and upon seeing the Letters of the designated scripts in the timeline checking whether the right language was still set. However, you're absolutely spot on with your take (obviously the date would translate correctly, as this is managed by the system rather than the translation strings).

I'm ashamed this slipped though as it's a rather plain error.

Perhaps the i18n file names do need to be updated to match the new locale identifiers? Or is there something else I might be missing in my test setup?

I think renaming them is a rather big hustle and touches the web code as well. We could abuse the Locale#fromSubtags() in conclusion with the scriptCode Hant | SIMPLIFIED in order to keep the current file naming as is.

savely-krasovsky pushed a commit to savely-krasovsky/immich that referenced this pull request Jun 8, 2025
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.

Using Chinese Traditional (zh_TW) will become Chinese Simplified (zh_CN) when restart the app
3 participants