Skip to content

fix(dashboard): Make sure fallback locale en also recognized by weather_status for F vs C usage #49986

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
Closed
joshtrichards wants to merge 2 commits into from
Closed

fix(dashboard): Make sure fallback locale en also recognized by weather_status for F vs C usage #49986

joshtrichards wants to merge 2 commits into from

Conversation

@joshtrichards
Copy link
Member

@joshtrichards joshtrichards commented Dec 28, 2024 •
edited
Loading

BROADER ISSUE DISCOVERED. On-hold until #49988 is reviewed

Summary

This should fix #23486. We use en as the fallback locale in numerous places. Since the underlying cause for this bug only occurs when the fallback is being relied upon seems to makes sense to include it here. Alternative would be to change the fallback locale to something like en_US across our code base, but that seems both overly drastic and unnecessary.

Refs: #23486 (comment)

TODO

  • ...

Checklist

@joshtrichards
fix(dashboard): Make sure fallback locale en also uses F
b7c3991 
Fixes #23486

Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards joshtrichards added bug 3. to review Waiting for reviews papercut Annoying recurring UX issue with possibly simple fix. feature: dashboard labels Dec 28, 2024
@joshtrichards joshtrichards added this to the Nextcloud 31 milestone Dec 28, 2024
@joshtrichards joshtrichards changed the title fix(dashboard): Make sure fallback locale en also uses F fix(dashboard): Make sure fallback locale en also recognized by weather_status to F vs C usage Dec 28, 2024
@joshtrichards joshtrichards changed the title fix(dashboard): Make sure fallback locale en also recognized by weather_status to F vs C usage fix(dashboard): Make sure fallback locale en also recognized by weather_status for F vs C usage Dec 28, 2024
@joshtrichards joshtrichards requested review from a team, Pytal, nfebe and skjnldsv and removed request for a team December 28, 2024 16:39
@joshtrichards
Copy link
Member Author

joshtrichards commented Dec 28, 2024

/compile /

@nextcloud-command
chore(assets): Recompile assets
21f1fe0 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@joshtrichards
Copy link
Member Author

joshtrichards commented Dec 29, 2024 •
edited
Loading

The more I think about this, this is not the correct approach and something else is going on here.

Accept-language: en-US,en;q=0.5

Results in our template returning:

<html class="ng-csp" data-placeholder-focus="false" lang="en" data-locale="en" translate="no" >

And we appear to pick the right locale on the Personal settings page, which is why the problem goes away when selected and saved.

Think I need to wait for my coffee to kick in...

@joshtrichards
Copy link
Member Author

joshtrichards commented Dec 29, 2024 •
edited
Loading

As I suspected the problem is broader.

We're not showing the actual (active) Locale in the Personal settings page (unless it's actually been changed and re-saved).

TLDR

The inconsistency is we call findLocal() two different ways, which produces different fallback return values and, creating further confusion, this leads to us showing the current (expected) Locale under Personal settings even though it really isn't the one it says.

Assuming we're comfortable with how locale handling is working in general, the fix just needs to happen in the Personal settings page.

Details

For the same user (when they have not yet saved a locale in their preferences):

  • findLocale() returns en as the locale in our templates (i.e. 99% of interactions)
  • findLocale() returns en_US as the locale in the Personal settings page (which isn't what's really being used)

Cause:

In our template we call it as findLocal(lang):

server/lib/private/L10N/Factory.php

Lines 115 to 116 in d8d708e

if ($locale === null || !$this->localeExists($locale)) {
$locale = $this->findLocale($lang);

...which means if a user doesn't have a locale saved yet we hit this fallback code:

server/lib/private/L10N/Factory.php

Lines 267 to 269 in d8d708e

// If no user locale set, use lang as locale
if ($lang !== null && $this->localeExists($lang)) {
return $lang;

This returns the language (en in my test case) as the fallback locale under in all circumstances. This is why data-locale is set to en.

But when the user loads up their Personal settings we populate the active locale without providing the language clue to findLocale():

server/apps/settings/lib/Settings/Personal/PersonalInfo.php

Line 268 in d8d708e

$userLocaleString = $this->config->getUserValue($uid, 'core', 'locale', $this->l10nFactory->findLocale());

...So we hit a different fallback:

server/lib/private/L10N/Factory.php

Lines 272 to 274 in d8d708e

// At last, return USA
return 'en_US';
}

And thus return a different initial locale for the same user.

This finally explains why the Personal settings page always shows me the right thing, but behavior elsewhere in Nextcloud doesn't match it.

Fixing

So I think we have two paths to consider here:

  1. When the user doesn't have a saved locale yet, always just use their base language (i.e. en in my test case). This is effectively what we're doing throughout Nextcloud at the moment other than on the Personal settings page. If that is considered reasonable (presumably it is), then I propose we fix the call to findLocale() on the Personal settings page to be consistent (otherwise it is effectively displaying incorrect information to the user until/unless they save their locale preferences) [Note: There's a chance this also explains some other locale related bugs; still looking through those)

  2. We remove the language value provided to findLocal() and make it consistent with the Personal settings page handling. In that case we'd be effectively changing everyone that doesn't have a saved locale yet to en_US <-- don't like this

@joshtrichards joshtrichards mentioned this pull request Dec 29, 2024
Merged
5 tasks
@joshtrichards joshtrichards added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 29, 2024
@joshtrichards joshtrichards mentioned this pull request Dec 29, 2024
Closed
@joshtrichards
Copy link
Member Author

joshtrichards commented Dec 30, 2024

Superseded by #49988

@susnux susnux deleted the fix-papercut-23486-weather-status-locale branch April 23, 2025 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-frontend

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner automatically assigned from nextcloud/server-frontend

@Pytal Pytal Awaiting requested review from Pytal Pytal is a code owner automatically assigned from nextcloud/server-frontend

Assignees

No one assigned

Labels

2. developing Work in progress bug feature: dashboard papercut Annoying recurring UX issue with possibly simple fix.

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

Unit for temperature in dashboard weather widget is always Celsius

3 participants

@joshtrichards @nextcloud-command