750167 - Remember which of "Top Sites", "Bookmarks" or "History" was last selected

Status: RESOLVED DUPLICATE of bug 909618

(Firefox for Android Graveyard :: General, defect)

Description

[Mardeg]

Whenever tapping the URL bar the awesomescreen shows "Top Sites" even when "Bookmarks" or "History" was previously selected.

Either by default or via a pref the previous selection should be remembered.
For times when the process involves hunting through/visiting history items or multiple use of bookmarklets, the extra tap every time to get back to the selection becomes tedious.

Comment 1

[Wesley Johnston (:wesj)]

This seems useful to me, but may be a bit tricky. I'm not sure if we want to remember this forever. Likely just for a short time (maybe until you switch tabs or locations?)

Comment 2

[lannguyen]

I think it is a good idea to remember the selection within the tab that being used. If user switch or open new tab, the app should show topsites as usual. What do you think?

Comment 3

[Wesley Johnston (:wesj)]

Sounds good to me for this bug. We can file follow up bugs to change it if we decide that's not right.

Comment 4

[Paul [pwd]]

(In reply to lannguyen from comment #2)
> I think it is a good idea to remember the selection within the tab that
> being used. If user switch or open new tab, the app should show topsites as
> usual. What do you think?

Wouldn't it make more sense to just remember whatever the user last viewed independent of new tab or current tab? I was actually going to file a bug to create a pref for users to be able to decide whether Top Sites, History or Bookmarks were shown by default in the awesome bar, but this seems like a much better solution.

Comment 5

[Wesley Johnston (:wesj)]

Paul... hmm... I really am not sure. I think I'd need to play with this to figure out what I'd want. But I cc'd ibarlow and see what he thinks.

To provide some background on how to fix. The AwesomeBar actually runs in its own activity here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AwesomeBar.java

I think you've probably got several options for how to save this state. Since it (probably?) shouldn't persist across sessions though, I think the best might be just to use a static int in GeckoAppShell.java? Happy to hear other ideas if you have them. BrowserApp.java gets different messages when things happen to tabs here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#90

so that's where you'd want to clear any saved state (if that's how we do this).

Comment 6

[Ian Barlow (:ibarlow)]

(In reply to Paul [sabret00the] from comment #4)
> (In reply to lannguyen from comment #2)
> > I think it is a good idea to remember the selection within the tab that
> > being used. If user switch or open new tab, the app should show topsites as
> > usual. What do you think?
> 
> Wouldn't it make more sense to just remember whatever the user last viewed
> independent of new tab or current tab? I was actually going to file a bug to
> create a pref for users to be able to decide whether Top Sites, History or
> Bookmarks were shown by default in the awesome bar, but this seems like a
> much better solution.

I agree with Paul, it should be remember your place globally. I highly doubt I could remember which awesomebar tab I was on *per site tab* i had open :)

Comment 7

[lannguyen]

I create a static variable to hold the position of the tab that is being selected. The app works as expected, but it introduces a small issue. Let say I choose History, when I tab the URL it did show history tab first, however, I was not able to click TopSites. If I want to go to topsites, I have to go to BooksMarks then topsites. 

Similarly, if I choose bookmarks, when I reopen URL I can't go back to topsites, I have to go to History first, then be able to go to topsites. It seems that you have to go to one intermediate tab before going back to TopSites.

Any idea why?

Comment 8

[lannguyen]

Attachment: Unable to navigate back to TopSites

Comment 9

[lannguyen]

Attachment: Save the position of the last tab that was selected by user.

The browser can now remember the tab that was previously selected by the user.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 704703 [details] [diff] [review]
Save the position of the last tab that was selected by user.

A few drive-by nits, as noted on IRC:

># User Lan Nguyen<[e-mail]>
                  ^-------------
I believe you need a space here | in your patch header. (At least, that's the convention I've seen; I'm not sure how much it matters.)

>Bug# 750167 => Implement: remember tabbar that previously selected by user for TopSites, History, and Bookmarks

That should say "Bug 750167: Make tabbar remember ...[etc]"

diff --git a/mobile/android/base/AwesomeBarTabs.java b/mobile/android/base/AwesomeBarTabs.java
>     private AwesomeBarTab mTabs[];
>-
>+    
  ^^^^
You modified this blank line, adding 4 space characters. Delete those space characters. (leaving this line blank, as it originally was)

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 704703 [details] [diff] [review]
Save the position of the last tab that was selected by user.

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

I'm going to be a bit more vague because I think we want to do this a bit differently.

I want to make sure this works even if the AwesomeBar activity is killed and we loose all this info. I think we need to implement onSaveInstanceState [1] in AwesomeBar.java [2] to save this. Unfortunately, I don't have much experience with these either, so I'm going to look into it too. A way to test might be to try setting "Don't keep Activities" to true in the Android developer options (that may also cause other issues because we don't always do this correctly in Fennec). That should make sure the AwesomeBar activity is killed every time you leave it.

The theming and click issue seem to be because we're not actually selecting the tab you think we are. The "tab" is selected, but we're not showing the correct pane. At the end on this inflate function we call filter(""), which kicks off the initial query in the all pages tab with a search for "", and forces us back into the "Top Sites" pane. We'll have to be smarter about how we kick off the initial query (just removing the filter function will show nothing, so you'll need to get the currently showing pane and start it's query some other way...). Just removing the filter("") call doesn't magically fix everything, so this may take a bit of detective work....

Good start, and I think we're narrowing down the problem, but a little more detective work to do.

Comment 12

[lannguyen]

Cool, let see if I can figure out a way around this.

Wesj, Let me talk to my professor to see if I can switch the telephone number to this as it is now a new bug. By the end when we can get this nicely done, I do not think our group still have time for the telephone one.

So I will talk with my professor tomorrow, I will keep you posted.
Thanks,
And yes, we will start looking into the files that you just suggested above.

Comment 13

[Brian Nicholson (:bnicholson)]

For saving/restoring data to the bundle, you can take a look at how we do this in GeckoApp.java. We save outgoing data here: http://hg.mozilla.org/mozilla-central/file/35e0c12f4332/mobile/android/base/GeckoApp.java#l626, and then restore that data in onCreate() here: http://hg.mozilla.org/mozilla-central/file/35e0c12f4332/mobile/android/base/GeckoApp.java#l1554.

Comment 14

[lannguyen]

Attachment: fix the theming issue, but screen still flashed sometimes

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 710314 [details] [diff] [review]
fix the theming issue, but screen still flashed sometimes

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

This seems good to me. To be honest, I don't think we need the SharedPreference stuff either, at least for a first round on this. Just some cleanup and commented out code to remove. I'm also seeing crashes with this at:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/AllPagesTab.java#597

because we're using mCursorAdapter where we should be using getCursorAdapter(). There are two other calls using mCursorAdapter in here as well that don't handle the case where its null and should probably be fixed. One of them is in AwesomeBarCursorAdapter.filter and probably should just call its own notifyDataSetChanged() method rather than calling one on a potentially different object. The other is in getUrlsWithoutFavicon().

::: mobile/android/base/AwesomeBarTabs.java
@@ -60,2 @@
>          }
> -

Don't remove this whitespace

@@ +173,5 @@
>                            mTabs[i].getTitleStringId(),
>                            i);
>          }
>  
> +        tabWidget.setCurrentTab(tabSelected);

We may not even need this line anymore.

@@ +179,4 @@
>          styleSelectedTab();
>  
>          // Initialize "App Pages" list with no filter
> +        //filter("");

Just delete this

::: mobile/android/base/awesomebar/AllPagesTab.java
@@ +132,5 @@
>              AwesomeBarCursorAdapter adapter = getCursorAdapter();
>              list.setAdapter(adapter);
>              list.setOnTouchListener(mListListener);
>          }
> +        filter("");

I'd move this inside the if block above it. We're basically using it to do some initialization the first time this is shown/created. We don't need to do it everytime someone gets this widget.

Comment 16

[lannguyen]

Attachment: All fixed, everything works fine in this patch, no filtering, theming, or flashing issue.

Comment 17

[lannguyen]

I did not touch on the nullpointer issue, because i don't know how to fix it and do not want to break anything. But the above patch works fine, there are no more issues, and the tabs are being remembered. Please review the patch and let me know if I need to fix something more. Thanks.

Comment 18

[Peter Retzer (:pretzer)]

Comment on attachment 711224 [details] [diff] [review]
All fixed, everything works fine in this patch, no filtering, theming, or flashing issue.

Hi, you should make sure to mark your attachments as patches if they are in the future. Also you can set the review flag to '?' to request a review. That way the reviewer will be notified via email about your request and it's less likely that your patch will be forgotten about. :-)

Comment 19

[lannguyen]

Thanks Peter, I will keep that in mind the next time. :P

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 711224 [details] [diff] [review]
All fixed, everything works fine in this patch, no filtering, theming, or flashing issue.

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

I think the code is good! There are pretty detailed instructions for that at: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I think the next step is probably to get a build up for people to see if this is the interaction they want. I was trying to do that and wound up clobbering. I can try again monday, or if you have time and some space somewhere, feel free to upload one and put up a link. I'm sure UX and mfinkle would like to see how this feels before we check it in.

::: mobile/android/base/AwesomeBarTabs.java
@@ +38,5 @@
>      
>      private AwesomeBarTab mTabs[];
>  
> +    // This value will keep track of tab selected
> +    // TopSites is set as the default tab

I don't think we need this comment. The variable name is pretty self explanatory.

@@ +39,5 @@
>      private AwesomeBarTab mTabs[];
>  
> +    // This value will keep track of tab selected
> +    // TopSites is set as the default tab
> +    private static int tabSelected = 0;

change tabSelected to selectedTab

Comment 21

[lannguyen]

Attachment: final patch that fixes bug 750167

Comment 22

[Mark Capella [:capella]]

This patch overlaps bug 848267 ... and previous patch for bug 837815 ... so fyi, without testing to be sure, I belive, 

If user selects readerlist from the banner in reader mode display a call to onPageSelected() is triggered which will cause your patch to act as if the user had manually clicked the bookmarks tab and therefore remember it as the last selected tab.

Comment 23

[Peter Retzer (:pretzer)]

Comment on attachment 712372 [details] [diff] [review]
final patch that fixes bug 750167

I just noticed that the request flag has now requestee assigned, you probably forgot that again ;-) Assigning to wesj as above, to get this moving again.

Comment 24

[mozilla2]

There is a limit in life to the times that someone is willing to click to do something as simple as locate their bookmarks.  Therefore I would suggest implementing the remember feature.  Failing that, why not just get rid of Top Sites altogether, it's unnecessary?

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 712372 [details] [diff] [review]
final patch that fixes bug 750167

We've got a very new about:home now.

Comment 26

[:Margaret Leibovic]

And now that the awesomescreen is about:home, this is a dupe of bug 909618.

Comment 27

[:Margaret Leibovic]

(In reply to mozilla2 from comment #24)
> There is a limit in life to the times that someone is willing to click to do
> something as simple as locate their bookmarks.  Therefore I would suggest
> implementing the remember feature.  Failing that, why not just get rid of
> Top Sites altogether, it's unnecessary?

And this issue is addressed as part of bug 942875.