showly-2.0 icon indicating copy to clipboard operation
showly-2.0 copied to clipboard

Add translation by country

Open VSeryi opened this issue 1 year ago • 3 comments
trafficstars

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

  • Changed logic to filter translations by language and country:
    • If not found, it falls back to language-specific translations.
    • If not found, it falls back to the default language.

Points for Improvement

  • Currently using 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

  • It would be beneficial to explore methods to reduce 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 😉).

VSeryi avatar Apr 13 '24 22:04 VSeryi

Added a commit (Add poster translations) to fix #797

EDIT: Splitted in other Pull Request #823

VSeryi avatar May 04 '24 00:05 VSeryi

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:

  1. In SettingsRepository I would like to end up only with a single AppLocale instead of both language and country:
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()
}

AppLocale, AppCountry and AppLanguage could all be moved into ui-model/locale package

SettingsRepository would only have this instead of language and country


  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

michaldrabik avatar May 14 '24 08:05 michaldrabik

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:

  1. In SettingsRepository I would like to end up only with a single AppLocale instead of both language and country:
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()
}

AppLocale, AppCountry and AppLanguage could all be moved into ui-model/locale package

SettingsRepository would only have this instead of language and country

  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

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).

VSeryi avatar May 16 '24 00:05 VSeryi