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

Add user locale/region setting #5623

Merged
merged 19 commits into from
Jun 29, 2018
Merged

Add user locale/region setting #5623

merged 19 commits into from
Jun 29, 2018

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Jul 5, 2017

This PR allows having a dedicated user locale/region setting. See #1781

Needs :

  • Where to put the list of locales (obviously this locales.json is going to be moved)
  • More tests (please point out where)
  • UI fixes on personal settings page
  • Provide a system wide setting for locale
  • Add a check on OCS endpoint for valid locale format
  • Pass locale to MomentJS
  • Don't reload the page, just pass the locale to momentJS.

@tcitworld tcitworld added the 2. developing Work in progress label Jul 5, 2017
@tcitworld tcitworld added this to the Nextcloud 13 milestone Jul 5, 2017
@mention-bot
Copy link

@tcitworld, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @rullzer and @LukasReschke to be potential reviewers.

return $userLocaleString === $value['code'];
});

if (count($userLocale) > 0)
Copy link
Member

Choose a reason for hiding this comment

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

!empty()

@@ -314,3 +315,29 @@
</a>
</form>
<?php } ?>

<form id="locale" class="section">
Copy link
Member

Choose a reason for hiding this comment

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

Move above the closing bracket? You don't need the dropdown when you can't save it.

</option>
<?php endforeach;?>
</select>
</form>
Copy link
Member

Choose a reason for hiding this comment

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

New line

