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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (10/09) - Screenshot

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)

Comment 1

[Aaron Train [:aaronmt]]

Tested Aurora (26.0) & it's affected here too.

Comment 2

[Sriram Ramasubramanian [:sriram]]

We are resetting the parent stack and populating the adapter again when we rotate. Probably we shouldn't do that.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: 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?

Comment 4

[Sriram Ramasubramanian [:sriram]]

I guess I could use  onSaveInstanceState();
But I will wait for the review.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Are we "retaining" the fragments at all? I have seen that referenced as a way to avoid losing state on configuration changes.

Comment 6

[Sriram Ramasubramanian [:sriram]]

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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 8

[Lucas Rocha (:lucasr)]

(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 9

[Lucas Rocha (:lucasr)]

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.

Comment 10

[Sriram Ramasubramanian [:sriram]]

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?

Comment 11

[Lucas Rocha (:lucasr)]

(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.

Comment 12

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

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!

Comment 13

[Lucas Rocha (:lucasr)]

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.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

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.

Comment 15

[Sriram Ramasubramanian [:sriram]]

http://docs.oracle.com/javase/6/docs/api/java/util/Collections.html#copy%28java.util.List,%20java.util.List%29 ??

Comment 16

[Lucas Rocha (:lucasr)]

(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

Comment 17

[Sriram Ramasubramanian [:sriram]]

Aah! This is what i was searching for. But I forgot the method. Posting a new method in few mins.

Comment 18

[Lucas Rocha (:lucasr)]

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.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Had to change it to a generic List<?> while using Collections.unmodifiableList().
Pushed with that: https://hg.mozilla.org/integration/fx-team/rev/06cc867fd5d1

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06cc867fd5d1

Comment 21

[Sriram Ramasubramanian [:sriram]]

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.

Comment 22

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1cb44a39126e