868553 - Make about:home use a ViewPager

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

This will be the first step of the about:home redesign.

Comment 1

[Brian Nicholson (:bnicholson)]

No idea how long this will take, but I'll just throw some numbers out there...

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Implement HomePager for about:home views, WIP

This patch adds a HomePager fragment. The HomePager holds a ViewPager view, and each item in the ViewPager is another fragment. about:home was already a fragment, so HomePager is currently a single-item pager showing the AboutHome fragment.

HomePager is itself a fragment so that we can dynamically attach/detach it when switching between it and the LayerView. It's been frustrating getting this to work, though, and I had to resort to some ugly hacks.

Since we're using nested fragments, we need to use getChildFragmentManager() for the ViewPager. Unfortunately, there's a nasty Android bug that throws an IllegalStateException whenever a parent fragment is reattached: https://code.google.com/p/android/issues/detail?id=42601. After fighting this bug for awhile and looking through the Android source, I ended up resorting to a reflection hack to manually remove the child manager reference whenever HomePager is detached.

Secondly, I've been trying to come up with a good way for BrowserApp to communicate directly with the AboutHome fragment. Right now, setLastTabsVisibility() and update() are methods in HomePager; these each pull out the AboutHome fragment from the adapter and called the wrapped method directly on AboutHome. I don't really like doing this, especially because of how messy it is to access children of a ViewPager. FragmentPagerAdapter#getItem(), despite its name, actually instantiates a fragment whenever it's called, so it's not useful if we're trying to get a handle to the actual fragments in the pager. I resorted to storing a reference to the fragment in instantiateItem() so fragments can be retrieved later by index.

These particular methods might not be that important in the end since we're doing a complete refactor, but we'll still probably have cases where BrowserApp needs to update fragments directly, so we should come up with a decent communication pattern. One idea I've considered is having listeners defined in BrowserApp that fragments can register themselves with by casting getActivity(). Another option is having HomePager expose each child fragment directly using something like "mHomePager.getFragment(HomePager.ABOUT_HOME)". I'll post another patch exploring other options.

Finally, regarding the name: I felt that this merged about:home/AwesomeBar multi-page beast we're creating is more than just an about:home page; it's now the primary access point for the browser. So rather than calling this AboutHomePager, I just named it HomePager with the idea that we could call this new view "home" rather than "about:home".

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Implement HomePager for about:home views, v1

This patch exposes the Fragments in the ViewPager via a getPage() method. This allows BrowserApp to call Fragment methods directly without having to duplicate each method in HomePager.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 746679 [details] [diff] [review]
Implement HomePager for about:home views, v1

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

I'm a bit worried about using nested fragments. It's just seems plain broken in many subtle ways. You found one bug but see, for example, http://delyan.me/04-28-2013/android-s-matryoshka-problem/. If you google "nested fragments" you'll find even more reports of weird behaviour on nested fragments.

Maybe HomePager should just be a custom ViewPager with a method load() and unload(). In load(), you can set the adapter to the HomePager which will then trigger the creation of the inner fragments as expected. In unloaded(), you can use setAdapter(null) and it will internally remove all fragments attached to the ViewPager using proper FragmentTransactions. This way we can avoid using getChildFragmentManager() entirely. The drawback would be that we'd have to keep a ViewPager instance around but this shouldn't be problem as long as we don't keep any pages in memory when about:home is not visible.

I found this SO question which seems to be exactly what I'm talking about here:
http://stackoverflow.com/questions/12765152/programmatically-removing-a-viewpager-when-are-or-how-can-i-ensure-contained

::: mobile/android/base/home/HomePager.java
@@ +69,5 @@
> +     * @param page Which page to retrieve
> +     * @return Fragment corresponding to given page, or null if not instantiated
> +     */
> +    public Fragment getPage(Page page) {
> +        return mPages.get(page);

I'd prefer to avoid exposing the specific pages inside the HomePager in the API. The reason being that we'll probably lazy load some of the pages which means we should not assume all pages are usable at any given time. So let's keep your original idea of a public API that internally routes the calls to the specific pages. This way we'll have more control over the internal state of the pager and its page fragments.

@@ +161,5 @@
> +            private final Class<?> clss;
> +            private final Bundle args;
> +
> +            TabInfo(Page _page, Class<?> _clss, Bundle _args) {
> +                page = _page;

I'd prefer "this.page = page" instead of this "_" prefix on the arguments.

Comment 6

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #5)
> Comment on attachment 746679 [details] [diff] [review]
> Implement HomePager for about:home views, v1
> 
> Review of attachment 746679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a bit worried about using nested fragments. It's just seems plain broken
> in many subtle ways. You found one bug but see, for example,
> http://delyan.me/04-28-2013/android-s-matryoshka-problem/. If you google
> "nested fragments" you'll find even more reports of weird behaviour on
> nested fragments.
> 
> Maybe HomePager should just be a custom ViewPager with a method load() and
> unload(). In load(), you can set the adapter to the HomePager which will
> then trigger the creation of the inner fragments as expected. In unloaded(),
> you can use setAdapter(null) and it will internally remove all fragments
> attached to the ViewPager using proper FragmentTransactions. This way we can
> avoid using getChildFragmentManager() entirely. The drawback would be that
> we'd have to keep a ViewPager instance around but this shouldn't be problem
> as long as we don't keep any pages in memory when about:home is not visible.
> 
> I found this SO question which seems to be exactly what I'm talking about
> here:
> http://stackoverflow.com/questions/12765152/programmatically-removing-a-
> viewpager-when-are-or-how-can-i-ensure-contained

Well that's unfortunate...hopefully we won't have to retain any state at the ViewPager level.

> ::: mobile/android/base/home/HomePager.java
> @@ +69,5 @@
> > +     * @param page Which page to retrieve
> > +     * @return Fragment corresponding to given page, or null if not instantiated
> > +     */
> > +    public Fragment getPage(Page page) {
> > +        return mPages.get(page);
> 
> I'd prefer to avoid exposing the specific pages inside the HomePager in the
> API. The reason being that we'll probably lazy load some of the pages which
> means we should not assume all pages are usable at any given time. So let's
> keep your original idea of a public API that internally routes the calls to
> the specific pages. This way we'll have more control over the internal state
> of the pager and its page fragments.

I don't understand this point. We aren't assuming anything -- it's documented in getPage() that it can return null if it's not instantiated, and we do null checks after calling the method in BrowserApp. And we'll still have to do similar checks internally for the same reasons.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> (In reply to Lucas Rocha (:lucasr) from comment #5)
> > Comment on attachment 746679 [details] [diff] [review]
> > Implement HomePager for about:home views, v1
> > 
> > Review of attachment 746679 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm a bit worried about using nested fragments. It's just seems plain broken
> > in many subtle ways. You found one bug but see, for example,
> > http://delyan.me/04-28-2013/android-s-matryoshka-problem/. If you google
> > "nested fragments" you'll find even more reports of weird behaviour on
> > nested fragments.
> > 
> > Maybe HomePager should just be a custom ViewPager with a method load() and
> > unload(). In load(), you can set the adapter to the HomePager which will
> > then trigger the creation of the inner fragments as expected. In unloaded(),
> > you can use setAdapter(null) and it will internally remove all fragments
> > attached to the ViewPager using proper FragmentTransactions. This way we can
> > avoid using getChildFragmentManager() entirely. The drawback would be that
> > we'd have to keep a ViewPager instance around but this shouldn't be problem
> > as long as we don't keep any pages in memory when about:home is not visible.
> > 
> > I found this SO question which seems to be exactly what I'm talking about
> > here:
> > http://stackoverflow.com/questions/12765152/programmatically-removing-a-
> > viewpager-when-are-or-how-can-i-ensure-contained
> 
> Well that's unfortunate...hopefully we won't have to retain any state at the
> ViewPager level.

We can save the ViewPager state in the activity if we need it.

> > ::: mobile/android/base/home/HomePager.java
> > @@ +69,5 @@
> > > +     * @param page Which page to retrieve
> > > +     * @return Fragment corresponding to given page, or null if not instantiated
> > > +     */
> > > +    public Fragment getPage(Page page) {
> > > +        return mPages.get(page);
> > 
> > I'd prefer to avoid exposing the specific pages inside the HomePager in the
> > API. The reason being that we'll probably lazy load some of the pages which
> > means we should not assume all pages are usable at any given time. So let's
> > keep your original idea of a public API that internally routes the calls to
> > the specific pages. This way we'll have more control over the internal state
> > of the pager and its page fragments.
> 
> I don't understand this point. We aren't assuming anything -- it's
> documented in getPage() that it can return null if it's not instantiated,
> and we do null checks after calling the method in BrowserApp. And we'll
> still have to do similar checks internally for the same reasons.

Well, I'd prefer to do one null check behind a well-defined API than spreading null checks every time you need to do something on a page in HomePager. Clients of HomePager shouldn't need to care about the state of the pages.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: Implement HomePager for about:home views, v2

With suggested changes.

Comment 11

[Brian Nicholson (:bnicholson)]

Fixing hours worked entry, sorry for bugspam.

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 747604 [details] [diff] [review]
Implement HomePager for about:home views, v2

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

Looks great overall but needs work on the unload() method I'm afraid.

::: mobile/android/base/home/HomePager.java
@@ +44,5 @@
> +     * Loads and initializes the pager.
> +     *
> +     * @param fm FragmentManager for the adapter
> +     */
> +    public void load(FragmentManager fm) {

I know I suggested the "load" name but now I wonder if "show" would better match the semantics here.

@@ +46,5 @@
> +     * @param fm FragmentManager for the adapter
> +     */
> +    public void load(FragmentManager fm) {
> +        mLoaded = true;
> +        TabsAdapter adapter = new TabsAdapter(fm);

Can't you just create the adapter once and reuse it on following load() calls? Something like:

if (mAdapter == null) {
    mAdapter = new TabsAdapter(fm);
    mAdapter.addTab(Page.ABOUT_HOME, AboutHome.class, null);
}
setAdapter(mAdapter);

@@ +48,5 @@
> +    public void load(FragmentManager fm) {
> +        mLoaded = true;
> +        TabsAdapter adapter = new TabsAdapter(fm);
> +        setAdapter(adapter);
> +        adapter.addTab(Page.ABOUT_HOME, AboutHome.class, null);

Maybe add the tab before setting the adapter? addTab() calls notifyDataSetChanged() which will cause extra work on the ViewPager just after the adapter was set.

@@ +55,5 @@
> +
> +    /**
> +     * Hides the pager and removes all child fragments.
> +     */
> +    public void unload() {

Same here. Maybe "hide" would be a better name?

@@ +58,5 @@
> +     */
> +    public void unload() {
> +        mLoaded = false;
> +        setVisibility(GONE);
> +        setAdapter(null);

When you set adapter to null here, ViewPager will call destroyItem() on each of its current items. IIRC, FragmentPagerAdapter's destroyItem() simply detaches the fragment but the fragment instance is kept around in the fragment manager to be reattached if you go back to that page. 

You'll have to run a FragmentTransaction just after setAdapter(null) to actually remove all those fragments.

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Implement HomePager for about:home views, v3

(In reply to Lucas Rocha (:lucasr) from comment #12)
> Comment on attachment 747604 [details] [diff] [review]
> Implement HomePager for about:home views, v2
> 
> Can't you just create the adapter once and reuse it on following load()
> calls? Something like:
> 
> if (mAdapter == null) {
>     mAdapter = new TabsAdapter(fm);
>     mAdapter.addTab(Page.ABOUT_HOME, AboutHome.class, null);
> }
> setAdapter(mAdapter);

I didn't want to do this for a couple of reasons:

1) The adapter implementation may hold onto resources. For instance, FragmentPagerAdapter holds a reference to the current fragment, which it never releases: http://androidxref.com/4.0.4/xref/frameworks/support/v4/java/android/support/v4/app/FragmentPagerAdapter.java#130

2) Keeping the TabsAdapter around holds onto the list of tabs and their initial bundle data, but we'll probably want to add tabs using different bundle arguments on different occasions.
 
> @@ +58,5 @@
> > +     */
> > +    public void unload() {
> > +        mLoaded = false;
> > +        setVisibility(GONE);
> > +        setAdapter(null);
> 
> When you set adapter to null here, ViewPager will call destroyItem() on each
> of its current items. IIRC, FragmentPagerAdapter's destroyItem() simply
> detaches the fragment but the fragment instance is kept around in the
> fragment manager to be reattached if you go back to that page. 
> 
> You'll have to run a FragmentTransaction just after setAdapter(null) to
> actually remove all those fragments.

For now, I just fixed this by using FragmentStatePagerAdapter so that fragments will be both detached and removed. We should use FragmentStatePagerAdapter anyway so that we force ourselves to correctly save the state as we implement each fragment. Of course, we can always go back to a FragmentPagerAdapter later if we find that memory usage isn't an issue. Filed bug 871021 to investigate this.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Implement HomePager for about:home views, v4

Removed unused class variable.

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 748258 [details] [diff] [review]
Implement HomePager for about:home views, v4

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

Looks good with the removal of mLoaded HomePager (unless you have a pretty good reason to keep it).

::: mobile/android/base/home/HomePager.java
@@ +45,5 @@
> +     *
> +     * @param fm FragmentManager for the adapter
> +     */
> +    public void show(FragmentManager fm) {
> +        mLoaded = true;

Do you still need mLoaded at all? I mean, you can easily track the visibility state now by simply using getVisibility() directly on the home pager view.

@@ +65,5 @@
> +    /**
> +     * @return Whether the pager and its fragments are being displayed
> +     */
> +    public boolean isVisible() {
> +        return mLoaded;

Simply return (getVisibility() == View.VISIBLE)?

Comment 16

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> Comment on attachment 748258 [details] [diff] [review]
> Implement HomePager for about:home views, v4
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +45,5 @@
> > +     *
> > +     * @param fm FragmentManager for the adapter
> > +     */
> > +    public void show(FragmentManager fm) {
> > +        mLoaded = true;
> 
> Do you still need mLoaded at all? I mean, you can easily track the
> visibility state now by simply using getVisibility() directly on the home
> pager view.

Unfortunately, we have a number of calls to getVisibility() that are off the UI thread (I think they're all related to the dynamic toolbar). The Android documentation states that View is single threaded, so all methods on View must be called on the UI thread (notice that mViewFlags is not volatile, and accesses to it aren't synchronized: http://androidxref.com/4.0.4/xref/frameworks/base/core/java/android/view/View.java#2046).

Comment 17

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/projects/fig/rev/f8d0784186b7

Comment 19

[Ed Morley [:emorley]]

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