Closed Bug 951054 Opened 11 years ago Closed 11 years ago

PageEntry strings should be recomputed when locale changes

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

(firefox28 unaffected, firefox29 affected, fennec29+)

RESOLVED FIXED
Firefox 29
Tracking Status
firefox28 --- unaffected
firefox29 --- affected
fennec 29+ ---

People

(Reporter: rnewman, Assigned: Margaret)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image Screenshot. (deleted) —
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.
Blocks: 917480
I can own this. Sorry for missing that in my review!
Assignee: nobody → margaret.leibovic
Regression in 29, don't want this to slip through the cracks.
tracking-fennec: --- → 29+
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".
Flags: needinfo?(rnewman)
Attached patch loader.diff (obsolete) (deleted) — Splinter Review
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!
Attachment #8350433 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(rnewman)
Agree with just defining onLocaleReady in HomePager. That's the pattern I was aiming for with onLocaleReady -- cascading through necessary components.
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.
Attachment #8350433 - Flags: review?(lucasr.at.mozilla) → review-
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.
Attached patch patch v2 (deleted) — Splinter Review
Good catch, I think I was just rushing to get this done before the holidays :) What do you think of this?
Attachment #8350433 - Attachment is obsolete: true
Attachment #8356855 - Flags: review?(lucasr.at.mozilla)
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 :-)
Attachment #8356855 - Flags: review?(lucasr.at.mozilla) → review+
(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 :)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: