1625746 - folderpane contextmenu: allow separately for accounts, folders, etc

Status: RESOLVED FIXED

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

Description

[klaus]

User Agent: Mozilla/5.0 (Windows NT 10.0; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

I have a use scenario where an action would be applied on accounts, but not folders.

So it would make sense to allow a deeper specification where a contextmenu goes. It is clear that certain actions on accounts won't be possible in folders, so these should not appear in the contextmenu of folders in the foldertree.

Comment 2

[klaus]

ah, it seems I was not clear in the other feature proposal. Maybe it is so related that all should go here. sorry for that.

So what would help:
a) contextmenu displayed for all folders in a list (but not accounts, messages, events, tasks etc.). Same for the other entities. Like: backup/archive/whatever this folder. At the moment, the contextmenu comes up for the full folderpane, for example.

b) contextmenu for one folder/message/event/task in a list only.
Like: copy this message to CRM. Could be tied to a certain sender, would only appear on selected message(s), not on all in a messagelist. Same for folders, events, .... Or: convert to task in business calendar (if from business partners defined somewhere), convert to event in private calendar (if from private senders defined somewhere/private addressbook). So different context menu entry for different messages in the same list.

Comment 3

[Geoff Lankow (:darktrojan)]

So if I'm reading this right you want your menu item to only show for some items and not others? You can do this by adding an onShown listener: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/onShown.

Comment 4

[klaus]

thanks for pointing me there.

Some consideration: might not work if what I need to decide to display is async and too slow (MDN).

If more people/addons need it, we will have millions of listeners. I don't know whether that is of concern. So the idea was: we have some basic entities (accounts, folders, messages, events, calendars, tasks, make it separate for them (if the basic Mozilla code would even allow that?).

Comment 5

[John Bieling (:TbSync)]

The requested feature is indeed possible via the onShown event. The documentation is currently missing the two fields selectedFolder and selectedMessages, which are included in the info object of the onShown event.

I observed two things, where feedback from Geoff would help:

1. If an account is clicked on, the path of the selectedFolder returns as "/", which can be used to distinguish between folder and account.
   -> This could be optimized to not include a path at all, as we do not want developers to rely on the structure of the path? Or will the general filesystem-like structure remain and "/" is save to be assumed as the root and developers can explicitly check for path == "/"?

2. If Thunderbird is set to view the folders unified, the onShown/onClicked info will return a fake account id. Simple reproduction steps: I have two accounts (checked via accounts.list()), my test account and the local account. Clicking on "Trash", which contains trash of my test account (id1) and the trash of the local account (id2), will include id3 as the account id of the selected folder.
   -> Should we do something about this?

Comment 6

[John Bieling (:TbSync)]

Attachment: bug1625746_add_missing_members.patch

Comment 7

[John Bieling (:TbSync)]

Attachment: bug1625746_add_missing_members_esr78.patch

Comment 8

[John Bieling (:TbSync)]

I change tactics here for the future. I find errors in our documentation quite often, but having them as individual bugs in review queue (and sometimes in parallel) makes working with the code difficult. I will just keep a general documentation changeset locally and report all found issues in a single bug maybe once per month.

Feedback from Geoff on Comment 5 is appreciated.

Comment 9

[Geoff Lankow (:darktrojan)]

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

1. If an account is clicked on, the path of the selectedFolder returns as "/", which can be used to distinguish between folder and account.
   -> This could be optimized to not include a path at all, as we do not want developers to rely on the structure of the path? Or will the general filesystem-like structure remain and "/" is save to be assumed as the root and developers can explicitly check for path == "/"?

Yes, that does feel a bit odd. Since it's actually representing an account perhaps we shouldn't have a selectedFolder at all, and instead have a selectedAccount? I'd be okay with a null path too, given that you can't do anything with the folder, because it's not real.

