951054 - PageEntry strings should be recomputed when locale changes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Richard Newman [:rnewman]]

Attachment: Screenshot.

https://hg.mozilla.org/mozilla-central/rev/fe13b62902c8 introduced this regression; PageItems hold on to a PageEntry, and the PageEntry is populated from a Parcel, including its title.

The title either needs to be dynamically read from a resource, or this needs to be re-inflated when the locale changes.

Comment 1

[:Margaret Leibovic]

I can own this. Sorry for missing that in my review!

Comment 2

[:Margaret Leibovic]

Regression in 29, don't want this to slip through the cracks.

Comment 3

[:Margaret Leibovic]

I think that we should re-load the home config when the locale changes, since this will give us more flexibility for the future when we also have third party pages that may want to be localized.

We're actually already telling the HomePager to redisplay itself when the locale changes:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1628

However, this only happens if the home pager is current visible, so we'll probably want to create a separate method that always gets called to say "mHomePager, go reset your config loader".

Comment 4

[:Margaret Leibovic]

Attachment: loader.diff

I don't think there's a big rush to land this fix, so I think lucasr should review it when he gets back from vacation.

Thinking about this more, perhaps a better approach would be to have some `handleLocaleReady` method on HomePager that takes care of this config reloading and redisplaying if necessary, just for the sake of modularity. But this patch fixes the issue, and I'm about to go on vacation myself :)

Oops, I accidentally needinfo'ed rnewman before. Clearing!

Comment 5

[Richard Newman [:rnewman]]

Agree with just defining onLocaleReady in HomePager. That's the pattern I was aiming for with onLocaleReady -- cascading through necessary components.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8350433 [details] [diff] [review]
loader.diff

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

::: mobile/android/base/BrowserApp.java
@@ +1618,5 @@
> +
> +        // Restart the home config loader to re-load pages with title strings in the new locale.
> +        if (mHomePager != null) {
> +            mHomePager.restartConfigLoader(getSupportLoaderManager());
> +        }

We shouldn't really leak HomeConfigLoader logic/vocabulary outside the home package. This is an implementation detail. This code should probably live behind the redisplay() call below.

::: mobile/android/base/home/HomePager.java
@@ +161,2 @@
>      public void redisplay(LoaderManager lm, FragmentManager fm) {
>          final HomeAdapter adapter = (HomeAdapter) getAdapter();

You should probably change the show() method to take some sort of argument to trigger a reload *instead* of init. This patch is making a restartLoader() call followed by a initLoader() which will probably cause the Loader callback (i.e. onLoadFinished) to be called twice.

Comment 7

[Lucas Rocha (:lucasr)]

Actually, I missed the case where the locale changes while HomePager is *not* visible. In which case we still have to force HomePager to reload its configuration the next time it shows up.

With that in mind, I'd probably just "schedule" (i.e. mark it as 'dirty' or something) a reload for the next time we show the HomePager. There's no need to reload the configuration immediately every time the locale changes. We should only do so if HomePager is visible.

The other point still stands though: we have to avoid a reloadLoader() followed by a initLoader() call.

Comment 8

[:Margaret Leibovic]

Attachment: patch v2

Good catch, I think I was just rushing to get this done before the holidays :)

What do you think of this?

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8356855 [details] [diff] [review]
patch v2

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

Looks good with the suggested method renaming.

::: mobile/android/base/BrowserApp.java
@@ +1632,5 @@
>      public void onLocaleReady(final String locale) {
>          super.onLocaleReady(locale);
> +
> +        if (mHomePager != null) {
> +            mHomePager.onLocaleReady(getSupportLoaderManager(), getSupportFragmentManager());

Stepping back a bit and considering the big picture for bug 949174 (which includes the locale aspect).

In the long-term, we probably want add to hooks around 'config invalidation' instead of just reloading the HomePager config for the locale-specific case. For example, once we get add-on-based pages, we'll need to invalidate the config when add-ons are updated to a new version to make sure that HomeConfig contains any layout/locale changes defined by the add-ons. That means, in the case of add-ons, that invalidation will involve more than just reloading the config, it will actually need to re-fetch certain data from the add-ons to 'refresh' the configuration accordingly. Keep in mind that HomeConfig will also act as a sort of 'cache' of all the necessary data to show HomePager even when Gecko is not up and running.

So, with that in mind, I'd prefer something like:

mHomePager.invalidate(getSupportLoaderManager(), getSupportFragmentManager());

For now, it will do exactly what your onLocaleReady() is doing. But later we can update it to actually refresh the config with latest data from add-ons. In practice, we'll listen to something like a 'list-addon-updated' message from Gecko and call HomePager.invalidate() in response.

::: mobile/android/base/home/HomePager.java
@@ +46,5 @@
>      private ConfigLoaderCallbacks mConfigLoaderCallbacks;
>  
>      private String mInitialPageId;
>  
> +    // Whether or not we need to restart the loader when we show the HomePager.

mfinkle will love this :-)

Comment 10

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 8356855 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8356855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good with the suggested method renaming.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1632,5 @@
> >      public void onLocaleReady(final String locale) {
> >          super.onLocaleReady(locale);
> > +
> > +        if (mHomePager != null) {
> > +            mHomePager.onLocaleReady(getSupportLoaderManager(), getSupportFragmentManager());
> 
> Stepping back a bit and considering the big picture for bug 949174 (which
> includes the locale aspect).
> 
> In the long-term, we probably want add to hooks around 'config invalidation'
> instead of just reloading the HomePager config for the locale-specific case.
> For example, once we get add-on-based pages, we'll need to invalidate the
> config when add-ons are updated to a new version to make sure that
> HomeConfig contains any layout/locale changes defined by the add-ons. That
> means, in the case of add-ons, that invalidation will involve more than just
> reloading the config, it will actually need to re-fetch certain data from
> the add-ons to 'refresh' the configuration accordingly. Keep in mind that
> HomeConfig will also act as a sort of 'cache' of all the necessary data to
> show HomePager even when Gecko is not up and running.
> 
> So, with that in mind, I'd prefer something like:
> 
> mHomePager.invalidate(getSupportLoaderManager(),
> getSupportFragmentManager());
> 
> For now, it will do exactly what your onLocaleReady() is doing. But later we
> can update it to actually refresh the config with latest data from add-ons.
> In practice, we'll listen to something like a 'list-addon-updated' message
> from Gecko and call HomePager.invalidate() in response.

This sounds like a good idea. A similar issue came up with bug 952311, although I filed that bug to actually update the config itself when new add-ons are added/removed.

> ::: mobile/android/base/home/HomePager.java
> @@ +46,5 @@
> >      private ConfigLoaderCallbacks mConfigLoaderCallbacks;
> >  
> >      private String mInitialPageId;
> >  
> > +    // Whether or not we need to restart the loader when we show the HomePager.
> 
> mfinkle will love this :-)

Meh, it gets the job done :)

Comment 11

[:Margaret Leibovic]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fd040290d044