Closed Bug 1056225 Opened 7 years ago Closed 7 years ago

Factor out shared pieces of HomeFragment

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: nalexander, Assigned: nalexander)

Details

Attachments

(2 files)

There's some duplication as HomeFragments have grown.  Two I've noticed:

1) Most HomeFragments have:

    // On URL open listener
    private OnUrlOpenListener mUrlOpenListener;

2) Most HomeFragments have:

    @Override
    public void onConfigurationChanged(Configuration newConfig) {
        super.onConfigurationChanged(newConfig);

        // Detach and reattach the fragment as the layout changes.
        if (isVisible()) {
            getFragmentManager().beginTransaction()
                                .detach(this)
                                .attach(this)
                                .commitAllowingStateLoss();
        }
    }

(With variable degrees of commenting.)
I took the most commented version.  There are two things to note here:

* Several panels did not define onConfigurationChanged.  It's not clear
  if these panels didn't need it (after some analysis?) or if they just
  didn't copy-paste thoroughly.  I think this version is always safe, if
  potentially inefficient.

* The order of operations for the Bookmarks panel is delicate.  I
  preserved the original order (save stack first, then detach and
  attach).  I tested that the folder stack was preserved across device
  rotations locally.
Attachment #8476132 - Flags: review?(margaret.leibovic)
(In reply to Nick Alexander :nalexander from comment #2)
> Created attachment 8476132 [details] [diff] [review]
> Part 2: Lift onConfigurationChanged to HomeFragment. r=margaret
> 
> I took the most commented version.  There are two things to note here:
> 
> * Several panels did not define onConfigurationChanged.  It's not clear
>   if these panels didn't need it (after some analysis?) or if they just
>   didn't copy-paste thoroughly.  I think this version is always safe, if
>   potentially inefficient.

A HomeFragment only needs to handle onConfiguration change (i.e. re-attach) if their UI needs to change (i.e. re-inflate layouts, use different styles, etc) for different device orientations. Handling configuration changes in all HomeFragments will simply cause some redundant re-inflations on device rotation. But please double-check any subtle assumptions around the fragment's life-cycle in each panel.

ps: I haven't looked at the patch. Just providing more context here.
(In reply to Lucas Rocha (:lucasr) from comment #3)
> (In reply to Nick Alexander :nalexander from comment #2)
> > Created attachment 8476132 [details] [diff] [review]
> > Part 2: Lift onConfigurationChanged to HomeFragment. r=margaret
> > 
> > I took the most commented version.  There are two things to note here:
> > 
> > * Several panels did not define onConfigurationChanged.  It's not clear
> >   if these panels didn't need it (after some analysis?) or if they just
> >   didn't copy-paste thoroughly.  I think this version is always safe, if
> >   potentially inefficient.
> 
> A HomeFragment only needs to handle onConfiguration change (i.e. re-attach)
> if their UI needs to change (i.e. re-inflate layouts, use different styles,
> etc) for different device orientations. Handling configuration changes in
> all HomeFragments will simply cause some redundant re-inflations on device
> rotation. But please double-check any subtle assumptions around the
> fragment's life-cycle in each panel.

Yep, that's my understanding.  I think I'll fold a version of your comment into the HomeFragment.onConfigurationChanged docstring.
Comment on attachment 8476132 [details] [diff] [review]
Part 2: Lift onConfigurationChanged to HomeFragment. r=margaret

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

Looks good to me.

It looks like BrowserSearch and HistoryPanel are the other panels that don't currently have this logic, so it might be worth just testing those two thing out to make sure there's no problems there.

::: mobile/android/base/home/BookmarksPanel.java
@@ +145,3 @@
>          }
> +
> +        super.onConfigurationChanged(newConfig);

I think you should add a comment here explaining that there's a reason for this ordering.
Attachment #8476132 - Flags: review?(margaret.leibovic) → review+
Landed, with reversed onConfigurationChange order in BookmarksPanel (and comments explaining why):

https://hg.mozilla.org/integration/fx-team/rev/11fb420e79b6
https://hg.mozilla.org/integration/fx-team/rev/cb646684a158
https://hg.mozilla.org/mozilla-central/rev/11fb420e79b6
https://hg.mozilla.org/mozilla-central/rev/cb646684a158
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.