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

Improve localization logic #36

Merged
merged 2 commits into from
Jun 12, 2019
Merged

Improve localization logic #36

merged 2 commits into from
Jun 12, 2019

Conversation

SamusAranX
Copy link
Contributor

@SamusAranX SamusAranX commented May 23, 2019

This should fix the issue where some language code/region code combinations would result in English text instead of the appropriate localization.

I noticed that most of the different region codes used the same localizations, so I removed the redundant copies. The only exceptions are the localizations for zh-CN, zh-HK, and zh-TW, because those are in fact different. To account for that, zh-* locales are now special cased in the code.

For all other language code/region code combinations, these changes should result in less error-prone localization, as the region code isn't considered at all anymore.

Additionally, the user's preferred languages are now checked for valid language codes before falling back to the system language.

@@ -15,13 +15,9 @@ struct Localization {
"de": "Einstellungen",
"el": "Προτιμήσεις",
"en": "Preferences",
"en-AU": "Preferences",
"en-GB": "Preferences",
Copy link
Owner

Choose a reason for hiding this comment

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

Even if these are the same, I still think we should keep them since they're coming from the system and for consistency between localization strings, as some string might differ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I compared all these strings using a script to make sure they're 100% the same. In case the translation for various variants of English changes, they can be put back in just by adding them to the dictionary, but for now, there's no use in keeping them.

Sources/Preferences/Localization.swift Outdated Show resolved Hide resolved
if locale.languageCode != nil {
preferredLocale = locale
break
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

This could be simplified to:

let preferredLocale = Locale.preferredLanguages
			.lazy
			.map { Locale(identifier: $0) }
			.first { $0.languageCode != nil }
			?? Locale.current

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The term "simplify" is very subjective in this context. 😄
I didn't do any performance comparisons, I just personally think my version is more readable.
Also, IIRC, locale.languageCode was never nil in my tests, so in the overwhelming majority of cases, the loop should stop after just one iteration anyway.

}

if let localizedString = localizedDict[languageCode] {
return localizedString
// Chinese is the only language where different region codes result in different translations
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure about that? What are you basing it on? It feels weird hard-coding this. From experience, assumptions about date, time, and localizations are usually wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is based on comparing the dictionary's entries and checking which region code variants are actually different.
I admit it does feel weird to special case this, and I'd be happy to change it, but as long as Chinese is the only language where different region codes produce different translations for "Preferences", I'd prefer keeping it this way.

Copy link
Owner

Choose a reason for hiding this comment

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

There must be some better system APIs we can use that handles the resolution here so we don't have to hard-code it like this. Good enough for now I guess, but I would want to fix this properly in the future.

@sindresorhus sindresorhus changed the title Improved localization logic Improve localization logic Jun 12, 2019
@sindresorhus sindresorhus merged commit f4615f4 into sindresorhus:master Jun 12, 2019
@sindresorhus
Copy link
Owner

Thanks :) 🙌

@SamusAranX SamusAranX deleted the localize-zh branch September 23, 2019 12:16
dezinezync pushed a commit to dezinezync/Preferences that referenced this pull request Dec 17, 2022
dezinezync pushed a commit to dezinezync/Preferences that referenced this pull request Dec 17, 2022
dezinezync pushed a commit to dezinezync/Preferences that referenced this pull request Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants