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

[Merged] Choose the language in Preferences #165

Closed
wants to merge 10 commits into from

Conversation

Gajenthran
Copy link
Contributor

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.

@Gajenthran Gajenthran changed the title Language in pref Choose the language in Preferences Mar 20, 2019
@FrancescoCeruti
Copy link
Owner

FrancescoCeruti commented Mar 21, 2019

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:

  • 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.
  • "locale" is more suited than "language" as settings key, is consistent with the current terminology
  • 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"
  • remember to increase the configuration version or the new settings might get ignored

@FrancescoCeruti FrancescoCeruti self-assigned this Mar 21, 2019
@FrancescoCeruti FrancescoCeruti added the UI Related to the User Interface label Mar 21, 2019
@Gajenthran
Copy link
Contributor Author

Gajenthran commented Mar 21, 2019

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
Copy link
Contributor

fnetX commented Mar 23, 2019

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
Copy link
Contributor Author

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

@FrancescoCeruti
Copy link
Owner

@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 🍰

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

@Gajenthran
Copy link
Contributor Author

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 added a commit that referenced this pull request Apr 10, 2019
Merged #165
Update: refactored ts/qm file generation
Update: refactored locale-select widget
Update: improved translations searching
Various changes
@FrancescoCeruti
Copy link
Owner

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

Thanks 😄

@FrancescoCeruti FrancescoCeruti changed the title Choose the language in Preferences [Merged] Choose the language in Preferences Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Related to the User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants