775070 - [TABLET] Tab side bar should keep its state after restart

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Andreea Pod]

Build: Firefox Beta 15.0 (2012-07-17)
Device: ASUS EEE Transformer (Android 4.0.3)

Steps to reproduce:
1. Open Fennec in landscape mode.
2. Open the tab side bar
3. Quit Fennec
4. Open Fennec in landscape mode again

Expected results: - tab side bar is still visible.

Actual results: - tab side bar is hidden.

Comment 1

[Aaron Train [:aaronmt]]

Why?

Comment 2

[Andreea Pod]

That was the behavior for the XUL version, see bug 690816.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

Ian do we want this behavior?

Comment 4

[Ian Barlow (:ibarlow)]

Well, it is what we had in XUL so we should probably carry it forward.

Comment 5

[Matt Brubeck (:mbrubeck)]

Bug 690816 is where we made this change in the old XUL version of mobile Firefox.

If anyone would like to work on this, the code for the tab sidebar is in:
http://hg.mozilla.org/mozilla-central/file/118cc431d56f/mobile/android/base/TabsPanel.java

You'll need to add some code to save the latest state of the sidebar, and some more code (maybe in BrowserApp) to restore the state on startup when the TabsPanel is created).

Comment 6

[Justin Warner]

I'd be interested in working on this.  

I checked out what Matt has posted above and semi-found out how I might go about doing this.  I was wondering however, is their a specific way Fennec stores temporary data for when it comes back from the "pause"?

Comment 7

[Matt Brubeck (:mbrubeck)]

I'm not certain, but I think the SharedPreferences API is the best thing to use to store this state:
http://developer.android.com/reference/android/content/SharedPreferences.html

Comment 8

[Justin Warner]

Matt, that's how I'm currently thinking of doing it.  Was just making sure that their wasn't a specific way Fennec operates that I should be using or if I would be free to go about this any way possible.  I'm assuming I can do this using the SharedPreferences.  Thanks!

Comment 9

[vchinen]

Hello Matt, I'm interested on working on this bug. However, I have a question: What exactly supposed to happen when you quit Fennec? Usually if you quit a program, the whole thing resets. Would the tabs reset also, or what's suppose to happen? Thank you

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to vchinen from comment #9)
> Hello Matt, I'm interested on working on this bug. However, I have a
> question: What exactly supposed to happen when you quit Fennec? Usually if
> you quit a program, the whole thing resets. Would the tabs reset also, or
> what's suppose to happen?

Right - when the user explicitly quits Fennec and then re-starts it, all of their tabs will be gone.  (The start page displays a list of "tabs from last time" for users who want to re-open the same tabs.)

For this bug, you shouldn't need to change that behavior at all - just store a preference for whether the tab sidebar is visible or hidden.

Comment 11

[vchinen]

Got it! Can I work on it?  Thanks!

Comment 12

[Matt Brubeck (:mbrubeck)]

Sure!

Comment 13

[Jenifer Wang]

Attachment: Patch #1 Feedback

We've added methods and SharedPreferences to save the tab state. The initial build and run works, but the browser crashes within seconds upon the second re-launch with tabs enabled.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 706940 [details] [diff] [review]
Patch #1 Feedback

It looks like this patch does not include all of your changes; is there another patch below it in your patch queue?

Does the Android system log contain a stack trace from the crash?

Comment 15

[Jenifer Wang]

Attachment: Second attempt for first patch

We've added some methods to save the tab state in this patch. We would like some feedback to see what we're missing as the tabs are still hidden when restarting the browser. Also, I deleted the entire source code and re-made this patch so now I'm unable to reproduce the crash any more.

Comment 16

[Shriram (irc: Mavericks)]

(In reply to Jenifer Wang from comment #15)
> We would like some feedback to see what we're missing as the tabs are still hidden when
> restarting the browser.
 
Something on the lines of side bar or such a member for Tabs. 
probably hasTabsSideBar() or a related one

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 707398 [details] [diff] [review]
Second attempt for first patch

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

Look good!  Can you take a look at my comments below, and put a new patch up for review?

::: mobile/android/base/BrowserApp.java
@@ +919,5 @@
>      public boolean onCreateOptionsMenu(Menu menu) {
>          super.onCreateOptionsMenu(menu);
>  
> +        SharedPreferences enabledState = getSharedPreferences(PREFS_NAME, 0);
> +        boolean currentTabState = enabledState.getBoolean("tabs_enabled", false);

I don't think onCreateOptionsMenu is the best location for this code.  Can you move it to BrowserApp.onCreate?

@@ +924,5 @@
> +
> +        if (currentTabState)
> +            autoShowTabs();
> +        else
> +            autoHideTabs();

The TabsPanel is already hidden by default, so you can remove the "else" branch here.

@@ +1208,5 @@
>      }
> +
> +    public void autoShowTabs() {        
> +        showTabs(TabsPanel.Panel.NORMAL_TABS);
> +    }

We should make sure to show the tabs only if the tablet "sidebar" layout is in use ("if (mTabsPanel.isSideBar())").  Also, you can use the showNormalTabs convenience method here if you want, just to make the code more concise.

Can you move this method so it's next to autoHideTabs?  Also, a minor nit: there's some trailing whitespace on the first line.

@@ +1212,5 @@
> +    }
> +
> +    public void storeTabPreference(boolean enabled) {
> +        SharedPreferences enabledState = getSharedPreferences(PREFS_NAME, 0);
> +        SharedPreferences.Editor editor = enabledState.edit();

The naming here is a little confusing to me -- can we call the SharedPreferences instance "prefs" instead?

::: mobile/android/base/TabsPanel.java
@@ -161,5 @@
>             mActivity.addTab();
>          else
>             mActivity.addPrivateTab();
> -
> -        mActivity.autoHideTabs();

I don't think this line should be removed.  (It is responsible for hiding the tab panel automatically on small-screen devices.)

Comment 18

[Jenifer Wang]

Attachment: Modifcations to patch from feedback

Comment 19

[Matt Brubeck (:mbrubeck)]

Comment on attachment 709287 [details] [diff] [review]
Modifcations to patch from feedback

Thanks!  This patch looks good to me with one minor change (see below).  I'm requesting an additional review since I'm not actively working on this code recently.

>+        SharedPreferences enabledState = getSharedPreferences("prefs", 0);

Could you rename this variable to "SharedPreferences prefs", please?  That would be slightly clearer to me, and shorter.

I've just learned that there are some more intrusive changes planned for the tab sidebar in bug 817728.  I think adding this preference should be a reasonable first step toward that larger change, while also being useful in the meantime.  If you want to continue working on this code, you should consider writing a patch for bug 817728!

Comment 20

[Jenifer Wang]

Attachment: Changed SharedPreferences variable name

Comment 21

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

Comment on attachment 709329 [details] [diff] [review]
Changed SharedPreferences variable name

I need to talk to Lucas about this a bit. We are planning to land a refactor to the tabs tray that will allow "pinning" the tabs tray open. Otherwise, the tabs tray will close as soon as possible.

If the tray is pinned open, we'll need something like this patch.
If the tray is not pinned, we'll want the tray to be closed between restarts.

One thing I noticed in the patch: We need to be careful with SharedPreferences. They can be slow, especially when writing/committing. We usually put SharedPreference code in a background thread.

Comment 22

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

Comment on attachment 709329 [details] [diff] [review]
Changed SharedPreferences variable name

Lucas is a better reviewer for this

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 709329 [details] [diff] [review]
Changed SharedPreferences variable name

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

Thanks for the patch Jennifer! I think this patch needs some more work to cover the issues I mentioned below. Also, as mfinkle said, this change conflicts a bit with the work we're doing on the new tabs UI. However, the new tabs UI will probably only land in Firefox 22. Maybe it makes sense to have this feature in Firefox 21 while we still have the current tabs UI in place. Ian, what do you think?

::: mobile/android/base/BrowserApp.java
@@ +204,5 @@
>          mAboutHomeStartupTimer = new Telemetry.Timer("FENNEC_STARTUP_TIME_ABOUTHOME");
>  
>          super.onCreate(savedInstanceState);
>  
> +        SharedPreferences prefs = getSharedPreferences("prefs", 0);

You should probably use GeckoApp.PREFS_NAME instead of "prefs" here.

@@ +205,5 @@
>  
>          super.onCreate(savedInstanceState);
>  
> +        SharedPreferences prefs = getSharedPreferences("prefs", 0);
> +        boolean currentTabState = prefs.getBoolean("tabs_enabled", false);

"tabs_enabled" is not obvious enough I think. Maybe "tabs_sidebar_visibility"?

@@ +208,5 @@
> +        SharedPreferences prefs = getSharedPreferences("prefs", 0);
> +        boolean currentTabState = prefs.getBoolean("tabs_enabled", false);
> +
> +        if (currentTabState)
> +            autoShowTabs();

It seems to me that the sticky tabs tray state should only apply to tablets where the tabs tray is a sidebar. On phones though, you don't want to start the app showing the tabs tray as it would look a bit weird.

@@ +535,5 @@
>      }
>  
> +    public void autoShowTabs() {        
> +        if (mTabsPanel.isSideBar())
> +            showNormalTabs();

This call will potentially trigger an animation to reveal the tabs tray, which might look a bit confusion as it happens on app startup. Maybe you have to tweak BrowserApp's onTabsLayoutChange() somehow to open the tabs tray without any animation in this case?

@@ +1210,5 @@
>          }).execute();
>      }
> +
> +    public void storeTabPreference(boolean enabled) {
> +        SharedPreferences prefs = getSharedPreferences("prefs", 0);

Use GeckoApp.PREFS_NAME instead of "prefs" here too.

@@ +1212,5 @@
> +
> +    public void storeTabPreference(boolean enabled) {
> +        SharedPreferences prefs = getSharedPreferences("prefs", 0);
> +        SharedPreferences.Editor editor = prefs.edit();
> +        editor.putBoolean("tabs_enabled", enabled);

Maybe move the pref key name to a private constant for consistency?

@@ +1213,5 @@
> +    public void storeTabPreference(boolean enabled) {
> +        SharedPreferences prefs = getSharedPreferences("prefs", 0);
> +        SharedPreferences.Editor editor = prefs.edit();
> +        editor.putBoolean("tabs_enabled", enabled);
> +        editor.commit();

The commit() call will block main thread. It should be done in our background thread. Have a look at GeckoAsyncTask.

Comment 24

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #23)

> Also, as mfinkle said, this change
> conflicts a bit with the work we're doing on the new tabs UI. However, the
> new tabs UI will probably only land in Firefox 22. Maybe it makes sense to
> have this feature in Firefox 21 while we still have the current tabs UI in
> place. Ian, what do you think?

Sure, it's a nice refinement to have, even for a short while.

Comment 25

[Jenifer Wang]

Attachment: Patch #4

We did try to use a final constant for SharedPreferences but the browser wouldn't run at all. There would be a small window stating that the browser has stopped working. 

commit() changed to apply() to set SharedPreferences. Hopefully this is okay to use so that it doesn't block main thread.

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 712762 [details] [diff] [review]
Patch #4

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

::: mobile/android/base/BrowserApp.java
@@ +1255,5 @@
> +    public void storeTabPreference(boolean enabled) {
> +        SharedPreferences prefs = getSharedPreferences("GeckoApp.PREFS_NAME", 0);
> +        SharedPreferences.Editor editor = prefs.edit();
> +        editor.putBoolean("tabs_sidebar_visibility", enabled);
> +        editor.apply();

The apply() method is only available from Gingerbread on. But we (still) need to support Froyo devices. You'll have to run commit() on a background thread instead. Have a look at other commit() calls in BrowserApp to see how we're doing it.

Comment 27

[Lucas Rocha (:lucasr)]

Hey Jeniffer, now that the new tabs UI is in mozilla-central, it's probably worth rebasing your patch to account for it.

Comment 28

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 29

[Mike Hoye [:mhoye]]

No motion for a while here, so deassigning and throwing back in the pool.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The tabs side bar doesn't exist in FF36 because of new tablet (bug 1014156) - I'm assuming this won't get fixed by then.