Expose intl.regional_prefs.use_os_locales in the UI

RESOLVED FIXED in Thunderbird 56.0

Status

Thunderbird
General
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

Trunk
Thunderbird 56.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

6 months ago
By popular demand, M-C have implemented bug 1379420, bug 1380916 and bug 1379905 so that chrome dates/times done via JS mozIntl and C++ (DateTimeFormat.cpp) will again display dates as chosen in the OS.

Whether the app locale of the OS locale will be followed depends on the value of a preference, intl.regional_prefs.use_os_locales.

We should expose this preference in the UI, something like:
(*) Use application language (English United States)
( ) Use custom regional settings defined in the OS (German Germany).

Let's do it quickly, so this can land on TB 56. Even a version without the languages in parenthesis to be refined later would be acceptable.

Richard, do you have some cycles for this?

Ben, this one is for you!!
(Assignee)

Comment 1

6 months ago
Zibi, can you please lend us a hand here: What is the official way to retrieve the app language and the OS language? I'm sure you have magic in the box for that.
Flags: needinfo?(gandalf)
(Assignee)

Comment 2

6 months ago
BTW, FF will eventually do this in bug 1379910.
(Assignee)

Comment 3

6 months ago
Created attachment 8889783 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - WIP (v1).

Now we can discuss what the strings should look like:

Languages
(*) Use application language (...)
( ) Use language from regional settings in the operating system (...)

Or maybe: "Regional Preferences" or "Date and Time formatting"?

So maybe:

Date and Time formatting
(*) Application locale (...)
( ) Locale from regional settings (...).

Suggestions please!

In bug 1379910 Zibi suggested a simple checkbox:
[ ] Use System Locale (ab-CD) for Regional Preferences.
I don't think that's crash hot since at least on Windows, there's a system locale and a regional formatting locale.

P.S.: I'm not assigning the bug to me (yet) in case someone has a great idea and wants to take over.
I think "Date and Time formatting" would be the best as the user sees a wrong time formatting and searches the prefs for this problem. With "languages" the user could associates it more with the UI localization.
See Also: → bug 1384172
(In reply to Jorg K (GMT+2) from comment #1)
> Zibi, can you please lend us a hand here: What is the official way to
> retrieve the app language and the OS language? I'm sure you have magic in
> the box for that.

The AppLocale - LocaleService.GetAppLocalesAsBCP47, OSLocale - OSPreferences::GetSystemLocale, OS Regional Preferences Locale - OSPreferences::GetRegionalPrefsLocales.

If you want to get the name out of the locale ID, you'll need to use ICU API for that.
Flags: needinfo?(gandalf)
(Assignee)

Comment 6

6 months ago
Thanks you, I see it now.

dom/tests/mochitest/chrome/test_intlUtils_getLocaleInfo.html:
const localeService =
  Cc["@mozilla.org/intl/localeservice;1"].getService(Ci.mozILocaleService);
let appLocale = localeService.getAppLocalesAsBCP47()[0];

intl/locale/tests/unit/test_osPreferences.js:
const osprefs =
  Components.classes["@mozilla.org/intl/ospreferences;1"]
  .getService(Components.interfaces.mozIOSPreferences);
const systemLocale = osprefs.systemLocale;
const systemLocales = osprefs.getSystemLocales();
const rgLocales = osprefs.getRegionalPrefsLocales();

As for turning the locale ID into a name, InlineSpellChecker.jsm has some code for that in getDictionaryDisplayName(dictionaryName). Would it be bad to use that?
Created attachment 8889954 [details]
prefs.png

This is how it would look with my wording.

This makes the prefs window very tall (591px). But it's hard to move it to a other tab that fits to the format theme.
Why not under Display?

Formatting seems to have plenty of space left and Display->Advanced can be used too.
Yeah, makes sense. I'm likely going to try to get rid of it eventually and move to ICU's getDisplayName [0]

So, if you prefer to do less work now, go for it :)

If you have capacity - maybe you'd like to take on the task to add it to mozIntl.getDisplayInfo?
The API would be like:

mozIntl.getDisplayNames("en-US", [
  "locales/en-US",
  "locales/de",
], "long");

should return:

{
  locale: 'en-US',
  style: 'long',
  values: {
    'locales/en-US': 'English (American)',
    'locales/de': 'German'
  }
}

