553747 - making unified folders extensible

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Shane Caraveo (:mixedpuppy)]

this is a draft of an idea for making unified folders a bit more extensible.  

The primary issue is that unified folders depends on folder flags (see nsMsgFolderFlags.idl) which is a 32bit bitmap, and while there are a couple unused bits, addons don't have a valid way to add new types of smart folders.

The idea is to use one of the unused bitmaps to mark smart folders created by addons.  smart folders with that bitmap must also have a string property on the folder (which I currently set to the name of the containing smart folder).

Code to create a new smart folder type is:

let gSmartMode = gFolderTreeView._modes["smart"];
gSmartMode.addonSmartFolderType("Mailing Lists", false);


To mark a folder as a smart folder you then do:

folder.setFlag(0x00010000 /*Components.interfaces.nsMsgFolderFlags.Unused4*/);
folder.msgDatabase.dBFolderInfo
  .setCharProperty("searchFolderType", "Mailing Lists");


The patch that I will attach contains the patch from bug 553633 as well, since it is necessary.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: extensibility for smart folder types

Comment 2

[Rain [:rain1] (inactive)]

Comment on attachment 433668 [details] [diff] [review]
extensibility for smart folder types

I haven't looked at the patch too deeply, but a couple of quick comments:

Right now we have a fake account called "smart mailboxes" which provides 

