1011008 - System locale changes not correctly reflected in running Fennec activities

Status: RESOLVED FIXED

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

Description

[Aaron Train [:aaronmt]]

Currently, the browser menu and banner on about:home (and maybe other areas I haven't seen yet?) do not change based on system language change.

I'm not entirely sure if this is a new issue, but the language selector is set to system-default.

Steps to Reproduce:

* Update build to build having patches in bug 917480 (e.g, 05/15)

* Change system language back to Français

* Open the browser menu, see English. Visit about:home, see the banner in English

* Restart browser,  open the browser menu, see Français. Visit about:home, see the banner in Français

Comment 1

[Aaron Train [:aaronmt]]

Margaret also pointed out 997589.

Comment 2

[Richard Newman [:rnewman]]

Snippets expected and entirely unrelated.

Comment 3

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

Not sure what the real issue is here. Is it a menu issue? Is it a snippet issue?

Comment 4

[Richard Newman [:rnewman]]

I'll investigate after PTO. I didn't test this scenario thoroughly or recently, because it's unimportant: doesn't affect functionality, fixes itself, and exceedingly unlikely to be seen in the wild.

This probably broke in 29 or 30.

Comment 5

[Richard Newman [:rnewman]]

Here's what we get when you change system locale while GeckoPreferences is open, then you hit Back to BrowserApp:

05-19 09:51:25.973 D/GeckoApplication(27079): onConfigurationChanged: es_ES, background: true
05-19 09:51:26.093 D/GeckoApplication(14937): onConfigurationChanged: es_ES, background: false
05-19 09:51:26.458 D/GeckoMemoryMonitor(27079): onTrimMemory() notification received with level 80
05-19 09:51:50.938 W/GeckoBatteryManager(27079): Already started!
05-19 09:51:50.943 D/GeckoPreferenceFragment(27079): Locale changed: es_ES
05-19 09:51:57.578 D/GeckoApp(27079): onConfigurationChanged: es_ES
05-19 09:51:57.608 W/GeckoBatteryManager(27079): Already started!
05-19 09:51:57.613 D/GeckoSessInfo(27079): Recording start of session: 1400518317618


The prefs fragment itself correctly changes locale.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #5)

> The prefs fragment itself correctly changes locale.

More accurately: it changes locale, correctly or not!

Comment 7

[Richard Newman [:rnewman]]

Attachment: Part 1: handle system locale changes correctly. v1

This patch:

* Includes some cleanup, debugging output, and minor fixes.
* Defines and centralizes a new pattern: an onConfigurationChanged handler that (via the LocaleManager) determines whether we're mirroring or not, and either checks to see if we must redisplay (i.e., the system locale changed) or if the config should be overwritten with our selected locale.

I tested this on tablet, and see no reason why it would be different on a phone, but will test there after I get some other stuff done.

Part 2 (to follow shortly) alters the a-s manifests to prevent those activities from misbehaving in certain contexts.

Part 3 (to follow after I write it) alters those activities to also follow the redisplay-on-config-change pattern, if they are allowed to appear in the history at all.

I've written doc fragments for this approach, which I'll integrate into my dev/architecture docs (still pending) this week.

Comment 8

[Richard Newman [:rnewman]]

Part 2 pull request:

https://github.com/mozilla-services/android-sync/pull/459

Comment 9

[Richard Newman [:rnewman]]

Attachment: Part 2: handle system locale changes correctly in background code.

Second try at uploading this patch...

Comment 10

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8425048 [details] [diff] [review]
Part 1: handle system locale changes correctly. v1

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

Tight patch, r=nalexander after you justify this/super question below.

::: mobile/android/base/BrowserApp.java
@@ +100,5 @@
>  import android.view.ViewStub;
>  import android.view.ViewTreeObserver;
>  import android.view.Window;
>  import android.view.animation.Interpolator;
> +import android.widget.ListView;

Meh.  Kill this file's clean-up, or make it a separate patch.

::: mobile/android/base/BrowserLocaleManager.java
@@ +188,5 @@
> +    /**
> +     * If we're currently mirroring the system locale, returns the supplied
> +     * configuration's locale, unless the current activity locale is correct.
> +     *
> +     * If we're not currently mirroring, updates the configuration object to

Please define what mirroring means (perhaps in the class comment), and elaborate on what that means in practice in the predicate |isMirroringSystemLocale|.  Something like,

We say we are "mirroring the system locale" if the user has not explicitly chosen a Firefox-specific locale.  When we are not mirroring, system locale changes do not impact Firefox and are essentially ignored.  When we are mirroring, system locale changes cause Firefox to restart in the new system locale.

::: mobile/android/base/GeckoApp.java
@@ +1247,5 @@
>              @Override
>              public void run() {
>                  final SharedPreferences prefs = GeckoApp.this.getSharedPreferences();
>  
>                  // Wait until now to set this, because we'd rather throw an exception than 

nit: trailing ws.

@@ +1342,2 @@
>          // Allow onConfigurationChanged to take care of the rest.
> +        super.onConfigurationChanged(getResources().getConfiguration());

I don't get it.  Why are we skipping this.onConfigurationChanged?

::: mobile/android/base/LocaleManager.java
@@ +18,5 @@
>      void updateConfiguration(Context context, Locale locale);
>      String setSelectedLocale(Context context, String localeCode);
>      boolean systemLocaleDidChange();
>      void resetToSystemLocale(Context context);
> +    Locale onSystemConfigurationChange(Context context, Resources resources, Configuration configuration, Locale currentActivityLocale);

This interface is no longer trivial... Javadoc needed... help me, Obi-Wan...

::: mobile/android/base/preferences/GeckoPreferenceFragment.java
@@ +42,5 @@
> +
> +        final Activity context = getActivity();
> +
> +        final LocaleManager localeManager = BrowserLocaleManager.getInstance();
> +        final Locale changed = localeManager.onSystemConfigurationChange(context, getResources(), newConfig, lastLocale);

nit: onSystemConfigurationChanged*d*, to mirror the Android name?

@@ +128,5 @@
>  
>      @Override
>      public void onResume() {
> +        applyLocale(Locale.getDefault());
> +        super.onResume();

This was the case before, but doing things (like updating preferences, etc) before calling |super.onResume()| freaks me out.  At the very least, add a comment here and below explaining that this is delicate, and that only work that could be done in |onCreate| is safe.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8425071 [details] [diff] [review]
Part 2: handle system locale changes correctly in background code.

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

Have a rubber-stamp.

Comment 12

[Richard Newman [:rnewman]]

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

> >          // Allow onConfigurationChanged to take care of the rest.
> > +        super.onConfigurationChanged(getResources().getConfiguration());
> 
> I don't get it.  Why are we skipping this.onConfigurationChanged?

For two reasons, one pragmatic and one real.

The pragmatic one is that it allows a loop! onConfigurationChanged can call onLocaleChanged, which calls onLocaleReady, which...

The real reason is that there's nothing in this.onConfigurationChanged that needs to happen. That method does GeckoApp-specific extraction of changes from new config objects:

* Extracts locales out of a config change. Of course we don't need to do that: we're specifically handling locale stuff here.
* Handles screen orientation changes. We're just enforcing a locale change, so that's not relevant, either.
* Invokes super.onConfigurationChanged.

So we just call up to super and move on.

Yes, this can mean that we call super.onConfigurationChanged twice (for the same reason that a loop is possible). That doesn't overly concern me.


Otherwise, will address comments. Thanks for the good review!

Comment 13

[Richard Newman [:rnewman]]

   https://hg.mozilla.org/integration/fx-team/rev/3d6211c85cb5
   https://hg.mozilla.org/integration/fx-team/rev/59f94d1fedda

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d6211c85cb5
https://hg.mozilla.org/mozilla-central/rev/59f94d1fedda