885882 - Add back ContextMenu functionality for TopBookmarksView

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Sriram Ramasubramanian [:sriram]]

The TopBookmarksView doesnt have context menu support. Add back!

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This adds back the functionality. I made a small change to show "open in new tab" / "open in private tab" based on how the bookmarks list works. Other than that, everything else works pretty much the same as old one. I'll give explanation of tricky pieces in next comment.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 777399 [details] [diff] [review]
Patch

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

::: mobile/android/base/home/BookmarksPage.java
@@ +193,5 @@
> +    @Override
> +    public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
> +        if (menuInfo == null) {
> +            return;
> +        }

If there is no menu info, we don't know what to do. So, return early.

@@ +198,5 @@
> +
> +        // HomeFragment will handle the default case.
> +        if (menuInfo instanceof HomeContextMenuInfo) {
> +            super.onCreateContextMenu(menu, view, menuInfo);
> +        }

If its of list view kind, then the HomeFragment will take care of it. Nothing to do here. This method overrides only the TopBookmarksView related context menu.

@@ +237,5 @@
> +    @Override
> +    public boolean onContextItemSelected(MenuItem item) {
> +        ContextMenuInfo menuInfo = item.getMenuInfo();
> +        if (menuInfo == null || !(menuInfo instanceof TopBookmarksContextMenuInfo)) {
> +            return false;

This will make the default HomeFragment take care of it.

@@ +245,5 @@
> +        final Activity activity = getActivity();
> +
> +        switch(item.getItemId()) {
> +            case R.id.open_new_tab:
> +            case R.id.open_private_tab: {

This case should ideally be handled by HomeFragment. However, here is the problem:
HomeContextMenuInfo and TopBookmarksContextMenuInfo are totally different. If we want to factor out info.url and info.title, so that HomeFragment can use a HomeFragmentContextMenuInfo, things would get crazy.

HomeFragmentContextMenuInfo extends AdapterContextMenuInfo;
HomeContextMenuInfo extends HomeFragmentContextMenuInfo;

and so on. Hence I moved these lines here, though retained similar id as the other one.

::: mobile/android/base/home/HomeFragment.java
@@ +51,5 @@
>      }
>  
>      @Override
>      public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
> +        if (menuInfo == null || !(menuInfo instanceof HomeContextMenuInfo)) {

If the menu info is null, we were still adding entries. That was wrong, and fixed now.

@@ -94,5 @@
> -            ContextMenuInfo menuInfo = item.getMenuInfo();
> -            info = (HomeContextMenuInfo) menuInfo;
> -        } catch(ClassCastException e) {
> -            throw new IllegalArgumentException("ContextMenuInfo is not of type HomeContextMenuInfo");
> -        }

We cannot throw an exception. From Android's code: 

https://github.com/android/platform_frameworks_support/blob/master/v4/java/android/support/v4/app/FragmentManager.java#L1973

FragmentManager doesn't send the contextmenu selection event to the intended fragment directly. It goes through each fragment added and passes it to them one by one. So, VisitedPage gets this event first -- which passes it to HomeFragment. If the HomeFragment doesn't know how to handle, BookmarksPage gets the event on the second iteration -- where it processes it.

::: mobile/android/base/home/HomeListView.java
@@ +67,5 @@
>  
>      @Override
>      public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> +        Object item = parent.getItemAtPosition(position);
> +        if (item instanceof Cursor) {

The ListView might have header views. We do a cast without even looking if its a valid cast! :O :O I don't know why it never throw a class cast exception!

::: mobile/android/base/resources/menu/top_bookmarks_contextmenu.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

I don't want to re-use the old file. There are still references of it in TopSitesView. And merging will be a big hassle. Hence copy-paste-edited a new file.

Comment 3

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> Created attachment 777399 [details] [diff] [review]
> Patch
> 
> This adds back the functionality. I made a small change to show "open in new
> tab" / "open in private tab" based on how the bookmarks list works. Other
> than that, everything else works pretty much the same as old one. I'll give
> explanation of tricky pieces in next comment.

I'm curious, what went into the decision to make the bookmarks list work that way? I feel like it's nice to give users the option to open the site in a private tab or a regular tab like we currently do.

Comment 4

[:Margaret Leibovic]

Comment on attachment 777399 [details] [diff] [review]
Patch

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

General comment: A lot of the things you mentioned in your last bug comment would be great as comments in the code.

r+, but please add some of those comments, and I'd like to hear the answers to my questions. Also, this patch didn't apply for me, so I'm trusting you that it works!

::: mobile/android/base/home/TopBookmarksView.java
@@ +109,5 @@
>                  }
>              }
>          });
> +
> +        setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {

The current TopSitesView code uses setOnCreateContextMenuListener and creates the ContextMenuInfo in here. Why the difference?

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +227,5 @@
>  <!ENTITY contextmenu_subscribe "Subscribe to Page">
>  <!ENTITY contextmenu_site_settings "Edit Site Settings">
> +<!ENTITY contextmenu_top_bookmarks_edit "Edit">
> +<!ENTITY contextmenu_top_bookmarks_pin "Pin Site">
> +<!ENTITY contextmenu_top_bookmarks_unpin "Unpin Site">

Make sure the corresponding abouthome_topsites_* strings gets removed in bug 880047.

::: mobile/android/base/resources/menu/top_bookmarks_contextmenu.xml
@@ +1,1 @@
> +<?xml version="1.0" encoding="utf-8"?>

The pedantic part of me wants you to do an hg rename and then change the ids/strings, but since this is a really small and simple file, I don't think it's a big deal, so it's okay to go ahead with this copy/paste business if you want.

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 777399 [details] [diff] [review]
> Patch
> 
> Review of attachment 777399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General comment: A lot of the things you mentioned in your last bug comment
> would be great as comments in the code.
> 
> r+, but please add some of those comments, and I'd like to hear the answers
> to my questions. Also, this patch didn't apply for me, so I'm trusting you
> that it works!

I'll add some of those comments in my patch before landing.

> 
> ::: mobile/android/base/home/TopBookmarksView.java
> @@ +109,5 @@
> >                  }
> >              }
> >          });
> > +
> > +        setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {
> 
> The current TopSitesView code uses setOnCreateContextMenuListener and
> creates the ContextMenuInfo in here. Why the difference?

That's because, we do a registerForContextMenu(mTopBookmarks) in BookmarksPage. This basically does the job of "mTopBookmarks.setOnCreateContextMenuListener(this);" internally. (I know! that was a misnomer from android :( ). So, I'm following the approach we've be using in new about:home. Basically, either 

Fragment.registerForContextMenu(View);

or 

View.setOnContextMenuListener(Fragment);

We are using the first one.

> 
> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +227,5 @@
> >  <!ENTITY contextmenu_subscribe "Subscribe to Page">
> >  <!ENTITY contextmenu_site_settings "Edit Site Settings">
> > +<!ENTITY contextmenu_top_bookmarks_edit "Edit">
> > +<!ENTITY contextmenu_top_bookmarks_pin "Pin Site">
> > +<!ENTITY contextmenu_top_bookmarks_unpin "Unpin Site">
> 
> Make sure the corresponding abouthome_topsites_* strings gets removed in bug
> 880047.
> 
> ::: mobile/android/base/resources/menu/top_bookmarks_contextmenu.xml
> @@ +1,1 @@
> > +<?xml version="1.0" encoding="utf-8"?>
> 
> The pedantic part of me wants you to do an hg rename and then change the
> ids/strings, but since this is a really small and simple file, I don't think
> it's a big deal, so it's okay to go ahead with this copy/paste business if
> you want.

I'll clean it up in the patch for bug 880047.

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> > Created attachment 777399 [details] [diff] [review]
> > Patch
> > 
> > This adds back the functionality. I made a small change to show "open in new
> > tab" / "open in private tab" based on how the bookmarks list works. Other
> > than that, everything else works pretty much the same as old one. I'll give
> > explanation of tricky pieces in next comment.
> 
> I'm curious, what went into the decision to make the bookmarks list work
> that way? I feel like it's nice to give users the option to open the site in
> a private tab or a regular tab like we currently do.

I don't know why we removed it there. I tried following that here. We can re-add "open in private tab" for normal tab case, by filing a follow-up, if UX wants that.

Comment 7

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> (In reply to :Margaret Leibovic from comment #3)
> > (In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> > > Created attachment 777399 [details] [diff] [review]
> > > Patch
> > > 
> > > This adds back the functionality. I made a small change to show "open in new
> > > tab" / "open in private tab" based on how the bookmarks list works. Other
> > > than that, everything else works pretty much the same as old one. I'll give
> > > explanation of tricky pieces in next comment.
> > 
> > I'm curious, what went into the decision to make the bookmarks list work
> > that way? I feel like it's nice to give users the option to open the site in
> > a private tab or a regular tab like we currently do.
> 
> I don't know why we removed it there. I tried following that here. We can
> re-add "open in private tab" for normal tab case, by filing a follow-up, if
> UX wants that.

Let's ask Ian what he thinks.

Basically, the question is, do we want to have both "Open in New Tab" and "Open in Private Tab" as context menu items (current behavior on trunk/release), or only show one of these based on the private-ness of the current tab (current behavior on fig)? I feel like we should offer both options, since the private-ness level of the current tab doesn't really have anything to do with what you might want a new tab to be.

Comment 8

[Ian Barlow (:ibarlow)]

Yeah, I agree. Just because my current context is private or non-private, it doesn't really define where I want to open the next tab. I'd be fine to add both options to this menu.

(on the other hand, there is the bigger question of "why would I open something I already have in my history in a private tab", but for the sake of consistency with our other context menus I am fine to have both options there.)

Comment 9

[Sriram Ramasubramanian [:sriram]]

Pushed with comments and couple of changes:
1. getActivity() inside a background thread could be null. So, storing the application context and using it.
2. Renamed onAddPin() to onPinBookmark() as the patch in bug 885884 changed it.
http://hg.mozilla.org/projects/fig/rev/f74010b736a4

Comment 10

[:Margaret Leibovic]

Please remember to file a follow-up bug about comment 8.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f74010b736a4