Closed Bug 800453 Opened 8 years ago Closed 8 years ago

Add-ons should be able to support SubMenus

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: sriram, Assigned: sriram)

References

Details

Attachments

(4 files)

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.
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)
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?
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.
(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.
NativeWindow.menu.add_v2(aOptions); ?
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+
Attached patch PatchSplinter Review
Made changes to Menu.add() as required.
Attachment #675673 - Flags: review?(mark.finkle)
( 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 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 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-
Whiteboard: [leave open]
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 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+
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.
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/f47b64fb07bf
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.