1060544 - Restore previously opened folder when returning to bookmarks via history

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Gary Sanders]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Consider a bookmark tree with several folders, each folder having one or more sub-folders.

1) Navigate to a link in one of the sub-folders and open the link.
2) Navigate back to bookmarks.


Actual results:

The current (last) bookmark location is not remembered after a link is opened. Opening the bookmark UI always returns to the root of the bookmark tree.


Expected results:

It was be much more user friendly if the behavior changed so that when returning to the bookmark UI, it always returns to the last folder location that was navigated to in the bookmark tree. Otherwise, with the current behavior, the user must keep re-navigating the bookmark tree to return to the last folder location.

Comment 1

[Aaron Train [:aaronmt]]

I think we should preserve the current folder tree layout, similar to how the desktop Firefox bookmark sidebar preserves the expanded layout when folders are open.

Comment 3

[kud]

+1 here.

I mostly use "desktop > unsorted bookmarks" bookmarks in my mobile, which makes me open lots of folders.
And when I've found the right bookmark, I long-tap, open in a new tab, which makes me a new tab, but also refresh the main tab with my bookmarks which makes me open another time "desktop > unsorted bookmarks", quite boring.

Comment 5

[Andrzej Hunt :ahunt]

I'm looking into this right now. It's especially annoying for the reading-list smartfolder, or our other smartfolders, since they're probably going to make the use of folders more common.

Comment 7

[Andrzej Hunt :ahunt]

I'm focusing on folder restore in this bug (that's complicated enough as it is), we've got a separate bug for scroll-position restoration in Bug 993552, hence renaming this bug. It's possible we could reuse some aspects of what I'm implementing here to implement scroll-position restoration too.

Comment 8

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1060544 - Part 1: allow persisting/restoring home panel state r?liuche

All home panel state is currently discarded when you navigate to another page.
We want to be able to restore state (e.g. to return to the currently opened bookmarks folder,
instead of returning to the root of the bookmarks folder). This commit adds storage and a framework
to allow homepanels to persist data.

Review commit: https://reviewboard.mozilla.org/r/47431/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47431/

Comment 9

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1060544 - Part 2: persist Bookmarks panel folder r?liuche

This allows us to restore the previously opened bookmarks folder when returning to
the bookmarks panel. I.e. when you open a bookmarks folder (including the smartfolders),
select a bookmark, and then return (via the back button, or quick-history via long-pressing
the back button), we will return to the bookmarks folder that was opened.

We don't persist the scroll position, that's a separate issue, in Bug 993552.

Review commit: https://reviewboard.mozilla.org/r/47433/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47433/

Comment 10

[Andrzej Hunt :ahunt]

These patches allow us to restore the bookmarks folder, however they seem overly complicated and I'd like to see if there's a simpler way of doing this.

We don't use the about:home url to store what panel we were using (the about:home?panel=... URLs are only to directly open a panel), it might be simpler to directly manipulate the current URL everytime we switch panel (and also every time we change panel state - i.e. we could have about:home?panel=bookmarks&state=1,2,3 for), which would allow history to correctly track panels. We rely on storing the last panel within Tab, and use that whenever reopening about:home to restore the previously opened panel.

Comment 11

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1060544 - Post: add comment explaining how our about:home restoration is incorrect r?liuche

Review commit: https://reviewboard.mozilla.org/r/49717/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49717/

Comment 12

[Andrzej Hunt :ahunt]

Comment on attachment 8742708 [details]
MozReview Request: Bug 1060544 - Part 1: allow persisting/restoring home panel state r?liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47431/diff/1-2/

Comment 13

[Andrzej Hunt :ahunt]

Comment on attachment 8742709 [details]
MozReview Request: Bug 1060544 - Part 2: persist Bookmarks panel folder r?liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47433/diff/1-2/

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Longstanding issue, but we have some recent duplicates and have a fix so I'd like to make sure this lands for 49. We could also consider uplifting to 48 if you think that is worth the potential risk.

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8742708 [details]
MozReview Request: Bug 1060544 - Part 1: allow persisting/restoring home panel state r?liuche

https://reviewboard.mozilla.org/r/47431/#review47661

