Skip to content

Fix jsConfigHelper lang/locale mixup #17673

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.

Sign up for GitHub

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

Jump to bottom
Merged
rullzer merged 2 commits into from
Oct 26, 2019
Merged

Fix jsConfigHelper lang/locale mixup #17673

rullzer merged 2 commits into from
Oct 26, 2019

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Oct 25, 2019

To test:

  1. Set your language to US english
  2. Set your locale to German
  3. Reload

Open your js console and get the value of

  • datepickerFormatDate
  • dayNames

Before:

  • datepickerFormatDate in German format `dd.MM.yy"
  • dayNames also in German

Now:

  • datepickerFormatDate in German format `dd.MM.yy"
  • dayNames also in English

@rullzer
Don't pass in the locale as the language
6a560fd 
This messes with the translation of the date names etc.

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Cleanup theming mess
e71f222 
* Do not do translations in the constructor. This gets called to early
so there is no user yet. Which means we can't obtain the locale. Which
means we store the wrong translation instance.

* Same for the theming app magic. Just use the parent call when needed.

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer rullzer added bug 3. to review Waiting for reviews labels Oct 25, 2019
@rullzer rullzer added this to the Nextcloud 18 milestone Oct 25, 2019
georgehrke
georgehrke approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

Works 👍

Thx a lot!

@kesselb kesselb mentioned this pull request Oct 25, 2019
Closed
@rullzer rullzer requested a review from kesselb October 25, 2019 11:55
nickvergessen
nickvergessen reviewed Oct 25, 2019
View reviewed changes
lib/private/legacy/defaults.php
return $this->theme->getSlogan();
} else {
if ($this->defaultSlogan === null) {
$l10n = \OC::$server->getL10N('lib');
Copy link
Member

@nickvergessen nickvergessen Oct 25, 2019

Choose a reason for hiding this comment

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

You can inject the IFactory?

Copy link
Member Author

@rullzer rullzer Oct 25, 2019

Choose a reason for hiding this comment

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

I'm afraid to inject anything here in this legacy code. I vote to keep this as is for now so it backports clean and then spend time in cleaning up thise theming madness ;)

@kesselb
Copy link
Contributor

kesselb commented Oct 25, 2019 •
edited
Loading

#11134 & #11132

Okay. The factory will figure out the right language somehow. Do we still need findLanguageFromLocale then and does everything works as before? I think there must be a reason for this previous change.

@rullzer
Copy link
Member Author

rullzer commented Oct 25, 2019

#11134 & #11132

Okay. The factory will figure out the right language somehow. Do we still need findLanguageFromLocale then and does everything works as before? I think there must be a reason for this previous change.

The reason is that the server funciton doesn't allow you to set a locale right now (but differnt issue)
Also the caching doesn't take the locale into account.

So this was 2 bugs.

  1. The JSConfigHelper got injected with a L10N instance that was set to the locale as language. Which translated all the days etc.
  2. Because of using translations in the contructor this was all initialized to soon (before there is a user set) so we could not fetch the proper locale. Which in turn then used the cached one without the locale etc. Lots of fun.

@skjnldsv
Copy link
Member

skjnldsv commented Oct 25, 2019

Make sure this still works #11132 :)

@georgehrke
Copy link
Member

georgehrke commented Oct 25, 2019

Make sure this still works #11132 :)

Should it?
#11134 is introducing exactly the unexpected behaviour for me.

If my language is german and my locale is US, i want my dates to be MM/DD/YYYY, my time to be 12hours with am/pm and the first day to be Sunday.
But since my language is german, I want the weekdays to be Montag, Dienstag, Mittwoch ...

@kesselb
Copy link
Contributor

kesselb commented Oct 26, 2019

image

Index: apps/settings/js/settings/personalInfo.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- apps/settings/js/settings/personalInfo.js	(revision e71f2220821974aaf2f2b3357c0505e13a541b5a)
+++ apps/settings/js/settings/personalInfo.js	(date 1572083533569)
@@ -372,7 +372,7 @@
 	$('#localeexample-time').text(moment().format('LTS'));
 	$('#localeexample-date').text(moment().format('L'));
 	$('#localeexample-fdow').text(t('settings', 'Week starts on {fdow}',
-		{fdow: moment().weekday(0).format('dddd')}));
+		{fdow: dayNames[firstDay]}));
 
 }, 1000);

@rullzer rullzer merged commit c280eff into master Oct 26, 2019
@rullzer rullzer deleted the fix/lang/locale/jshelper/mess branch October 26, 2019 10:14
@rullzer
Copy link
Member Author

rullzer commented Oct 26, 2019

/backport to stable17

@rullzer
Copy link
Member Author

rullzer commented Oct 26, 2019

/backport to stable16

@rullzer
Copy link
Member Author

rullzer commented Oct 26, 2019

/backport to stable15

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 26, 2019
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Oct 26, 2019

backport to stable17 in #17686

@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Oct 26, 2019

The backport to stable15 failed. Please do this backport manually.

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 26, 2019
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Oct 26, 2019

backport to stable16 in #17687

@kesselb kesselb mentioned this pull request Oct 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@georgehrke georgehrke georgehrke approved these changes

@kesselb kesselb kesselb approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug

Projects

None yet

Milestone

Nextcloud 18.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@rullzer @kesselb @skjnldsv @georgehrke @nickvergessen