884340 - Allow Addons Make Changes in the Tools Menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Add-on Manager, defect)

Description

[Mark Capella [:capella]]

Attachment: WIP Make menu changes to Tools Item

I wonder if there's any interest in a change like the attached, to allow add-ons to make menu additions, removals, etc to the bottom of the Tools Menu in addition to the Main Menu like we do currently.

This would be closer to what the desktop does, and would avoid creating huge Main Menu lists. 

I've been testing this WIP towards that end.

Comment 1

[:Margaret Leibovic]

Comment on attachment 764176 [details] [diff] [review]
WIP Make menu changes to Tools Item

Let's also ask Sriram to look at this, since he's done most of our menu work.

Comment 2

[Wesley Johnston (:wesj)]

I don't think we want another function signature here. i.e. We should just support this in the arguments.length == 1 case. I also think this should probably use the syntax we already have for submenus. i.e. something like:

NativeWindow.menu.add({
    name: "My menuitem",
    parent: NativeWindow.menu.ToolsMenu
});

Comment 3

[Mark Capella [:capella]]

Well, this does support the arguments.length == 1, for example in testing I was using calls like

  qQuitTool1SubMenuId = aWindow.NativeWindow.menu.add({
    menu: "ToolsMenu",
    parent: qQuitTool1MenuId,
    name: "Add a Sibling",
    icon: gAddonData.resourceURI.spec + "icon.png",
    callback: function() {
      aWindow.NativeWindow.menu.add({
        menu: "ToolsMenu",
        parent: qQuitTool1MenuId,
        name: "New sibling will quit",
        icon: gAddonData.resourceURI.spec + "icon.png",
        callback: function() {
          quitMenu(aWindow);
        }
      });
    }
  });

I added the (arguments.length == 4) case to be thorough, but really wanted to abandon extending in that direction if that's what you're also thinking.

Not being a super strong JS hacker yet, the bit about|NativeWindow.menu.ToolsMenu| eludes me ... can you say more things?

Comment 4

[Mark Capella [:capella]]

Attachment: Sample Testing Addon

Quick Test addon haced from QuitNow, showing testing of Menu Adds, Removes, Updates, and Multi Level Menus off Main and the Tools anchors.

Comment 5

[Mark Capella [:capella]]

Attachment: The Real Sample Testing Addon

One of those days...

Comment 6

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 764176 [details] [diff] [review]
WIP Make menu changes to Tools Item

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

Overall looks good.
Couple of suggestions:
1. "menu" needn't be a string. It better be a boolean. "underTools" or "mainMenu" or something.
This will override the "parent" parameter, if the value is tools.
2. That would remove the need for changes in handleMessage(). And the need for new method "getRootMenuToModify()".
3. Remove under a submenu is done under bug 857816. Please use the patch there and make this dependent on that. (That might need to be cleaned up a bit).
4. Please avoid the "length == 4" case. We've deprecated "length == 3" and any new addon should use the "length == 1" case.

Currently, lets restrict the addon not to be able to change the "parent" via an update call. We don't want an addon to move from main menu to tools menu on the fly.
update() is only for updating params like checkable, checked, enabled.

The name for the extra parameter should be given by mfinke :D (as this is going to be an external API).

f- as I would like to see the changes mentioned here. (But the approach is right).

::: mobile/android/base/BrowserApp.java
@@ +107,5 @@
>          public boolean checkable;
>          public boolean checked;
>          public boolean enabled;
>          public boolean visible;
> +        public String menu;

This better be a boolean "underTools" or something.

@@ +972,5 @@
> +                    info.menu = message.getString("menu");
> +                } catch (Exception ex) {
> +                    info.menu = "";
> +                }
> +

This should be a boolean. Check the javascript file comments.

@@ +980,5 @@
>                      public void run() {
>                          addAddonMenuItem(menuItemInfo);
>                      }
>                  });
> +

Not needed.

@@ +990,5 @@
> +                    info.menu = message.getString("menu");
> +                } catch (Exception ex) {
> +                    info.menu = "";
> +                }
> +

Not needed. id should be enuough. Please check bug 857816.

@@ +1000,3 @@
>                      }
>                  });
> +

Not needed.

@@ +1007,5 @@
> +                try { // menu is optional
> +                    info.menu = message.getString("menu");
> +                } catch (Exception ex) {
> +                    info.menu = "";
> +                }

Not needed. This should be done inside updateAddonMenuItem().

@@ +1019,3 @@
>                      }
>                  });
> +

Not needed.

@@ +1376,5 @@
>      }
>  
>      private void addAddonMenuItem(final MenuItemInfo info) {
> +        // Cache Menu Add requests from installed Addons re-loaded during startup
> +        // These will be fed back / processed at onCreateOptionsMenu() time

Not needed.

@@ +1381,3 @@
>          if (mMenu == null) {
>              if (mAddonMenuItemsCache == null)
> +                // Create the Menu Add cache if first request

Not needed.

@@ +1388,5 @@
>          }
>  
> +        // Process Menu Add requests fed back from cache, or dynamically generated during Addon Installs / Uninstalls
> +        // Use parent MenuItem if specified, else default to mMenu
> +        Menu rootMenu = getRootMenuToModify(info.menu);

Menu menu;
if (info.underTools) {
    menu = mMenu.findItem(R.id.tools);
} else if (info.parent == 0) {
    menu = mMenu;
} else {
    ... all big code there ...
}

@@ +1408,5 @@
>                  menu = parent.getSubMenu();
>              }
>          }
>  
> +        // Add the add-on menu item

Please remove all the comments. They aren't needed.