This does look pretty complicated, but I think the approach is right. An alternative would have been to use events and listeners, so that you didn't need to pass a listener all the way down from BrowserApp to HomeFragment adapters. However, this does make it slightly easier to add scroll state, I guess. Keeping track of the panel state in each Tab does make this code more fragile (if the panelId gets out of sync with the panelData - right now, there's only the Bookmark handler, but we might want to add something for the History panel, or add scroll state).

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2436
(Diff revision 2)
>          final boolean isUserSearchTerm = selectedTab != null &&
>                  !TextUtils.isEmpty(selectedTab.getUserRequested());
>          if (isUserSearchTerm && SwitchBoard.isInExperiment(getContext(), Experiments.SEARCH_TERM)) {
>              showBrowserSearchAfterAnimation(animator);
>          } else {
> -            showHomePagerWithAnimator(panelId, animator);
> +            showHomePagerWithAnimator(panelId, null, animator);

Shouldn't we be passing in panelRestoreData here? This is the case where, say, the user clicks on a bookmark, loads the page, and then clicks on the urlbar - we should load the same state.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2769
(Diff revision 2)
>                          currentTab.setMostRecentHomePanel(panelId);
>                      }
>                  }
>              });
>  
> +            mHomePager.setPanelStateChangelistener(new HomeFragment.PanelStateChangeListener() {

Add a comment here circa set listener to save panel state to the tab.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeAdapter.java:27
(Diff revision 2)
>  public class HomeAdapter extends FragmentStatePagerAdapter {
>  
>      private final Context mContext;
>      private final ArrayList<PanelInfo> mPanelInfos;
> -    private final HashMap<String, Fragment> mPanels;
> +    private final HashMap<String, HomeFragment> mPanels;
> +    private final HashMap<String, Bundle> mRestoreBundles;

Nit: These types can be Map, unless you specifically need HashMap.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeAdapter.java:104
(Diff revision 2)
> +        // We have no guarantees as to whether our desired fragment is instantiated yet: therefore
> +        // we might need to either pass data to the fragment, or store it for later.
> +        if (fragment != null) {
> +            fragment.restoreData(data);
> +        } else {
> +            mRestoreBundles.put(id, data);

What's the case where the fragment doesn't exist but we're calling setRestoreData? Is it enough to try to restore the state for the panel, and not do anything if we don't have a fragment?

I'd imagine that this kind of state is something that we want to store for short term interactions (e.g., open a link from a bookmark folder, immediately go back) and if we were to persist the folder depth longer, it might be disorienting, so maybe it doesn't matter in cases where the fragment doesn't exist (then again, I'm not sure what these cases are).

(If we don't need that, we can just remove the whole mRestoreBundles data structure.)

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:89
(Diff revision 2)
> +     * for history/restoration. E.g. when a folder is opened/closed in bookmarks.
> +     */
> +    public interface PanelStateChangeListener {
> +
> +        /**
> +         *

Nit: Can probably get rid of this empty * line.

::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:461
(Diff revision 2)
>  
>      public void setOnPanelChangeListener(OnPanelChangeListener listener) {
>         mPanelChangedListener = listener;
>      }
>  
> +    public void setPanelStateChangelistener(HomeFragment.PanelStateChangeListener listener) {

Nit: Camelcase here - listener is not capitalized in your method.

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8742709 [details]
MozReview Request: Bug 1060544 - Part 2: persist Bookmarks panel folder r?liuche

https://reviewboard.mozilla.org/r/47433/#review47673

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksPanel.java:70
(Diff revision 2)
>      // Callback for cursor loaders.
>      private CursorLoaderCallbacks mLoaderCallbacks;
>  
>      @Override
> +    public void restoreData(@NonNull Bundle data) {
> +        ArrayList<FolderInfo> stack = data.getParcelableArrayList("parentStack");

Nit: final?

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksPanel.java:90
(Diff revision 2)
>  
>          mList = (BookmarksListView) view.findViewById(R.id.bookmarks_list);
>  
>          mList.setContextMenuInfoFactory(new HomeContextMenuInfo.Factory() {
>              @Override
> -            public HomeContextMenuInfo makeInfoForCursor(View view, int position, long id, Cursor cursor) {
> +            public HomeContextMenuInfo makeInfoForCursor(View view, int position, long id,

Nit: nothing changed here.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksPanel.java:173
(Diff revision 2)
>          }
>      }
>  
>      @Override
>      protected void load() {
> -        getLoaderManager().initLoader(LOADER_ID_BOOKMARKS_LIST, null, mLoaderCallbacks);
> +        final Bundle bundle;

You don't need to set the bundle here at all - that's for selection args for the loader.

Comment 17

[Chenxia Liu [:liuche]]

Comment on attachment 8747077 [details]
MozReview Request: Bug 1060544 - Post: add comment explaining how our about:home restoration is incorrect r?liuche

https://reviewboard.mozilla.org/r/49717/#review47679

Comment 18

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/47431/#review47661

> Shouldn't we be passing in panelRestoreData here? This is the case where, say, the user clicks on a bookmark, loads the page, and then clicks on the urlbar - we should load the same state.

I don't think so: we currently don't restore the existing panel when clicking on the URL bar (since there's no location_change event in that case - and we only restore the existing panel on location_change), and I was assuming we'd want to keep that . However maybe that's unintentional, I'll ask :antlam.

Comment 19

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a23be37119b

Comment 20

[Chenxia Liu [:liuche]]

I don't think we should base this behavior on "whether a location change happens" because users won't distinguish between the two - there really shouldn't be different behavior between tapping "back" to the homepager vs tapping on the urlbar to go to the homepager, because taking a different path to the same place shouldn't change your destination.

Comment 21

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/09b791894c43736972be1b29918ee718792a8164
Bug 1060544 - Part 1: allow persisting/restoring home panel state r=liuche

https://hg.mozilla.org/integration/fx-team/rev/726e4ef13cceed4ca28c4b88bf5b0c7e8856a33f
Bug 1060544 - Part 2: persist Bookmarks panel folder r=liuche

https://hg.mozilla.org/integration/fx-team/rev/6ec6d9e1f660dd473ce69648fe1863792e4e740c
Bug 1060544 - Post: add comment explaining how our about:home restoration is incorrect r=liuche

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09b791894c43
https://hg.mozilla.org/mozilla-central/rev/726e4ef13cce
https://hg.mozilla.org/mozilla-central/rev/6ec6d9e1f660