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

Make language configurable #8493

Merged
merged 9 commits into from
Mar 31, 2021
Merged

Make language configurable #8493

merged 9 commits into from
Mar 31, 2021

Conversation

fmoc
Copy link
Contributor

@fmoc fmoc commented Mar 16, 2021

Closes #8466.

This PR adds two new methods to select the language within the GUI application: a config option in the general settings page, and a --language parameter.

screenshot_2021-03-16_17-29-30

The current implementation of listing all available locales relies on the translation file's filenames. It extracts the locale part and tries to resolve the native language name using QLocale::nativeLanguageName. This seems to work relatively well, except for one file called TW. I suspect we have to rename that file to tw.

@CLAassistant
Copy link

CLAassistant commented Mar 16, 2021

CLA assistant check
All committers have signed the CLA.

@michaelstingl
Copy link
Contributor

  1. Better move to "Advanced" section?
  2. I think we'd need a branding option to hide this. (default=on)
  3. Regarding displaying the language name, in oC10, the name of the language is part of the language file itself:
    "__language_name__" : "Deutsch (Förmlich: Sie)",
    (Source: https://github.com/owncloud/core/blob/621a09501d9afd81afcc5f62765b9858ed5a7cce/lib/l10n/de_DE.json#L45)
  4. In the desktop sync app, we only have client_de.ts. With the selection in the UI, we could also make the other German translations available?

@TheOneRing
Copy link
Member

1. Better move to "Advanced" section?

I only would expect technically advanced settings there not ui settings.

2. I think we'd need a branding option to hide this. (default=on)

Do you see issues caused by the feature?
I'd rather see it as an accessibility feature.

3. Regarding displaying the language name, in oC10, the name of the language is part of the language file itself:
   `    "__language_name__" : "Deutsch (Förmlich: Sie)",`
   (Source: https://github.com/owncloud/core/blob/621a09501d9afd81afcc5f62765b9858ed5a7cce/lib/l10n/de_DE.json#L45)

4. In the desktop sync app, we only have [`client_de.ts`](https://github.com/owncloud/client/blob/master/translations/client_de.ts). With the selection in the UI, we could also make the other German translations available?

We currently have Austrian and German in here I guess, we could add explicit translations for some of them.
Currently the code is translating the short language code de_DE to its official name.

Copy link
Member

@TheOneRing TheOneRing left a comment

Choose a reason for hiding this comment

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

1. Better move to "Advanced" section?

I only would expect technically advanced settings there not ui settings.

2. I think we'd need a branding option to hide this. (default=on)

Do you see issues caused by the feature?
I'd rather see it as an accessibility feature.

I see we have, enforcedLocale but I'd expect that was rather a workaround for a lack of a language selection?

3. Regarding displaying the language name, in oC10, the name of the language is part of the language file itself:
   `    "__language_name__" : "Deutsch (Förmlich: Sie)",`
   (Source: https://github.com/owncloud/core/blob/621a09501d9afd81afcc5f62765b9858ed5a7cce/lib/l10n/de_DE.json#L45)

4. In the desktop sync app, we only have [`client_de.ts`](https://github.com/owncloud/client/blob/master/translations/client_de.ts). With the selection in the UI, we could also make the other German translations available?

We currently have Austrian and German in here I guess, we could add explicit translations for some of them.
Currently the code is translating the short language code de_DE to its official name.

@@ -86,6 +86,8 @@ set(client_SRCS
servernotificationhandler.cpp
guiutility.cpp
elidedlabel.cpp
translationshelper.h
Copy link
Member

Choose a reason for hiding this comment

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

don't add the header

Copy link
Member

@TheOneRing TheOneRing left a comment

Choose a reason for hiding this comment

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

Looks pretty good :)

src/gui/application.cpp Outdated Show resolved Hide resolved
src/gui/generalsettings.cpp Outdated Show resolved Hide resolved
src/gui/generalsettings.cpp Outdated Show resolved Hide resolved
src/gui/generalsettings.cpp Outdated Show resolved Hide resolved
src/gui/generalsettings.cpp Show resolved Hide resolved
src/gui/translationshelper.cpp Outdated Show resolved Hide resolved
src/gui/translationshelper.h Outdated Show resolved Hide resolved
@fmoc
Copy link
Contributor Author

fmoc commented Mar 24, 2021

Better move to "Advanced" section?

UX wise, I think it makes sense to make this setting as accessible as possible. The reason you want to change language through the UI is, usually, because an application is using the wrong language by default. Imagine someone has to find this dropdown while the application is in a foreign language they don't understand.

Can be used by users to overwrite the language, e.g., for testing, or if the auto-detection doesn't work.
Fabian Müller added 7 commits March 30, 2021 18:32
Stores language user can select in the GUI's settings.
Allows users to specify their preferred language, similar to --language. If not set, the existing auto-detection will be used.
This is a lot more user friendly than having to work with locale names, which often can't easily be associated with the language they represent.

There is also a fallback for cases when Qt cannot provide the native name for a locale provided by the translations helper. This usually indicates an issue in the filename of the translations. Right now, this is the case with "client_TW.qm".
Last bit missing to make the language picker functional.
Copy link
Member

@TheOneRing TheOneRing left a comment

Choose a reason for hiding this comment

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

Changelog entry is missing

@fmoc fmoc merged commit 40877ee into master Mar 31, 2021
@delete-merged-branch delete-merged-branch bot deleted the issue-8466 branch March 31, 2021 09:43
@sonarcloud
Copy link

sonarcloud bot commented Mar 31, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

Add language picker to the cli and the ui
4 participants