-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add translation by country #809
base: master
Are you sure you want to change the base?
Conversation
9e9649f
to
da958ad
Compare
cf15192
to
a5ddcb9
Compare
e5640c6
to
a1c3c72
Compare
In general this is good initiative and wanted to add it myself at some point finally. 👍🏻 If you are up for some refactors I would have some propositions as I see it:
data class AppLocale(
val language: AppLanguage,
val country: AppCountry,
) {
companion object {
fun default() = AppLocale(
language = AppLanguage.getDefault(),
country = AppCountry.getDefault(),
)
}
fun getLocaleCode() = "${language.code}-${country.code}".lowercase()
}
companion object Key {
private const val LOCALE = "KEY_LOCALE"
...
}
...
var locale: AppLocale
get() {
val value = preferences.getString(LOCALE, null) ?: AppLocale.default().getLocaleCode()
val (languageCode, countryCode) = value.split("-")
return AppLocale(
appLanguage = AppLanguage.fromCode(languageCode),
appCountry = AppCountry.fromCode(countryCode),
)
}
set(value) = preferences.edit(true) { putString(LOCALE, value.getLocaleCode()) } When having this I would continue the further refactors. It would clear things a bit IMHO. wdyt |
a1c3c72
to
4a04dec
Compare
4a04dec
to
2096edb
Compare
Completely agree (the Pair didn't leave a very readable code with the first and second). I've updated the pull request with the changes you mentioned with the only difference of separating the locale with _ instead of - because it's more similar to the way Android use it (without taking into account the uppercase country). |
|
||
companion object { | ||
const val DELIMITER = "_" | ||
fun default() = AppLocale( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking this should actually be converted into property to avoid creating a new object all the time.
@@ -33,28 +33,31 @@ class TranslationsRepository @Inject constructor( | |||
private val mappers: Mappers, | |||
) { | |||
|
|||
fun getLanguage() = miscPreferences.getString(LANGUAGE, DEFAULT_LANGUAGE) ?: DEFAULT_LANGUAGE | |||
fun getLocale() = miscPreferences.getString(LOCALE, null)?.let { AppLocale.fromCode(it) } ?: AppLocale.default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be cleaner to just get rid of this method from TranslationRepository and get locale from settings repository. Wherever TranslationRepository is injected.
val language = translationsRepository.getLanguage() | ||
if (language == DEFAULT_LANGUAGE) { | ||
val locale = translationsRepository.getLocale() | ||
if (locale == AppLocale.default()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These checks (and also the ones with != ) will need to be updated because now it will only check against "en-us". And for users from ex. United Kingdom (en_GB) it would always try to load english translation which does not make sense really.
Was also thinking of getting rid of these checks accross the code and put them into TranslationRepository itself. If english language is requested return null or empty list. 🤔
Overview
This Pull Request introduces a version for Showly 2.0 that allows movie and series translations to be displayed based on the user's configured country.
It resolved Issues as #546 and #496
Changes
Points for Improvement
Pair<String, String>
for language representation. A custom class could be more suitable for future scalability and clarity. I used it due to uncertainty about the appropriate placement and naming for a custom class.Suggestions
only_local
requests, aiming to translate everything upfront and avoid the need for user-triggered translations.Acknowledgements
A huge thank you to everyone involved in developing Showly 2.0.
It's my go-to app every day, even though the ads are becoming more frequent (just a little joke 😉).