Closed
Bug 1056225
Opened 10 years ago
Closed 10 years ago
Factor out shared pieces of HomeFragment
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: nalexander, Assigned: nalexander)
Details
Attachments
(2 files)
17.07 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
12.42 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8476131 -
Flags: review+
Assignee | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•