917480 - Create language selection UI for unsupported Android locales

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Locale switching and selection, defect)

Description

[Jeff Beatty [:gueroJeff]]

We're currently restricted to shipping locales in the multilocale build that are supported on Android. By the end of Q1 next year, we'll have added all of these supported locales to the multilocale build. In order to continue to add locales, we'll need to include a language selection dialogue in the start-up UI to allow the user to install a build and select their preferred locale at start-up.

Use case:
User in Africa's native language is Songhai, which is not supported in Android. The user will download the multilocale Firefox for Android APK from the Play store and open it. At start-up, a dialogue opens asking the user to select their preferred language. The user selects Songhai from a list of Firefox supported locales. The user then can use Firefox for Android in their own language, with relevant font, keyboard, date, time, etc. support for their locale.

Comment 1

[Aaron Train [:aaronmt]]

Bug 653141 might be helpful for reference in past attempts

Comment 2

[Erin Lancaster [:elan]]

note: we need a solution for this by end of Q1 or we'll run out of android-supported locales. just want to add context for product/feature/release drivers.

Comment 3

[joan]

Use case: Asturian, Occitan, Galician and Basque are not supported in Android. So, currently is not possible to use Firefox for Android in these languages

Comment 4

[Richard Newman [:rnewman]]

needinfo? Axel to give us enough pointers to make sure we don't break this magic:

> Yes, makes sense (though AFAIK we made useragent.locale as well as the
> selectedLocale stuff Axel mentions do the right thing even on Android so
> that stuff works cross-product).

Comment 5

[Axel Hecht [:Pike]]

The gecko process is set up as matchOS, which makes gecko use the best matching locale to the process.

As long as the java part and the gecko part use the same locale codes, things just work. There's a difference between en-US vs en_US (I think java is using unix codes?). There are exceptions, like Hebrew, which uses the locale code for yiddish on java.

Comment 6

[Richard Newman [:rnewman]]

Considerations/notes to self:

* Unlike the test add-on, the topmost activity here will not be BrowserApp. It'll be GeckoPreferences. That means we have a stack to deal with.

* On some Android releases we don't use fragments, right?

* We ought to redisplay the Settings UI when the locale change is committed.

* A user can make a change and then leave the app before returning to BrowserApp, so we need to make the change immediately -- we can't delegate it to BrowserApp. 

