Closed Bug 958171 Opened 6 years ago Closed 6 years ago

HomePager doesn't handle live config changes properly

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: lucasr, Assigned: lucasr)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Gets into a bad state where the title strip doesn't match the current page and the panels don't load their content on swipe.
Comment on attachment 8358495 [details] [diff] [review]
Fix adapter update logic for when HomeConfig changes (r=margaret)

ViewPager (and, more specifically, the fragment-based ones) is a bit weird in that the fragments are owned by the view, not the adapter. So, a notifyDataSetChanged() will not necessary do the right thing for us e.g. destroy existing fragments, force recreation, etc.

So, with this patch, we
1) force fragment destruction by setting a null adapter on the pager
2) Update the existing adapter with the new list of panel configs
3) Re-install the adapter with the final state
4) Set the pager position based on the configured default panel

This patch needs to be rebased on top of changes from bug 958185.
Attachment #8358495 - Flags: review?(margaret.leibovic)
Comment on attachment 8358495 [details] [diff] [review]
Fix adapter update logic for when HomeConfig changes (r=margaret)

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

Tricky, but your explanation makes sense.

I wanted to ask if we still need the setAdapter() call in show(), but if we get rid of that we'll need some other way to get at the adapter we create there. Maybe we should be creating the adapter in updateUiFromPageEntries? But if we do that we'll need the FragmentManager. I think this patch is fine, I'm just thinking aloud here :)
Attachment #8358495 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8358495 [details] [diff] [review]
> Fix adapter update logic for when HomeConfig changes (r=margaret)
> 
> Review of attachment 8358495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tricky, but your explanation makes sense.
> 
> I wanted to ask if we still need the setAdapter() call in show(), but if we
> get rid of that we'll need some other way to get at the adapter we create
> there. Maybe we should be creating the adapter in updateUiFromPageEntries?
> But if we do that we'll need the FragmentManager. I think this patch is
> fine, I'm just thinking aloud here :)

Yeah. The main reason why we have to create the adapter in show() is that we'll have to set the canLoadHint depending on the given animation.
https://hg.mozilla.org/mozilla-central/rev/f56c88f006b2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.