search

FrancescoCeruti / linux-show-player

Linux Show Player - Cue player designed for stage productions
https://linux-show-player.org
GNU General Public License v3.0
205 stars 49 forks source link

[Merged] Choose the language in Preferences #165

Closed Gajenthran closed 5 years ago

Gajenthran commented 5 years ago

Concerning the issue #158, I put the option to change the language. I make sure that the --locale option on the command line still work. I put it in "General" below the "Icons theme" and "UI theme". However, I think there is a little problem (UI problem): for the language in the combobox, I put only the 2 first letters of the language. For example, for "French", I put "fr". I don't know if it's really annoying for 9 languages but it might be annoying if we decide to add more languages.

PS: I only touch general.py, main.py, default.json and lisp.json. Extremely sorry, I create another pull request for the same issue. That's my first time so I forgot to use the command to pull.

FrancescoCeruti commented 5 years ago

Hi :)

As a general rule, when you develop a new feature to merge in one of the upstream breaches (develop in this case) you should start from a fresh copy of that branch (or a branch which is in sync with the upstream). Especially you shouldn't start from a branch where you have already other pull requests, this keep things cleaner ;)

As for the code, we can improve a few things:

Gajenthran commented 5 years ago

As a general rule, when you develop a new feature to merge in one of the upstream breaches (develop in this case) you should start from a fresh copy of that branch (or a branch which is in sync with the upstream). Especially you shouldn't start from a branch where you have already other pull requests, this keep things cleaner ;)

Thanks for the advice ! I follow what you said but the only thing that I didn't know is that we have to sync our works with the upstream (I thought that a simple git pullwas enough).

As for the code, we can improve a few things:

  • the default locale should not be en but "" or null, which should be interpreted as "system default", also provide this as an option in the settings combobox.

Done !

  • "locale" is more suited than "language" as settings key, is consistent with the current terminology

Done ! And I completely agree with you.

  • in the UI we should display the language full name, I believe that the QLocale class contains something that can be used, not sure, another option is to build a dictionary to map "locale code" -> "full name"

I create a dictionary as you said, but I put it in general.py. I hesitated to create the dictionary into a file but I prefer to let you handle that (I don't think that general.py is the right place to put the dictionary).

Hope it will help you !

fnetX commented 5 years ago

I don't know if you thought about using translated language names or if it's a good idea to do so.

(Like to use the local language names instead of international ones, German gets translated into Deutsch, French into Français etc)

Gajenthran commented 5 years ago

Yes, I do that : it's easier. I think it's not necessary to translate into each languages.

FrancescoCeruti commented 5 years ago

@Gajenthran I've look into the QLocale docs, we could use something like this:

ql = QLocale(locale)
combobox_text = "{} ({})".format(
    QLocale.languageToString(ql.language()),
    ql.nativeLanguageName()
)

For QLocale("it") this will output "Italian (italiano)". This way we can can cover both cases :cake:

Anyway I'll suggest to have a "custom" widget to keep things cleaner, something like FadeComboBox in lisp/ui/widgets/fades.py

Gajenthran commented 5 years ago

Thanks for your suggestions @FrancescoCeruti ! As you said, I put QLocale method to have the name in both language.

And I create also a "custom" widget (it's clearly better because it avoids mixing Locale functionnalities in general.py).

FrancescoCeruti commented 5 years ago

I've merged it manually, with a few changes, see the commit reference above.

Thanks :smile: