909618 - Remember about:home tab selection

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, enhancement, P1)

Description

[Zac C (:zac)]

Nightly for Android 26.0a1 (2013-08-26)
Device: HTC Desire HD
Android 2.3.5


STR:
1. Open Nightly
2. Change awesome bar tab from 'bookmarks' to 'history'
3. Create a new tab and navigate to a page
4. Navigate back to initial Awesome bar tab

Actual: 
initial awesome bar tab has reverted to 'Bookmarks'

Expected:
Awesome bar tab to remain 'history' as the user selected.

Comment 1

[Aaron Train [:aaronmt]]

Enhancement?

Comment 2

[Zac C (:zac)]

I'm happy with an enhancement. It doesn't sound like a hard thing to fix up :)

At the moment it assumes you have bookmarks.

Comment 3

[Sola Ogunsakin [:sola]]

Attachment: URL Storage Implementation

Patch is currently functional.

First Pass Implementation: Add a HomePager.PageChangeListener to allow parent of HomePager view to listen to page change events. Storing the current about:home page in tab's URL under the parameter "page" (e.g about:home?page=history). when an about:home tab is selected, the "page" parameter is used to load the correct page.

Comment 4

[Sola Ogunsakin [:sola]]

Comment on attachment 8361826 [details] [diff] [review]
URL Storage Implementation

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

::: mobile/android/base/home/HomePager.java
@@ +152,5 @@
> +    /**
> +     * Shows the given page in the view pager. If pageId is null
> +     * the default page from the HomePager's configuration. ViewPager
> +     * have loaded before this method is called.
> +     *

Shows the given page in the view pager. If pageId is null,
the default page from the HomePager's configuration is shown. ViewPager
has to be loaded before this method is called.

Comment 5

[Sola Ogunsakin [:sola]]

Attachment: Screenshot_2014-01-17-11-08-29.png

Is the extra parameter ok from a UI standpoint?

Comment 6

[Sola Ogunsakin [:sola]]

Attachment: Internal Storage Implementation

Alternative Implementation: Only difference is storing current about:home pageId in the Tab object (as oppossed to URL). Tab object field is called mCurrentAboutHomePageId.

Which implementation is preferred?

Comment 7

[:Margaret Leibovic]

Drive-by: we should probably be using the term "panel", not "page" here, especially for any user-facing feature. It would be good to make a clean-up patch that does this before moving forward with a patch to actually implement this feature.

Comment 8

[:Margaret Leibovic]

I don't think Tab should know anything about about:home. That's why we fixed bug 950919.

Comment 9

[Sola Ogunsakin [:sola]]

alright so the initial implementation is better? https://bugzilla.mozilla.org/show_bug.cgi?id=909618#c4

Comment 10

[:Margaret Leibovic]

(In reply to Sola Ogunsakin [:sola] from comment #9)
> alright so the initial implementation is better?
> https://bugzilla.mozilla.org/show_bug.cgi?id=909618#c4

Yes, I think the approach of storing the current panel in the URL is better than storing that information on the tab, but let's hear back from ibarlow about the user-facing impact of this change.

However, changing the URL won't work for the case where we're just opening about:home by entering editing mode, since we aren't actually on "about:home" in that case.

Honestly, I want to hear back from ibarlow in general about whether or not we should implement this feature. From an engineering standpoint it seems like the cost of complication outweighs the potential benefit.

Comment 11

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #10)

> Honestly, I want to hear back from ibarlow in general about whether or not
> we should implement this feature. From an engineering standpoint it seems
> like the cost of complication outweighs the potential benefit.

I think there was a similar bug a while back about this, and my position at the time was that remembering the state of about:home seemed a little too clever and possibly unexpected. 

Now that we're on the verge of landing the ability for users to rearrange and set different defaults for this page, I'm even less inclined to add this functionality.

Comment 12

[Sola Ogunsakin [:sola]]

alright will abort

Comment 14

[Ian Barlow (:ibarlow)]

Comment on attachment 8361842 [details]
Screenshot_2014-01-17-11-08-29.png

just clearing the feedback flag here

Comment 15

[Lucas Rocha (:lucasr)]

I'm starting to think this will be very important for the Hub stuff. The use case I have in mind is: tapping on items in a Hub panel then going back to the same panel. This is interaction is a bit annoying right now because the about:home is reset to default state when you go back to it after tapping on an item.

Comment 16

[Ian Barlow (:ibarlow)]

Yeah, I agree. Looking through this bug, I may have misread this summary, there was some 'new tab' interaction bits that were a little unclear...

I would suggest that we make sure the following behaviours are put in place:

1. Opening a new tab always opens the homepage to its default panel.
2. After openining a website from a homepage panel, tapping back should return to that homepage panel, regardless of whether it's the default. So for example if I open an article from my Reading List, tapping back should take me back to the Reading List panel.

Comment 17

[Caspy7]

It's not explicitly stated so I thought I'd bring it up.
I haven't seen or used these tabs, but get the impression they're lists.  If you scroll down one and then are taken to some content, then going back should take you back to the same scroll position as well, yes?
Hate to scroll way down and then lose your place.
Again, perhaps this is implied or already covered.

Comment 18

[Chenxia Liu [:liuche]]

We talked briefly about this during the Hub standup about some of the questions that Sola and I had while discussing this bug.

Basically, we narrowed this down to two likely scenarios that we'd want to try out in prototypes:
- The current about:home panel should be remembered per tab, so every time about:home is opened by tapping on the url bar, we want to display the last-opened about:home panel.
- The current about:home panel should be remembered across tabs, and only reset to the default when fennec restarts

Sola, can you make these two prototypes and post the apks? No need to mess around with the history JS API, just store these in Java.

Storing scroll position is another ask, but get the initial per-tab/global behavior up in prototypes first.

My main question is, how much does this conflict with allowing the user to set a default panel?

Comment 19

[Sola Ogunsakin [:sola]]

store "about:home" state across tabs: https://www.dropbox.com/s/qzxgjvn98vz9xtu/remember-across-tabs.apk

store "about:home" state per tab: https://www.dropbox.com/s/2mpigrr6d8uyolw/remember-per-tab.apk

Comment 20

[Chenxia Liu [:liuche]]

Thanks, Sola!

Lucas, any thoughts on these two implementations? I have a slight preference for per-tab, because it seems to preserve the use of setting a default panel better. In general, I think if the user opens a new tab, showing the default makes sense because the user took some specific action that they can associate with the panel being shown. I suppose that feedback isn't there if Fennec is started by an external app that opens up a new tab, but this doesn't seem like that big of a problem.

Comment 21

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #20)
> Thanks, Sola!
> 
> Lucas, any thoughts on these two implementations? I have a slight preference
> for per-tab, because it seems to preserve the use of setting a default panel
> better. In general, I think if the user opens a new tab, showing the default
> makes sense because the user took some specific action that they can
> associate with the panel being shown. I suppose that feedback isn't there if
> Fennec is started by an external app that opens up a new tab, but this
> doesn't seem like that big of a problem.

The remember-across-tabs doesn't seem to use the default panel when you create a new tab, feels a bit wrong. I'm leaning torwards the per-tab approach but we should get input from ibarlow.

Comment 22

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #21)
> (In reply to Chenxia Liu [:liuche] from comment #20)
> > Thanks, Sola!
> > 
> > Lucas, any thoughts on these two implementations? I have a slight preference
> > for per-tab, because it seems to preserve the use of setting a default panel
> > better. In general, I think if the user opens a new tab, showing the default
> > makes sense because the user took some specific action that they can
> > associate with the panel being shown. I suppose that feedback isn't there if
> > Fennec is started by an external app that opens up a new tab, but this
> > doesn't seem like that big of a problem.
> 
> The remember-across-tabs doesn't seem to use the default panel when you
> create a new tab, feels a bit wrong. I'm leaning torwards the per-tab
> approach but we should get input from ibarlow.

Agreed. Per tab feels more right here. Thanks for posting these builds, Sola!

Comment 23

[Sola Ogunsakin [:sola]]

Attachment: bug-909618.patch

-stores state of about:home per tab

Comment 24

[Sola Ogunsakin [:sola]]

Attachment: bug-909618.patch

-stores state of about:home per tab

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8402839 [details] [diff] [review]
bug-909618.patch

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

::: mobile/android/base/AboutPages.java
@@ +46,5 @@
> +    public static final String getPanelIdFromAboutHomeUrl(String aboutHomeUrl) {
> +        return StringUtils.getQueryParameter(aboutHomeUrl, PANEL_PARAM);
> +    }
> +
> +    public static final String getHomeUrlForPanelId(String panelId) {

nit:  getHomeUrlForPanelId ->  getAboutHomeUrlForPanel for consistency.

This function doesn't seem to be used anywhere in this patch though. Remove it?

::: mobile/android/base/BrowserApp.java
@@ +203,5 @@
>  
>      private DynamicToolbar mDynamicToolbar = new DynamicToolbar();
>  
> +    // Stores the most recent home panel shown in a tab.
> +    private HashMap<Integer, String> mMostRecentHomePanelForTab = new HashMap<Integer, String>();

This state should not live in the BrowserApp activity because BrowserApp is not guaranteed to be live e.g. when you switch to the Settings activity, BrowserApp might be background killed. So, you'll have to store this either in the Tabs instance as a SparseArray or in the Tag instance itself as a String member. I personally prefer the latter.

Strictly speaking, the about:home state for the tab should even be stored with the session so that we're able to restore the state even if the app gets background killed. We don't have to implement this in this bug though (and it's not hugely relevant anyway). File a follow-up for the record.

@@ -1695,5 @@
>              onCreateOptionsMenu(mMenu);
>          }
>      }
>  
> -    private void showHomePager(String pageId) {

It would be better to do the argument renaming in a separate patch btw.

::: mobile/android/base/home/HomePager.java
@@ +415,5 @@
> +
> +    /**
> +     * Interface for listening into ViewPager panel changes
> +     */
> +    public interface PanelChangeListener {

This should be OnPanelChangeListener. Move the declaration of this interface to the top of HomePager class, just after OnNewTabsListener.

Comment 26

[Sola Ogunsakin [:sola]]

Attachment: bug-909618.patch

-store about:home state in Tab instance

Comment 27

[Lucas Rocha (:lucasr)]

Comment on attachment 8403399 [details] [diff] [review]
bug-909618.patch

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

Looks good with the suggested changes. One aspect of this change that we haven't talked about so far is scroll position. We probably want to also restore the scroll position in the most recent about:home panel when you go back to it. We can do discuss/implement this in a separate bug (please file it).

::: mobile/android/base/AboutPages.java
@@ +42,5 @@
>          // whether or not this URL is "about:home".
>          return HOME.equals(url.split("\\?")[0]);
>      }
>  
> +    public static final String getPanelIdFromAboutHomeUrl(String aboutHomeUrl) {

For future patches: this change should have been done in a separate patch as it's unrelated to the main intent of this patch.

::: mobile/android/base/BrowserApp.java
@@ +1682,5 @@
> +                // home panel for this tab.
> +                final String mostRecentHomePanel = tab.getMostRecentHomePanel();
> +                if (mostRecentHomePanel != null) {
> +                    panelId = mostRecentHomePanel;
> +                }

No need to do these null checks here. It's fine if getMostRecentHomePanel() returns null. Just do:

String panelId = AboutPages.getPanelIdFromAboutHomeUrl(tab.getURL());
if (panelId == null) {
    // No panel was specified in the URL. Try loading the most recent
    // home panel for this tab.
    panelId = tab.getMostRecentHomePanel();
}
showHomePager(panelId);

@@ +1739,5 @@
>                  final ViewStub homeBannerStub = (ViewStub) findViewById(R.id.home_banner_stub);
>                  final HomeBanner homeBanner = (HomeBanner) homeBannerStub.inflate();
>                  mHomePager.setBanner(homeBanner);
>  
> +                mHomePager.setOnPanelChangeListener(new HomePager.OnPanelChangeListener() {

This listener should be set regardless of the guest mode. Move this setOnPanelChangeListener() call outside this 'if', just after the inflate call.

@@ +1763,5 @@
>  
>          mHomePagerContainer.setVisibility(View.VISIBLE);
>          mHomePager.load(getSupportLoaderManager(),
>                          getSupportFragmentManager(),
> +                        panelId, animator);

Ditto. Please do these variable renamings in a separate patch in future patches.

Comment 28

[Sola Ogunsakin [:sola]]

Attachment: bug-909618.patch

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 8403466 [details] [diff] [review]
bug-909618.patch

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

Looks great, just needs a final round of fixes.

::: mobile/android/base/AboutPages.java
@@ +21,5 @@
>      public static final String UPDATER         = "about:";
>  
>      public static final String URL_FILTER = "about:%";
>  
> +    public static final String PANEL_PARAM = "panel";

Update about:reader to use the new parameter name:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#368

::: mobile/android/base/BrowserApp.java
@@ +1543,5 @@
>          animator.setUseHardwareLayer(false);
>  
>          mBrowserToolbar.startEditing(url, animator);
> +
> +        final Tab tab = Tabs.getInstance().getSelectedTab();

'selectedTab' above already has a reference to the selected tab.

@@ +1713,5 @@
> +            // Home pager already visible, make sure it shows the correct panel.
> +            mHomePager.load(getSupportLoaderManager(),
> +                            getSupportFragmentManager(),
> +                            panelId,
> +                            animator);

Missed this bit in the original patches, sorry. I'd prefer this to be just a selectPanel(panelId) call or something. Keep in mind that the list of panels if loaded asynchronously. Make sure you handle the case where you call selectPanel() before the list of panels is around (have a look at how mInitialPanelId is used).

::: mobile/android/base/Tab.java
@@ +68,5 @@
>      private ErrorType mErrorType = ErrorType.NONE;
>      private static final int MAX_HISTORY_LIST_SIZE = 50;
>      private volatile int mLoadProgress;
>      private volatile int mRecordingCount = 0;
> +    private String mMostRecentHomePanel;

Use volatile here as getMostRecentHomePanel() might end up being called off main thread in some cases.

Comment 30

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #29)
> ::: mobile/android/base/Tab.java
> @@ +68,5 @@
> >      private ErrorType mErrorType = ErrorType.NONE;
> >      private static final int MAX_HISTORY_LIST_SIZE = 50;
> >      private volatile int mLoadProgress;
> >      private volatile int mRecordingCount = 0;
> > +    private String mMostRecentHomePanel;
> 
> Use volatile here as getMostRecentHomePanel() might end up being called off
> main thread in some cases.

Scratch that, all tab events are emitted in the UI thread.

Comment 31

[Sola Ogunsakin [:sola]]

Attachment: bug-909618.patch

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 8404142 [details] [diff] [review]
bug-909618.patch

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

Looks great but we need to handle the showPanel() correctly.

::: mobile/android/base/BrowserApp.java
@@ +1711,3 @@
>          if (isHomePagerVisible()) {
> +            // Home pager already visible, make sure it shows the correct panel.
> +            mHomePager.showPanel(panelId, true);

This case is going to happen on a tab switch most of the time. We probably don't want to do a smooth scroll in this case as it would be a bit confusing. So, get rid of the smoothScroll argument in showPanel() as just use setCurrentItem(i, false) in there.

@@ +1731,5 @@
> +                @Override
> +                public void onPanelSelected(String panelId) {
> +                    final Tab currentTab = Tabs.getInstance().getSelectedTab();
> +                    if (currentTab != null) {
> +                        currentTab.updateURL("about:home?panel=" + panelId);

Add a getAboutHomeUrlForPanel() that does this consistently, using PANEL_PARAM. I think you had this method in one of your original patches, right?

::: mobile/android/base/home/HomePager.java
@@ +262,5 @@
> +     * @param panelId      of the home panel to be shown.
> +     * @param smoothScroll True to smoothly scroll to the new item, false to transition immediately
> +     */
> +    public void showPanel(String panelId, boolean smoothScroll) {
> +        if (!mLoaded) {

The isAboutHomeVisible() check in BrowserApp already does that for us. So, strictly speaking, no need to check this here. 

mLoaded is actually a rather misleading name. It comes from the time when we loaded the list of panels synchronously. Let's build-up this logic a bit so that we can better tack the different HomePager loading states. Here's what I suggest:

1. Rename mLoaded to mVisible.
2. Rename isLoaded() to isVisible() accordingly. 
3. Add a mLoadState member of type LoadState enum UNLOADED/LOADING/LOADED.
4. Init mLoadState with UNLOADED in HomePager's constructor.
5. When load() is called, set mLoadState to LOADING.
6. When ConfigLoaderCallbacks.onLoadFinished() is called, set mLoadState to LOADED.
7. In your showPanel() method here, early return if mVisible is false (no need to throw).
8. In showPanel() do something like this:

switch (mLoadState) {
    case LOADING:
        mInitialPanelId = panelId;
        break;

    case LOADED:
        final int position = ((HomeAdapter) getAdapter()).getItemPosition(panelId);
        if (position > -1) {
            setCurrentItem(position, false);
        }
        break;

    default:
        // Do nothing.
}

Implement 1-2 in a separate patch.
Implement 3-6 in another patch.
Implement 7-8 in this patch.

Comment 33

[Sola Ogunsakin [:sola]]

(In reply to Lucas Rocha (:lucasr) from comment #32)
> @@ +1731,5 @@
> > +                @Override
> > +                public void onPanelSelected(String panelId) {
> > +                    final Tab currentTab = Tabs.getInstance().getSelectedTab();
> > +                    if (currentTab != null) {
> > +                        currentTab.updateURL("about:home?panel=" + panelId);
> 
> Add a getAboutHomeUrlForPanel() that does this consistently, using
> PANEL_PARAM. I think you had this method in one of your original patches,
> right?
> 
sorry I uploaded the wrong patch. this was a debug.

Comment 34

[Sola Ogunsakin [:sola]]

Attachment: Part 1: Rename mLoaded

Comment 35

[Sola Ogunsakin [:sola]]

Attachment: Part 2: Add LoadState member

Comment 36

[Sola Ogunsakin [:sola]]

Attachment: Part 3: Remember about:home tab selection

Comment 37

[Lucas Rocha (:lucasr)]

Comment on attachment 8405015 [details] [diff] [review]
Part 1: Rename mLoaded

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

Nice. Upgrading to r+.

Comment 38

[Lucas Rocha (:lucasr)]

Comment on attachment 8405016 [details] [diff] [review]
Part 2: Add LoadState member

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

Awesome. Upgrading to r+.

Comment 39

[Lucas Rocha (:lucasr)]

Comment on attachment 8405017 [details] [diff] [review]
Part 3: Remember about:home tab selection

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

Looks great. Please post an updated patch without the smoothScroll argument.

::: mobile/android/base/BrowserApp.java
@@ +1711,3 @@
>          if (isHomePagerVisible()) {
> +            // Home pager already visible, make sure it shows the correct panel.
> +            mHomePager.showPanel(panelId, false);

Remove the smoothScroll argument, no need for it now. We can reconsider it if we find a use case for it later. Besides, boolean arguments kinda suck because they are hard to read on the caller side.

Comment 40

[Sola Ogunsakin [:sola]]

Attachment: Part 3: Remember about:home tab selection

Comment 41

[Sola Ogunsakin [:sola]]

try https://tbpl.mozilla.org/?tree=Try&rev=2b5522f28993

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/307d6fde4fc3
https://hg.mozilla.org/integration/fx-team/rev/11bafe20387d
https://hg.mozilla.org/integration/fx-team/rev/fd36023adb65

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/307d6fde4fc3
https://hg.mozilla.org/mozilla-central/rev/11bafe20387d
https://hg.mozilla.org/mozilla-central/rev/fd36023adb65

Comment 44

[:Margaret Leibovic]

Sola, can you request approval to uplift this to aurora?

Comment 45

[Sola Ogunsakin [:sola]]

Attachment: bug-909618.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 909618
User impact if declined: Home page wont remember what tab user was on
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f967fc0648c
https://hg.mozilla.org/releases/mozilla-aurora/rev/b2be07b69a74
https://hg.mozilla.org/releases/mozilla-aurora/rev/6290c8194ab9

Comment 47

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.

Comment 48

[Flaviu Cos]

Verified as fixed in build 30 beta 10;
Device Motorola RAZR (Android 4.0.4).