>diff --git a/mail/base/content/folderPane.js b/mail/base/content/folderPane.js
>@@ -943,39 +954,42 @@ let gFolderTreeView = {
>    * @param position optional place to put folder item in map. If not specified,
>    *                 folder item will be appended at the end of map.
>    * @returns The smart folder's ftvItem if one was added, null otherwise.
>    */
>   _addSmartFoldersForFlag: function ftv_addSFForFlag(map, accounts, smartRootFolder,
>                                                      flag, folderName, position)
>   {
>     // If there's only one subFolder, just put it at the root.
>-    let subFolders = gFolderTreeView._allFoldersWithFlag(accounts, flag, false);
>+    let folderType = null;
>+    if (flag == 0x00010000 /*nsMsgFolderFlags.Unused4*/) {

- In nsMsgFolderFlags.idl, please rename Unused4 to what you're using it for now. (ExtendedSmart? dunno)
- You can use nsMsgFolderFlags.foo directly.

>-      let folderUri = smartRootFolder.URI + "/" + folderName;
>-      smartFolder = smartRootFolder.getChildWithURI(folderUri, false, true);
>+      smartFolder = smartRootFolder.getChildNamed(folderName);

In general it's a good idea not to mix patches up :) Usually if a patch depends on another patch in a different bug then one bug is marked as dependent on the other.

>+      
>+      /* support for addons to add special folder types */

Javadoc/doxygen style comment please, and please move this below _allFlags.

>+      addonSmartFolderType: function ftv_addSmartFolderType(aFolderName, isDeep) {
>+        this._flagNameList.push([0x00010000 /*Components.interfaces.nsMsgFolderFlags.Unused2*/,

That isn't Unused2.

As I mentioned in bug 553633, we'll need a method that'll return a mode.

Comment 3

[Rain [:rain1] (inactive)]

More quick comments:

I'd prefer if searchFolderType were renamed to smartFolderType and addonSmartFolderType to addSmartFolderType.

Completing the incomplete thought at the top: Right now we have a fake account called "smart mailboxes" which provides us all the smart mailboxes. I'm a little worried that multiple addons will clobber each other in _flagNameList. In any case the exact semantics of addSmartFolderType's interaction with _flagNameList aren't very clear. What does it mean to have multiple entries with the same flag in _flagNameList?

How do addons clean up if they wish to remove a smart folder they created?

The key should be the same regardless of localization, so you'll need to decouple it from what is displayed. You should do that in any case because addons should do their work in their own namespace (by prefixing the key they use). You'll also need to make sure addons are able to localize folder names.

_allFlags is cached, so you'll need to make sure it's updated (possibly by removing the caching).

This needs a lot of tests. Scenarios that come to mind include:
- making sure that updates happen if a new folder is created with that type
- making sure that multiple addon-created smart folders work.
- some sort of cleanup

Comment 4

[David :Bienvenu]

As I said on IRC, I don't like stealing folder flags for this. Those folder flags are eventually going to get used, and that will break your extension/extensibility.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: new patch for addons

This patch:
- fixes having search folders with spaces in the name
- consolidates getProperties for items in the tree
- allows for css to override folder icons
- allows addons to add new unified folder types without using nsMsgFolderFlags
- allows a smart folder to not search all it's children

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to comment #5)
> Created an attachment (id=438191) [details]
> new patch for addons

Shane is this work in progress ? If not it lacks tests, and review requests.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=438191) [details] [details]
> > new patch for addons
> 
> Shane is this work in progress ? If not it lacks tests, and review requests.

I'm currently working on mozmill tests for this, when that is ready I will request the reviews, hoping to do that today.

Shane

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: mozmill tests for custom smart folders

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: mozmill tests for custom smart folders

minor fix, meant one test to recurs into subfolders

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: allfoldersview.png

An example of what this patch allows an addon to do.  This shows the mailling list manager and bulk filters plugins I released on addons last week.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: unifiedfoldersview.png

ui of custom smart folders in the unified view

Comment 12

[Rain [:rain1] (inactive)]

Comment on attachment 438496 [details]
mozmill tests for custom smart folders

Could you please submit it in patch format so that our usual tools work with it? Thanks.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: folderpane patch + mozmill tests

mozmill test file added to patch

Comment 14

[David :Bienvenu]

Comment on attachment 438513 [details] [diff] [review]
folderpane patch + mozmill tests

this patch changed the order of the inboxes under the smart inbox, which is pretty jarring. The order of traversal when building up the smart sub-folder items probably changed. The default account really should be first, and the inboxes should match the order in the all folders view.

don't need braces here:
+    if (flag == 0) {
+      return gFolderTreeView._allFoldersWithStringProperty(accounts, folderName, deep);
+    }

could even use the ? operator...

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to comment #14)
> (From update of attachment 438513 [details] [diff] [review])
> this patch changed the order of the inboxes under the smart inbox, which is
> pretty jarring. The order of traversal when building up the smart sub-folder
> items probably changed. The default account really should be first, and the
> inboxes should match the order in the all folders view.
> 
> don't need braces here:
> +    if (flag == 0) {
> +      return gFolderTreeView._allFoldersWithStringProperty(accounts,
> folderName, deep);
> +    }
> 
> could even use the ? operator...

removing the sortFolderItems I added returns the order to the previous way, I'm looking at how I can sort only the new custom folders.

Shane

Comment 16

[David :Bienvenu]

Comment on attachment 438513 [details] [diff] [review]
folderpane patch + mozmill tests

great, thx, I'll try a new patch once you've got one...

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: folderPane.patch + mozmill tests

fix items from comment 14

Comment 18

[David :Bienvenu]

Comment on attachment 439601 [details] [diff] [review]
folderPane.patch + mozmill tests

+   * Retreives a specific mode object

Retrieves

+        // if this folder doesn't have a property set, check it's children

"Its"

+    return flag == 0 ?
+      gFolderTreeView._allFoldersWithStringProperty(accounts, folderName, deep) :
+      gFolderTreeView._allFoldersWithFlag(accounts, flag, deep);
+  },

should switch to return flag ? and switch the order of the terms, instead of (flag == 0)

+      else if (flag) {
         child.useServerNameOnly = true;
+      } else {
+        child.addServerName = true;
       }

don't need braces (and we don't do } else { )

+          if (this.getSmartFolderTypeByName(smartFolderName) ||
+              this.getSmartFolderTypeByName(aFolder.name)) {
+            return true;
+          }

don't need braces here either.

This patch looks ok, but it has bit-rotted a bit. Can you attach a new patch that's not bit-rotted, and fixes the above nits, so I can make sure the mozmill tests work?

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: updated patch for folderPane.js

I'm still working on a test failure, but this patch is against the latest pull on trunk.  The test failure is test_folder_flag_changes in test-smart-folders.js.  The change to using getChildNamed rather than getChildWithURI is the cause of the failure.  Reverting those changes causes folders with spaces in the name to fail.  I have a different fix in mind for that.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Attachment: folderPane.patch + mozmill tests

new patch fixes test failure in test-smart-folders.js caused by using getChildNamed, now decoding the URI instead.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: folderPane.patch + mozmill tests

final patch, all tests passing

Comment 22

[David :Bienvenu]

Comment on attachment 440084 [details] [diff] [review]
folderPane.patch + mozmill tests

+   * Retreives a specific mode object

Retrieves still spelled wrong.

+      } else {
+        // if this folder doesn't have a property set, check Its children
+        this._subFoldersWithStringProperty(child, folders, aFolderName, deep);
+      }

still don't need braces here (and we don't do } else {.

+          if (this.getSmartFolderTypeByName(smartFolderName) ||
+              this.getSmartFolderTypeByName(aFolder.name))
+            return true;

you can simply return the value of the if expression here.

+    if (child._folder.URI == childFolder.URI) return true;
+    if (recurse) {
+      if (FTVItemHasChild(child, childFolder, recurse)) return true;
+    }

return should go on its own line for the first if. For the second one, you don't need braces, but simpler would be to combine the if statements:

if (recurse && FTVItemHasChild(child, childFolder, recurse))
  return true;

I assume the sleeps() were for debugging. Those should get removed.

r=me, with those nits all fixed. If you attach a new patch with the nits fixed, I can check it in for you.

I did see a ton of assertions running the folder-display tests, but I doubt they're related to this patch.

Comment 23

[Shane Caraveo (:mixedpuppy)]

Attachment: final folderpane patch

Here's a final patch with changes from the last review.  There is one additional change to "isSmartFolder".  The check on the smartFolderName string property was limited to virtual folders, however, one of my extensions marks real folders to be children of a smart folder.  Removing that limitation allows those to work correctly in the unified folder mode.

Comment 24

[David :Bienvenu]

fix checked in.