1598078 - [API Request] Implement the tools_menu context type

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[John Bieling (:TbSync)]

Firefox has a tools_menu context type, which allows add-on authors to add menu entries to the tools menu.

I think it is a good thing for Thunderbird as well.

Maybe we can also have file_menu to add entries to the file menu?

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

I agree with tools_menu, but I'm not too much of a fan for file menu itself. We should expose menus where there are other complementary APIs involved. For example, the Message menu might make sense, because there are items related to the selected message. There aren't any APIs I'm aware of that relate to file menu operations.

When defining the names for the context type we also need to keep in mind Thunderbird has more than one window type where extensions make sense. so we might want to use e.g. main_tools_menu (with tools_menu being an alias), and main_message_menu, or something like that.

Comment 2

[John Bieling (:TbSync)]

The "restart" Addon (currently using experiments) mentioned in one of the topicbox threads is a good candidate for the file menu, to place "restart" close to "exit".

Comment 3

[klaus]

Philipp, restart is a good candidate to go into file menu, I am just converting an restart addon to webext or experiment.

I read that you are hesitant about file menu, but how many 'bad' menu entries have addons put into file menu in the last years? Are all these restrictions worth the efford? Wouldn't a bad addon author rather start deleting messages than flood the file menu with unwanted menuitems?

Maybe have a separator that shows where TB menuitems end and where addon menuitems start? The user always has the option to deinstall the addon.

I would more see a problem if an addon could make addon deinstallation impossible or difficult by moving those menu entries out of the visible area.

Comment 4

[John Bieling (:TbSync)]

Attachment: bug1598078_implement_tools_menu.patch

Going for the tools_menu. File menu needs to go in different bug.

Did not implement an alias for tools_menu as suggested by Fallen, as the API did not seem to support something like that. This is the same tools menu as used in Firefox, so we should be ok.

Other tool menus (address book, composer) will have to have a (thunderbird-only) prefix.

I added a test and I added an extra wait for the menus being created, as I run into issues where the tools menu was clicked before the API added its menu. I updated all other tests to wait for that as well.

Is this ok?
Any objections to adding the tools menu for composer and addressbook as well?

Comment 5

[Geoff Lankow (:darktrojan)]

Comment on attachment 9203311 [details] [diff] [review]
bug1598078_implement_tools_menu.patch

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

Yeah, I'm okay with this. It's weird that it doesn't extend to the hamburger menu, but Firefox does the same so don't add it.

::: mail/components/extensions/parent/ext-menus.js
@@ +588,5 @@
>    onVideo: "video",
>  
>    onBrowserAction: "browser_action",
>    onTab: "tab",
> +  onTools: "tools_menu",

Use the same identifier as Firefox: inToolsMenu

@@ +594,5 @@
>    selectedFolder: "folder_pane",
>    attachments: "compose_attachments",
>  };
>  
>  const getMenuContexts = contextData => {

In this function "all" should not include "tools_menu", and there's a comment that maybe should say Thunderbird instead of Firefox.

::: mail/components/extensions/schemas/menus.json
@@ +38,5 @@
>      "types": [
>        {
>          "id": "ContextType",
>          "type": "string",
> +        "enum": ["all", "page", "frame", "selection", "link", "editable", "password", "image", "video", "audio", "browser_action", "tab", "message_list", "folder_pane", "compose_attachments","tools_menu"],

There's a space missing here, or you could wrap the list.

@@ +43,1 @@
>          "description": "The different contexts a menu can appear in. Specifying <code>all</code> is equivalent to the combination of all other contexts except for <code>tab</code>."

This should be updated, all doesn't include tools_menu.

::: mail/components/extensions/test/browser/browser_ext_menus.js
@@ +291,5 @@
>      window.MsgToggleFolderPane();
>    }
>  });
>  
> +add_task(async function test_tools_menu() {

You'll find this doesn't work on Mac. We can't test clicking on menus there because they're part of the OS. 
Add `.__skipMe = AppConstants.platform == "macosx"` at the end (between the round bracket and the semicolon).

While you're there add a blank line before the next function.

Comment 6

[John Bieling (:TbSync)]

Attachment: bug1598078_implement_tools_menu_v2.patch

Fixed all review comments and added the other two menu entries.

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to John Bieling (:TbSync) from comment #4)

Any objections to adding the tools menu for composer and addressbook as well?

Sorry, I never really responded to this, or realised quite what your intention was.

I can see that there's a potential use case for an address book tool or a compose tool that would want to only appear on the tools menu of the appropriate window(s). That it can be done by adding a single property at creation time seems good. But it's inflexible. What works now may not in the future. For example, it's planned to move the address book to a tab in the main window, which makes the addressbook_tools_menu context obsolete. Also what happens if (when) we decide to include other menus, do we just keep adding contexts for every menu in every window?

Here's how I think we should approach it: for the tools_menu context, add the menu item to the tools menu in all of the windows that have one (including messageWindow.xhtml which you seem to have forgotten about) and let the extension choose whether it is relevant. This can be done with an onShown listener using the tab argument provided:

browser.menus.onShown.addListener(
  (info, tab) => browser.menus.update(info.menuIds[0], { visible: tab.mailTab })
);

To facilitate this, as I think we've discussed before, I think we should add tab type information to all tabs returned by API functions¹, then you could set the visibility based on tab.type == "addressbook" or something like that. (In this way it wouldn't matter if the address book was in a window or a tab. Also we can add othe types such as "calendar" for more flexibility.) Perhaps we could even set the visibility based on tab type inside the menus API.

¹ I'm happy to implement this myself unless you want to. I may have even filed a bug somewhere already.

Comment 8

[John Bieling (:TbSync)]

Attachment: bug1598078_implement_tools_menu_v3.patch

bad patch

Comment 9

[John Bieling (:TbSync)]

Attachment: bug1598078_implement_tools_menu_v4.patch

bad patch

Comment 10

[John Bieling (:TbSync)]

Attachment: bug1598078_implement_tools_menu_v5.patch

Oh yes, I like that tab type approach. For this bug I reverted to a single "tools_menu" context and added the missing messagewindow test (now with a subtest_tools_menu to reduce redundancy).

The type on the tab is independent of this bug and we can move this forward without waiting for the tab type, right? I will have a look and if I see how to do that, I assign myself, if not I will NI you.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9204214 [details] [diff] [review]
bug1598078_implement_tools_menu_v5.patch

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

::: mail/components/extensions/parent/ext-menus.js
@@ +1008,5 @@
>      const menu = event.target;
>      switch (menu.id) {
> +      case "taskPopup": {
> +        let win = menu.ownerGlobal;
> +        let info = { menu, inToolsMenu: true, tab: win };

It took me a moment to work out why you're setting `tab` to `win`. Since that applies in all windows except messenger.xhtml, set info.tab in an else block below.

@@ +1016,5 @@
> +        ) {
> +          info.tab = tabTracker.activeTab;
> +          // Calendar and Task view do not have a browser/URL.
> +          if (info.tab.linkedBrowser) {
> +            info.pageUrl = info.tab.linkedBrowser.currentURI.spec;

currentURI might not exist. Weird but true.

::: mail/components/extensions/test/browser/browser_ext_menus.js
@@ +294,5 @@
>      window.MsgToggleFolderPane();
>    }
>  });
>  
> +async function subtest_tools_menu(type, expectedInfo, expectedTab) {

"type" isn't very descriptive. "windowType"? In fact, you should be doing the window opening/closing in the calling function and passing a window.

@@ +307,5 @@
> +      await focusWindow(testwindow);
> +      break;
> +    case "message":
> +      // Even though we specify a message in openNewWindowForMessage,
> +      // we still have to select the folder.

Eh?! … Ohhh, I see the problem. Of course we have multiple ways to open a message window and of course I chose a bad one.

You should be able to avoid this messing about with folders using `MailUtils.openMessageInNewWindow` in head.js  instead of MsgOpenNewWindowForMessage.

Comment 12

[John Bieling (:TbSync)]

Attachment: bug1598078_implement_tools_menu_v6.patch

I fixed all reported issues. Seems I missed you on matrix, so I do ask for another review to be on the save side.

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9204526 [details] [diff] [review]
bug1598078_implement_tools_menu_v6.patch

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

::: mail/components/extensions/parent/ext-menus.js
@@ +1017,5 @@
> +          info.tab = tabTracker.activeTab;
> +          // Calendar and Task view do not have a browser/URL.
> +          if (info.tab.linkedBrowser?.currentURI) {
> +            info.pageUrl = info.tab.linkedBrowser.currentURI.spec;
> +          }

Why not just `info.pageUrl = info.tab.linkedBrowser?.currentURI?.spec`?

::: mail/components/extensions/test/browser/head.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +var { MailUtils } = ChromeUtils.import("resource:///modules/MailUtils.jsm");

Please put this after MailServices. I usually try to maintain alphabetical order (of the imported objects) where possible.

Comment 14

[John Bieling (:TbSync)]

Attachment: bug1598078_implement_tools_menu_v7.patch

Fixed the reported issues.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4e67bbcddb96
Implement the tools_menu context type. r=darktrojan

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ace8d4aec10f
follow-up - Fix browser_ext_messageDisplayAction.js after change to openNewWindowForMessage. rs=bustage-fix

Comment 17

[John Bieling (:TbSync)]

Comment on attachment 9204555 [details] [diff] [review]
bug1598078_implement_tools_menu_v7.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):
I am evaluating.