Closed
Bug 800453
Opened 12 years ago
Closed 12 years ago
Add-ons should be able to support SubMenus
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 20
People
(Reporter: sriram, Assigned: sriram)
References
Details
Attachments
(4 files)
10.10 KB,
patch
|
mfinkle
:
feedback+
|
Details | Diff | Splinter Review |
10.26 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
8.71 KB,
patch
|
mfinkle
:
review-
|
Details | Diff | Splinter Review |
8.90 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
Addons should be able to support submenu structure. This should tie up with the standard submenu in pre-honeycomb phones and our custom menu starting with honeycomb.
Assignee | ||
Comment 1•12 years ago
|
||
This works as expected. The NativeWindow.menu.add() is overriden to take a json object. To add a submenu, addons should: 1. add a top-level menu. we will return the id of this menu. 2. add should use this id as "parent" for the submenu items.
Attachment #670463 -
Flags: feedback?(mark.finkle)
Assignee | ||
Comment 2•12 years ago
|
||
Additionally, addons should be able to support 1. checkable 2. checked 3. enabled 4. visible 5. action-item (optional) behavior. Do we send messages "Menu:Visible", "Menu:Enabled" to GeckoApp to do that?
Comment 3•12 years ago
|
||
Drive by... I don't really like the idea of overriding NativeWindow.menu.add() to take different kinds of objects. Looking at the patch quickly, it's confusing to still use aName as the parameter that gets passed to _add. Perhaps it would be better to add an aOptions parameter to add() instead that lets add-on authors specify that they want a sub-menu. It's bothersome to change public-facing APIs when they land, so it would be good to think about this first :) Also, yay! More APIs for add-ons! This is especially good because it can help them not clutter up the main menu.
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Margaret Leibovic [:margaret] from comment #3) > Drive by... I don't really like the idea of overriding > NativeWindow.menu.add() to take different kinds of objects. Looking at the > patch quickly, it's confusing to still use aName as the parameter that gets > passed to _add. Perhaps it would be better to add an aOptions parameter to > add() instead that lets add-on authors specify that they want a sub-menu. > It's bothersome to change public-facing APIs when they land, so it would be > good to think about this first :) > > Also, yay! More APIs for add-ons! This is especially good because it can > help them not clutter up the main menu. This is still a WIP. I checked with mfinkle before taking the approach of passing a single JSON object to add(). So that, we can add the arguments later if needed. aName is because, I couldn't a better way to override it.
Assignee | ||
Comment 5•12 years ago
|
||
NativeWindow.menu.add_v2(aOptions); ?
Comment 6•12 years ago
|
||
Comment on attachment 670463 [details] [diff] [review] WIP - Proof of concept >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js I think you have the basic idea, but let's not keep the old params: > add: function(aName, aIcon, aCallback) { add: function() { >+ if (arguments.length == 1) { >+ return this._add(aName); >+ } else { >+ return this._add({ >+ name: aName, >+ icon: aIcon, >+ callback: aCallback >+ }); >+ } >+ }, Instead of using a separate method, just make a local "options" object let options; if (arguments.length == 1) { options = arguments[0]; } else if (arguments.length == 3) { options = { name: arguments[0], icon: arguments[1], callback: arguments[2] }; } Now "options" can be sent to Java
Attachment #670463 -
Flags: feedback?(mark.finkle) → feedback+
Assignee | ||
Comment 7•12 years ago
|
||
Made changes to Menu.add() as required.
Attachment #675673 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 8•12 years ago
|
||
( I found this code to be small enough to be added to this bug. ) This adds support for addons to add attributes to menu item. NativeWindow.menu.set(id, attribute, value); would do that. Currently this supports: 1. checkable 2. checked 3. enabled 4. visible
Attachment #676391 -
Flags: review?(mark.finkle)
Comment 9•12 years ago
|
||
Comment on attachment 675673 [details] [diff] [review] Patch We discussed this at the FE meeting and decided that it was safe to use for add-ons devs. We need to update the NativeWindow API docs.
Attachment #675673 -
Flags: review?(mark.finkle) → review+
Comment 10•12 years ago
|
||
Comment on attachment 676391 [details] [diff] [review] Patch (2/2): Support for attributes for MenuItems >diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java >+ } else if (event.equals("Menu:Attribute")) { >+ final int id = message.getInt("id") + ADDON_MENU_OFFSET; >+ final String attribute = message.getString("attribute"); >+ final boolean value = message.getBoolean("value"); >+ mMainHandler.post(new Runnable() { >+ public void run() { >+ setAddonMenuItem(id, attribute, value); >+ } >+ }); We talked about changing this code to handle a JSON object with "visible", "enabled" and "checked" properties instead of sending the "Menu:Attribute" message for each property. Also, let's change the message to "Menu:Update" >+ private void setAddonMenuItem(int id, String attribute, boolean value) { You might just want to send the JSONObject into this function >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+ set: function(aId, anAttribute, aValue) { let's use: update: function(aId, aOptions) where aOptions could be: { visible: true, enabled: true, checked: true } >+ sendMessageToJava({ >+ gecko: { >+ type: "Menu:Attribute", "Menu:Update" >+ attribute: anAttribute, >+ value: aValue ? true : false Send the aOptions object instead You can land the first patch without waiting for this one. We can land it when it's ready.
Attachment #676391 -
Flags: review?(mark.finkle) → review-
Assignee | ||
Comment 11•12 years ago
|
||
Pushed part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/9131874bd803
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 13•12 years ago
|
||
This supports updation of attributes. (Yaay!) I was thinking of removing the "checkable" attribute. But then here is a problem. Manually setting it either in Java or Gecko will lead to something like: if (checked) checkable = true; else checkable = false; Now say a list like phony's clients have a submenu structure. If I tap on a menu item, the new one is checked, the old one should be in "unchecked" state. However, a "checked:false" on old menu item will remove the check box too. That doesn't seem right.
Attachment #688965 -
Flags: review?(mark.finkle)
Comment 14•12 years ago
|
||
Comment on attachment 688965 [details] [diff] [review] Patch (2/2): Attributes >+ private void updateAddonMenuItem(int id, JSONObject options) { >+ // Set attribute for the menu item in cache, if available >+ if (mAddonMenuItemsCache != null && !mAddonMenuItemsCache.isEmpty()) { >+ for (MenuItemInfo item : mAddonMenuItemsCache) { >+ if (item.id == id) { >+ try { >+ item.checkable = options.getBoolean("checkable"); >+ } catch (JSONException e) {} >+ >+ try { >+ item.checked = options.getBoolean("checked"); >+ } catch (JSONException e) {} >+ >+ try { >+ item.enabled = options.getBoolean("enabled"); >+ } catch (JSONException e) {} >+ >+ try { >+ item.visible = options.getBoolean("visible"); >+ } catch (JSONException e) {} >+ break; >+ } >+ } >+ } Might be nice to try and hold on to the "item" so you can use it below, instead of doing the try/catch + getBoolean() dance again.
Attachment #688965 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 15•12 years ago
|
||
I was thinking about it. But, item is of type MenuItemInfo and menuItem is of type MenuItem. They both aren't the same. Hence, we cannot hold that. If we do, we would do something like, if (item != null) item.checkable = ...; else if (menuItem != null) menuItem.setCheckable(...); And this will repeat for each of the try/catches. And I couldn't avoid try/catch dance because, not all will be present all the time.
Assignee | ||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f47b64fb07bf
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f47b64fb07bf
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
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
•