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

use formal version of German by default #7351

Merged
merged 3 commits into from
Dec 6, 2017

Conversation

schiessle
Copy link
Member

Use formal version of German by default, if not explicitly defined differently.

Germans are sometimes a bit old school and prefer the formal "Sie" instead of "Du". 😉 So I think it makes sense to use this version as default if the browser is set to German.

@schiessle schiessle added the 3. to review Waiting for reviews label Nov 30, 2017
@schiessle schiessle added this to the Nextcloud 13 milestone Nov 30, 2017
@schiessle schiessle force-pushed the change-default-german-language branch from 0be9cb5 to 7d4df77 Compare November 30, 2017 11:54
@codecov
Copy link

codecov bot commented Nov 30, 2017

Codecov Report

Merging #7351 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #7351      +/-   ##
============================================
+ Coverage     50.91%   50.92%   +<.01%     
- Complexity    24698    24704       +6     
============================================
  Files          1586     1586              
  Lines         94113    94125      +12     
  Branches       1361     1361              
============================================
+ Hits          47920    47929       +9     
- Misses        46193    46196       +3
Impacted Files Coverage Δ Complexity Δ
config/config.sample.php 0% <ø> (ø) 0 <0> (ø) ⬇️
lib/private/L10N/Factory.php 72.22% <100%> (+1.63%) 72 <5> (+5) ⬆️
apps/files_trashbin/lib/Trashbin.php 72.28% <0%> (-0.25%) 136% <0%> (ø)
lib/private/legacy/util.php 58.65% <0%> (-0.03%) 225% <0%> (+1%)

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Please no. This will lead to a world of pain if we are going to do such weird fallbacks for every other language.

If people want the formal German they should set their browser/os properly. Or else set it in the settings.

@schiessle
Copy link
Member Author

schiessle commented Nov 30, 2017

The problem is that the browser sends per default "de" and not "de_DE". At least Firefox, but from user feedback this is also true for other browsers. So it is not the users fault who can't know our internal hack and who set the browser correctly to German but didn't get the expected result. As the whole "de" vs "de_DE" hack is our "fault" I think it is also our responsibility to pick the right one if the browser is set to German. And the default should definitely be the formal one. Nobody is offended by calling them "Sie" but many are offended by calling them "Du"

@schiessle
Copy link
Member Author

Added @MorrisJobke and @blizzz as additional reviewers because I think that's a quite "German problem" 😉

@rullzer
Copy link
Member

rullzer commented Nov 30, 2017

You can also set German/Germany (de_DE) in firefox. I see no reason to prefer German/Germany over German/Liechtenstein here.

@blizzz
Copy link
Member

blizzz commented Nov 30, 2017

And the default should definitely be the formal one. Nobody is offended by calling them "Sie" but many are offended by calling them "Du"

Well… I would not sign this for Berlin :)

@rullzer rullzer dismissed their stale review November 30, 2017 13:18

Silly language you have. Do as you please.

@@ -159,6 +159,8 @@ public function findLanguage($app = null) {
try {
// Try to get the language from the Request
$lang = $this->getLanguageFromRequest($app);
// use formal version of german ("Sie" instead of "Du") by default
$lang = strtolower($lang) === 'de' ? 'de_DE' : $lang;
Copy link
Member

Choose a reason for hiding this comment

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

Then don't do this here, but in the actual method that sets it during the first login.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say this should go into getLanguageFromRequest, so it still works for guests.
But I'd really like to see this optional.

@MorrisJobke
Copy link
Member

Germans are sometimes a bit old school and prefer the formal "Sie" instead of "Du". 😉 So I think it makes sense to use this version as default if the browser is set to German.

If we do this at all, we should only change this during the first login, otherwise this method has some erratic behaviour. We should set it to de_DE for all users that come along with de and they can switch back afterwards in the settings, because this is the order as of now:

  • user settings
  • browser default
  • instance default

@nickvergessen
Copy link
Member

nickvergessen commented Nov 30, 2017

I don't really think we should autocorrect user preferences.
If you tell your browser use "de" we shouldn't say "you probably mean de_DE, although you could have selected that instead"?

Currently the priority is: force_lang > user preference > request lang > default_lang > en

I really really really do not want to have all people seeing the formal german on all the instances by default. This is really a minority, so either patch or config for them, but no way distance-by-default.

@nickvergessen
Copy link
Member

I guess the main issue here is "Guests" in which case the "solution" of Morris also wouldn't work, because it only works on login?

@schiessle
Copy link
Member Author

schiessle commented Nov 30, 2017

If you tell your browser use "de" we shouldn't say "you probably mean de_DE, although you could have selected that instead"?

This implies that the user knows that "de" is the non-formal version and de_DE is the formal version which I don't think is common sense outside of the Nextcloud universe.

But I like your idea to also look at the default_language and only do it if this is set to de_DE... I will add it. Thanks for this really good suggestion

@schiessle schiessle force-pushed the change-default-german-language branch 2 times, most recently from 1207187 to a37f701 Compare November 30, 2017 14:38
@schiessle
Copy link
Member Author

@nickvergessen @MorrisJobke @rullzer updated, I think this is now a really good solution.

@schiessle schiessle force-pushed the change-default-german-language branch from a37f701 to a479535 Compare November 30, 2017 14:44
try {
// Try to get the language from the Request
$lang = $this->getLanguageFromRequest($app);
// use formal version of german ("Sie" instead of "Du") if the default
// language is set to 'de_DE'
if (strtolower($lang) === 'de' && strtolower($defaultLanguage) === 'de_de') {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should try to move this into getLanguageFromRequest
or at least check languageExists because if de exists, but de_DE not (lets say for an app), it would fail

Copy link
Member Author

Choose a reason for hiding this comment

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

@nickvergessen moved it to getLanguageFromRequest

@jancborchardt
Copy link
Member

So, with the updated behavior the default for Germans is still the friendlier informal way, unless specifically set to de_DE in system-wide settings or Nextcloud itself?

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@schiessle schiessle force-pushed the change-default-german-language branch from a479535 to 8b73434 Compare November 30, 2017 16:29
@schiessle
Copy link
Member Author

schiessle commented Nov 30, 2017

So, with the updated behavior the default for Germans is still the friendlier informal way, unless specifically set to de_DE in system-wide settings or Nextcloud itself?

exactly, only if the admin sets 'default_language' to 'de_DE' in the config.php, it will be preferred.

@MorrisJobke
Copy link
Member

exactly, only if the admin sets 'default_language' to 'de_DE' in the config.php, it will be preferred.

Could you document this in the sample config? Then I'm fine with this here 👍

// use formal version of german ("Sie" instead of "Du") if the default
// language is set to 'de_DE' if possible
if (strtolower($lang) === 'de'
&& strtolower($defaultLanguage) === 'de_de' &&
Copy link
Member

Choose a reason for hiding this comment

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

And please check this variable first - https://3v4l.org/msAEZ (totally fine for now, but let's fix it upfront: https://3v4l.org/dmBJK)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@schiessle
Copy link
Member Author

@MorrisJobke documentation added 😃

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

This version I can deal with 😉

@rullzer rullzer merged commit 6e45034 into master Dec 6, 2017
@rullzer rullzer deleted the change-default-german-language branch December 6, 2017 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants