785199 - Give remote tabs a context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Aaron Train [:aaronmt]]

+ Open in a New Tab
+ Copy Link
+ Share

Comment 1

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

Is this for the about:home screen or the tab bar?

I assume the latter but it wouldn't hurt for both. :)

Comment 2

[Aaron Train [:aaronmt]]

The listed tabs in the pull-down tab menu.

Side Note: I'm positive that we used to have a context-menu for top-site thumbnail listings on about:home ...

Comment 3

[Aaron Train [:aaronmt]]

> Side Note: I'm positive that we used to have a context-menu for top-site thumbnail listings on about:home ...

This is now available. Still would be nice to have some more functionality for remote-tabs.

Comment 4

[Nick Alexander :nalexander [he/him]]

I'm happy to mentor this ticket.  I don't know much about adding a context menu in this case, but it looks like the process is detailed in [1].  Actually making that all happen is a multi-part ticket, I expect.  Potential contributor: post your work in progress for feedback early and often!

[1] http://stackoverflow.com/a/2354595

Comment 5

[Nick Alexander :nalexander [he/him]]

Alternately, we might want to take the approach ibarlow suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=770735#c9 and give the Remote Tabs panel an action bar mode.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #5)
> Alternately, we might want to take the approach ibarlow suggested in
> https://bugzilla.mozilla.org/show_bug.cgi?id=770735#c9 and give the Remote
> Tabs panel an action bar mode.

My feeling is that we'll still want the context menu to do something like adding show/hide a remote device (see https://bugzilla.mozilla.org/show_bug.cgi?id=977161).  An action bar would be great for https://bugzilla.mozilla.org/show_bug.cgi?id=899645.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Add context menu to Remote Tabs home panel.

This mostly falls out of HomeListView, so tagging margaret for review.
It's unclear of adding this generic mechanism is worth it for
expandable list views, since they're rare (!); but it's in place now.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8478410 [details] [diff] [review]
Add context menu to Remote Tabs home panel.

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

This makes me think that we could add support for expandable hub panels... but that would be a future project :)

::: mobile/android/base/home/HomeContextMenuInfo.java
@@ +62,5 @@
> +    /**
> +     * Interface for creating ContextMenuInfo instances from ExpandableListAdapters.
> +     */
> +    public interface ExpandableFactory {
> +        public HomeContextMenuInfo makeInfoForAdapter(ExpandableListAdapter adapter, View view, int position, long id);

I would put the adpater as the final parameter, to match the ordering of parameters in makeInfoForCursor.

::: mobile/android/base/home/HomeExpandableListView.java
@@ +83,5 @@
> +    }
> +
> +    public void setOnUrlOpenListener(OnUrlOpenListener listener) {
> +        mUrlOpenListener = listener;
> +    }

Are you using these methods anywhere? If you don't need them yet, you could drop them.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +131,5 @@
> +                    return null;
> +                }
> +                final int groupPosition = ExpandableListView.getPackedPositionGroup(packedPosition);
> +                final int childPosition = ExpandableListView.getPackedPositionChild(packedPosition);
> +                final Object child = adapter.getChild(groupPosition, childPosition);

I think it might be worth putting this logic in HomeExpandableListView and just passing the child to the makeInfo method. It's hard to say, since this is the only consumer right now, but HomeContextMenuInfo is very site-specific, and I don't know that the same object would be appropriate for group items. For comparison, in HomeListView, we don't even try to make a ContextMenuInfo for header rows.

But I don't want to push on this too much. We can refactor as needed if future HomeExpandableListView consumers want to share this logic.

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8478410 [details] [diff] [review]
> Add context menu to Remote Tabs home panel.
>
> Review of attachment 8478410 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This makes me think that we could add support for expandable hub panels...
> but that would be a future project :)
>
> ::: mobile/android/base/home/HomeContextMenuInfo.java
> @@ +62,5 @@
> > +    /**
> > +     * Interface for creating ContextMenuInfo instances from ExpandableListAdapters.
> > +     */
> > +    public interface ExpandableFactory {
> > +        public HomeContextMenuInfo makeInfoForAdapter(ExpandableListAdapter adapter, View view, int position, long id);
>
> I would put the adpater as the final parameter, to match the ordering of
> parameters in makeInfoForCursor.

Good idea, done.

> ::: mobile/android/base/home/HomeExpandableListView.java
> @@ +83,5 @@
> > +    }
> > +
> > +    public void setOnUrlOpenListener(OnUrlOpenListener listener) {
> > +        mUrlOpenListener = listener;
> > +    }
>
> Are you using these methods anywhere? If you don't need them yet, you could
> drop them.

Well spotted: these were copy-pasta from HomeListView.  I've removed them.

> ::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
> @@ +131,5 @@
> > +                    return null;
> > +                }
> > +                final int groupPosition = ExpandableListView.getPackedPositionGroup(packedPosition);
> > +                final int childPosition = ExpandableListView.getPackedPositionChild(packedPosition);
> > +                final Object child = adapter.getChild(groupPosition, childPosition);
>
> I think it might be worth putting this logic in HomeExpandableListView and
> just passing the child to the makeInfo method. It's hard to say, since this
> is the only consumer right now, but HomeContextMenuInfo is very
> site-specific, and I don't know that the same object would be appropriate
> for group items. For comparison, in HomeListView, we don't even try to make
> a ContextMenuInfo for header rows.

I think this is actually wrong in HomeContextMenuInfo.  It limits the
use of HomeListView rows to Cursors.  If the Cursor extraction and
header testing was per-panel, HomeListView would be easier to use when
one has synthesized data (like the adapter that backs the Remote Tabs
panel).  It's clearly fine now, but slightly less flexible in future.

> But I don't want to push on this too much. We can refactor as needed if
> future HomeExpandableListView consumers want to share this logic.

Agreed.

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Add context menu to Remote Tabs home panel. r=margaret

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8483270 [details] [diff] [review]
Add context menu to Remote Tabs home panel. r=margaret

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

Updated to account for review comments; carrying forward margaret's r+.

Comment 13

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/f7d2afe6fbb0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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