@@ -111,7 +112,7 @@ public function get($app, $lang = null) {

if (!isset($this->instances[$lang][$app])) {
$this->instances[$lang][$app] = new L10N(
$this, $app, $lang,
$this, $app, $lang, $locale,
Copy link
Member

Choose a reason for hiding this comment

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

locale is not filled with the users config?!

@nickvergessen
Copy link
Member

Migrate the core apps to use this

I disagree. This should happen automatically. It should automatically pick up the user's config and use that localle if none was given explicit, just like with the language. The cases where we set the language explicit are a different story of course, but there shouldn't be a lot.

@codecov
Copy link

codecov bot commented Jul 7, 2017

Codecov Report

Merging #5623 into master will increase coverage by 20.26%.
The diff coverage is 46.84%.

@@              Coverage Diff              @@
##             master    #5623       +/-   ##
=============================================
+ Coverage     31.71%   51.98%   +20.26%     
- Complexity    26010    26037       +27     
=============================================
  Files          1661     1661               
  Lines         96137    96230       +93     
  Branches       1290     1290               
=============================================
+ Hits          30494    50026    +19532     
+ Misses        65643    46204    -19439
Impacted Files Coverage Δ Complexity Δ
core/templates/layout.user.php 0% <ø> (ø) 0 <0> (ø) ⬇️
config/config.sample.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...ings/templates/settings/personal/personal.info.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/Settings/Personal/PersonalInfo.php 0% <0%> (ø) 21 <5> (+5) ⬆️
lib/private/TemplateLayout.php 0% <0%> (ø) 49 <0> (ø) ⬇️
apps/provisioning_api/lib/Controller/AUserData.php 60.75% <100%> (+0.5%) 15 <0> (ø) ⬇️
core/js/js.js 65.38% <50%> (-0.15%) 0 <0> (ø)
...rovisioning_api/lib/Controller/UsersController.php 71.49% <55.55%> (-0.35%) 139 <0> (+4)
lib/private/L10N/L10N.php 81.96% <71.42%> (-4%) 6 <3> (+1)
lib/private/L10N/Factory.php 65.53% <87.5%> (+4.11%) 109 <26> (+20) ⬆️
... and 390 more

@georgehrke
Copy link
Member

Migrate the core apps to use this

Ideally core apps would just use momentJS and we set moment.locale($locale) in core.js

@tcitworld
Copy link
Member Author

Ideally core apps would just use momentJS and we set moment.locale($locale) in core.js

Done.

@georgehrke
Copy link
Member

georgehrke commented Jul 17, 2017

  • There is too much space between Language and Locale:

nextcloud chromium today at 3 39 49 pm

  • Additionally I would like to show some examples for the selected locale.

@georgehrke
Copy link
Member

AFAIK you can simply update the locale with moment.locale(newLocale). There is no need to do a full reload.

@georgehrke
Copy link
Member

How did you acquire the data for lib/private/L10N/locales.json?

Is there an automated way to update it?

@MorrisJobke
Copy link
Member

@tcitworld Could you please resolve the conflict? Thanks 👍

@tcitworld
Copy link
Member Author

There is too much space between Language and Locale:

Yeah, don't really know why.

Additionally I would like to show some examples for the selected locale.

Meaning ?

How did you acquire the data for lib/private/L10N/locales.json?

Originally stolen from https://gist.github.com/jacobbubu/1836273, but I've found newer versions at https://gist.github.com/jasef/337431c43c3addb2cbd5eb215b376179 and https://gist.github.com/ndbroadbent/588fefab8e0f1b459fcec8181b41b39c

I guess it could be retrieved from elsewhere (for instance Debian has a languages code list at cat /usr/share/i18n/SUPPORTED), but I didn't found any perfect up-to-date list.

Could you please resolve the conflict? Thanks

Fixed.

@georgehrke
Copy link
Member

Meaning ?

I thought about something like this:
dedicated locale-region option issue 1781 nextcloud-server safari today at 2 07 48 pm

@doodhout
Copy link

Any progress on this?

Currently biding my time using en_GB as a language+locale setting, but having the option to choose both would be nice.

@georgehrke
Copy link
Member

@tcitworld What's the current state? :)
Do you need any help?

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 27, 2018
@georgehrke
Copy link
Member

It's done from a coding point of view imho. Just the layouting fixes :)

* * The code (en_US, fr_CA, ...) of the locale that is used for this IL10N object
*
* @return string locale
* @since 13.0.0
Copy link
Member

Choose a reason for hiding this comment

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

14 ;)

/**
* @param string|null $lang user language as default locale
* @return string locale If nothing works it returns 'en_US'
* @since 13.0.0
Copy link
Member

Choose a reason for hiding this comment

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

14 ;)

*
* @param string $string
* @return string Unique function name
* @since 9.0.0
Copy link
Member

Choose a reason for hiding this comment

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

And this especially ;)

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍 (beside the little nitpicks the code looks good as well)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@georgehrke
Copy link
Member

Fixed the docblock since mistakes

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke merged commit 61842f6 into master Jun 29, 2018
@MorrisJobke MorrisJobke deleted the locale-setting branch June 29, 2018 06:03
@melroy89
Copy link
Contributor

melroy89 commented Dec 19, 2018

Does this also introduces a way for an admin to set the DEFAULT locale/region? Let's say you create a new user.

EDIT: I see, this is not an GUI option set. I think the only way is to set defaults within the config file. Correct?

@melroy89
Copy link
Contributor

melroy89 commented Dec 19, 2018

Ps. Also update the docs? https://docs.nextcloud.com/server/stable/admin_manual/configuration_server/language_configuration.html?highlight=language#

with the new default_locale and force_locale?...

@tcitworld
Copy link
Member Author

@Danger89

I see, this is not an GUI option set. I think the only way is to set defaults within the config file. Correct?

Yup, just like for the language.

@skjnldsv
Copy link
Member

@tcitworld can you provide a pr for the docs ? :)

@skjnldsv skjnldsv added the pending documentation This pull request needs an associated documentation update label Dec 19, 2018
tcitworld added a commit to nextcloud/documentation that referenced this pull request Dec 19, 2018
@tcitworld
Copy link
Member Author

@skjnldsv nextcloud/documentation#1051

@skjnldsv skjnldsv removed the pending documentation This pull request needs an associated documentation update label Dec 19, 2018
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants