Closed Bug 988909 Opened 6 years ago Closed 6 years ago

Context menus for dynamic panels

Categories

(Firefox for Android :: Awesomescreen, defect, P2)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 --- verified
firefox32 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 3 open bugs)

Details

Attachments

(3 files, 5 obsolete files)

We should think about what kinds of context menus we want to support on dynamic panels. As a first pass, I think that if a view has a BROWSER item handler, we could the same context menu items we show for other about:home lists.

I imagine would be straightforward to implement "Open in New Tab", "Open in Private Tab", "Share", and "Add to Home Screen", especially since PanelListView is already a HomeListView.

I bet users will also want "Remove", but that would be more complicated, since we'd need to let the add-on know about that, and right now we only support batch delete operations.

In the future, it might be nice to allow add-ons to register context menu items for their panels, but this seems like it could be a can of worms we don't want to deal with just yet.
I'd be happy to start with 

Open in new tab
Open in private tab
Share
Add to home screen

---

We can figure out how to handle "remove" as a followup (if we even want that)
Assignee: nobody → margaret.leibovic
This context menu code turned out to be more tricky than I expected :/

This adds support for context menus for PanelListView, hooking into the existing context menu logic for HomeListView. However, this approach won't work for PanelGridView, since it doesn't extend HomeListView.

It felt nice taking advantage of the exiting HomeListView system, but I think we should investigate a different approach that can handle context menus for all DatasetBacked views (all DatasetBacked views use cursors with the same columns, so we should be able to have shared code to generate the context menus from a cursor).
Sola, do you have any more work planned to refactor these home context menus? I saw a few bugs go by recently, but I'm not sure what the state of your work is.

And since you're the most recent home context menu expert, let me know if you would be interested in taking over work on this bug :)
Flags: needinfo?(oogunsakin)
(In reply to :Margaret Leibovic from comment #3)
> Sola, do you have any more work planned to refactor these home context
> menus? I saw a few bugs go by recently, but I'm not sure what the state of
> your work is.
> 
> And since you're the most recent home context menu expert, let me know if
> you would be interested in taking over work on this bug :)

Don't have any more refactoring bugs atm. Sure, I'd love to take this bug :)
Flags: needinfo?(oogunsakin)
(In reply to Sola Ogunsakin [:sola] from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > Sola, do you have any more work planned to refactor these home context
> > menus? I saw a few bugs go by recently, but I'm not sure what the state of
> > your work is.
> > 
> > And since you're the most recent home context menu expert, let me know if
> > you would be interested in taking over work on this bug :)
> 
> Don't have any more refactoring bugs atm. Sure, I'd love to take this bug :)

Awesome! You can use the patch I started as inspiration, but as I mentioned in comment 2, I don't actually think this is the best approach. You've spent more time working in this code than me, so hopefully you can come up with a nice elegant solution :)

The main thing to think about is that in the future we may support more data-backed views (besides just LIST and GRID), and we would want to support context menus for those with as little copy/paste as possible.

Luckily, these dataset backed cursors should be simpler to deal with than the ones we use for our built-in panels, so you won't have to deal with things like bookmark items, reading list items, etc.

It's not the end of the world if we don't ship with this for Fx30, but I've gotten feedback that people would really like contextual actions for their feed panels, so it would be wonderful if we could come up with a simple uplift-able fix.
Assignee: margaret.leibovic → oogunsakin
Priority: -- → P2
Attachment #8407749 - Flags: feedback?(margaret.leibovic)
Attachment #8407750 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8407747 [details] [diff] [review]
Part 1: Create a central ContextMenuInfoFactory for dynamic panels

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

::: mobile/android/base/home/PanelLayout.java
@@ +227,5 @@
> +    }
> +
> +    public PanelLayout(Context context, PanelConfig panelConfig, DatasetHandler datasetHandler,
> +            OnUrlOpenListener urlOpenListener, ContextMenuRegistry contextMenuRegistry,
> +            ContextMenuInfoFactory contextMenuInfoFactory) {

This constructor looks like it's starting to grow a bit unwieldy. Instead of passing both a ContextMenuRegistry and a ContextMenuInfoFactory, I think we should just augment the ContextMenuRegistry interface to also take care of managing the ContextMenuInfoFactory. I decided to make this ContextMenuRegistry interface because I needed some way to call registerForContextMenu from the Fragment itself, but we can turn into into a more generic interface that deals with all aspects of DynamicPanel (and maybe in the future HomeFragement) context menus.

Also, if ContextMenuInfoFactory is going to be used more generically than just for HomeListViews, I think we should move it out of HomeListView, perhaps into HomeFragment instead.
Attachment #8407747 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 8407749 [details] [diff] [review]
Part 2: Context menus for list views

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

This is the quickest path forward, but I think we can take this opportunity to make something more future-proof.

::: mobile/android/base/home/PanelLayout.java
@@ +343,5 @@
>          if (view == null) {
>              switch(viewConfig.getType()) {
>                  case LIST:
>                      view = new PanelListView(getContext(), viewConfig);
> +                    ((PanelListView) view).setContextMenuInfoFactory(mContextMenuInfoFactory);

I don't like how this still depends on the fact that PanelListView is a HomeListView. Perhaps we could make some interface that both HomeListView and PanelGridView could implement that takes care of creating ContextMenuInfos for items. That way any future dynamic panel view could implement that interface.

Better yet, it looks like the code you add to PanelGridView in your next patch is mostly copy/paste from HomeListView, so maybe we can make a common OnItemLongClickListener class, and have HomeListView/PanelGridView call setOnItemLongClickListener with that class in their constructors.
Attachment #8407749 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 8407750 [details] [diff] [review]
Part 3: Context menus for grid views

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

::: mobile/android/base/home/PanelGridView.java
@@ +54,5 @@
> +
> +                mContextMenuInfo = mContextMenuInfoFactory.makeInfoForCursor(view, position, id, cursor);
> +                return showContextMenuForChild(PanelGridView.this);
> +            }
> +        });

I addressed this in the review of my last patch. I would prefer to see this copy/paste code moved into a shared OnItemLongClick listener that can be used by all PanelViews.

::: mobile/android/base/home/PanelLayout.java
@@ +349,5 @@
>                      break;
>  
>                  case GRID:
>                      view = new PanelGridView(getContext(), viewConfig);
> +                    ((PanelGridView) view).setContextMenuInfoFactory(mContextMenuInfoFactory);

If we do keep this logic, we should make setContextMenuInfoFactory part of the PanelView interface, then you could just call this once below on the panelView.
Attachment #8407750 - Flags: feedback?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #10)
> Better yet, it looks like the code you add to PanelGridView in your next
> patch is mostly copy/paste from HomeListView, so maybe we can make a common
> OnItemLongClickListener class, and have HomeListView/PanelGridView call
> setOnItemLongClickListener with that class in their constructors.

OnItemLongClickListener#onItemLongClick requires access to mContextMenuInfo which isn't public. We would need to define another interface to make mContextMenuInfo available for modification. Seems like such an interface would be a "one-off" thing only useful in this context. Should I go ahead?
Attached patch bug-988909.patch (obsolete) — Splinter Review
I think this patch that addresses some of your concerns (except the OnItemLongClickListener one).

- Made setContextMenuInfoFactory a part of PanelView so PanelListView is not dependent on HomeListView.
- Combined ContextMenuInfoFactory and ContextMenuRegistry to make ContextMenuDelegate. 

I left ContextMenuInfoFactory in HomeListView to simplify this patch. I'll move it to HomeFragment if we decide to go in this direction.
Attachment #8407747 - Attachment is obsolete: true
Attachment #8407749 - Attachment is obsolete: true
Attachment #8407750 - Attachment is obsolete: true
Attachment #8408775 - Flags: feedback?(margaret.leibovic)
Attachment #8408775 - Attachment description: bug-988909-factory.patch → bug-988909.patch
Attachment #8408775 - Attachment filename: bug-988909-factory.patch → bug-988909.patch
Comment on attachment 8408775 [details] [diff] [review]
bug-988909.patch

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

This looks like it's on the right track. Duplicating the OnItemLongClick code isn't the end of the world, but it would be worthwhile to file a follow-up bug to factor that out before we add more view types.

::: mobile/android/base/home/PanelLayout.java
@@ +210,5 @@
>          public void resetDataset(int viewIndex);
>      }
>  
>      public interface PanelView {
> +        public void setContextMenuInfoFactory(ContextMenuInfoFactory factory);

I would avoid giving this the same name as the method in HomeListView, since that could lead to some confusion. Since we're actually passing in the delegate, maybe this should be setContextMenuDelegate.

::: mobile/android/base/home/PanelListView.java
@@ +10,5 @@
>  import org.mozilla.gecko.db.BrowserContract.HomeItems;
>  import org.mozilla.gecko.home.HomeConfig.ItemHandler;
>  import org.mozilla.gecko.home.HomeConfig.ViewConfig;
>  import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
> +import org.mozilla.gecko.home.PanelLayout.ContextMenuDelegate;

Were there supposed to be more changes in this file?
Attachment #8408775 - Flags: feedback?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #14)
> ::: mobile/android/base/home/PanelLayout.java
> @@ +210,5 @@
> >          public void resetDataset(int viewIndex);
> >      }
> >  
> >      public interface PanelView {
> > +        public void setContextMenuInfoFactory(ContextMenuInfoFactory factory);
> 
> I would avoid giving this the same name as the method in HomeListView, since
> that could lead to some confusion. Since we're actually passing in the
> delegate, maybe this should be setContextMenuDelegate.

ContextMenuDelegate extends ContextMenuInfoFactory to add a registerView which is used by PanelLayout. My thinking was that since PanelView has no need for ContextMenuDelegate#registerView, it only needs the ContextMenuInfoFactory portion.

> ::: mobile/android/base/home/PanelListView.java
> @@ +10,5 @@
> >  import org.mozilla.gecko.db.BrowserContract.HomeItems;
> >  import org.mozilla.gecko.home.HomeConfig.ItemHandler;
> >  import org.mozilla.gecko.home.HomeConfig.ViewConfig;
> >  import org.mozilla.gecko.home.HomePager.OnUrlOpenListener;
> > +import org.mozilla.gecko.home.PanelLayout.ContextMenuDelegate;
> 
> Were there supposed to be more changes in this file?

oops I'll remove that
Blocks: 999670
I'm just going to try to wrap up this bug myself.

This first patch does some refactoring to move the ContextMenuInfoFactory interface out of HomeListView, since I want to be able to use it in PanelViews, which don't necessarily have anything to do with HomeListView.
Assignee: oogunsakin → margaret.leibovic
Attachment #8404773 - Attachment is obsolete: true
Attachment #8408775 - Attachment is obsolete: true
Attachment #8414830 - Flags: review?(lucasr.at.mozilla)
Here's a cleaned up version of sola's patch.
Attachment #8414835 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8414830 [details] [diff] [review]
(Part 1) Move ContextMenuInfoFactory out of HomeListView and into HomeContextMenuInfo

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

Good.
Attachment #8414830 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8414835 [details] [diff] [review]
(Part 2) Support context menus in PanelViews

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

Nice.

::: mobile/android/base/home/DynamicPanel.java
@@ +225,5 @@
>      /**
>       * Lazily creates layout for panel data.
>       */
>      private void createPanelLayout() {
> +        final PanelLayout.ContextMenuDelegate contextMenuDelegate = new PanelLayout.ContextMenuDelegate() {

nit: Import ContextMenuDelegate directly to make this code a little less verbose?

::: mobile/android/base/home/PanelGridView.java
@@ +44,5 @@
>          adapter = new PanelViewAdapter(context, viewConfig);
>          setAdapter(adapter);
>  
>          setOnItemClickListener(new PanelGridItemClickListener());
> +        setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {

I assume the same change has been made to PanelListView in another patch?

::: mobile/android/base/home/PanelLayout.java
@@ +229,5 @@
>          public boolean canGoBack();
>          public void goBack();
>      }
>  
> +    public interface ContextMenuDelegate {

Not entirely happy with this name but I can live with it. Maybe something like this instead?

public interface ContextMenuRegistry {
    public void add(View) or register(View)
}

Up to you.

nit: remove empty line.
Attachment #8414835 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #19)

> ::: mobile/android/base/home/PanelGridView.java
> @@ +44,5 @@
> >          adapter = new PanelViewAdapter(context, viewConfig);
> >          setAdapter(adapter);
> >  
> >          setOnItemClickListener(new PanelGridItemClickListener());
> > +        setOnItemLongClickListener(new AdapterView.OnItemLongClickListener() {
> 
> I assume the same change has been made to PanelListView in another patch?

This just magically works for PanelListView because it extends HomeListView. This is one thing I didn't really like about this patch, but that is more an issue of confusion that stems from the fact that HomeListView does a lot more than PanelGridView.

> ::: mobile/android/base/home/PanelLayout.java
> @@ +229,5 @@
> >          public boolean canGoBack();
> >          public void goBack();
> >      }
> >  
> > +    public interface ContextMenuDelegate {
> 
> Not entirely happy with this name but I can live with it. Maybe something
> like this instead?
> 
> public interface ContextMenuRegistry {
>     public void add(View) or register(View)
> }
> 
> Up to you.

Oh, I just realized that's what I named it in my original patch. I think sola changed it when we were attempting to have this inerface do more. I do prefer ContextMenuRegistry, thanks for calling that out.
https://hg.mozilla.org/integration/fx-team/rev/a05fc0ef38b3
https://hg.mozilla.org/integration/fx-team/rev/78bb434f0227

This was one of the main complaints I was getting about my home feeds add-on, so maybe we should consider uplifting this as well, but let's let it bake on nightly a bit first.
Flags: needinfo?(margaret.leibovic)
Tested with:
Device: LG Nexus 4 (Android 4.4.2)
Build: Firefox for Android 32.0a1 (2014-05-06)
Installing a few add-ons: goal.com, instagram panel and vimeo panel and the context menu appears and works ok.
Status: RESOLVED → VERIFIED
Attached patch folded patchSplinter Review
[Approval Request Comment]
Bug caused by (feature/regressing bug #): new home panels APIs
User impact if declined: no context menus on dynamic panels
Testing completed (on m-c, etc.): landed on m-c 5/1
Risk to taking this patch (and alternatives if risky): should only affect add-ons, baked on Nightly for a week, no regressions
String or IDL/UUID changes made by this patch: none
Attachment #8419569 - Flags: approval-mozilla-aurora?
Flags: needinfo?(margaret.leibovic)
Attachment #8419569 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested with:
Device: LG Nexus 4 (Android 4.4.2)
Build: Firefox for Android 31.0a2 (2014-05-11)
Installing a few add-ons: wikipedia panel, goal.com and the context menu appears and works ok.
Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release).

Filter on epic-hub-bugs.
Blocks: 1014030
Uplift to Beta? For example the targeting for world cup feed panel is fx30 release and needs a context menu.
(In reply to Teodora Vermesan (:TeoVermesan) from comment #28)
> Uplift to Beta? For example the targeting for world cup feed panel is fx30
> release and needs a context menu.

I agree this is a nice feature, but I don't think it's high enough priority to uplift to beta this late in the cycle.
You need to log in before you can comment on or make changes to this bug.