* Indeed, BrowserApp might not be running at all (Don't Keep Activities), so we can't do something like sending a Java Bridge message to hit the current testing code path.

* We can't *just* make the change in GeckoPreferences, though: BrowserApp/GeckoApp is in charge of BrowserHealthRecorder, and also needs to redisplay itself. The only situation in which this really matters is in returning from Settings to the browser, so we can use onActivityResult here.

So this is going to look slightly redundant: we'll need to commit the change in onPreferenceChange, do some redisplay, and return a result to BrowserApp which will also do some BrowserLocaleManager stuff (the current GeckoApp#setLocale code, pretty much).

* Additionally, we need to generate the locale list from the build system.

* And finally, we need to handle locales that Locale.getDisplayName() fails for. There are probably some.

I'll need to work with Ian this week and next to figure out exactly where in Settings we put this. Currently I'm working under the assumption of a "Language" top-level, with "Browser Language" being a simple list picker, and the mocks from the web content switcher being in this section rather than hiding a couple of levels deep.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Pre: commenting.

Comment 8

[Richard Newman [:rnewman]]

Attachment: Part 0: refactor checking of isGeckoPref inside GeckoPreferences.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Part 1: add basic locale preference handling to GeckoPreferences.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 2: add LocaleListPreference.

Comment 11

[Richard Newman [:rnewman]]

Attachment: Part 3: add strings, and add 'Language' section to top-level preferences.

Comment 12

[Richard Newman [:rnewman]]

Attachment: Part 4: open GeckoPreferences from BrowserApp and pay attention to the response.

Comment 13

[Richard Newman [:rnewman]]

Attachment: Part 5: handle locale changes in GeckoPreferences.* * *

More locale switching.

Comment 14

[Richard Newman [:rnewman]]

Attachment: Part 6: make GeckoPreferenceFragment handle locale changes after initial display.* * *

More locale switching in fragment.

Comment 15

[Richard Newman [:rnewman]]

Attachment: Part 7: don't refresh BrowserApp locale unless GeckoPreferences reports a locale change.

Comment 16

[Richard Newman [:rnewman]]

Attachment: Part 8: reduce logging levels.

Comment 17

[Richard Newman [:rnewman]]

This is a fairly complicated bug, so I opted to implement it as an understandable journey, rather than a scary chunk. As such, please ignore some of the TODOs and logging lines that might still be present in earlier patches; they're addressed later.

Parts 0 and 1 prepare GeckoPreferences to handle the "locale" SharedPreference.

Parts 2 and 3 add a "Language" section to the top-level settings screen, with a "Browser language" picker inside. We currently hard-code en-US and es-ES; this will be addressed by a later part, generating code from MOZ_CHROME_MULTILOCALE.

Part 4 (and Part 7, to an extent) adjust the relationship between BrowserApp and GeckoPreferences. It's necessary to make this more of a call-response setup so that BrowserApp can respond to a change in locale -- e.g., poking BrowserHealthRecorder -- and *not* refresh itself if the Settings interaction didn't result in a locale change.

Parts 5 and 6 make Settings live. When you pick a locale, the screen changes in front of you, and hitting Back just works.

Part 8 is a little bit of cleanup.


Still to do:

* Certain mis-timed onConfigurationChange events can cause a redisplay loop when entering GeckoPreferences. (Rotate screen while the language picker dialog is focused; rotate screen after app launch but before Gecko init; maybe others.)

* The locale list needs to be generated at build time.

* Ian needs to comment on the locale picker UI.

* The system default locale flow isn't currently handled at all, and isn't shown as a subtext in the picker.

Comment 18

[Richard Newman [:rnewman]]

Oh, and I should note: this hasn't been tested on a pre-fragment device, nor has it been tested on a tablet.

Comment 19

[Richard Newman [:rnewman]]

x86: http://people.mozilla.com/~rnewman/l10n-i386.apk

Comment 20

[Richard Newman [:rnewman]]

Try (kinda pointless, being a single-locale build an' all):

https://tbpl.mozilla.org/?tree=Try&rev=0a64e0a65317

Comment 21

[Richard Newman [:rnewman]]

Attachment: Locale switching (roll-up patch).* * *

Bug 917480 - Pre: commenting.
* * *
Bug 917480 - Part 0: refactor checking of isGeckoPref inside GeckoPreferences.
* * *
Bug 917480 - Part 1: add basic locale preference handling to GeckoPreferences.
* * *
Bug 917480 - Part 2: add LocaleListPreference.
* * *
Bug 917480 - Part 3: add strings, and add 'Language' section to top-level preferences.
* * *
Bug 917480 - Part 4: open GeckoPreferences from BrowserApp and pay attention to the response.
* * *
Bug 917480 - Part 5: handle locale changes in GeckoPreferences.
* * *
Bug 917480 - Part 6: make GeckoPreferenceFragment handle locale changes after initial display.
* * *
Bug 917480 - Part 7: don't refresh BrowserApp locale unless GeckoPreferences reports a locale change.
* * *
Bug 917480 - Part 8: reduce logging levels.
* * *
Bug 917480 - Part 9: trying to get rotation to work.

Comment 22

[Richard Newman [:rnewman]]

Attachment: Part 9: manifest changes for locale changes on API 17.

The looping activity restart is

https://code.google.com/p/android/issues/detail?id=53079

In order to work with API 17, we need both "locale" and "layoutDirection" in the handled events part of the manifest.

I hit this when implementing Bug 936756, but couldn't use those options at the time (we weren't targeting the right API level, I think). Now it works.

  "layoutDirection"
  "The layout direction has changed. For example, changing from left-to-right (LTR) to right-to-left (RTL). Added in API level 17."

Comment 23

[Richard Newman [:rnewman]]

ARM APK:

http://people.mozilla.com/~rnewman/l10n-arm.apk

Comment 24

[Richard Newman [:rnewman]]

Attachment: Part 10: don't fetch display metrics when calling Resource.updateConfiguration.

We (and everyone else, apparently) are doing pointless work, if the Android source is to be believed. This patch changes

  res.updateConfiguration(config, res.getDisplayMetrics();

to

  res.updateConfiguration(config, null);

everywhere.


    public DisplayMetrics getDisplayMetrics() {
        if (DEBUG_CONFIG) Slog.v(TAG, "Returning DisplayMetrics: " + mMetrics.widthPixels
                + "x" + mMetrics.heightPixels + " " + mMetrics.density);
        return mMetrics;
    }

...

    public void updateConfiguration(Configuration config,
            DisplayMetrics metrics) {
        updateConfiguration(config, metrics, null);
    }

...

            if (metrics != null) {
                mMetrics.setTo(metrics);
            }

Comment 25

[Richard Newman [:rnewman]]

Comment on attachment 8410616 [details] [diff] [review]
Pre: commenting.

Nick kindly volunteered to tackle these reviews. I'll default to him for these unless someone else seems more suitable.

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 8410621 [details] [diff] [review]
Part 3: add strings, and add 'Language' section to top-level preferences.

Please ignore the TODO: it's been removed locally.

Comment 27

[Richard Newman [:rnewman]]

Comment on attachment 8410622 [details] [diff] [review]
Part 4: open GeckoPreferences from BrowserApp and pay attention to the response.

See Comment 17 for reviewing guidance.

Comment 28

[Richard Newman [:rnewman]]

Comment on attachment 8410623 [details] [diff] [review]
Part 5: handle locale changes in GeckoPreferences.* * *

See Comment 17 for reviewing guidance.

This component doesn't seem to show 'addl-review', so Chenxia, please consider this f? a r?.

Comment 29

[Richard Newman [:rnewman]]

Comment on attachment 8411987 [details] [diff] [review]
Part 10: don't fetch display metrics when calling Resource.updateConfiguration.

Arbitrarily picking kats to lend a second eye to this.

Comment 30

[Richard Newman [:rnewman]]

Remaining work before this can land:

* Handle and display "System default".
* Generate the locale list from the build system.
* Handle any locales that Locale.getDisplayName() fails for.
* Testing on pre-fragments devices (anyone have a 2.3 device hanging around?) and tablets.

Comment 31

[Richard Newman [:rnewman]]

Attachment: Ian's proposed layout

Comment 32

[Jeff Beatty [:gueroJeff]]

Comment on attachment 8412114 [details]
Ian's proposed layout

Do we need to expose lang codes to users? Could we just use the languages? Will languages be translated or remain in English?

Comment 33

[Richard Newman [:rnewman]]

The top half of that image is for Bug 881510, which hasn't been built yet. The browser locale picker is a pop-up.

For the pop-up, as currently implemented, the languages are shown in the current locale without locale codes --

  |============================|
  | Idioma del navegador       |
  |----------------------------|
  | inglés (Estados Unidos)  o |
  | español (España)         * |
  |----------------------------|
  |         Cancelar           |
  |----------------------------|

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8411987 [details] [diff] [review]
Part 10: don't fetch display metrics when calling Resource.updateConfiguration.

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

Some versions of android seem to have done other things on a non-null metrics. See for example https://github.com/android/platform_frameworks_base/commit/e2515eebf42c763c0a2d9f873a153711778cfc17#diff-f124e5c8f8cda3f9c5653805402598f5L1411

If you are sure that this has no negative repercussions on any version of Android under any circumstances, then fine by me! :)

Comment 35

[Chenxia Liu [:liuche]]

Comment on attachment 8410617 [details] [diff] [review]
Part 0: refactor checking of isGeckoPref inside GeckoPreferences.

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

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +447,5 @@
> +        if (TextUtils.isEmpty(key)) {
> +            return false;
> +        }
> +
> +        if (key.startsWith(NON_PREF_PREFIX)) {

Are you keeping these separate for clarity? I would combine them because the OR will short-circuit in the null key case.

Comment 36

[Chenxia Liu [:liuche]]

Comment on attachment 8410618 [details] [diff] [review]
Part 1: add basic locale preference handling to GeckoPreferences.

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

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +94,5 @@
>      private static final String PREFS_DISPLAY_REFLOW_ON_ZOOM = "browser.zoom.reflowOnZoom";
>      private static final String PREFS_SYNC = NON_PREF_PREFIX + "sync";
>  
> +    // This isn't a Gecko pref, even if it looks like one.
> +    private static final String PREFS_BROWSER_LOCALE = "locale";

Is the reason you're not using NON_PREF_PREFIX that "locale" is already a SharedPreference?

Comment 37

[Richard Newman [:rnewman]]

(In reply to Chenxia Liu [:liuche] from comment #36)

> Is the reason you're not using NON_PREF_PREFIX that "locale" is already a
> SharedPreference?

Correct. "locale" is already deployed and used by the existing locale handling code.

If we want to start using NON_PREF_PREFIX, we can fix that in the migration when we migrate to per-profile locales.

If we want to eventually switch to a different mechanism for GeckoPreferences -- I'm sure there's a better way to do this! -- then it's moot.

And as you can see, it's not a difficult addition.

Comment 38

[Richard Newman [:rnewman]]

Attachment: Part 3: add strings, and add 'Language' section to top-level preferences.* * *

Updated: localized string "System default", UI reflects ibarlow's design.

Comment 39

[Richard Newman [:rnewman]]

Attachment: Part 11: compute locale list at build time.

Comment 40

[Richard Newman [:rnewman]]

Attachment: Part 12: handle resetting to the system default locale.

I tested this, and it seems to work for me...

Comment 41

[Nick Alexander :nalexander [he/him]]

rnewman: sorry for the wait; review by EOD today (Monday).

Comment 42

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410616 [details] [diff] [review]
Pre: commenting.

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

::: mobile/android/base/BrowserLocaleManager.java
@@ +220,5 @@
>          Configuration config = res.getConfiguration();
> +
> +        // We should use setLocale, but it's unexpectedly missing
> +        // on real devices.
> +        config.locale = locale; 

nit: trailing ws.

Comment 43

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8413097 [details] [diff] [review]
Part 3: add strings, and add 'Language' section to top-level preferences.* * *

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

I don't understand the pref persistence, but I'll keep reading and revoke r+ rather than have to go back and add it.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +74,5 @@
> +<!ENTITY pref_category_language "Language">
> +<!ENTITY pref_browser_locale "Browser language">
> +
> +<!-- Localization note (locale_system_default) : This string indicates that
> +     Firefox will use the locale currently selected in Android's settings. -->

use: for chrome, or for content, or for both?

::: mobile/android/base/preferences/LocaleListPreference.java
@@ +87,5 @@
>  
>          // We leave room for "System default".
>          final String[] entries = new String[count + 1];
>          final String[] values = new String[count + 1];
> +        entries[0] = getContext().getString(R.string.locale_system_default);

Fold this back into the earlier patch (the one that you didn't ask for r?) on.

::: mobile/android/base/resources/xml/preferences_locale.xml
@@ +10,5 @@
> +    <PreferenceCategory android:title="@string/pref_browser_locale">
> +        <!-- No title set here. We set the title to the current locale in
> +             GeckoPreferences. -->
> +        <org.mozilla.gecko.preferences.LocaleListPreference
> +            android:key="locale"

This seems wrong.  Shouldn't this be per-profile?  And shouldn't this be not_a_preference?

Comment 44

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410622 [details] [diff] [review]
Part 4: open GeckoPreferences from BrowserApp and pay attention to the response.

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

::: mobile/android/base/BrowserApp.java
@@ +129,5 @@
>  
>      private static final String BROWSER_SEARCH_TAG = "browser_search";
> +
> +    // Request ID for startActivityForResult.
> +    private static final int ACTIVITY_REQUEST_PREFERENCES = 1001;

I'm a big fan of generating these things from relevant strings, like "locales".hash() or similar, but whatever.

@@ +1711,5 @@
> +    public void onActivityResult(int requestCode, int resultCode, Intent data) {
> +        Log.i(LOGTAG, "onActivityResult: " + requestCode + ", " + resultCode + ", " + data);
> +        switch (requestCode) {
> +        case ACTIVITY_REQUEST_PREFERENCES:
> +            // We just returned from preferences. If our locale changed,

This is a well-commented footgun.  This is saying that *consumers* of the preference activity are responsible for doing things; that's just waiting to break.  Changing the pref should notify, or send the correct intent, or whatever.

Justify this choice.

Comment 45

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410623 [details] [diff] [review]
Part 5: handle locale changes in GeckoPreferences.* * *

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

I believe that this does what it says on the tin.

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +131,5 @@
> +        // At present we only need to do this for the top-level prefs view
> +        // and the locale switcher itself.
> +        // The others don't allow you to change locales, and have their
> +        // titles set in their fragment descriptors.
> +        if (res == R.xml.preferences) {

Is there no way to fish this from the resource?  I.e., instantiate the PreferenceCategory and save what is a terribly fragile switch statement?

@@ +359,5 @@
>      @Override
>      public void onActivityResult(int requestCode, int resultCode, Intent data) {
> +        // We might have just returned from a settings activity that allows us
> +        // to switch locales, so reflect any change that occurred.
> +        checkLocale();

This is not tenable.  There are ways to get back to an Activity other than by creating it or returning from a spawned activity directly.  Is the new Fennec contract to be that every entry point calls checkLocale?  Clearly not.  But it is reasonable for activities to handle a particular broadcast intent, or similar.

Per our discussion on Vidyo, I think I understand this a little better, and this might be reasonable.

@@ +688,5 @@
>  
> +    /**
> +     * Immediately handle the user's selection of a browser locale.
> +     *
> +     * Note that we don't handle this by sending a message to GeckoApp, for

Could you explain what one might hope/expect sending such a message would do?

@@ +697,5 @@
> +     * * The user might not hit Back (or Up) -- they might hit Home and never
> +     *   come back.
> +     *
> +     * We handle the case of the user returning to GeckoApp via the
> +     * onActivityResult mechanism.

via GeckoApp.onActivityResult, correct?

@@ +734,3 @@
>      @Override
>      public boolean onPreferenceChange(Preference preference, Object newValue) {
> +        Log.i(LOGTAG, "onPreferenceChange: " + preference.getKey() + ", " + newValue);

I don't think we should do this.  Prefs could be construed as private information.

Comment 46

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410624 [details] [diff] [review]
Part 6: make GeckoPreferenceFragment handle locale changes after initial display.* * *

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

::: mobile/android/base/preferences/GeckoPreferenceFragment.java
@@ +61,5 @@
> +     * for us to redisplay this fragment in a different locale.
> +     */
> +    protected String getTitle() {
> +        final int res = getResource();
> +        if (res == R.xml.preferences_locale) {

Can you explain these partial lists?  I'd prefer some static data structure that does this mapping, perhaps in GeckoPreferences, so that it's clear what needs to be done when you add a new preference (category?).

And again, this must be possible to fish from |res|.

@@ +87,5 @@
> +            }
> +        }
> +    }
> +
> +    protected void onLocaleChanged(Locale newLocale) {

I'm finding it hard to differentiate things that Android invokes, namely |on...|, from internal helpers.  I think you're doing it on purpose, but I'm not a fan.  I prefer your other pattern, namely |update...|.

@@ +107,5 @@
> +        if (!currentLocale.equals(lastLocale)) {
> +            // Locales differ. Let's redisplay.
> +            onLocaleChanged(currentLocale);
> +        } else {
> +            // Fix the parent title regardless.

I would find it slightly more clear if we always called updateTitle: that is, remove the call from onLocaleChanged.

Comment 47

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #44)

> Justify this choice.

Further to our chat, and to summarize: this basically boils down to "when does BrowserApp need to detect and reflect locale changes?".

There are two times that locales can change: the system locale can change (which GeckoApplication will handle), or the user picks a new one in GeckoPreferences.

We don't need to do anything if BrowserApp isn't running: it'll do the right thing when it's created.

So the only time (after creation) that BrowserApp needs to handle a locale change is when it's further down the stack from a GeckoPreferences instance that it launched, and the user is switching the locale there.

Rather than always defensively checking for locales in onResume (which has a small incremental cost), we detect the selection when returning from GeckoPreferences to BrowserApp.

There's a possibility that there are flows that can switch a user to BrowserApp without returning from a launched GeckoPreferences. They're in the same task, so the task switcher can't do it. Perhaps the data reporting notification can -- that still needs to be tested, and if that's troublesome we can weigh the risk/reward of changing approaches -- but perhaps not.

The general rule I'm outlining here is:

All activities should check for locale changes:

• When they're created
• On resume, if they have a different task affinity from GeckoPreferences
• On activity result, if they can launch GeckoPreferences.

GeckoPreferences itself is a special case, because it's live while a locale change is occurring.

I think you have a preference for defensively programming against future avenues for switching locales -- assume that the user can switch locales anywhere.

I have a preference for not doing so, because (a) YAGNI, (b) this is kinda voodoo anyway, so we will likely need to burn some incense and shake some gourds anyway if we add those avenues.

Comment 48

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410625 [details] [diff] [review]
Part 7: don't refresh BrowserApp locale unless GeckoPreferences reports a locale change.

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

::: mobile/android/base/BrowserApp.java
@@ +1714,5 @@
>          case ACTIVITY_REQUEST_PREFERENCES:
>              // We just returned from preferences. If our locale changed,
>              // we need to redisplay at this point, and do any other browser-level
>              // bookkeeping that we associate with a locale change.
> +            if (resultCode != GeckoPreferences.RESULT_LOCALE_DID_CHANGE) {

Log.d?

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +105,5 @@
>      // These values are chosen to be distinct from other Activity constants.
>      private static final int REQUEST_CODE_PREF_SCREEN = 5;
>      private static final int RESULT_CODE_EXIT_SETTINGS = 6;
>  
> +    // So that callers know whether to refresh to accommodate a locale change.

// Result returned when a preference that changes a (which?) locale has been changed.  Callers can use this code to refresh themselves to accomodate a locale change.

@@ +106,5 @@
>      private static final int REQUEST_CODE_PREF_SCREEN = 5;
>      private static final int RESULT_CODE_EXIT_SETTINGS = 6;
>  
> +    // So that callers know whether to refresh to accommodate a locale change.
> +    public static final int RESULT_LOCALE_DID_CHANGE = 7;

RESULT_CODE_...

Comment 49

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410626 [details] [diff] [review]
Part 8: reduce logging levels.

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

Whatever, fold this in.  And consider not logging prefs at all.

Comment 50

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8411987 [details] [diff] [review]
Part 10: don't fetch display metrics when calling Resource.updateConfiguration.

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

This just scares me.  I see no documentation for |updateConfiguration| at all.  I see no reason why you should expect this to work and no justifying comment in the surrounding code or commit message.  Land it with your own review, if you think it's worth it.

::: mobile/android/base/BrowserLocaleManager.java
@@ +220,5 @@
>          Configuration config = res.getConfiguration();
>  
>          // We should use setLocale, but it's unexpectedly missing
>          // on real devices.
>          config.locale = locale; 

nit: remove trailing ws.

Comment 51

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8413098 [details] [diff] [review]
Part 11: compute locale list at build time.

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

I'm pretty sure this is not what we want.  This works due to your non-standard build approach, where you define MOZ_CHROME_MULTILOCALE at top-level.  On TBPL, that is only exposed to the packager: see [1].  It is possible that we expose the set of languages to the build step, but I'd actually prefer to make multi-lang builds more re-packy and less re-buildy, so I'd rather do otherwise.

This definitely doesn't do the right thing for single locale repacks.  What do you want for single locale repacks?  If the built locale only has res/*-de, what languages should get displayed in the picker?  Only de?  Should we remove the locale-switching preference entirely?

In fact, this is quite tricky.  Repacks cannot (at this time) change code.  They can modify existing resources.  That suggests that we should expose MOZ_CHROME_MULTILOCALE in strings.xml.in.  Using resources requires a context, but I think that's okay for LocaleListPreference.

I believe that strings.xml.in gets re-processed as part of re-packing, but it is subtle.  I'll need to think on this.  We might want a generated raw resource that doesn't get translated.  Tricky.

In any case, I'm pretty sure we don't want to bury this list deep in the preferences code.  I'd like to see the processed list in something closer to top level, or an accessor to it; and I see no reason to expose MOZ_CHROME_MULTILOCALE directly.

[1] https://github.com/mozilla/build-mozharness/blob/master/mozharness/mozilla/l10n/multi_locale_build.py#L200

::: mobile/android/base/Makefile.in
@@ +43,5 @@
>    $(NULL)
>  
> +ifdef MOZ_CHROME_MULTILOCALE
> +DEFINES += -DMOZ_CHROME_MULTILOCALE="$(MOZ_CHROME_MULTILOCALE)"
> +endif 

nit: trailing ws.

::: mobile/android/base/preferences/LocaleListPreference.java
@@ +17,5 @@
>  import android.util.AttributeSet;
>  
>  public class LocaleListPreference extends ListPreference {
>  
> +    private static String[] SHIPPING_LOCALE_TAGS = 

nit: trailing ws.

Comment 52

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8413099 [details] [diff] [review]
Part 12: handle resetting to the system default locale.

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

System locale is special, because we need to extract the locale, set to that locale, and then forget that we have a locale set at all, correct?  Add it to the commit comment.

::: mobile/android/base/BrowserLocaleManager.java
@@ +38,5 @@
>  
>      private static final String EVENT_LOCALE_CHANGED = "Locale:Changed";
>      private static final String PREF_LOCALE = "locale";
>  
>      // This is volatile because we don't impose restrictions

These?

@@ +121,5 @@
>  
>          receiver = new BroadcastReceiver() {
>              @Override
>              public void onReceive(Context context, Intent intent) {
> +                // We don't trust Locale.getDefault() here, so use the one Android supplies.

why not?  We used it for the default systemLocale...

::: mobile/android/chrome/content/browser.js
@@ +1607,5 @@
> +          // The value provided to Locale:Changed should be a BCP47 language tag
> +          // understood by Gecko -- for example, "es-ES" or "de".
> +          console.log("Locale:Changed: " + aData);
> +
> +          // TODO: do we need to be more nuanced here -- e.g., checking for the

This comment needs to be updated, since you changed the code below!

@@ +1616,5 @@
> +          console.log("Switching to system locale.");
> +          Services.prefs.clearUserPref("general.useragent.locale");
> +        }
> +
> +        Services.prefs.setBoolPref("intl.locale.matchOS", !aData);

Hmm, this pref is special.  I think your logic is correct.

Comment 53

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8411935 [details] [diff] [review]
Part 9: manifest changes for locale changes on API 17.

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

Add to commit comment:

locale and layoutDirection need to be together; business about the activities that we handle.

::: mobile/android/base/AndroidManifest.xml.in
@@ +86,5 @@
>          <activity android:name=".App"
>                    android:label="@string/moz_app_displayname"
>                    android:taskAffinity="@ANDROID_PACKAGE_NAME@.BROWSER"
>                    android:alwaysRetainTaskState="true"
> +                  android:configChanges="keyboard|keyboardHidden|mcc|mnc|orientation|screenSize|locale|layoutDirection"

nit: funky indenting.

Comment 54

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #45)

> Is there no way to fish this from the resource?  I.e., instantiate the
> PreferenceCategory and save what is a terribly fragile switch statement?

I did poke around to find an alternative, but (a) fragments are bastards -- reaching into the parent or child from the other seems to have been made deliberately difficult, because coupling, and (b) this check-the-int approach has been used elsewhere, which makes me think someone else tried and failed, too.

I'll spend a little more time.


> > +     * Immediately handle the user's selection of a browser locale.
> > +     *
> > +     * Note that we don't handle this by sending a message to GeckoApp, for
> 
> Could you explain what one might hope/expect sending such a message would do?

Same approach we took before -- GeckoApp was the center of "please do everything needed to change the locale", including touching LocaleManager and redisplaying itself.

(Let me know if you want that in the comment.)


> > +     * We handle the case of the user returning to GeckoApp via the
> > +     * onActivityResult mechanism.
> 
> via GeckoApp.onActivityResult, correct?

Yeah, that's the mechanism :)


> > +        Log.i(LOGTAG, "onPreferenceChange: " + preference.getKey() + ", " + newValue);
> 
> I don't think we should do this.  Prefs could be construed as private
> information.

Yeah, that shouldn't be in there at all. Normally I add an "XXX" or something...



> > +    protected String getTitle() {
> > +        final int res = getResource();
> > +        if (res == R.xml.preferences_locale) {
> 
> Can you explain these partial lists?  I'd prefer some static data structure
> that does this mapping, perhaps in GeckoPreferences, so that it's clear what
> needs to be done when you add a new preference (category?).

Basically these are the fragments/prefs views whose title might change due to a locale switch -- again, this is the 'special' chain BrowserApp > GeckoPreferences:preferences > GeckoPreferences[Fragment]:locale.

No other preference screen will be on the stack if the locale changes, so they don't need to be able to adapt.

getTitle returns null for everything else, and updateTitle handles this, so we hit the active case for that chain, and we don't care about the rest.


> > +    protected void onLocaleChanged(Locale newLocale) {
> 
> I'm finding it hard to differentiate things that Android invokes, namely
> |on...|, from internal helpers.  I think you're doing it on purpose, but I'm
> not a fan.  I prefer your other pattern, namely |update...|.

Would you prefer (a) the name changed, (b) @Override and an interface, (c) a comment, (d) something else?

Comment 55

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #50)

> This just scares me.  I see no documentation for |updateConfiguration| at
> all.  I see no reason why you should expect this to work and no justifying
> comment in the surrounding code or commit message.

There wasn't a comment for the original calls, either, and they're just as scary!

I expect this to work based on reading the Android code for updateConfiguration -- the null case just doesn't touch display metrics, so we simply avoid a fetch-and-set for the same value.

As kats mentioned in his review, that might not be the case for all Android versions, but we can verify that by reading the source code.

Comment 56

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #51)

> I'm pretty sure this is not what we want.

Entirely expected -- unless things have changed, we don't have a way to try this out on infra, right?

I'm going to be very dependent on you build folks for guidance, here.


> It is
> possible that we expose the set of languages to the build step, but I'd
> actually prefer to make multi-lang builds more re-packy and less re-buildy,
> so I'd rather do otherwise.

Your call!

My end goal for this is to load locales from JSON, allowing OTA update, but that imposes a cost that we don't have to pay if we build stuff into code.

Making this effectively zero cost is one of the main reasons why I put stuff into a .java.in rather than a separate file.


> This definitely doesn't do the right thing for single locale repacks.  What
> do you want for single locale repacks?  If the built locale only has
> res/*-de, what languages should get displayed in the picker?  Only de? 
> Should we remove the locale-switching preference entirely?

Removing it is my preferred fallback. Do we ship any Fennec single locale repacks? (I.e., is this a case we care about?)

Comment 57

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #43)

> > +            android:key="locale"
> 
> This seems wrong.  Shouldn't this be per-profile?

Bug 949495 Comment 17, and Comment 37.

> And shouldn't this be not_a_preference?

See Comment 37.

Comment 58

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #46)

> > +    protected void onLocaleChanged(Locale newLocale) {
> 
> I'm finding it hard to differentiate things that Android invokes, namely
> |on...|, from internal helpers.  I think you're doing it on purpose, but I'm
> not a fan.  I prefer your other pattern, namely |update...|.

I've inlined this particular instance.

Comment 59

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #53)

> nit: funky indenting.

I think you're seeing linewrap in Splinter; it's 18-spaced like all of the other attributes on that activity.

I did take this opportunity to remove tabs from the file.

Comment 60

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #50)

> This just scares me.  I see no documentation for |updateConfiguration| at
> all.  I see no reason why you should expect this to work and no justifying
> comment in the surrounding code or commit message.  Land it with your own
> review, if you think it's worth it.

I've checked 2.2:

http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/2.2_r1.1/android/content/res/Resources.java#1266

and 4.4.2:

http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.4.2_r1/android/content/res/Resources.java#1529

along with a smattering of others, and the logic seems sound here. My general feeling is that what we do prior to this change -- fetching display metrics from the current config and forcing the config to re-apply them -- is likely to be more error prone (and more expensive) than skipping those blocks altogether, so I'm inclined to land this as a separate commit and rock on.

Comment 61

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #52)

> >          receiver = new BroadcastReceiver() {
> >              @Override
> >              public void onReceive(Context context, Intent intent) {
> > +                // We don't trust Locale.getDefault() here, so use the one Android supplies.
> 
> why not?  We used it for the default systemLocale...

I've amended the comment:

                // We don't trust Locale.getDefault() here, because we make a
                // habit of mutating it! Use the one Android supplies, because
                // that gets regularly reset.
                // The default value of systemLocale is fine, because we haven't
                // yet swizzled Locale during static initialization.

Comment 62

[Richard Newman [:rnewman]]

Review comments addressed (though I'm not going to upload all the parts unless you want to see them again).

Waiting on:

* An OK for part 2, into which I've folded part 3.
* A change of mind after discussions on part 4, or an alternative proposal.
* A proposal for generating the locale list in a way that's fun times for the build system.

Comment 63

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410619 [details] [diff] [review]
Part 2: add LocaleListPreference.

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

Notes for discussion, not a review.

::: mobile/android/base/preferences/LocaleListPreference.java
@@ +16,5 @@
> +
> +public class LocaleListPreference extends ListPreference {
> +
> +    private static String[] SHIPPING_LOCALE_TAGS = {
> +        "en-US",

So obviously this shouldn't land; but it gets addressed -- in some way -- later on.

@@ +21,5 @@
> +        "es-ES",
> +    };
> +
> +    public LocaleListPreference(Context context) {
> +        super(context);

Can this passthrough to the two-arg constructor?

@@ +39,5 @@
> +        public final String tag;
> +
> +        private final Locale locale;
> +        private final String nativeName;
> +

This TODO shouldn't land as is; not sure what is to happen here.

@@ +86,5 @@
> +
> +        // We leave room for "System default".
> +        final String[] entries = new String[count + 1];
> +        final String[] values = new String[count + 1];
> +        entries[0] = "System default";       // TODO: string

Can't land.

Comment 64

[Richard Newman [:rnewman]]

A quick note re single-locale builds:

Right now, if I understand correctly, a single-locale build depends on your system locale for correct behavior. Imagine you have an en-US single-locale build, running on an es-ES OS. Its strings will be something like

  "Last synced: hace dos semanas"

... because some come from the app, and some come from the OS.

The only way to get this to be correct is to apply the single locale in exactly the same way that locale switching does!

We don't currently do that, so the only way that single-locale builds will look right in current Fennec is if they agree with your OS locale.

This is an opportunity: we can, if we wish to pay the price, detect a single-locale build and make the process locale match, just as if the user opened the locale switcher and picked the only option.

I'm not inclined to pay that cost right now, because that's runtime detection of a packaging step, which pretty much means reading a file. (Nick is investigating the best way to encode that packaging information.)

We might choose to do this in the future, if we decide to ship single-locale builds to GA for some reason.


Further, I plan to hide the Browser Language section of Settings in single-locale builds. It would be frustrating and low-value, and we'd have to special-case the special-case "System default" entry! Better to just not show that UI.

Comment 65

[Richard Newman [:rnewman]]

> Can this passthrough to the two-arg constructor?

Based on reading the Android source, yes; done so.

> This TODO shouldn't land as is; not sure what is to happen here.

Removed the TODO; we can address this when we get bidi support.

> > +        entries[0] = "System default";       // TODO: string
> 
> Can't land.

Already replaced in a later part.

Comment 66

[Richard Newman [:rnewman]]

Attachment: Part 2: add 'Language' section to top-level preferences, allowing for browser locale switching.

Roll-up of reviewed middle parts, plus the old Part 4. The only remaining part is the build-time stuff, which we can roll in when it's done.

Comment 67

[Richard Newman [:rnewman]]

Attachment: Part 3: manifest changes for locale changes on API 17.

locale and layoutDirection need to appear together in android:configChanges
attributes. We specify these in any activity that we don't want Android to
automatically relaunch when a locale change occurs, which is most or all of
them.

Comment 68

[Richard Newman [:rnewman]]

Attachment: Part 4: handle resetting to the system default locale.

Comment 69

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #60)
> (In reply to Nick Alexander :nalexander from comment #50)
> 
> > This just scares me.  I see no documentation for |updateConfiguration| at
> > all.  I see no reason why you should expect this to work and no justifying
> > comment in the surrounding code or commit message.  Land it with your own
> > review, if you think it's worth it.
> 
> I've checked 2.2:
> 
> http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/
> android/2.2_r1.1/android/content/res/Resources.java#1266
> 
> and 4.4.2:
> 
> http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/
> android/4.4.2_r1/android/content/res/Resources.java#1529
> 
> along with a smattering of others, and the logic seems sound here. My
> general feeling is that what we do prior to this change -- fetching display
> metrics from the current config and forcing the config to re-apply them --
> is likely to be more error prone (and more expensive) than skipping those
> blocks altogether, so I'm inclined to land this as a separate commit and
> rock on.

r+ to landing this change now, as a separate ticket.

Comment 70

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #57)
> (In reply to Nick Alexander :nalexander from comment #43)
> 
> > > +            android:key="locale"
> > 
> > This seems wrong.  Shouldn't this be per-profile?
> 
> Bug 949495 Comment 17, and Comment 37.
> 
> > And shouldn't this be not_a_preference?
> 
> See Comment 37.

Mmm.  Well, at least all of our reviewers are catching some of our issues.

Comment 71

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8414753 [details] [diff] [review]
Part 2: add 'Language' section to top-level preferences, allowing for browser locale switching.

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

For the benefit of future archaeologists: rnewman and I had several discussions about when and where to insert the "my locale changed, update me" code throughout the App.  rnewman (eventually) convinced me that the current approach, summarized in https://bugzilla.mozilla.org/show_bug.cgi?id=917480#c47, is reasonable.

::: mobile/android/base/preferences/GeckoPreferenceFragment.java
@@ +62,5 @@
> +     * Return the title to use for this preference fragment. This allows
> +     * for us to redisplay this fragment in a different locale.
> +     *
> +     * We only return titles for the preference screens that are in the
> +     * flow for selecting a locale, and thus might need to be redisplayed.

Have you tested this on a tablet?  Pref screens are handled differently on tablets.  Tell you what, I want to experiment with these patches myself, so I'll do some tests on tablet.

Comment 72

[Richard Newman [:rnewman]]

This has not yet been tested on tablets or pre-fragment devices. Wanted to get sign off on main experience and approach first. Tablet is next on my list.

Comment 73

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #62)
> Review comments addressed (though I'm not going to upload all the parts
> unless you want to see them again).
> 
> Waiting on:
> 
> * An OK for part 2, into which I've folded part 3.
> * A change of mind after discussions on part 4, or an alternative proposal.

These got rolled together, right?  Further to our discussion, these look good.  I'm going to device test on a tablet this afternoon.

> * A proposal for generating the locale list in a way that's fun times for
> the build system.

This is more interesting.  I think the way to make this work right now is to write JSON into the APK at package time; the question is where in the APK.  Does this go into the root, like package-name.txt?  Into assets/?  Into omni.ja?  I'm not certain yet.

Comment 74

[Richard Newman [:rnewman]]

This works just fine on a smaller 4.2 tablet (Kindle Fire HD or HDX). That size doesn't have the two-pane prefs layout, though. Will try my ancient TF-101 tomorrow if I can, or resort to emulators, Amazon Prime, or the kindness of coworkers.

Comment 75

[Nick Alexander :nalexander [he/him]]

rnewman: questions, and requests for follow-ups:

1) the list of shipped languages will certainly grow, and may contract, over time.  Bitter experience dictates that we bake in a (plan for a) migration story up front.  Can we message that the list of locales has expanded?  Only when there may be a better fit (i.e., user is using es-ES, but system locale is different (es-US?))?  What happens when the user's chosen locale goes away?  Should we timestamp the user's pref selection, to ease upgrade handling, and maybe to note that "no selection" is different than "selected system default"?

2) I think you've already considered fallbacks: you've talked about how we ship es-ES, which doesn't become the default when you're running es-US.  Can you brain dump what exactly the issue is, and what it would take to default to es-* when system locale is es-*?

3) follow-up for language chooser UI telemetry, please.  This is an amazing feature; I want to know how much use it gets.

4) I wonder how we can reach out to inform our localizer population about this feature that they would likely be interested in.

Comment 76

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #75)
> rnewman: questions, and requests for follow-ups:
> 
> 1) the list of shipped languages will certainly grow, and may contract, over
> time.  Bitter experience dictates that we bake in a (plan for a) migration
> story up front.  Can we message that the list of locales has expanded?  Only
> when there may be a better fit (i.e., user is using es-ES, but system locale
> is different (es-US?))?  What happens when the user's chosen locale goes
> away?  Should we timestamp the user's pref selection, to ease upgrade
> handling, and maybe to note that "no selection" is different than "selected
> system default"?

Yeah, I've been thinking about this. My feeling is that removal isn't a big problem; I don't think we have a long history of pulling locales once we add them to maemo-locales, and this is something I think we can retroactively handle with a little extra code in BrowserLocaleManager at the time. (Is this locale present in the list? If not, throw a notification.) 

We might already record system default in a discrete way: empty string versus no pref. But that's a good idea.

Messaging on growth is a thorny one, which is really a milder case of just landing this feature! How does one find out that a user might be better suited by another locale? I'm interested in Jeff's thoughts on that. 

> 
> 2) I think you've already considered fallbacks: you've talked about how we
> ship es-ES, which doesn't become the default when you're running es-US.  Can
> you brain dump what exactly the issue is, and what it would take to default
> to es-* when system locale is es-*?

We have a separate bug on file for that, which will involve shipping 'es', just as we do 'fr'. 

Essentially: neither Gecko nor Android thinks that "es-ES" is a fallback for "es-US", because that's not how locale codes work. That means if your phone is set to US Spanish, you see English Fennec. The right answer is to always ship roots for the tree. 

It's possible that we could use locale switching to implement a fallback tree to address a broader set of locales without having to build and ship them, but the experience wouldn't be as good. We could recognize that we don't support the system locale, and try to offer something else, but that becomes increasingly irrelevant as we ship more locales than Android. 

Again, Jeff, please weigh in. 

> 3) follow-up for language chooser UI telemetry, please.  This is an amazing
> feature; I want to know how much use it gets.

Don't worry, I already promised mfinkle :) I should have that ready when this lands. 


> 4) I wonder how we can reach out to inform our localizer population about
> this feature that they would likely be interested in.

Jeff's reaching out to them to help test this, as far as I know. I expect a flurry of blog posts when 32 hits Aurora!

Comment 77

[Richard Newman [:rnewman]]

Attachment: Part 5: UI telemetry for locale switching. v1

Building and testing this now. I also took the liberty of documenting the existing events more thoroughly, in the docs. Where docs are supposed to be.

Note that I'm capturing this from GeckoPreferences, in the spirit of (a) recording the user activity, not the effect, and (b) avoiding adding coupling to an implementation of LocaleManager.

Note also that we can get a measure of users who use locale switching via FHR: their app locale and OS locale will differ.

Comment 78

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #76)
 
> We have a separate bug on file for that, which will involve shipping 'es',
> just as we do 'fr'. 

Bug 933315.

Comment 79

[Richard Newman [:rnewman]]

Attachment: Part 6: tablet mode. WIP.

These are the necessary tweaks to get things 99% working on a full-size tablet.

The one remaining blocking bug: when we redisplay GeckoPreferences, we refetch the title for the currently selected section, and at redisplay time that's not valid, so the title becomes "Fennec rnewman" (or "Firefox").

A nice to have: when we redisplay preferences, we could instead regenerate the prefs list and reload the fragment, which might solve the above and would also kill some flicker.

I'll finish this up today and tomorrow.

Comment 80

[Richard Newman [:rnewman]]

Tablet status update:

After an indescribably long slog, I've got this to a half-decent point on tablet, without finishing the prefs activity as I did in Comment 79.

The primary issues are around precisely when fragments are inflated, and coordinating between the action bar 'title', the real title (which sometimes applies, and sometimes doesn't!), and invalidation.

By employing various attacks on these (skipping parts of ListPreference, detaching and reattaching the fragment, setting titles directly, and a workaround for an Android selection bug), the end result: everything works apart from the greyed-out pseudo-title duplicate header at the top of the prefs fragment.

Nothing I do can make that show the correct title without user intervention. Various approaches screw the "Settings" title at the top of the screen, do nothing at all (like setting the title!), or cause other bizarre bugs, like only one of the headers in the list actually having a label and everything else being white.

Given that it's redundant, I'm inclined to try to remove those header titles. Thoughts?

The alternatives are that we ship with that visual wart, we spend more time trying to figure it out, or we polish the bigger hammer (Comment 79).

I'll clean up this variant, then try the latter.

Comment 81

[Richard Newman [:rnewman]]

Today's status update:

Thanks to a pointer from Chenxia, I figured out how to set the breadcrumb title for the fragment. After some cleanup, then, it was time to flip back to testing on the phone.

Inexplicably, I'm now getting duplicate GeckoPreferences somehow being created on my behalf, once for each additional time you pick a locale -- you have to hit back more than once when you're done.

Whack-a-mole.

Meanwhile, Nick is making progress in Bug 1004556.

Comment 82

[Richard Newman [:rnewman]]

Attachment: Part 5: UI telemetry for locale switching. v2

Also organizes the contract file and adds some docs. Sorry for the overlap, but I didn't want to wrangle the patch stack any further.

Comment 83

[Richard Newman [:rnewman]]

Attachment: Part 6: tablet mode. WIP. v2

Saving my place. This works fine on tablet, but needs a little more refinement on the phone.

Comment 84

[Richard Newman [:rnewman]]

Attachment: Part 6: tablet mode. v3

This is the minimal set of changes required to get all the titles correct and correct back stack behavior on both tablet and phone. Haven't tested 2.3 yet, so a Part 7 might yet follow.

Comment 85

[Richard Newman [:rnewman]]

Note to self: now that Bug 1004556 has landed, Part 2d (Part 11 in the original attachment series) needs to be reworked.

Also: review ping, mfinkle :)

Comment 86

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #85)
> Note to self: now that Bug 1004556 has landed, Part 2d (Part 11 in the
> original attachment series) needs to be reworked.

I'm working on this as we speak.

Comment 87

[Richard Newman [:rnewman]]

Attachment: Part 2d for rework

Comment 88

[Aaron Train [:aaronmt]]

We'll want tests around this :-)

Comment 89

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8417810 [details] [diff] [review]
Part 5: UI telemetry for locale switching. v2

>diff --git a/mobile/android/base/TelemetryContract.java b/mobile/android/base/TelemetryContract.java

>+        public static final String BROWSER_LOCALE_RESET = "browser.locale.reset.1";
>+        public static final String BROWSER_LOCALE_SELECTED = "browser.locale.selected.1:";

* Why use the "browser." prefix? I'd drop it unless we have other "locale" Events to think about.
* Why append the locale to the Event name and not send as an Extra. I now notice that "policynotification.success" does something similar, but seems odd.

>-        // Stop holding a resource (reader, bookmark, etc) for viewing later.
>-        // Note: Only used in JavaScript for now, but here for completeness.
>-        public static final String UNSAVE = "unsave.1";

You missed adding "unsave" back

Comment 90

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #89)

> * Why use the "browser." prefix? I'd drop it unless we have other "locale"
> Events to think about.

Content locale, system locale. There's an argument for using locale.browser; up to you.

> * Why append the locale to the Event name and not send as an Extra. I now
> notice that "policynotification.success" does something similar, but seems
> odd.

Three reasons:

1. It's not an extra, it's a core part.
2. If we want to check the selection of an individual locale, it's easier than checking two things.
3. It's easy enough to do a prefix check when we don't.


> You missed adding "unsave" back

Good catch!

Comment 91

[Richard Newman [:rnewman]]

Attachment: Part 5: UI telemetry for locale switching. v3

Unbitrotted, restored unsave.

Comment 92

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8419157 [details] [diff] [review]
Part 6: tablet mode. v3

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

So this is a pretty rubbery stamp on this stuff.  I have questions, but most are of little consequence.  It's pretty much impossible to read this sequence of patches and be confident that it works as intended; I think we land and deal with fallout as it happens.

::: mobile/android/base/BrowserApp.java
@@ +1782,5 @@
>                  @Override
>                  public void run() {
>                      final LocaleManager localeManager = BrowserLocaleManager.getInstance();
>                      final Locale locale = localeManager.getCurrentLocale(getApplicationContext());
>                      Log.d(LOGTAG, "Persisted locale was " + locale);

nit: this can be read as "I persisted ..." and as "I read the persisted ...".  Prefer active voide: "Read persisted locale...".

::: mobile/android/base/preferences/GeckoPreferenceFragment.java
@@ +64,5 @@
>       *
>       * We only return titles for the preference screens that are in the
>       * flow for selecting a locale, and thus might need to be redisplayed.
> +     *
> +     * This method sets the title that you see on non-multi-panel devices.

s/panel/pane/

@@ +89,3 @@
>  
> +        final PreferenceActivity activity = (PreferenceActivity) getActivity();
> +        final boolean isMultiPane = activity.isMultiPane();

nit: inline this; it's small, and there's a comment in the if block.

@@ +153,1 @@
>              resid = isMultiPane ? R.xml.preferences_customize_tablet : R.xml.preferences;

I'm guessing that this doesn't change behaviour, because by the time onResume runs |isMultiPane| agrees with |onIsMultiPane|?  Can you elaborate if necessary?

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +70,5 @@
>  import android.widget.ListView;
>  import android.widget.Toast;
>  
>  public class GeckoPreferences
> +extends PreferenceActivity

nit: funky indenting is funky.

@@ +161,5 @@
> +        // If we're a multi-pane view, the activity title is really
> +        // the header bar above the fragment.
> +        // Find out which fragment we're showing, and use that.
> +        if (isMultiPane()) {
> +            int title = getIntent().getIntExtra(EXTRA_SHOW_FRAGMENT_TITLE, -1);

What does this do when title = -1?

@@ +194,5 @@
> +
> +            // Detach and reattach the current prefs pane so that it
> +            // reflects the new locale.
> +            final FragmentManager fragmentManager = getFragmentManager();
> +            int id = getResources().getIdentifier("android:id/prefs", null, null);

Ooh, this is gnarly.

@@ +209,5 @@
> +
> +            // Because Android just rebuilt the activity itself with the
> +            // old language, we need to update the top title and other
> +            // wording again.
> +            if (onIsMultiPane()) {

These guards seem weird.  You're in an |isMultiPane| block (so we're showing multiple panes) and you're checking if you should show multiple panes?  Can this change on a locale change?  Can we just set the action bar title no matter what?

@@ +348,5 @@
>              String resourceName = intentExtras.getString(INTENT_EXTRA_RESOURCES);
>              fragmentArgs.putString(INTENT_EXTRA_RESOURCES, resourceName);
> +
> +            // TODO: need to update title here?
> +            // updateTitle("AA: " + getString(R.string.pref_header_language));

Does this need to be elaborated upon, or removed?

@@ +359,5 @@
>              }
> +
> +            // We're being created, there's no resource specified,
> +            // and the fragment isn't going to reload after us? It
> +            // must be a locale change, so we must do these two things.

What two things?  "So we must set both our title and our ActionBar title."

@@ +408,5 @@
>      @Override
>      public void onPause() {
> +        // Symmetric with onResume.
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB) {
> +            if (isMultiPane()) {

Can you explain the difference between |isMultiPane| and |onIsMultiPane|?  |isMultiPane| == current state, |onIsMultiPane| == future state?

@@ +475,5 @@
>          switch (requestCode) {
>            case REQUEST_CODE_PREF_SCREEN:
> +              switch (resultCode) {
> +              case RESULT_CODE_EXIT_SETTINGS:
> +                  updateActionBarTitle(R.string.settings_title);

At first, I didn't understand this: we're about to finish our activity, why update anything; and why only the action bar?  But you're going to tell me that this is the special GeckoPreferences contract, where it sniffs a locale change and does special things; and one of those things is that the action bar will persist, so needs to be updated.  Correct?

@@ +854,5 @@
>      }
>  
> +    /**
> +     * Implementation for the {@link OnSharedPreferenceChangeListener} interface,
> +     * which we use to watch changes in our prefs file.

Include that we might not always be registered as a listener; or include the same registration check in the onSPC, to make it clear that this is special.

Is it reasonable to *always* do this listening, rather than conditioning on tablet?  /me prefers a single control flow.

@@ +1005,1 @@
>                         .setPositiveButton(R.string.button_ok, new DialogInterface.OnClickListener() {  

nit: kill trailing ws throughout.

Comment 93

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #92)

> >              resid = isMultiPane ? R.xml.preferences_customize_tablet : R.xml.preferences;
> 
> I'm guessing that this doesn't change behaviour, because by the time
> onResume runs |isMultiPane| agrees with |onIsMultiPane|?  Can you elaborate
> if necessary?

Yes. onIsMultiPane is basically a screen size decision, whereas isMultiPane also reflects whether the header list is visible. In this case I think we care about the UI that's actually being shown (isMultiPane), not whether the screen is big enough for multiple panes (onIsMultiPane).

Some of this was, as with previous patches, experimental: I found some activity lifecycle oddities that went away when I changed to what appeared to be a more suitable predicate, so I didn't push the matter too much.


> >  public class GeckoPreferences
> > +extends PreferenceActivity
> 
> nit: funky indenting is funky.

This was what Eclipse picked for me :)


> > +            int title = getIntent().getIntExtra(EXTRA_SHOW_FRAGMENT_TITLE, -1);
> 
> What does this do when title = -1?

Throw a ResourceNotFoundException. I don't think that's a bad thing (it didn't happen in testing, and it would let us find problems), but we can revisit prior to Beta.


> > +            // Detach and reattach the current prefs pane so that it
> > +            // reflects the new locale.
> > +            final FragmentManager fragmentManager = getFragmentManager();
> > +            int id = getResources().getIdentifier("android:id/prefs", null, null);
> 
> Ooh, this is gnarly.

Yeah, tell me about it.

Our ability to manipulate the displayed fragment is very limited. I tried the tagging approach, and others, and none of them were reliable (if they worked at all).

> 
> @@ +209,5 @@
> > +
> > +            // Because Android just rebuilt the activity itself with the
> > +            // old language, we need to update the top title and other
> > +            // wording again.
> > +            if (onIsMultiPane()) {
> 
> These guards seem weird.  You're in an |isMultiPane| block (so we're showing
> multiple panes) and you're checking if you should show multiple panes?  Can
> this change on a locale change?  Can we just set the action bar title no
> matter what?

I thought you'd pick up on this :)

isMultiPane is sometimes true when onIsMultiPane is not -- that is, without that guard, this supposedly tablet-only code will run on a KitKat phone with an xhdpi screen, which will stomp all over the titles being shown. Which is odd.

Most likely this means that the enclosing isMultiPane guard can just go (if the code is being run on a phone regardless!), and thus the only care we have to take is around handling of the action bar, but I'm holding off on doing that until I can test on 2.3.


> > +            // TODO: need to update title here?
> > +            // updateTitle("AA: " + getString(R.string.pref_header_language));
> 
> Does this need to be elaborated upon, or removed?

Removed. Forgot to delete it in my cleanup pass.


> Can you explain the difference between |isMultiPane| and |onIsMultiPane|? 
> |isMultiPane| == current state, |onIsMultiPane| == future state?

Explained above. Further discussion:

http://commonsware.com/blog/2012/10/16/conditional-preference-headers.html

public boolean onIsMultiPane()
Called to determine if the activity should run in multi-pane mode. The default implementation returns true if the screen is large enough.

public boolean isMultiPane()
Returns true if this activity is showing multiple panes -- the headers and a preference fragment.
 
> @@ +475,5 @@
> >          switch (requestCode) {
> >            case REQUEST_CODE_PREF_SCREEN:
> > +              switch (resultCode) {
> > +              case RESULT_CODE_EXIT_SETTINGS:
> > +                  updateActionBarTitle(R.string.settings_title);
> 
> At first, I didn't understand this: we're about to finish our activity, why
> update anything; and why only the action bar?  But you're going to tell me
> that this is the special GeckoPreferences contract, where it sniffs a locale
> change and does special things; and one of those things is that the action
> bar will persist, so needs to be updated.  Correct?

This is a lifecycle thing, as I recall. Different parts of this patch affect phone, tablet, or both; this is one that affects one of those, after things have been inflated, in a way that doesn't trample the other.

(Common symptom: move that line somewhere else, and "Locale" changes to "Settings" when you choose a locale. Or "Settings" changes to "Locale", or shows in the wrong language. This was more trial and error than I would have preferred.)


> Is it reasonable to *always* do this listening, rather than conditioning on
> tablet?  /me prefers a single control flow.

I would too, but alas no. That led to us being called *twice* in some circumstances, which IIRC would for some reason cause Android to create a new GeckoPreferences instance.

Comment 94

[Richard Newman [:rnewman]]

Update:

* Tablet fragment header isn't set when launching straight into Settings from a notification. Will roll up a small extra patch.

* Discussed telemetry with mfinkle today. He has a gentle preference for adding the locale as an extra, to make analysis scripts simpler. I have a few use cases in mind that make tracking the locale you're switching *from*, which also needs to go into an extra.

* Waiting on my 2.3 device so I can test there.

Comment 95

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8420216 [details] [diff] [review]
Part 5: UI telemetry for locale switching. v3

>diff --git a/mobile/android/base/TelemetryContract.java b/mobile/android/base/TelemetryContract.java
>+        public static final String BROWSER_LOCALE_RESET = "browser.locale.reset.1";
>+        public static final String BROWSER_LOCALE_SELECTED = "browser.locale.selected.1:";

Let's flip this to "locale.browser.XXX"

>diff --git a/mobile/android/base/preferences/GeckoPreferences.java b/mobile/android/base/preferences/GeckoPreferences.java
>                     if (null == localeManager.setSelectedLocale(context, newValue)) {
>                         localeManager.updateConfiguration(context, Locale.getDefault());
>                     }
>+                    Telemetry.sendUIEvent("browser.locale.selected:" + newValue);

As we have discussed, I think this approach works fine for finding this one Event in the Telemetry scripts, but makes it harder to run scripts that want to focus on the locale itself. Adding it as an Extra would suffice from what I have seen in scripts so far.

As for ways to also track the locale being unselected, I would suggest a separate Event: "locale.browser.unselected" mirroring the "selected" Event. I would not try to add too much into a single Event.

Comment 96

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #95)
> Comment on attachment 8420216 [details] [diff] [review]
> Part 5: UI telemetry for locale switching. v3
> 
> >diff --git a/mobile/android/base/preferences/GeckoPreferences.java b/mobile/android/base/preferences/GeckoPreferences.java
> >                     if (null == localeManager.setSelectedLocale(context, newValue)) {
> >                         localeManager.updateConfiguration(context, Locale.getDefault());
> >                     }
> >+                    Telemetry.sendUIEvent("browser.locale.selected:" + newValue);
> 
> As we have discussed, I think this approach works fine for finding this one
> Event in the Telemetry scripts, but makes it harder to run scripts that want
> to focus on the locale itself. Adding it as an Extra would suffice from what
> I have seen in scripts so far.

I was not explicit in saying it, but if you want to keep the current appending behavior, in addition to the Extra approach, I'm fine with that.

Comment 97

[Richard Newman [:rnewman]]

Attachment: Parse multilocale.json. v1

This slots in at the end of Part 2. Separate patch for slightly easier review; it'll fold in and replace the bits that it overwrites.

Comment 98

[Richard Newman [:rnewman]]

Woo, minor fixes for 2.2, tested on small and large tablets plus 2.2, and all is good. Nick, just need your signoff on that last patch, then I can fold these up and land.

Comment 99

[Richard Newman [:rnewman]]

QA notes:

This feature allows you to select one of two options:

* To use the system-provided locale, as Fennec has obeyed in previous releases.
* To use one of the locales we ship in Fennec, regardless of system locale.

The option to control this is in Menu > Settings > Language > Browser language.

Testing this feature involves verifying that a change is immediately applied, and that all entry points into the application reflect the selected locale.

Entry points to check:

* Data reporting notification. This launches Settings in the "Mozilla" section. Titles should be correct: on tablet, for example, you should see "Settings" in the top left, and "Mozilla" as a heading. You only get this on first run, so you'll need to Clear Data to get back to a clean slate and test this out.

* Launching the browser. Top sites should be in the correct language, as well as other UI elements.

* Clicking a link from another application.

* Web apps. These will pick up Fennec's UI language, I think. I don't know what the specified behavior should be here.

* Sync settings when accessed via Fennec and via Android Settings > Accounts & sync > Firefox.

* Sent tabs from other devices: the launched notification should be in the last locale you picked.

Notes:

* All of these should work correctly if you kill the browser between runs, or if the browser isn't running (e.g., Sync entry points).

* Error pages and other chrome content should be in the correct locale.

* Setting the browser locale has the side effect of changing your Accept-Language header. You should get pages in non-phone locales sometimes; depends on the site.

* Verify that switching to Android Settings and changing the system locale does the right thing if "System default" is selected, and does the different right thing if a specific locale is selected.

Known issues:

* Tablet headers: Bug 1008461 (might be fixed by these patches, might not), Bug 1008459.

* Sometimes, switching locales in settings and then modifying other settings -- e.g., in Customize > Search -- will cause BrowserApp to not reflect your chosen locale when returning from Settings. I'll track that as a follow-up; I think I know why.

Comment 100

[Richard Newman [:rnewman]]

Attachment: Part 5: UI telemetry for locale switching. v4

Comments addressed.

Comment 101

[Richard Newman [:rnewman]]

Current testing APK:

http://people.mozilla.com/~rnewman/l10n-armv6.apk

Should work fine on all ARM devices.

Comment 102

[Richard Newman [:rnewman]]

Question for Roland (and Jeff and others for that matter!):

What kind of user documentation would be valuable for this feature? I'm going to write up something small for release notes, and come up with some strawman wording for a snippet, but should we have a guide written in advance?

And if so, should I write that or do we have better folks for the job?

Comment 103

[Richard Newman [:rnewman]]

Draft relnote, for when this lands:

  You can now use Firefox for Android in any one of our 38 (??) supported languages, regardless of your Android language, and switch without restarting your browser. Just open the menu and go to Settings > Language.

Setting relnote-firefox per <https://wiki.mozilla.org/Release_Management/Relnotes_rules>.

Comment 104

[Sylvestre Ledru [:Sylvestre]]

Richard, Sentences are not allowed in the release notes. Could you propose an other formulation?

Comment 105

[Richard Newman [:rnewman]]

  Switch between any of Firefox for Android's 40 supported languages without leaving the browser

Comment 106

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #105)
>   Switch between any of Firefox for Android's 40 supported languages without
> leaving the browser

And that'll actually be 49 in Firefox 32, not 40.

Comment 107

[Jeff Beatty [:gueroJeff]]

(In reply to Richard Newman [:rnewman] from comment #76)
> (In reply to Nick Alexander :nalexander from comment #75)
> > rnewman: questions, and requests for follow-ups:
> > 
> > 1) the list of shipped languages will certainly grow, and may contract, over
> > time.  Bitter experience dictates that we bake in a (plan for a) migration
> > story up front.  Can we message that the list of locales has expanded?  Only
> > when there may be a better fit (i.e., user is using es-ES, but system locale
> > is different (es-US?))?  What happens when the user's chosen locale goes
> > away?  Should we timestamp the user's pref selection, to ease upgrade
> > handling, and maybe to note that "no selection" is different than "selected
> > system default"?
> 
> Yeah, I've been thinking about this. My feeling is that removal isn't a big
> problem; I don't think we have a long history of pulling locales once we add
> them to maemo-locales, and this is something I think we can retroactively
> handle with a little extra code in BrowserLocaleManager at the time. (Is
> this locale present in the list? If not, throw a notification.) 
> 
> We might already record system default in a discrete way: empty string
> versus no pref. But that's a good idea.
> 
> Messaging on growth is a thorny one, which is really a milder case of just
> landing this feature! How does one find out that a user might be better
> suited by another locale? I'm interested in Jeff's thoughts on that. 
WRT removing locales, to this point, we have not had to remove locales from maemo-locales, and I don't anticipate we will need to in the future. That being said, the strategy for removing locales here would be similar to the process we follow for removing locales from the Firefox desktop build line-up, which involves a lot of community outreach and support and only results in removal as a last resort.

Messaging is a thorny issue. It's likely that a lot of that will be dependent upon the local community to promote their locale. Geographically targeted snippets could also be a good means for announcing the availability of other localizations within a particular region. We often have the same challenge for increasing awareness of availability for new locales (or simply minority locales) on Firefox desktop. Those methods have worked for us in the past and could work for us in the future. It would be good also to collaborate with Product Marketing to explore other avenues.
> 
> > 
> > 2) I think you've already considered fallbacks: you've talked about how we
> > ship es-ES, which doesn't become the default when you're running es-US.  Can
> > you brain dump what exactly the issue is, and what it would take to default
> > to es-* when system locale is es-*?
> 
> We have a separate bug on file for that, which will involve shipping 'es',
> just as we do 'fr'. 
> 
> Essentially: neither Gecko nor Android thinks that "es-ES" is a fallback for
> "es-US", because that's not how locale codes work. That means if your phone
> is set to US Spanish, you see English Fennec. The right answer is to always
> ship roots for the tree. 
> 
> It's possible that we could use locale switching to implement a fallback
> tree to address a broader set of locales without having to build and ship
> them, but the experience wouldn't be as good. We could recognize that we
> don't support the system locale, and try to offer something else, but that
> becomes increasingly irrelevant as we ship more locales than Android. 
> 
> Again, Jeff, please weigh in. 

Well said rnewman. Providing base language support for locales by using lang codes instead of full lang-region locale codes will allow us to cover our bases here, which is part of best practices (bp) for locale codes. In addition, as Android increases its own locale support, more and more of these locales will become part of the system default lang switching classification, so this will be less of a problem. To be honest, I think es is an isolated incident, as we already follow the locale code bp for all other locales.
> 
> > 3) follow-up for language chooser UI telemetry, please.  This is an amazing
> > feature; I want to know how much use it gets.
> 
> Don't worry, I already promised mfinkle :) I should have that ready when
> this lands. 
> 
As do I :-)
> 
> > 4) I wonder how we can reach out to inform our localizer population about
> > this feature that they would likely be interested in.
> 
> Jeff's reaching out to them to help test this, as far as I know. I expect a
> flurry of blog posts when 32 hits Aurora!

Yes, I will be making an announcement to the l10n community this week, now that things are really wrapping up here. They will be thrilled!

As for your question about documentation, I do think that there needs to be some documentation readily available on SUMO, as I anticipate this being a widely used feature.

Comment 108

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8421251 [details] [diff] [review]
Parse multilocale.json. v1

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

Fine, but I'd much prefer the fallback logic be in the BLM.

::: mobile/android/base/BrowserLocaleManager.java
@@ +296,5 @@
>      }
> +
> +    /**
> +     * Examines <code>multilocale.json</code>, returning the included list of
> +     * locale codes.

nit: trailing ws.

@@ +312,5 @@
> +     *              "pt-PT", "ro", "ru", "sk", "sl", "sv-SE", "th",
> +     *              "tr", "uk", "zh-CN", "zh-TW", "en-US"]}
> +     * </code>
> +     */
> +    public static Set<String> getPackagedLocaleCodes(final Context context) {

Collection?  And shouldn't this be exposed as part of the LocaleManager interface?

::: mobile/android/base/preferences/LocaleListPreference.java
@@ +67,5 @@
> +        Set<String> shippingLocales = BrowserLocaleManager.getPackagedLocaleCodes(getContext());
> +
> +        // Future: single-locale builds should be specified, too.
> +        if (shippingLocales == null) {
> +            return new LocaleDescriptor[] { new LocaleDescriptor(Locale.US, "en-US") };

My PUT is that this fallback locale should be part of the BLM interface, not the consumer.  And no matter what, "en-US" should be a constant, like FALLBACK_LOCALE or something.

Comment 109

[Aaron Train [:aaronmt]]

From a set of different eyes playing around for a bit with the APK posted

Nit(s):

Must we localize the actual language selection list? This differs from how Android system language changes in that if I swap to Japanese, I at least know that I can find my way back to English. Can we keep the list in English?

Comment 110

[Kevin Brosnan [Ex-Mozilla]]

Android does not localize the language list. They display each language in the form native readers expect to see. ex German is never shown if the user is in English, the language is always displayed as Deutsch, 日本語 instead of Japanese and etc.

Comment 111

[Julen Ruiz Aizpuru]

Yes, please let the list in the form native readers would expect to read, otherwise some people might not even find their own languages, which would defeat the purpose of this bug.

Also, there doesn't seem to be any sorting of the language list. FWIW, mozilla.org's language picker uses the locale code as a sort key.

Comment 112

[Richard Newman [:rnewman]]

(In reply to Julen Ruiz Aizpuru from comment #111)
> Yes, please let the list in the form native readers would expect to read,
> otherwise some people might not even find their own languages, which would
> defeat the purpose of this bug.

Anyone else have an opinion on this? Jeff?

> Also, there doesn't seem to be any sorting of the language list. FWIW,
> mozilla.org's language picker uses the locale code as a sort key.

It's sorted, but of course whether the sorting makes any sense to you depends on whether that aligns with your expectations!

Comment 113

[Théo Chevalier]

(In reply to Richard Newman [:rnewman] from comment #112)
> (In reply to Julen Ruiz Aizpuru from comment #111)
> > Yes, please let the list in the form native readers would expect to read,
> > otherwise some people might not even find their own languages, which would
> > defeat the purpose of this bug.
> 
> Anyone else have an opinion on this? Jeff?

IMO, we should do the same as we do on Firefox OS: "English" displayed in English, "French" displayed in French, etc.

Comment 114

[chris hofmann]

yeah, that will make the language selection much more recognizable.  we can probably leverage the language list from some already existing one on the web or in products.  ( https://www.mozilla.org/en-US/firefox/all/  )   Axel might know how this works elsewhere.

One problem might be with having enough fonts on-board to properly render some names so we should consider what will happen in those cases.

Comment 115

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #108)

> > +    public static Set<String> getPackagedLocaleCodes(final Context context) {
> 
> Collection?

Done.


> And shouldn't this be exposed as part of the LocaleManager
> interface?

(a) no value in doing so, (b) static. I think of this as being a property of Fennec's own LocaleManager, used by Fennec's locale switching UI. If we need to override this at some point, we can rework it then.


> My PUT is that this fallback locale should be part of the BLM interface, not
> the consumer.  And no matter what, "en-US" should be a constant, like
> FALLBACK_LOCALE or something.

That sounds fair.

Comment 116

[Richard Newman [:rnewman]]

(In reply to chris hofmann from comment #114)
> yeah, that will make the language selection much more recognizable.  we can
> probably leverage the language list from some already existing one on the
> web or in products.  ( https://www.mozilla.org/en-US/firefox/all/  )   Axel
> might know how this works elsewhere.

Java handily does this for us — you can ask for the display representation of a locale in whatever locale you choose.

  …
  suomi
  svenska (Sverige)
  Türkçe
  …

Looks like this (the end of the list):

https://dl.dropboxusercontent.com/u/3911373/Fennec/917480/locale-list.png

Comment 117

[Richard Newman [:rnewman]]

(In reply to Julen Ruiz Aizpuru from comment #111)

> Also, there doesn't seem to be any sorting of the language list. FWIW,
> mozilla.org's language picker uses the locale code as a sort key.

As implemented I'm sorting by native name, relying on the stable US Collator — English before español, Bahasa Indonesia before both, Kanji and other idiographic languages at the end.

FWIW, Android's own locale picker, as best I can tell from the code, also sorts by label.

Not ideal, for sure, but I'm not sure any other list approach is any better.

Is there more prior art (other than mozilla.org)?

Comment 118

[Richard Newman [:rnewman]]

Status: I have a patch queue ready to go with review comments reflected, folded down to 6 parts, looks like Comment 116.

My screencast needs to be updated, and assuming I'm not still down with the flu I'll be hitting the dev docs tomorrow. (If I am, it'll be Monday.)

I plan to land this tonight or tomorrow in fx-team, because we're at the stage where I think bitrotting and lack of wider testing is worse than having to land follow-ups.

We can iterate on user docs and any further changes before the merge to Aurora on June 9th.

Testing is encouraged, and thanks!

Comment 119

[Richard Newman [:rnewman]]

Parts Pre through 6:

   https://hg.mozilla.org/integration/fx-team/rev/8e90d6525389
   https://hg.mozilla.org/integration/fx-team/rev/7cc9eb361a9d
   https://hg.mozilla.org/integration/fx-team/rev/b0819a1afd15
   https://hg.mozilla.org/integration/fx-team/rev/cb8d6f98fbc2
   https://hg.mozilla.org/integration/fx-team/rev/1c25bbb1fd76
   https://hg.mozilla.org/integration/fx-team/rev/84cc0fb8d2f9
   https://hg.mozilla.org/integration/fx-team/rev/7cf6451e1c88
   https://hg.mozilla.org/integration/fx-team/rev/2d5ab443d3c0

Comment 120

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e90d6525389
https://hg.mozilla.org/mozilla-central/rev/7cc9eb361a9d
https://hg.mozilla.org/mozilla-central/rev/b0819a1afd15
https://hg.mozilla.org/mozilla-central/rev/cb8d6f98fbc2
https://hg.mozilla.org/mozilla-central/rev/1c25bbb1fd76
https://hg.mozilla.org/mozilla-central/rev/84cc0fb8d2f9
https://hg.mozilla.org/mozilla-central/rev/7cf6451e1c88
https://hg.mozilla.org/mozilla-central/rev/2d5ab443d3c0

Comment 121

[Sylvestre Ledru [:Sylvestre]]

Added in the release notes with Richard's proposition (comment #105)

Comment 122

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

Added joni savage [e-mail], the SUMO content editor

joni will write the article for Firefox 32

it might be a modification to

https://support.mozilla.org/en-US/kb/change-default-language-firefox-mobile

or it might be a new article

Comment 123

[Richard Newman [:rnewman]]

(In reply to Roland Tanglao :rolandtanglao from comment #122)

> joni will write the article for Firefox 32

Great! Joni, let me know if there's anything you need from me.

Comment 124

[Richard Newman [:rnewman]]

I landed some somewhat-better-than-stub docs:

https://hg.mozilla.org/integration/fx-team/rev/582caed4aacf

Comment 125

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/582caed4aacf