@@ +1459,5 @@
>          item.setEnabled(info.enabled);
>          item.setVisible(info.visible);
>      }
>  
> +    private void removeAddonMenuItem(final MenuItemInfo info) {

Please apply patch in bug 857816. That should do most of this stuff I believe.

@@ +1540,5 @@
>          }
>      }
>  
> +    // Get root menu for changes, either ToolsMenu (lookup), or default to Main Menu
> +    private Menu getRootMenuToModify(String aMenuToModify) {

This is not needed. search for mMenu.findItem(R.id.tools).

::: mobile/android/chrome/content/browser.js
@@ +1555,5 @@
>  
>    menu: {
>      _callbacks: [],
>      _menuId: 0,
> +

nit: Remove this line.

@@ +1560,4 @@
>      add: function() {
>        let options;
>        if (arguments.length == 1) {
>          options = arguments[0];

This options should have the optional "menu" parameter.

@@ +1572,5 @@
> +            name: arguments[0],
> +            icon: arguments[1],
> +            callback: arguments[2],
> +            menu: arguments[3]
> +          };

We don't need this case. We wanted to make sure we don't change the API signature. So, any new additions will go through "length == 1" case. "length == 3" is deprecated. Also "menu" should go inside options.

I don't like passing a string from addons. May be a boolean like "tools: true" or "main: false" should be good. This argument will override any "parent" value specified.

@@ +1590,5 @@
> +    remove: function(aId, aMenu) {
> +      sendMessageToJava({
> +        type: "Menu:Remove",
> +        id: aId,
> +        menu: aMenu

"menu" might not be needed. There is a bug on remove tracked by bug 857816. So that should take care of remove inside a submenu.

@@ +1599,5 @@
>        if (!aOptions)
>          return;
>  
>        sendMessageToJava({
>          type: "Menu:Update", 

nit: whitespace.

@@ +1602,5 @@
>        sendMessageToJava({
>          type: "Menu:Update", 
>          id: aId,
> +        options: aOptions,
> +        menu: aMenu

menu should be inside "options".

Comment 7

[Mark Capella [:capella]]

I suppose I can push to try to find out, but I was anticipating that for builds using browser_app_menu.xml in base/resources/menu that id wouldn't be defined / resolved at compile time ... hence the string search for |(R.string.tools)|

|This is not needed. search for mMenu.findItem(R.id.tools)|

Comment 8

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Capella [:capella] from comment #7)
> I suppose I can push to try to find out, but I was anticipating that for
> builds using browser_app_menu.xml in base/resources/menu that id wouldn't be
> defined / resolved at compile time ... hence the string search for
> |(R.string.tools)|
> 
> |This is not needed. search for mMenu.findItem(R.id.tools)|

As far as the id generation goes -- the @+id/xyz are generated into an R.java before compiling. So, R.id.tools will be available for the code to compile and that can be used. We shouldn't be searching against R.string.tools -- ever!

Comment 9

[Wesley Johnston (:wesj)]

(In reply to Mark Capella [:capella] from comment #3)

> Not being a super strong JS hacker yet, the bit
> about|NativeWindow.menu.ToolsMenu| eludes me ... can you say more things?

Yeah, well first I'm suggesting you don't add a new "menu" parameter that does something the same, but different than the current "parent" parameter. I don't think we want addon devs to care what's Native UI and what's not. Heck, someday we may move the tools menu to be in JS anyway. There's nothing in there that can run without Gecko being started.

i.e. you example:
>   qQuitTool1SubMenuId = aWindow.NativeWindow.menu.add({
>     parent: aWindow.NativeWindow.ToolsMenu || qQuitTool1MenuId
>     name: "Add a Sibling",
>     callback: function() {
>       aWindow.NativeWindow.menu.add({
>         parent: aWindow.NativeWindow.ToolsMenu || qQuitTool1MenuId,
>         name: "New sibling will quit",
>         callback: function() {
>           quitMenu(aWindow);
>         }
>       });
>     }
>   });

Then, I think that we should add a field in NativeWindow (maybe not in NativeWindow.menu... I'm not sure) that returns an ID for the Tools menu. In Java, we can look at what was passed for "parent" and if its looks special, we can make sure we attach to the right menu.

Just glancing at srirams comment's, maybe there's even a way to send the Java menu id to javascript and use it (but note this might be called before we've ever had a chance to talk to Java really...), but I'd be fine if it was just a constant

if ("GECKO_TOOLS_MENU".equals(parent)) {
  parent = R.id.tools;
}

Comment 10

[Mark Capella [:capella]]

Attachment: Latest Test Addon Source

Update AddOn test source code

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v2)

Cool Feeback, simplified the patch considerably ... this is what the next iteration looks like ...

Comment 12

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Capella [:capella] from comment #11)
> Created attachment 765035 [details] [diff] [review]
> Patch (v2)
> 
> Cool Feeback, simplified the patch considerably ... this is what the next
> iteration looks like ...

I guess mfinkle should decide on how the API should be. This patch looks good, but cannot be reviewed without the external API specification.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

I haven't looked at the patch yet, but does this patch take the place of bug 857816 or build on it.

* I like the aWindow.NativeWindow.ToolsMenu idea. Getting those values might be a pain, but since the built-in menus change so infrequently, we could just hard code some values in JS.
* I like not using any new function params. Since we already pass an object literal, we should build on it, like Wes and Sriram have suggested.

Comment 14

[Mark Capella [:capella]]

Builds / depends on it ... I had actually noticed some of my test cases failing ... and before I needed the answer sriram has provided it :)

Comment 15

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 765035 [details] [diff] [review]
Patch (v2)

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

I'll wait for the patch that uses NativeWindow.toolsMenu approach.

Comment 16

[Mark Capella [:capella]]

Attachment: Patch (v3)

Ok, irc chatted with mfinkle a little, and I think this is closer to acceptable ... The user isn't required to specify a "menu" option, all depends on the "parent", with a aWindow.NativeWindow.GECKO_TOOLS_MENU() providing a parent for the tools menu.

Comment 17

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 771810 [details] [diff] [review]
Patch (v3)

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

Needs more cleanup. Looking better though.

::: mobile/android/base/BrowserApp.java
@@ +1408,5 @@
>          } else {
> +            Menu toolsMenu = null;
> +            MenuItem toolsMenuItem = mMenu.findItem(R.id.tools);
> +            if (toolsMenuItem != null && toolsMenuItem.hasSubMenu())
> +                toolsMenu = toolsMenuItem.getSubMenu();

I find this bunch of code not needed. We are guaranteed to have a sub menu for "tools" menu item.

Menu toolsMenu = mMenu.findItem(R.id.tools).getSubMenu(); is enough.

@@ +1416,4 @@
>              } else {
> +                Menu parentMenu = null;
> +                MenuItem parent = null;
> +                if (toolsMenu != null) {

When would we hit this piece of code? ToolsMenu check is done in the above if clause. This else condition is for normal / addon-added menu.

@@ +1433,5 @@
> +                    if (parent.getIcon() != null)
> +                        ((SubMenu) menu).getItem().setIcon(parent.getIcon());
> +                } else {
> +                    menu = parent.getSubMenu();
> +                }

The entire logic should be like:

if (info.parent == 0) {
    menu = mMenu;
} else if (info.parent == -1) {
    menu = mMenu.findItem(R.id.tools).getSubMenu();
} else {
    ... existing block;
}

Please change the add part to add an ADDON_MENU_OFFSET to the info.parent, only if the info.parent is not "-1".

::: mobile/android/chrome/content/browser.js
@@ +1577,5 @@
>    menu: {
>      _callbacks: [],
>      _menuId: 0,
> +
> +    GECKO_TOOLS_MENU: function() {

We never name methods in ALL_CAPS. Please change it.

I guess,

 toolsMenu: -1,

is enough.

Or may be a getter for it.

Comment 18

[Sriram Ramasubramanian [:sriram]]

I would like to know the discussion points from IRC.
1. What's the method to get the toolsMenu?
"NativeWindow.menu.tools"?
2. What's the id that should be returned? "-1" ?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #17)

> ::: mobile/android/chrome/content/browser.js

> > +
> > +    GECKO_TOOLS_MENU: function() {
> 
> We never name methods in ALL_CAPS. Please change it.
> 
> I guess,
> 
>  toolsMenu: -1,

Yes. This. And since this is acting like a menuID, I think it should be: toolsMenuID

> Or may be a getter for it.

No getter is needed

(In reply to Sriram Ramasubramanian [:sriram] from comment #18)
> I would like to know the discussion points from IRC.
> 1. What's the method to get the toolsMenu?
> "NativeWindow.menu.tools"?

NativeWindow.menu.toolsMenuID

> 2. What's the id that should be returned? "-1" ?

That should not matter. Anyone using the constant directly should be shot. For now "-1" is fine, but we might change it later.

Comment 20

[Mark Capella [:capella]]

Re: GECKO_TOOLS_MENU, I used that so it matches the const in the Java and search for string would turn up both sides. I guess that's not terrible important and I'll use your suggestion.

Re:
The entire logic should be like:

if (info.parent == 0) {
    menu = mMenu;
} else if (info.parent == -1) {
    menu = mMenu.findItem(R.id.tools).getSubMenu();
} else {
    ... existing block;
}


Unless I messed up something, I'm not able to use the code. It fails for the situation where a user has added an item to the tools menu, then adds an item to it as the parent and the "convert to submenu" routine kicks it.

   mMenu.removeItem(parent.getItemId());
   menu = mMenu.addSubMenu(Menu.NONE, parent.getItemId(), Menu.NONE, parent.getTitle());

Here mMenu needs to reference the parent of the item being converted (The toolsmenu item in this example) or fall back to the actual mMenu root.

The .removeItem() part works since it scans the whole menu... but the subsequent .addSubMenu (As-Is) does not put the converted SubMenu item back under tools menu.


Re: Please change the add part to add an ADDON_MENU_OFFSET to the info.parent, only if the info.parent is not "-1".

I can't do this, as we need a way to tell where the user has specified no parent (value will be 0) or where he has specified his previously added item as the parent, and it is the first user added ID in the list (0 + ADDON_MENU_OFFSET).

Comment 21

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Capella [:capella] from comment #20)
> Re: GECKO_TOOLS_MENU, I used that so it matches the const in the Java and
> search for string would turn up both sides. I guess that's not terrible
> important and I'll use your suggestion.

It actually is important as it would be exposed to the addon developers. And it's better to follow the naming conventions used so far (like NativeWindow.menu , NativeWindow.toast). As per mfinkle's suggestion, please use: toolsMenuID

> 
> Re:
> The entire logic should be like:
> 
> if (info.parent == 0) {
>     menu = mMenu;
> } else if (info.parent == -1) {
>     menu = mMenu.findItem(R.id.tools).getSubMenu();
> } else {
>     ... existing block;
> }
> 
> 
> Unless I messed up something, I'm not able to use the code. It fails for the
> situation where a user has added an item to the tools menu, then adds an
> item to it as the parent and the "convert to submenu" routine kicks it.
> 
>    mMenu.removeItem(parent.getItemId());
>    menu = mMenu.addSubMenu(Menu.NONE, parent.getItemId(), Menu.NONE,
> parent.getTitle());
> 
> Here mMenu needs to reference the parent of the item being converted (The
> toolsmenu item in this example) or fall back to the actual mMenu root.
> 
> The .removeItem() part works since it scans the whole menu... but the
> subsequent .addSubMenu (As-Is) does not put the converted SubMenu item back
> under tools menu.
> 

I doubt that this would be the case. I'll try this locally.

> 
> Re: Please change the add part to add an ADDON_MENU_OFFSET to the
> info.parent, only if the info.parent is not "-1".
> 
> I can't do this, as we need a way to tell where the user has specified no
> parent (value will be 0) or where he has specified his previously added item
> as the parent, and it is the first user added ID in the list (0 +
> ADDON_MENU_OFFSET).

So the reason for adding an offset of 1000 was so that the "addon id" and the existing java id doesn't mix. In case of "info.parent" being 0, it will be treated as main menu. And anything else would be treated with an offset. Now the change would be to make "tools menu" as "-1".

...
int parent = message.getInt("parent");
info.parent = parent <= 0 ? parent : parent + ADDON_MENU_OFFSET;
...

Comment 22

[Mark Capella [:capella]]

I meant "my way for the const was not important", so of course I'll please use: toolsMenuID  :)

Comment 23

[Sriram Ramasubramanian [:sriram]]

There is no "tools" in v8 devices. How will this work there? We have a tools submenu only on API level 11+.

Comment 24

[Mark Capella [:capella]]

Yep - And I've tested that. Use of tools where none exists is trreated as using the main menu (ignored effectively).

Comment 25

[Mark Capella [:capella]]

Ok status report:

   I'm waiting for sriram to post his refined version of a patch for me to compare with what I think he wants, since the one he provided over IRC today pointed out a few things he wasn't aware of.

   We're close to mutual understanding, but at this point, we may have to let someone else do the final review as I'm not sure who we'd call the author :D

Comment 26

[Mark Capella [:capella]]

Attachment: Latest Addon Testcase

Latest addon TestCode

Comment 27

[Mark Capella [:capella]]

Attachment: Patch (v4)

And my slightly tweaked (still functional) patch as it stands.

Comment 28

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This takes care of removing and adding the menu item as a submenu. The actual parent menu is found and the menu item is readded as a part of that menu.

Comment 29

[Mark Capella [:capella]]

Attachment: Patch (v5)

Ick ... recursion my old nemesis ... we meet again ....

This should be about it sriram :) I finished the findParentMenu() routine you sketched out, so double check it, but I've tested it and it works with my examples.

I can't see any other logic holes we might have introduced, either, so let me know?

Comment 30

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 772670 [details] [diff] [review]
Patch (v5)

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

Looks good.

::: mobile/android/base/BrowserApp.java
@@ +1393,5 @@
>      }
>  
> +    private Menu findParentMenu(Menu menu, MenuItem item) {
> +        final int itemId = item.getItemId();
> +

No new line.

@@ +1399,5 @@
> +        for (int i = 0; i < count; i++) {
> +            MenuItem menuItem = menu.getItem(i);
> +            if (menuItem.getItemId() == itemId) {
> +                return menu;
> +            }

New line here.

Comment 31

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22236ba0181d

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/22236ba0181d