2. If Thunderbird is set to view the folders unified, the onShown/onClicked info will return a fake account id. Simple reproduction steps: I have two accounts (checked via accounts.list()), my test account and the local account. Clicking on "Trash", which contains trash of my test account (id1) and the trash of the local account (id2), will include id3 as the account id of the selected folder.
   -> Should we do something about this?

Again these folders aren't real (but in a different way) so the other API functions won't do anything with them anyway. Maybe the best thing to do here is null the accountId property and set "virtual" as the type.

Up to now the accountId and path properties have always been given even when it's useless information so not giving them in some circumstances could cause a few problems with extensions.

Comment 10

[John Bieling (:TbSync)]

Attachment: bug1625746_return_selectedAccount_if_no_folder_but_account_is_selected.patch

This patch returns selectedAccount instead of selectedFolder, if the user opened the context menu on an account entry.

I also added the schema definitions for MailAccount and MailIdentity (since I needed MailAccount), which existed only in the overlay files. Also added a few other missing entries.

I did not do anything about the virtual folder thing, as the "fake" accountId of the unified folder returns a proper MailAccount of type "none" with name "Unified Folders". So this does look ok. (We could think about renaming "none" to "virtual", but that should go into a different bug).

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9206154 [details] [diff] [review]
bug1625746_return_selectedAccount_if_no_folder_but_account_is_selected.patch

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

This is fine but the commit message is pretty vague.

::: mail/components/extensions/parent/ext-mail.js
@@ +1415,5 @@
>  /**
> + * Converts an nsIMsgAccount to a simple object
> + * @param {nsIMsgAccount} account
> + * @return {Object}
> + */

I think putting this above convertMailIdentity would be more logical, to maintain some sort of conceptual hierarchy. (Accounts have identities and folders, folders have messages.)

::: mail/components/extensions/parent/ext-menus.js
@@ +690,5 @@
> +        let folder = convertFolder(contextData[folderType]);
> +        if (folderType == "selectedFolder" && folder.path == "/") {
> +          info.selectedAccount = convertAccount(
> +            MailServices.accounts.getAccount(folder.accountId)
> +          );

Add a comment about what we're doing here and when it applies.

::: mail/components/extensions/schemas/menus.json
@@ +575,5 @@
>                },
> +              "selectedMessages": {
> +                "$ref": "messages.MessageList",
> +                "optional": true
> +              },

It might be time to move all this to an OnShownData type, like onClicked has. Up to you, it's only really documentation that it affects.

Comment 12

[John Bieling (:TbSync)]

Attachment: bug1625746_add_selectedAccount_property.patch

Edit: Patch was missing some descriptions in the JSON schema file.

Comment 13

[John Bieling (:TbSync)]

Attachment: bug1625746_add_selectedAccount_property_v2.patch

Asking again for review because I added a test.

Comment 14

[Geoff Lankow (:darktrojan)]

Comment on attachment 9206346 [details] [diff] [review]
bug1625746_add_selectedAccount_property_v2.patch

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

r+ with a small change.

::: mail/components/extensions/test/browser/browser_ext_menus.js
@@ +375,5 @@
>    );
>    testwindow.close();
>  }).__skipMe = AppConstants.platform == "macosx";
>  
> +async function subtest_folder_pane_root_folder(...permissions) {

This doesn't need to be a separate function. Just add the code to subtest_folder_pane.

@@ +406,5 @@
> +  await extension.unload();
> +}
> +add_task(async function test_folder_pane_root_folder() {
> +  return subtest_folder_pane_root_folder("accountsRead");
> +});

You missed adding the no permissions version of this function. Not that it matters once you've fixed the comment above,.

Comment 15

[John Bieling (:TbSync)]

Attachment: bug1625746_add_selectedAccount_property_v3.patch

Updated the test as suggested.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b48894c4c8c5
Change menus API behavior when clicking on an account in the folder tree. r=darktrojan

Comment 17

[John Bieling (:TbSync)]

Comment on attachment 9206651 [details] [diff] [review]
bug1625746_add_selectedAccount_property_v3.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.