Closed Bug 925068 Opened 6 years ago Closed 6 years ago

Regression: device rotation drops the top-most root folder item in view in nested bookmarks

Categories

(Firefox for Android :: Awesomescreen, defect)

27 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 27
Tracking Status
firefox26 --- fixed
firefox27 --- verified
fennec 26+ ---

People

(Reporter: aaronmt, Assigned: sriram)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files, 1 obsolete file)

Currently on a device rotation, the top-most root folder item to navigate back a directory is dropped in view in the listview for bookmarks.

Steps to reproduce:

i) Sync (have folders)
ii) Navigate down a folder
iii) Rotate device

Have not tested Aurora (26.0), but Nightly is affected here.

--
Samsung Galaxy SIV (Android 4.3)
Tested Aurora (26.0) & it's affected here too.
Assignee: nobody → sriram
tracking-fennec: ? → 26+
We are resetting the parent stack and populating the adapter again when we rotate. Probably we shouldn't do that.
Attached patch Patch (obsolete) — Splinter Review
(Quick and dirty patch)

Basically on rotation, we detach and re-attach the fragment. This causes the adapter to be created new. Hence we lose the state of the adapter on rotation. This patch doesn't nullify the adapter on detaching, and will create a new one if the adapter is null. I am not sure if holding an adapter when needed is good (my mind says no. my heart thinks of aurora). But if this is a problem, we should do something about storing the state while rotating. Does fragment provide any other option?
Attachment #816833 - Flags: review?(lucasr.at.mozilla)
I guess I could use  onSaveInstanceState();
But I will wait for the review.
Are we "retaining" the fragments at all? I have seen that referenced as a way to avoid losing state on configuration changes.
The fragments are detached and attached on rotation. This is so that the views are removed and re-created on rotation. This resets the adapter and everything gets messy.
(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> The fragments are detached and attached on rotation. This is so that the
> views are removed and re-created on rotation. This resets the adapter and
> everything gets messy.

Do we want the views removed and recreated on rotation? Retaining the fragments would stop the recreation:
http://stackoverflow.com/questions/5164126/retain-the-fragment-object-while-rotating
(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> > The fragments are detached and attached on rotation. This is so that the
> > views are removed and re-created on rotation. This resets the adapter and
> > everything gets messy.
> 
> Do we want the views removed and recreated on rotation? Retaining the
> fragments would stop the recreation:
> http://stackoverflow.com/questions/5164126/retain-the-fragment-object-while-
> rotating

We do need to recreate the views in BookmarksPage because the styles are different for portrait and landscape.
Comment on attachment 816833 [details] [diff] [review]
Patch

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> Created attachment 816833 [details] [diff] [review]
> Patch
> 
> (Quick and dirty patch)
> 
> Basically on rotation, we detach and re-attach the fragment. This causes the
> adapter to be created new. Hence we lose the state of the adapter on
> rotation. This patch doesn't nullify the adapter on detaching, and will
> create a new one if the adapter is null. I am not sure if holding an adapter
> when needed is good (my mind says no. my heart thinks of aurora). But if
> this is a problem, we should do something about storing the state while
> rotating. Does fragment provide any other option?

The problem with holding a reference to the adapter across configuration changes is that it assumes the activity will not be re-created on device rotation, which would cause the existing adapter to point to a dangling activity instance. This is a bit fragile as it's prone to break in subtle ways if we ever change the way the BrowserApp activity behaves on device rotation.

Our fragments don't go through the usual flow of state saving when you rotate the device because we're handling the fragment re-creation programatically (i.e. see our onConfigurationChanged() implementation). This means that onSaveInstanceState() is not called in our about:home fragments on device rotation. So, relying on the platform's state saving is, unfortunately, not an option here.

With that said, I'd feel slightly more comfortable with saving the mParentStack across configuration changes than the adapter itself.
Attachment #816833 - Flags: review?(lucasr.at.mozilla) → review-
onSaveInstanceState() cannot be used by us. There are some weird problems with Fragment in using that. That's what bnicholson mentioned in his big comment. Any other idea of holding onto mParentStack?
(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> onSaveInstanceState() cannot be used by us. There are some weird problems
> with Fragment in using that. That's what bnicholson mentioned in his big
> comment. Any other idea of holding onto mParentStack?

What big comment from bnicholson? I guess you mean my comment above?

You could try to store the parent stack just before nullifying the adapter in the fragment and re-using the stored parent stack when you instantiate the adapter again when the fragment is reattached.
Attached patch PatchSplinter Review
I am saving the parent stack in BookmarksPage and re-using it on rotation. If nothing exists, the BookmarksListAdapter adds the default root. All good. Yaaay!
Attachment #818642 - Flags: review?(lucasr.at.mozilla)
Attachment #816833 - Attachment is obsolete: true
Comment on attachment 818642 [details] [diff] [review]
Patch

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

Looks good, just want to see the suggested changes addressed before giving r+.

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +39,5 @@
>  
>      // Refresh folder listener.
>      private OnRefreshFolderListener mListener;
>  
> +    public BookmarksListAdapter(Context context, Cursor cursor, LinkedList<Pair<Integer, String>> stack) {

nit: parentStack for clarity and consistency?

@@ +55,5 @@
> +        }
> +    }
> +
> +    public LinkedList<Pair<Integer, String>> getParentStack() {
> +        return mParentStack;

nit: strictly speaking, you should probably return a copy of the parent stack to avoid the state of the adapter to be changed by the caller of getParentStack().

::: mobile/android/base/home/BookmarksPage.java
@@ +46,5 @@
>      // Adapter for list of bookmarks.
>      private BookmarksListAdapter mListAdapter;
>  
> +    // Adapter's parent stack.
> +    private LinkedList<Pair<Integer, String>> mParentStack;

mParentStack looks a bit too generic. Maybe mSavedParentStack? mParentStackToRestore?

@@ +88,5 @@
>  
>          final Activity activity = getActivity();
>  
>          // Setup the list adapter.
> +        mListAdapter = new BookmarksListAdapter(activity, null, mParentStack);

You should probably set mParentStack to null just after using it.

@@ +130,5 @@
>          // using commit() would throw an IllegalStateException since it can't
>          // be used between the Activity's onSaveInstanceState() and
>          // onResume().
>          if (isVisible()) {
> +            mParentStack = mListAdapter.getParentStack();

Add a comment explaining why you're saving the parent stack at this point.
Attachment #818642 - Flags: review?(lucasr.at.mozilla) → feedback+
Attached patch PatchSplinter Review
Changed the names.
I don't understand the copy part though. What's it going to cause? Not GC-ed? If so, how do I copy? Collections doesn't provide me an option.
Attachment #819182 - Flags: review?(lucasr.at.mozilla)
(In reply to Sriram Ramasubramanian [:sriram] from comment #15)
> http://docs.oracle.com/javase/6/docs/api/java/util/Collections.
> html#copy%28java.util.List,%20java.util.List%29 ??

Just making getParentStack() return an unmodifiable view of the list should be enough:

http://developer.android.com/reference/java/util/Collections.html#unmodifiableList%28java.util.List%3C?%20extends%20E%3E%29
Aah! This is what i was searching for. But I forgot the method. Posting a new method in few mins.
Comment on attachment 819182 [details] [diff] [review]
Patch

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

Looks good.

::: mobile/android/base/home/BookmarksListAdapter.java
@@ +50,5 @@
> +            // Add the root folder to the stack
> +            Pair<Integer, String> rootFolder = new Pair<Integer, String>(Bookmarks.FIXED_ROOT_ID, "");
> +            mParentStack.addFirst(rootFolder);
> +        } else {
> +            mParentStack = parentStack;

parentStack will be unmodifiable. You have to copy the elements of parentStack into mParentStack. This should work:

mParentStack = new LinkedList<Pair<Integer, String>>(parentStack);

@@ +55,5 @@
> +        }
> +    }
> +
> +    public LinkedList<Pair<Integer, String>> getParentStack() {
> +        return mParentStack;

Change this to:

return Collections.unmodifiableList(mParentStack);

::: mobile/android/base/home/BookmarksPage.java
@@ +89,5 @@
>          final Activity activity = getActivity();
>  
>          // Setup the list adapter.
> +        mListAdapter = new BookmarksListAdapter(activity, null, mSavedParentStack);
> +        mSavedParentStack = null;

nit: not necessary, null is the default value anyway.

@@ +134,5 @@
>          // onResume().
>          if (isVisible()) {
> +            // The parent stack is saved just so that the folder state can be
> +            // restored on rotation.
> +            mSavedParentStack = mListAdapter.getParentStack();

nit: add empty line here.
Attachment #819182 - Flags: review?(lucasr.at.mozilla) → review+
Had to change it to a generic List<?> while using Collections.unmodifiableList().
Pushed with that: https://hg.mozilla.org/integration/fx-team/rev/06cc867fd5d1
https://hg.mozilla.org/mozilla-central/rev/06cc867fd5d1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment on attachment 819182 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not saving the folder stack while rotating.
User impact if declined: User won't see folder title.
Testing completed (on m-c, etc.): Landed on 10/20
Risk to taking this patch (and alternatives if risky): Very less. Storing the stack and using it.
String or IDL/UUID changes made by this patch: None.
Attachment #819182 - Flags: approval-mozilla-aurora?
Attachment #819182 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.