Closed
Bug 988909
Opened 10 years ago
Closed 10 years ago
Context menus for dynamic panels
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect, P2)
Tracking
(firefox31 verified, firefox32 verified)
VERIFIED
FIXED
Firefox 32
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(3 files, 5 obsolete files)
9.90 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
10.03 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
20.50 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → margaret.leibovic
Assignee | ||
Comment 2•10 years ago
|
||
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).
Assignee | ||
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
(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)
Assignee | ||
Comment 5•10 years ago
|
||
(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
Comment 6•10 years ago
|
||
Attachment #8407747 -
Flags: feedback?(margaret.leibovic)
Comment 7•10 years ago
|
||
Attachment #8407749 -
Flags: feedback?(margaret.leibovic)
Comment 8•10 years ago
|
||
Attachment #8407750 -
Flags: feedback?(margaret.leibovic)
Assignee | ||
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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+
Comment 12•10 years ago
|
||
(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?
Comment 13•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8408775 -
Attachment description: bug-988909-factory.patch → bug-988909.patch
Attachment #8408775 -
Attachment filename: bug-988909-factory.patch → bug-988909.patch
Assignee | ||
Comment 14•10 years ago
|
||
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+
Comment 15•10 years ago
|
||
(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
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
Here's a cleaned up version of sola's patch.
Attachment #8414835 -
Flags: review?(lucasr.at.mozilla)
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
(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.
Assignee | ||
Comment 21•10 years ago
|
||
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)
https://hg.mozilla.org/mozilla-central/rev/a05fc0ef38b3 https://hg.mozilla.org/mozilla-central/rev/78bb434f0227
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment 23•10 years ago
|
||
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.
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
status-firefox32:
--- → verified
Assignee | ||
Comment 24•10 years ago
|
||
[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)
Updated•10 years ago
|
status-firefox31:
--- → affected
Updated•10 years ago
|
Attachment #8419569 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 26•10 years ago
|
||
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.
Assignee | ||
Comment 27•10 years ago
|
||
Setting P2 hub bugs to block hub v2 EPIC bug (targeting fx31 release). Filter on epic-hub-bugs.
Blocks: 1014030
Comment 28•10 years ago
|
||
Uplift to Beta? For example the targeting for world cup feed panel is fx30 release and needs a context menu.
Assignee | ||
Comment 29•10 years ago
|
||
(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.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•