895173 - ContextMenu on HomeListView is broken

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

The "Share" and "Add to Home screen" always uses the url as "about:home" and not the actual list item.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

That was due to name collision. Ideally I thought Fragment would get better precedence in handling the event than the Activity. Unfortunately the activity gets to handle it first and then lets others handle it! Gang-Of-Four fail!!

Renamed the menu items so that only HomeFragment can process them. I can go ahead and rename all id's to "home_" if you like it (may be a separate patch on same bug).

Comment 2

[:Margaret Leibovic]

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

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

That seems like an easy pitfall! Can you add some comments above the onContextItemSelected declaration, explaining that it only gets called if onContextItemSelected in BrowserApp didn't already handle it?

I don't think we need to rename everything home_*, since it looks like the other context menu items really are very specific to the homepage.

::: mobile/android/base/home/HomeFragment.java
@@ +124,5 @@
>              }
>  
>              case R.id.open_private_tab:
>              case R.id.open_new_tab: {
> +            	Log.i("Sriram", "new tab");

Oops, left in some logging :)

Comment 3

[Sriram Ramasubramanian [:sriram]]

Pushed with comments:
http://hg.mozilla.org/projects/fig/rev/475acbe5775e

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: OCD

I was thinking about it day and night..
and somehow the names didn't feel right!
So I changed them with all my might..
and putting it for review by your sight!!

:P :P

Comment 5

[:Margaret Leibovic]

Comment on attachment 779364 [details] [diff] [review]
OCD

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

::: mobile/android/base/resources/menu/top_bookmarks_contextmenu.xml
@@ +9,3 @@
>            android:title="@string/contextmenu_open_new_tab"/>
>  
> +    <item android:id="@+id/home_open_private_tab"

I think these should be top_bookmarks_open_new_tab and top_bookmarks_open_private_tab, since that's more consistent with the rest of this file.

Also, even though the id conflict issue won't affect these two context menus, it's nice if the ids are different so that we're never confused about the code that handles them.

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 779364 [details] [diff] [review]
> OCD
> 
> Review of attachment 779364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/menu/top_bookmarks_contextmenu.xml
> @@ +9,3 @@
> >            android:title="@string/contextmenu_open_new_tab"/>
> >  
> > +    <item android:id="@+id/home_open_private_tab"
> 
> I think these should be top_bookmarks_open_new_tab and
> top_bookmarks_open_private_tab, since that's more consistent with the rest
> of this file.
> 
> Also, even though the id conflict issue won't affect these two context
> menus, it's nice if the ids are different so that we're never confused about
> the code that handles them.

I made this "home_" on purpose to reflect the fact that they do the same thing as in HomeFragment. Ideally this should be handled by HomeFragment, but, the ContextMenuInfo's kind changes, and so we want to handle it in BookmarksPage.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: OCD

Changed.

Comment 8

[:Margaret Leibovic]

Comment on attachment 779418 [details] [diff] [review]
OCD

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

I like that more, thanks.

Comment 9

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/18b8d700be7f

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/475acbe5775e
https://hg.mozilla.org/mozilla-central/rev/18b8d700be7f