[0] http://icu-project.org/apiref/icu4c/classicu_1_1Locale.html#a9255df735dfbc5af6b2883edc4ca4f15
(In reply to Frank-Rainer Grahl (:frg) from comment #8)
> Why not under Display?
> 
> Formatting seems to have plenty of space left and Display->Advanced can be
> used too.

I thought first about it too. But I think it's more for the messages display and not the global display.
(Assignee)

Comment 11

6 months ago
Created attachment 8889966 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - WIP (v2) - not working

Aceman, can dynamic labels be done in XUL? This isn't working.

BTW, I'm trying to make it:

Date and Time formatting
(*) Application locale (...)
( ) Locale from regional settings (...)
(Assignee)

Updated

6 months ago
Attachment #8889966 - Attachment description: 1384007-intl.regional_prefs.use_os_locales.patch - WIP (v1) - not working → 1384007-intl.regional_prefs.use_os_locales.patch - WIP (v2) - not working
(Assignee)

Comment 12

6 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> If you have capacity - maybe you'd like to take on the task to add it to
> mozIntl.getDisplayInfo?
Sorry, my task is to keep the show on the road for Thunderbird, I haven't done anything significant in M-C for a while.
> I thought first about it too. But I think it's more for the messages display and not the global display.

Semantics. You can always say it wasn't so :) But that is where I would probably look in the first place even before Generalas an unsuspecting user. Would be different if it would say 'Message Display' or so.
(In reply to Jorg K (GMT+2) from comment #12)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> > If you have capacity - maybe you'd like to take on the task to add it to
> > mozIntl.getDisplayInfo?
> Sorry, my task is to keep the show on the road for Thunderbird, I haven't
> done anything significant in M-C for a while.

Fair :)

Then feel free to use the .properties. Once we have it in mozIntl, it shouldn't be hard to switch I hope.
(Assignee)

Comment 15

6 months ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #14)
> Then feel free to use the .properties.
.properties? I was talking about using getDictionaryDisplayName() from InlineSpellChecker.jsm. Have I misunderstood your comment #9 "Go for it"?

mozIntl.getDisplayNames() already exists, what is mozIntl.getDisplayInfo?
> .properties? I was talking about using getDictionaryDisplayName() from InlineSpellChecker.jsm. Have I misunderstood your comment #9 "Go for it"?

The properties file is used by InlineSpellChecker.jsm.

I think I would prefer to generalize the function out of InlineSpellChecker, because it sounds architecturally wrong to put an external dependency on it as is (because the function is intended for InlineSpellChecker only and not expected to be used outside of it).

Maybe you could move it to mozIntl.js as `mozIntl.getLocaleNames(['de', 'fr', 'en-US']);` etc - and then call it from InlineSpellChecker and your code?

I'm not going to block you if you'll go the easier path, but it may create a hassle if ever someone will decide to move/remove the inlinespellchecker.

> mozIntl.getDisplayNames() already exists, what is mozIntl.getDisplayInfo?

I meant getDisplayNames. Sorry! :)
(Assignee)

Comment 17

6 months ago
Oops, my NI got lost. Aceman, question in comment #11.
Flags: needinfo?(acelists)
(Assignee)

Comment 18

6 months ago
Created attachment 8890238 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - WIP (v3).

This works but shows the locale IDs so far. To be continued.
Attachment #8889783 - Attachment is obsolete: true
Attachment #8889966 - Attachment is obsolete: true
Flags: needinfo?(acelists)
(Assignee)

Comment 19

6 months ago
Created attachment 8890305 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - WIP (v4).

OK, this is working. I'll need to localise the strings still.

No access keys since they don't seem to work with the dynamic labels.
Also I made it

Date and Time formatting
(*) Application locale: ...
( ) Locale from regional settings: ...

to avoid double parenthesis:
(*) Application locale (English (United States))

Maybe is should be "Date and Time Formatting" or "Date and time formatting".

Suggestions welcome.
Attachment #8890238 - Attachment is obsolete: true
Attachment #8890305 - Flags: feedback?(richard.marti)
Attachment #8890305 - Flags: feedback?(acelists)
(Assignee)

Comment 20

6 months ago
Created attachment 8890309 [details]
Screenshot
Assignee: nobody → jorgk
Attachment #8889954 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(In reply to Jorg K (GMT+2) from comment #19)
> Created attachment 8890305 [details] [diff] [review]
> 1384007-intl.regional_prefs.use_os_locales.patch - WIP (v4).
> 
> No access keys since they don't seem to work with the dynamic labels.

Have you tried using the first part of the label instead of "set dynamically"?
(Assignee)

Comment 22

6 months ago
Created attachment 8890336 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - (v5).

OK, here is the full solution with localisable strings. I made the layout horizontal, so it would take less space.
Attachment #8890305 - Attachment is obsolete: true
Attachment #8890305 - Flags: feedback?(richard.marti)
Attachment #8890305 - Flags: feedback?(acelists)
Attachment #8890336 - Flags: ui-review?(richard.marti)
Attachment #8890336 - Flags: review?(acelists)
(Assignee)

