Closed Bug 1060544 Opened 6 years ago Closed 4 years ago

Restore previously opened folder when returning to bookmarks via history

Categories

(Firefox for Android :: Awesomescreen, defect)

31 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 + fixed

People

(Reporter: lex21, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

(Keywords: uiwanted)

Attachments

(3 files)

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.
OS: Windows 7 → Android
Hardware: x86_64 → ARM
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: uiwanted
Duplicate of this bug: 1062859
Summary: Bookmark folder location not remembered after opening a link → Bookmark folder location not remembered after opening a link; retain folder and scroll position in bookmark listing more often
+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.
Duplicate of this bug: 1256747
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.
Assignee: nobody → ahunt
Duplicate of this bug: 1263550
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.
Status: NEW → ASSIGNED
Summary: Bookmark folder location not remembered after opening a link; retain folder and scroll position in bookmark listing more often → Restore previously opened folder when returning to bookmarks via history
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/
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/
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.
Attachment #8742708 - Flags: feedback?(liuche)
Blocks: 1268887
Review commit: https://reviewboard.mozilla.org/r/49717/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49717/
Attachment #8742708 - Attachment description: MozReview Request: Bug 1060544 - Part 1: allow persisting/restoring home panel state → MozReview Request: Bug 1060544 - Part 1: allow persisting/restoring home panel state r?liuche
Attachment #8742709 - Attachment description: MozReview Request: Bug 1060544 - Part 2: persist Bookmarks panel folder → MozReview Request: Bug 1060544 - Part 2: persist Bookmarks panel folder r?liuche
Attachment #8747077 - Flags: review?(liuche)
Attachment #8742708 - Flags: feedback?(liuche) → review?(liuche)
Attachment #8742709 - Flags: review?(liuche)
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 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/
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 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.
Attachment #8742708 - Flags: review?(liuche) → review+
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.
Attachment #8742709 - Flags: review?(liuche) → review+
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
Attachment #8747077 - Flags: review?(liuche) → review+
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.
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.
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
Blocks: 1332955
You need to log in before you can comment on or make changes to this bug.