1499617 - Folder tab (3-pane) WebExtensions API

Status: RESOLVED FIXED

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

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-1.diff

This isn't complete by any means, but I think it's ready enough to give a good idea of what I'm doing.

There's some overlap with the tabs API, in that the first thing an extension should do is call browser.mailTabs.getAll() to find all the open 3-pane views, then browser.mailTabs.update() to change properties on those views. I've left the Quick Filter bar separate for now, but that might change.

As for doing things with folders and messages, I need to flesh out what a folder or message object looks like, which I'll do in bug 1488176. The current behaviour (using URIs and a bunch of headers) isn't intended to stay.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-2.diff

Iteration 2. There's still a few remaining things to do, but this is starting to look good. With this and the tabs API, an extension would be able to know what the user is doing in the UI.

See also the filter extension I made a week ago which uses some of this API. It's a webext version of a legacy extension I have. https://github.com/darktrojan/sample-extensions/tree/master/filter

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9020958 [details] [diff] [review]
1499617-webext-mailTabs-2.diff

I can't say anything qualified here. Ask Aceman if you need another opinion.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9020958 [details] [diff] [review]
1499617-webext-mailTabs-2.diff

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

Just a quick pass through

::: mail/components/extensions/parent/ext-mail.js
@@ +1051,5 @@
> + */
> +var messageTracker = {
> +  _nextId: 1,
> +  _messages: new Map(),
> +

I find the ids here quite confusing. 
For lookups maybe we can just use msgHdr.messageId as key?

::: mail/components/extensions/parent/ext-mailTabs.js
@@ +20,5 @@
> +  let sortColumn = folderDisplay.tree.treeBoxObject.columns.getSortedColumn() || null;
> +  let sortOrder = null;
> +  if (sortColumn) {
> +    sortColumn = sortColumn.id;
> +    sortOrder = folderDisplay.tree.view.sortOrder == 1 ? "ascending" : "descending";

I'd put () around the ternary for readabiliyty

@@ +119,5 @@
> +
> +        async update(tabId, args) {
> +          let tab = tabId ? tabManager.get(tabId) : tabManager.wrapTab(tabTracker.activeTab);
> +          if (!tab || !tab.isMail3Pane) {
> +            throw new ExtensionError("This is not a mail tab.");

`Not a tab; tabId=${tabId}`

@@ +131,5 @@
> +            if (tab.active) {
> +              let treeView = Cu.getGlobalForObject(tab.nativeTab).gFolderTreeView;
> +              let folder = MailServices.folderLookup.getFolderForURL(uri);
> +              if (folder) {
> +                treeView.selectFolder(folder);

maybe let it blow up if the folder doesn't exist? or throw? silently failing when assumptions fail makes things hard to debug

@@ +173,5 @@
> +
> +        async getSelectedMessages(tabId) {
> +          let tab = tabId ? tabManager.get(tabId) : tabManager.wrapTab(tabTracker.activeTab);
> +          if (!tab || !tab.isMail3Pane) {
> +            throw new ExtensionError("This is not a mail tab.");

here too

@@ +214,5 @@
> +
> +        onDisplayedFolderChanged: new EventManager({
> +          context,
> +          name: "mailTabs.onDisplayedFolderChanged",
> +          register: fire => {

parenthesis for the argument, like (fire) =>, seems clearer to me

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-3.diff

Comment 6

[Geoff Lankow (:darktrojan)]

For an easier-to-read overview, see https://thunderbird-webextensions.readthedocs.io/en/latest/mailTabs.html

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-4.diff

Oops, found a crucial mistake. Also note you'll need attachment 9022534 [details] [diff] [review] from bug 1488176 to test this.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9022535 [details] [diff] [review]
1499617-webext-mailTabs-4.diff

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

::: mail/components/extensions/parent/ext-mailTabs.js
@@ +39,5 @@
> +    mailTabObject.sortType = null;
> +    mailTabObject.sortOrder = null;
> +  }
> +  if (context.extension.hasPermission("accountsRead")) {
> +    mailTabObject.displayedFolder = convertFolder(folderDisplay.displayedFolder);

maybe move permission checks to start of the function?

@@ +124,5 @@
> +     *
> +     * @param {?Integer} tabId          The tab id to get
> +     * @return {Tab}                    The matching tab, or the active tab
> +     */
> +    function getTabOrActive(tabId) {

simply getTag would not be as awkward

@@ +183,5 @@
> +              let folder = MailServices.folderLookup.getFolderForURL(uri);
> +              if (folder) {
> +                treeView.selectFolder(folder);
> +              } else {
> +                throw new ExtensionError("Folder not found.");

please include the uri in the message (or accountId + path)

@@ +209,5 @@
> +              if (folderPaneVisible) {
> +                folderPaneSplitter.setAttribute("state", "open");
> +              } else {
> +                folderPaneSplitter.setAttribute("state", "collapsed");
> +              }

could use a ternary for the state value

::: mail/components/extensions/schemas/mailTabs.json
@@ +95,5 @@
> +                  "byReceived",
> +                  "byCorrespondent"
> +                ]
> +              },
> +              "sortOrder": {

we also support a secondary sort order

@@ +176,5 @@
> +                "type": "boolean",
> +                "description": "Shows only starred messages.",
> +                "optional": true
> +              },
> +              "addrBook": {

bad preexisting naming. inAddrBook, or contactsOnly would be much more understandable

Comment 9

[Magnus Melin [:mkmelin]]

Like Richard commented on IRC, the tag buttons are tree-state, so that needs some fixing

Comment 10

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin [:mkmelin] from comment #9)
> Like Richard commented on IRC, the tag buttons are tree-state, so that needs
> some fixing

Okay, the filters are three-state, but the buttons certainly aren't. If they were once, they're broken.

(In reply to Magnus Melin [:mkmelin] from comment #8)
> ::: mail/components/extensions/parent/ext-mailTabs.js
> @@ +39,5 @@
> > +    mailTabObject.sortType = null;
> > +    mailTabObject.sortOrder = null;
> > +  }
> > +  if (context.extension.hasPermission("accountsRead")) {
> > +    mailTabObject.displayedFolder = convertFolder(folderDisplay.displayedFolder);
> 
> maybe move permission checks to start of the function?

Why? It only applies to that line.

> @@ +124,5 @@
> > +     *
> > +     * @param {?Integer} tabId          The tab id to get
> > +     * @return {Tab}                    The matching tab, or the active tab
> > +     */
> > +    function getTabOrActive(tabId) {
> 
> simply getTag would not be as awkward

I don't think it's awkward, it's more accurate about what the function does.

> ::: mail/components/extensions/schemas/mailTabs.json
> @@ +95,5 @@
> > +                  "byReceived",
> > +                  "byCorrespondent"
> > +                ]
> > +              },
> > +              "sortOrder": {
> 
> we also support a secondary sort order

We support a lot more sort options than this, but I'm keeping it simple. I'm not sure extensions actually need control over the sort order at all.

> 
> @@ +176,5 @@
> > +                "type": "boolean",
> > +                "description": "Shows only starred messages.",
> > +                "optional": true
> > +              },
> > +              "addrBook": {
> 
> bad preexisting naming. inAddrBook, or contactsOnly would be much more
> understandable

Yeah. Going with "contact" at the moment, since it's similar to the others.

Comment 11

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #10)
> Okay, the filters are three-state, but the buttons certainly aren't. If they
> were once, they're broken.

Only the tag buttons are tri-state, atm.

> (In reply to Magnus Melin [:mkmelin] from comment #8)
> > ::: mail/components/extensions/parent/ext-mailTabs.js
> > @@ +39,5 @@
> > > +    mailTabObject.sortType = null;
> > > +    mailTabObject.sortOrder = null;
> > > +  }
> > > +  if (context.extension.hasPermission("accountsRead")) {
> > > +    mailTabObject.displayedFolder = convertFolder(folderDisplay.displayedFolder);
> > 
> > maybe move permission checks to start of the function?
> 
> Why? It only applies to that line.

Yes but why defer the check to as late as possible? Does the data returned without the permission make much sense? In general, returning different data depending on if you have permissions or not seems to make the testing matrix larger for little reason. 

> > simply getTag would not be as awkward
> 
> I don't think it's awkward, it's more accurate about what the function does.

Well it's still a tab, and without the argument, the current tab. OTOH, I wouldn't know what an Active is.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-5.diff

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-tests-1.diff

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9026839 [details] [diff] [review]
1499617-webext-mailTabs-5.diff

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

Looks reasonable to me. r=mkmelin

::: mail/components/extensions/schemas/mailTabs.json
@@ +233,5 @@
> +                "optional": true
> +              },
> +              "contact": {
> +                "type": "boolean",
> +                "description": "Shows only messages from people in the address book.",

this and the others should ideally also be exposed as tri-state (which the UI doesn't allow, though I think there's at least some add-on that does allow it, one by alta88 I think)

but maybe for a followup

Comment 15

[Geoff Lankow (:darktrojan)]

Well, they are optional, so the values possible are true/false/unspecified, but I could make them string arguments with three possible values. But what values? Best I can think of right now are "present", "absent", "any".

Comment 16

[Magnus Melin [:mkmelin]]

true/false/undefined is fine if that's possible (so you make false distinct from undefined)

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-tests-2.diff

These don't actually run as mochitest isn't ready yet, but they might as well be reviewed.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9029348 [details] [diff] [review]
1499617-webext-mailTabs-tests-2.diff

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

Looks reasonable, r=mkmelin

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/71c8bc1307b8
Folder tab (3-pane) WebExtensions API; r=mkmelin
https://hg.mozilla.org/comm-central/rev/4331140b9077
Folder tab (3-pane) WebExtensions API, mochitests; r=mkmelin

Comment 20

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

Comment on attachment 9026839 [details] [diff] [review]
1499617-webext-mailTabs-5.diff

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

Sorry again for the late comments and thanks for letting me give feedback. Here are some things that could be handled in a follow-up:

::: mail/components/extensions/schemas/mailTabs.json
@@ +17,5 @@
> +  },
> +  {
> +    "namespace": "mailTabs",
> +    "permissions": [
> +      "mailTabs"

Does this need a permission? Permissions make sense if the functions and events contain/provide information that could be considered private and should not be available without informing the user. It seems to me that the sensitive data, such as folder or message data, is gated by other permissions. Is there any sensitive data I missed?

@@ +83,5 @@
> +        "async": true,
> +        "parameters": []
> +      },
> +      {
> +        "name": "getCurrent",

The browser.tabs.getCurrent() function doesn't actually get the current tab, but the tab the current script is running in. To avoid false comparisons, maybe we should instead make this browser.mailTabs.query({ active: true, currentWindow: true }) ? This way in the future we could also query for a certain displayedFolder or one of the other properties.

@@ +85,5 @@
> +      },
> +      {
> +        "name": "getCurrent",
> +        "type": "function",
> +        "description": "Returns the current mail tab in the most recent window, or throws an exception if the current tab is not a mail tab.",

So the promise rejects when the mail tab is not selected? This way I'd have to use try/catch every time I use this function, and I am guessing you'd see a lot of:

let tab = await browser.mailTabs.getCurrent().catch(() => null);
if (tab) {
  ...
}

Maybe we should just return null instead of throwing?

@@ +128,5 @@
> +                "type": "string",
> +                "description": "Sorts the list of messages. <var>sortOrder</var> must also be given.",
> +                "optional": true,
> +                "enum": [
> +                  "byNone",

Would there ever be something not prefixed with "by" here? Maybe we can drope the prefix if not.

@@ +162,5 @@
> +                ]
> +              },
> +              "layout": {
> +                "type": "string",
> +                "description": "Sets the arrangement of the folder pane, message list pane, and message display pane. Note that setting this applies it to all mail tabs.",

Maybe it would make sense to split this out into something akin to browser.browserSettings, since it is not per-tab. A good followup though, hopefully before people get used to using this.

@@ +200,5 @@
> +          }
> +        ]
> +      },
> +      {
> +        "name": "setQuickFilter",

I'd enjoy if we can eventually make the quick filter expandable via add-on, which would require allowing additional properties here, or a specific property that allows to set a number of extension specific filters.

@@ +263,5 @@
> +          }
> +        ]
> +      }
> +    ],
> +    "events": [

Other ideas for events:
- sort order/type changed
- folder pane show/hide
- message pane show/hide
- layout changed (though also better in mailSettings)

Comment 21

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

Comment on attachment 9026839 [details] [diff] [review]
1499617-webext-mailTabs-5.diff

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

::: mail/components/extensions/schemas/tabs.json
@@ +226,5 @@
>            {
>              "type": "object",
>              "name": "queryInfo",
>              "properties": {
> +              "isMail3Pane": {

Can we get the "3pane" part out of this? This is a technical detail that I think would make sense to hide from the API. Maybe just "mailTab", whichh would also make it consistent w.r.t. use of "is". The others are not named e.g. "isActive" after  all.

Comment 22

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #20)
> Does this need a permission? Permissions make sense if the functions and
> events contain/provide information that could be considered private and
> should not be available without informing the user. It seems to me that the
> sensitive data, such as folder or message data, is gated by other
> permissions. Is there any sensitive data I missed?

I guess not. It's there because I put it in at the start when the other stuff didn't exist and just left it there.

> The browser.tabs.getCurrent() function doesn't actually get the current tab,
> but the tab the current script is running in. To avoid false comparisons,
> maybe we should instead make this browser.mailTabs.query({ active: true,
> currentWindow: true }) ? This way in the future we could also query for a
> certain displayedFolder or one of the other properties.

I was not aware (or I forgot) of that getCurrent behaviour. I think it's helpful to have this shortcut, but maybe it should be renamed browser.mailTabs.active to avoid confusion?

There's no query method because you can use browser.tabs.query with isMail3Pane: true for the same effect. 

> So the promise rejects when the mail tab is not selected?
> Maybe we should just return null instead of throwing?

Actually that's just inaccurate documentation. It returns null.

> @@ +162,5 @@
> > +              "layout": {
> > +                "type": "string",
> > +                "description": "Sets the arrangement of the folder pane, message list pane, and message display pane. Note that setting this applies it to all mail tabs.",
> 
> Maybe it would make sense to split this out into something akin to
> browser.browserSettings, since it is not per-tab. A good followup though,
> hopefully before people get used to using this.

Or we could change the UI so that it can be different in different tabs.

> I'd enjoy if we can eventually make the quick filter expandable via add-on,
> which would require allowing additional properties here, or a specific
> property that allows to set a number of extension specific filters.

The same actually applies to sorting, if we made it possible to add columns via API the sort method would need to be able to cope with it. If we ever do that we can change the schema without breaking it.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #21)
> > +              "isMail3Pane": {
> 
> Can we get the "3pane" part out of this? This is a technical detail that I
> think would make sense to hide from the API. Maybe just "mailTab", whichh
> would also make it consistent w.r.t. use of "is". The others are not named
> e.g. "isActive" after  all.

Yeah, now that you mention it, I agree.

Comment 23

[Geoff Lankow (:darktrojan)]

Reopening to address Philipp's comments.

Comment 24

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-followup-1.diff

Comment 25

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

(In reply to Geoff Lankow (:darktrojan) from comment #22)
> (In reply to Philipp Kewisch [:Fallen] [:📆] from comment #20)
> > The browser.tabs.getCurrent() function doesn't actually get the current tab,
> > but the tab the current script is running in. To avoid false comparisons,
> > maybe we should instead make this browser.mailTabs.query({ active: true,
> > currentWindow: true }) ? This way in the future we could also query for a
> > certain displayedFolder or one of the other properties.
> 
> I was not aware (or I forgot) of that getCurrent behaviour. I think it's
> helpful to have this shortcut, but maybe it should be renamed
> browser.mailTabs.active to avoid confusion?

There is browser.tabs.getSelected() which is like the shortcut you mention. It has been deprecated by Chrome, and also Firefox. See https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/getSelected

I don't think we should be introducing a shortcut if the related method is deprecated elsewhere, and using query() doesn't seem that much longer.

> 
> There's no query method because you can use browser.tabs.query with
> isMail3Pane: true for the same effect. 
Can you do browser.tabs.query({ displayedFolder: myFolderId }) ? Maybe moot point to discuss until we have added that feature for mailTabs though. At that point we can consider making browser.mailTabs.query() fall through to browser.tabs.query but add additional properties that we can query by.

tl;dr; for both items if we can do browser.tabs.query({ mailTab: true, active: true }); I think we should just get rid of the getSelected/getActive method.


> > Maybe it would make sense to split [.layout] into something akin to
> > browser.browserSettings, since it is not per-tab. A good followup though,
> > hopefully before people get used to using this.
> 
> Or we could change the UI so that it can be different in different tabs.
Not sure if this needs to be that fine grained, but if thats the direction we are going sure thing :)

Comment 26

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

Comment on attachment 9033841 [details] [diff] [review]
1499617-webext-mailTabs-followup-1.diff

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

r+ with the getActive method removed as per previous comment.

Comment 27

[Geoff Lankow (:darktrojan)]

Attachment: 1499617-webext-mailTabs-followup-2.diff

Comment 28

[Geoff Lankow (:darktrojan)]

I threw out the getAll method too, as it's the same as query({}).

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f8ef458411f4
follow-up - Folder tab (3-pane) WebExtensions API improvements; r=Fallen