Comment 23

6 months ago
(In reply to Richard Marti (:Paenglab) from comment #21)
> Have you tried using the first part of the label instead of "set
> dynamically"?
I don't understand the question. What's the first part?
With label="set dynamically" and accelerator key "a" I get no accelerator key since this is applied to "set dyn_a_mically" and that gets replaced later. If I use "r" for the regional settings, I get "(R)" after the string since the "set dynamically" label doesn't have an "r". I see no way to get the letter of the dynamic label underlined.

I tried:
        <hbox>
          <radio id="appLocale"
                 value="false"
                 accesskey="a"/>
          <spacer flex="1"/>
        </hbox>
        <hbox>
          <radio id="rsLocale"
                 value="true"
                 accesskey="r"/>
          <spacer flex="1"/>
        </hbox>
and got no access keys whatsoever. I could set them in code :-(

Besides, very few users will ever touch this setting, and those who do will do it once in their lives.

Feel free to play with it.
(Assignee)

Comment 24

6 months ago
The best I can do is do label=" ", then I get:
(*) Application locale: English (United States)(A)   (*) Regional settings locale: German (Germany)(R)
which looks funny.
The (A) and (R) are placed after the label which doesn't contain those letters, and then the labels get replaced.
My proposal didn't work. Let's wait for aceman.

(In reply to Jorg K (GMT+2) from comment #22)
> Created attachment 8890336 [details] [diff] [review]
> 1384007-intl.regional_prefs.use_os_locales.patch - (v5).
> 
> OK, here is the full solution with localisable strings. I made the layout
> horizontal, so it would take less space.

Leave it vertical. With horizontal we could have issues with some locales with long labels. The pref doesn't shrink, so it's a other pane using this height.

Please, can you use "Date and Time Formatting"?
(Assignee)

Comment 26

6 months ago
Comment on attachment 8890336 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - (v5).

I have a better idea, have a label with %S and only replace that dynamically. Another version later.
Attachment #8890336 - Flags: ui-review?(richard.marti)
Attachment #8890336 - Flags: review?(acelists)

Comment 27

6 months ago
(In reply to Jorg K (GMT+2) from comment #11)
> Aceman, can dynamic labels be done in XUL? This isn't working.

Yes you can do dynamic labels (set by JS code), but then the strings need to be in a .properties file that is included as a stringbundle in the xul file.
(Assignee)

Comment 28

6 months ago
Created attachment 8890459 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - WIP (v6).

OK, here I've changed the title and made it vertical again.

I'm doing a dynamic label by modifying the existing one. I think that's easier than making it a property like in (v5) since the localiser can see it all in context.

Whatever I do, I can't get accesses keys to work.

So Aceman, which approach do you prefer, v5 or v6 and do you have a good idea about the assess keys? If you prefer v6, we either need to get the access keys to work or pull the code out.
Attachment #8890459 - Flags: feedback?(acelists)
(Assignee)

Comment 29

6 months ago
There is a larger problem here. For an estimated 95% of our users, the two options will be the same. We have the largest user groups in the US, Germany, France and Japan. All these will use a localised version on an OS to match.

We developers all get very exited about this since we use an en-US version and would like it to do dates differently, but for most of our users this is really a non-issue.

So is it a good idea to place this onto the "General" pane and waste prime space?

Maybe it should be banned to "Advanced / General"?
Or only show when they are different.
(Assignee)

Comment 31

6 months ago
That's just a support nightmare: A user interface that depends on some external parameters. So go and say:
Tools > Options, (Advanced, )General, Formatting, tell me what you see, and the persons answers: Nothing, not there :-(

Comment 32

6 months ago
Comment on attachment 8890336 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch - (v5).

Review of attachment 8890336 [details] [diff] [review]:
-----------------------------------------------------------------

I like this version using properties better as that is the standard method.

Maybe move the option into Display ->Advanced or Advanced->General->System integration .

::: mail/components/preferences/general.js
@@ +1,5 @@
>  /* -*- Mode: JavaScript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +Components.utils.import("resource://gre/modules/InlineSpellChecker.jsm");

Empty line before the import.

@@ +247,5 @@
> +                                                          [appLocale]);
> +    let rsLocaleString = this.mBundle.getFormattedString("rsLocaleString",
> +                                                         [rsLocale]);
> +    appLocaleLabel.setAttribute("label", appLocaleString);
> +    rsLocaleLabel.setAttribute("label", rsLocaleString);

Have the accesskeys in the .properties file too, get them here and then set via 'appLocaleRadio.accessKey = appLocaleAccessKey;', that worked for me (code taken from the v6 patch).

::: mail/components/preferences/general.xul
@@ +155,5 @@
> +      <hbox align="center">
> +      <radiogroup id="formatLocale"
> +                  class="indent"
> +                  preference="intl.regional_prefs.use_os_locales"
> +                  orient="horizontal">

The stacking should be vertical as the 2 option do not fit inside the Preferences dialog when horizontal.

::: mail/locales/en-US/chrome/messenger/preferences/general.dtd
@@ +38,5 @@
>  <!ENTITY browse.accesskey                 "B">
>  
>  <!ENTITY defaultSearchEngine.label        "Default Search Engine">
> +
> +<!ENTITY dateTimeFormatting.label         "Date and time formatting">
\ No newline at end of file

Date and Time Formatting, that seems to be the style for headers in the dialog.
Attachment #8890336 - Flags: feedback+
(Assignee)

Comment 33

6 months ago
Created attachment 8890542 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch (v7).

So please confirm that this is how you like it (accesskey working!) before I move it to "Advanced / General".
Attachment #8890336 - Attachment is obsolete: true
Attachment #8890459 - Attachment is obsolete: true
Attachment #8890459 - Flags: feedback?(acelists)
Attachment #8890542 - Flags: feedback?(acelists)

Comment 34

6 months ago
Comment on attachment 8890542 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch (v7).

Review of attachment 8890542 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I like this, thanks.

::: mail/components/preferences/general.xul
@@ +160,5 @@
> +        <hbox>
> +          <radio id="appLocale"
> +                 value="false"/>
> +                 <!-- label and accesskey will be set dynamically -->
> +          <spacer flex="1"/>

Are the spacers needed?
Attachment #8890542 - Flags: feedback?(acelists) → feedback+
(Assignee)

Comment 35

6 months ago
(In reply to :aceman from comment #34)
> Are the spacers needed?
Copied from elsewhere on the page. I'll move it now.
(Assignee)

Comment 36

6 months ago
Created attachment 8890559 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch (v8).

That should be it then. If you want different access keys, let me know. These appear to work in the "in content" view.
Attachment #8890542 - Attachment is obsolete: true
Attachment #8890559 - Flags: ui-review?(richard.marti)
Attachment #8890559 - Flags: review?(acelists)
Comment on attachment 8890559 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch (v8).

This makes the prefs now really tall. Before 533px and now 603px on Win10, not tested on Linux or macOS. We can add it now to Advanced/General to have it in TB but we need to decide what we want to do to make it less tall in a followup bug (maybe enable the incontent prefs ;-) ).

I'm not so happy with the sentence "Regional settings locale:" because it doesn't say from where ths setting comes. Maybe a "Systems regional settings locale:" would be better.
Attachment #8890559 - Flags: ui-review?(richard.marti) → ui-review+
(Assignee)

Comment 38

6 months ago
I'm using in-content prefs ever since I've discovered them ;-) In fact, I have options open all the time.

In Windows 10 the control panel is called "Regional", in Windows 10 it's "Regional and Language Options". I don't have Linux with me right now, but looking on the web I see "Regional Formats" (https://i.stack.imgur.com/rHCLE.png).

Even on Mac this seems to be associated with "region" (http://cdn.osxdaily.com/wp-content/uploads/2012/06/change-regional-settings-mac.jpg) so I don't think adding "System" won't clarify anything. Besides, it's on the Advanced panel ;-)

Comment 39

6 months ago
Comment on attachment 8890559 [details] [diff] [review]
1384007-intl.regional_prefs.use_os_locales.patch (v8).

Review of attachment 8890559 [details] [diff] [review]:
-----------------------------------------------------------------

But please fix the expanding of the preferences dialog when the Advanced pane is selected due to these new options in a followup.
Attachment #8890559 - Flags: review?(acelists) → review+
(Assignee)

Comment 40

6 months ago
On my Windows 10 the panel grows from 585px to 637px. There are 37px of empty space above the [OK] [Cancel] buttons, surely we can lose some of that.

Comment 41

6 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/db038d905a4e
Expose intl.regional_prefs.use_os_locales in the UI. r=aceman,ui-r=Paenglab
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
(Assignee)

Updated

6 months ago
Target Milestone: --- → Thunderbird 56.0
What is the different between 'intl.locale.matchOS' and 'intl.regional_prefs.use_os_locales'???
You need to log in before you can comment on or make changes to this bug.