Closed Bug 1011008 Opened 8 years ago Closed 8 years ago

System locale changes not correctly reflected in running Fennec activities


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

Not set


(firefox32 fixed, fennec32+)

Firefox 32
Tracking Status
firefox32 --- fixed
fennec 32+ ---


(Reporter: aaronmt, Assigned: rnewman)




(2 files)

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
Margaret also pointed out 997589.
Snippets expected and entirely unrelated.
Not sure what the real issue is here. Is it a menu issue? Is it a snippet issue?
Assignee: nobody → rnewman
tracking-fennec: ? → 32+
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.
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.
(In reply to Richard Newman [:rnewman] from comment #5)

> The prefs fragment itself correctly changes locale.

More accurately: it changes locale, correctly or not!
Summary: Browser menu/banner not updating to reflect browser language changes → System locale changes not correctly reflected in running Fennec activities
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.
Attachment #8425048 - Flags: review?(nalexander)
Second try at uploading this patch...
Attachment #8425071 - Flags: review?(nalexander)
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/
@@ +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/
@@ +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/
@@ +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/
@@ +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/
@@ +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.
Attachment #8425048 - Flags: review?(nalexander) → review+
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.
Attachment #8425071 - Flags: review?(nalexander) → review+
(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!
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.