305340 - Smart folders, with Recent Searches in the Folder Pane

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[M-Rick]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr-FR; rv:1.7.10) Gecko/20050717 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; fr-FR; rv:1.7.10) Gecko/20050717 Firefox/1.0.6

Add smart folders like in Apple Mail.

http://www.apple.com/macosx/features/mail/

Stop Looking, Start Finding

Search across even the largest mailbox with pinpoint accuracy. Mail displays the
most relevant emails at the top of the search results and lets you change your
search on the fly. Search selected mailboxes or across all mailboxes. Search
across the entire message or content or just the from, to or subject fields.
http://images.apple.com/macosx/features/mail/images/aunthelen20050412.gif

Reproducible: Always

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

UI idea -> enhancement.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

bug 446306 and bug 452281 have shared designs for what we're calling "Starred Searches"

In a similar method to when you star (bookmark) a URL in firefox we're going to allow people to build up searches in Thunderbird and then Star those searches.  Starred searches will appear in the Folder Pane list below your other folders.  See both references bugs.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

setting our sights for b3 and assigning to david

There are two pieces to this bug.  One is having a Searches section of the Folder Pane where Saved Searches can live across accounts.  The other is a Recent Searches section that creates a section of the Folder pane to provide us with a focal point in the Folder Pane when in a search results tab.

Comment 4

[David :Bienvenu]

Attachment: wip

This gets started implementing mail.app's All Folders view where there's a smart INBOX that combines all the inboxes, smart Drafts that combines all the drafts, etc. I'm not sure if there's a separate bug for that...This adds a new mode to the folder pane that adds cross-folder views for all the special folders, with sub-folders for the actual special folders. One interesting thing is that I think these smart folders want to add their sub-folders, except for the inbox, because of the IMAP servers that have all folders as sub-folders of the inbox.

Comment 5

[David Ascher (:davida)]

In Mail.app (and I think in other mail apps), the term "smart folder" is used to refer to what we call saved searches.  I worry that using the term smart folder in the code to mean these specific things is going to confuse someone down the line, although they may already have their head fried by then, so they may not notice.  

(there's a separate topic as to whether our saved searches should be called something else, which I believe is true, but which is orthogonal to the issue of variable names in this patch)

Why wouldn't the Inbox want to have per-server subfolders?  IIRC and FWIW, that's what Mail.app does.  It's a good question whether the IMAP inbox subfolders should be subfolders of these sub-inboxes, or not.  I suspect in some IMAP setups, they should be, and in others, not.

Is there an IMAP way to know whether a folder is "an inbox-kind-of-thing"?

Comment 6

[David :Bienvenu]

(In reply to comment #5)
> In Mail.app (and I think in other mail apps), the term "smart folder" is used
> to refer to what we call saved searches.  I worry that using the term smart
> folder in the code to mean these specific things is going to confuse someone
> down the line, although they may already have their head fried by then, so they
> may not notice.  
yeah, I should call them something else, maybe xAccountFolders - though they are technically saved searches :-)
> 
> Why wouldn't the Inbox want to have per-server subfolders?  IIRC and FWIW,
> that's what Mail.app does. 

No, even if all the folders in an imap account are sub-folders of the inbox, they aren't shown in the "Mailboxes" part of the mail.app folder pane, at least in my case. I'll switch what I call that part of the hierarchy in my patch to Mailboxes, though I'm not sure what Bryan wanted it called.

> It's a good question whether the IMAP inbox
> subfolders should be subfolders of these sub-inboxes, or not.  I suspect in
> some IMAP setups, they should be, and in others, not.
> 
> Is there an IMAP way to know whether a folder is "an inbox-kind-of-thing"?

I assume we're talking about the same thing, but just to be clear, in some cases, the IMAP server only allows folders that are sub-folders of the INBOX, so all folders are sub-folders of the INBOX. If the server supports namespaces, it will tell you that the personal namespace is "INBOX.", so in that case, we do know. There are bugs about promoting those INBOX sub-folders to top-level folders and I think it's generally a desirable thing to do, but it's a little bit tricky not to break anything. Perhaps something for b3.

Conversely, servers that don't require all folders to be sub-folders of the inbox don't tend to allow the INBOX to have sub-folders, though I'm sure that's not an absolute.

Comment 7

[David Ascher (:davida)]

David, simple Q -- i notice that in your new folder mode (yeah!) the smart folders are all listed under a server -- i assume that's temporary, and that we want all those folders to be "roots" in the folderpane, in the right order, w/ the icons, etc?

Mailboxes does sound good to me, btw.

Comment 8

[David :Bienvenu]

I tried having the xAccountFolders as top-level folders first but I didn't care for how it looked, or the fact that you couldn't collapse them all away easily - mail.app puts them under a fake account called Mailboxes so I tried something like that instead. It's something we can play with, of course. We could have six or seven xAccountFolders (inbox, drafts, templates, sent, trash, archives, outbox) and that takes up a fair amount of the folder pane.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Maybe we need some subtle style changes, but with those mailboxes sorted at the top we wouldn't want to allow them to collapse.  Maybe there's some way we can accommodate collapsing, but the collapse is more of a large usability loss for a smaller efficiency gain for users with lots of folders.

Comment 10

[David :Bienvenu]

Attachment: more wip

This adds a property to virtual folders that allows us to create saved searches over all folders with a given flag set (it occurred to me that regexp over uri's wasn't going to work for users who pick different folders as their special folders). And I made it so we recalculate the scopes for these special saved searches at startup, so we'll notice if accounts are added or removed, or different folders specified as special. I also made it so the messages in sub-folders of Archive and Sent show up in the Archive/Sent smart mailboxes, since that's desirable (unlike, say, the Inbox). I'm not sure if I want to do that for Trash, but I'm leaning towards not.

This patch also takes advantage of the other work I did to allow you to hide accounts, so smart mailboxes don't show up elsewhere in the UI.

I need to make it so creating/removing accounts updates the smart mailboxes w/o requiring a restart, but this feature is becoming pretty useful for me already.

Comment 11

[David :Bienvenu]

moving to m5 - I worked a bit on getting adding/removing of accounts to update the smart mailboxes but it turned out that I needed to add notifications in those events...

Comment 12

[David :Bienvenu]

Attachment: backend changes, almost ready for review

These are the backend changes - I'm going to make a pre-review pass and then request review.

Comment 13

[David :Bienvenu]

Attachment: front end changes, almost ready for review

these are the front end changes, almost all in folderPane.js - I'm going to make a quick pre-review pass and then ask for review.

Comment 14

[David :Bienvenu]

Attachment: backend changes, ready for review

These backend changes do the following:

Implement smart mailboxes, i.e., mailboxes that are saved searches over all the messages of all the folders with a given flag, e.g., all Inboxes. I made these a special kind of saved search, and update the scope whenever that kind of saved search is loaded on startup.

Update saved search scopes when folders are removed.

Update smart mailboxes when folders are added, by checking if the flag of the added folder corresponds to the smart mailbox flag.

Added folder notifications when accounts are added or removed, for both the root folder, and the folders in the account. This allows me to keep the smart mailboxes up to date, and will allow me to change the folder pane to use the normal folder notifications instead of listening for the account list pref changing. I have not tried these changes with SM, though I don't think they should affect it. This should help indexers and other listeners know when accounts are removed (previously, I don't think they would have a clue).

Comment 15

[David :Bienvenu]

Attachment: front end changes, requires backend changes

These are the front end changes that add a new mode to the folder pane, Smart Folders, that is quite similar to Mail.app. It also changes the way the folder pane detects accounts getting added and removed.

I'd like to get this in reasonably soon so people can play with it. Once the backend work is landed, and the new mode added, it's fairly easy to tweak the new mode's behavior and look. 

I forgot to mention that this gives us a global Archives folder again, by including all the sub-folders of the Archive folder in the smart mailbox.

Comment 16

[[e-mail]]

(In reply to comment #14)
> This should help indexers and other listeners know when
> accounts are removed (previously, I don't think they would have a clue).
I thought we had a notification for this, but unfortunately turned out to be described as obsolete or deprecated or some such :-(

Comment 17

[[e-mail]]

Comment on attachment 373240 [details] [diff] [review]
backend changes, ready for review

>+    // Force built-in folders to be created and discovered. Then, notify listeners
>+    // about them.
>+    nsCOMPtr<nsISimpleEnumerator> enumerator;
>+    rv = rootFolder->GetSubFolders(getter_AddRefs(enumerator));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    PRBool hasMore;
>+    while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore)
>+    {
>+      nsCOMPtr<nsISupports> item;
>+      enumerator->GetNext(getter_AddRefs(item));
>+
>+      nsCOMPtr<nsIMsgFolder> msgFolder(do_QueryInterface(item));
>+      if (!msgFolder)
>+        continue;
>+      nsCOMPtr<nsIMsgFolder> parent;
>+      msgFolder->GetParent(getter_AddRefs(parent));
>+      if (parent)
Won't the root folder be the parent of its subfolders?

>+#define  SEARCH_FOLDER_FLAG "searchFolderFlag"
Is the double-spacing deliberate?

>+                if (buffer.Length())
Nit: !buffer.IsEmpty() [multiple occurrences]

>+        else if (dbFolderInfo && Substring(buffer, 0, 17).Equals(SEARCH_FOLDER_FLAG"="))
>+        {
>+          buffer.Cut(0, 17);
>+          dbFolderInfo->SetCharProperty(SEARCH_FOLDER_FLAG, buffer);
Nasty mixture of #define and inline constants. At least with the inline strings you could see how the integer's value was related to the string. I guess you could use sizeof(SEARCH_FOLDER_FLAG) instead of 17 but you might want to add a separate definition.

>+    VirtualFolderChangeListener *listener =
>+      static_cast<VirtualFolderChangeListener *>(m_virtualFolderListeners[i]);
I can't see a way around this except to change m_virtualFolderListeners to nsTArray<nsRefPtr<VirtualFolderChangeListener> >

>+  // Create an nsIMutableArray from the nsISupportsArray
Is there a followup bug on converting ListDescendents to an nsIMutableArray?

>+        // found a saved search over folders w/ the same flag as the new folder.
>+        if (vfFolderFlag & folderFlags)
>+        {
>+          nsCString searchURI;
>+          dbFolderInfo->GetCharProperty(kSearchFolderUriProp, searchURI);
>+
>+          if (searchURI.Length())
Surely if a saved search exists, then it will have a URI?

>+        // remove |folderURI
>+        searchURI.Cut(index, removedFolderURI.Length() - 1);
>+        // check if first or last chars are '|' and remove them, if so.
>+        if (searchURI.First() == '|')
This will always be true, since either
a) index > 0
b) index = 0 and this is the only uri, so the '|' you appended is still there
c) index = 0 and there is at least one other uri, so the '|' that separated the two URIs is still there

>+          searchURI.Cut(0, 1);
>+        if (!searchURI.IsEmpty() &&searchURI.CharAt(searchURI.Length() - 1) == '|')
Again, the CharAt (which would have been a Last(), right?) will always be true if the string isn't empty (correctly checked this time!)

I think it might be very slightly better to trim the trailing '|' first, then check if the string is empty, and if not, trim the leading '|' and update.

Comment 18

[David :Bienvenu]

Attachment: backend patch - fix adressing most of Neil's comments.

This addresses most of Neil's comments - I didn't get rid of the static_cast, since we do that in several other places, and fixing it would blow up the size of the patch.

There's a bug to move to frozen linkage, and maybe even one specifically to get rid of nsISupportsArray.

I left in a check for searchURI.IsEmpty() before adding '|'. In theory, we shouldn't have an empty scope (searchURI is the scope, not the saved search's URI), but better safe than sorry here, I think.

I also switched the order of removing the change listeners, so that we don't remove a listener out from under the array iteration.

Comment 19

[[e-mail]]

Comment on attachment 373656 [details] [diff] [review]
backend patch - fix adressing most of Neil's comments.

>+    // Force built-in folders to be created and discovered. Then, notify listeners
>+    // about them.
>+    nsCOMPtr<nsISimpleEnumerator> enumerator;
>+    rv = rootFolder->GetSubFolders(getter_AddRefs(enumerator));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    PRBool hasMore;
>+    while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore)
>+    {
>+      nsCOMPtr<nsISupports> item;
>+      enumerator->GetNext(getter_AddRefs(item));
>+
>+      nsCOMPtr<nsIMsgFolder> msgFolder(do_QueryInterface(item));
>+      if (!msgFolder)
>+        continue;
>+      nsCOMPtr<nsIMsgFolder> parent;
>+      msgFolder->GetParent(getter_AddRefs(parent));
parent == rootFolder, no?

>+        // remove |folderURI
>+        searchURI.Cut(index, removedFolderURI.Length() - 1);
>+        // remove last '|' we added
>+        searchURI.SetLength(searchURI.Length() - 1);
>+        // remove leading '|' we added (or one after |folderURI, if first URI)
>+        if (!searchURI.IsEmpty())
>+          searchURI.Cut(0, 1);
>+
>+        // saved search is empty now - delete it.
>+        if (searchURI.IsEmpty())
Techically we don't need to check IsEmpty twice, because if this was the last URI then it would already be empty after the SetLength, whereas if there's still a URI left then the extra | doesn't affect the emptiness.

Some IMAP stuff from another tree crept in ;-)

Comment 20

[David :Bienvenu]

Attachment: patch w/o imap diffs, and addressing Neil's parent comment

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 373240 [details] [diff] [review]
backend changes, ready for review

I think this one is obsolete now.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 373838 [details] [diff] [review]
patch w/o imap diffs, and addressing Neil's parent comment

>+    // this is the point at which we can notify listeners about the
>+    // creation of the root folder, which implies creation of the new server.
>+    nsCOMPtr<nsIMsgFolder> rootFolder;
>+    rv = aIncomingServer->GetRootFolder(getter_AddRefs(rootFolder));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIFolderListener> mailSession =
>+             do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    mailSession->OnItemAdded(nsnull, rootFolder);
>+    nsCOMPtr<nsIMsgFolderNotificationService> notifier(do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID, &rv));
>+    NS_ENSURE_SUCCESS(rv, rv);

nit: I think its generally better for readability if we have a blank line after NS_ENSURE_SUCCESS (in nsMsgAccountManager::GetAllFolders as well).

> 
>+  nsCOMPtr<nsIMsgFolderNotificationService> notifier(do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
>+  nsCOMPtr<nsIFolderListener> mailSession =
>+           do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv);

nit: you wrapped the second but not the first. 

>   rv = aAccount->GetIncomingServer(getter_AddRefs(server));
>-  if (NS_SUCCEEDED(rv) && server) {
>+  if (NS_SUCCEEDED(rv) && server)
>     rv = RemoveIncomingServer(server, PR_FALSE);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  }

We're not using rv now, so we don't need to assign it.

Comment 23

[David :Bienvenu]

Attachment: fix issues Standard8 saw with prev front end patch

this should fix the other folder views.

The issue was that I wanted to use addSubFolders, so I had to move it out to the ftv (folder tree view), but that also means I needed to do the same for _enumerateFolders.

Comment 24

[David :Bienvenu]

Comment on attachment 373850 [details] [diff] [review]
fix issues Standard8 saw with prev front end patch

My thought was that we would try this as a new folder pane mode first, and once any kinks are worked out, use it as the default folder pane mode. But this way gloda can add saved gloda searches here, if it wants.

Comment 25

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: smart folder screenshot w/ annotations

I like it a lot I just have some information redundancy questions.

For the child elements of the Inbox, Trash, etc.  can we remove the title folder name?

Right now I'm see this:

[-] ($) Inbox
 |-- ($) Inbox - [e-mail]

When I think we want to be as simple as this.  Not reusing the folder name or icon in the child elements.

[-] ($) Inbox
 |-- [e-mail]

Maybe the icon bit could be done through an attribute, like "smart_folder" such that CSS selectors could be made for these.  The extra attribute might make sense no matter what.  Also I think we'll likely need to bring in tree lines to give a tight parent / child relationship if we remove the icon and duplicate name.
 
And the Sent and Drafts should be using their regular icon.

Comment 26

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 373850 [details] [diff] [review]
fix issues Standard8 saw with prev front end patch

minus for now until we can figure out some of the minor issues with duplicate information

Comment 27

[David :Bienvenu]

(In reply to comment #25)
 
> For the child elements of the Inbox, Trash, etc.  can we remove the title
> folder name?

I need to teach the folder pane how to do that, but it should be possible.


> When I think we want to be as simple as this.  Not reusing the folder name or
> icon in the child elements.

that should be possible, though I don't know how the icon stuff works.


> And the Sent and Drafts should be using their regular icon.

They look right in my build on windows.

Are we going to add an icon for the Archives folder?

Comment 28

[Joey Minta]

(In reply to comment #27)
> I need to teach the folder pane how to do that, but it should be possible.
Just override the |text| getter of the ftvItems you're using here?

> that should be possible, though I don't know how the icon stuff works.
Override the getProperties function of the ftvItems?

Comment 29

[David :Bienvenu]

Attachment: get rid of special folder names for per-account inboxes

this uses just the server name for the special folders.

I'll see if I can add a property for these folders - one issue is that I still want almost everything setPropertyAtoms does so I can't not call it.

Comment 30

[David :Bienvenu]

Attachment: add selector for smart mailboxes

this adds a selector for the smart mailbox children so we can somehow hide the icons...

The following css basically gets rid of the icons:

/* leave tabmail icon alone, override folder pane icon for children
   of smart folders
*/
treechildren::-moz-tree-image(folderNameCol, specialFolder-Smart) {
  -moz-image-region: rect(0 1px 1px 0px);
}

but there must be a better way, eh? 

One note - if we lose the icons, we lose the new indicator, which is rather handy for inboxes, when you have multiple accounts.

Comment 31

[David :Bienvenu]

Attachment: fix count change assertions

this fixes the row count change assertions Standard8 was seeing - basically, we were getting notified about the new smart folders account from inside the generator, which kicked off a rebuild. Not what we want to happen. This patch should fix that.

clarkbw is going to worry about hiding the icons, since the way folder pane icons are done in the themes is undergoing a bit of flux.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 373838 [details] [diff] [review]
patch w/o imap diffs, and addressing Neil's parent comment

r=Standard8 subject to fixing the items mentioned in comment 22

Comment 33

[David :Bienvenu]

Attachment: backend changes addressing Standard8's comments

this addresses those comments, carrying forward standard8's r, requesting sr from Neil

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 373931 [details] [diff] [review]
fix count change assertions

>+    let smartName = document.getElementById("bundle_messenger").
>+      getString("folderPaneHeader_smart");

nit: . on the start of the second line please. Also align with the . after document.

>+  _addSmartFoldersForFlag: function ftv_addSmartFoldersForFlag(map, accounts, smartRootItem,
>+                                                               flag, folderName)
>+  {
>+    let smartFolder;
>+    try {
>+      smartFolder = smartRootItem._folder.getChildNamed(folderName);
>+    } catch (ex){smartFolder = null;};

nit: prefer this to be on a new line, or at least have some spaces around it.

The only potential issue I see now is if users use a TB 3 followed by downgrading to TB 2 - Smart Folders will still be there and can't be deleted. I'm not sure how we want to handle this.

However it'd be good to get this in for the test day tomorrow to get some feedback on it. r=Standard8, but we should think about the upgrade/downgrade experience as well.

Comment 35

[David :Bienvenu]

I wonder why TB 2 can't delete the smart folders account. It should look like a fairly normal account to TB2.

Comment 36

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 373931 [details] [diff] [review]
fix count change assertions

looks really good, thanks!

Comment 37

[[e-mail]]

Comment on attachment 374066 [details] [diff] [review]
backend changes addressing Standard8's comments

>+        if (!searchURI.IsEmpty())
>+          searchURI.Cut(0, 1);
>+
>+        // saved search is empty now - delete it.
>+        if (searchURI.IsEmpty())
>+        {
>+          nsCOMPtr<nsIMsgFolder> parent;
>+          rv = savedSearch->GetParent(getter_AddRefs(parent));
>+          NS_ENSURE_SUCCESS(rv, rv);
>+          db = nsnull;
>+          dbFolderInfo = nsnull;
>+          parent->PropagateDelete(savedSearch, PR_TRUE, nsnull);
>+        }
>+        else
>+          dbFolderInfo->SetCharProperty(kSearchFolderUriProp, searchURI);
[I still think that the Cut could be moved into this else "block".]

Comment 38

[David :Bienvenu]

fix checked in, with Neil's last comment addressed.

Let's open new bugs for any remaining issues. I know Bryan wants to change the themes so the sub-folders of the smart folders don't have icons, so I'll start with that bug.

Comment 39

[David :Bienvenu]

Mini feature description and test plan:

This feature adds a new folder pane mode, "Smart Folders", after "Recent Folders". In this mode, there is a new account at the top of the folder pane. This account contains top level folders for all the special folders, Inbox, Sent, Archives, Templates, Drafts and Trash. These folders are saved searches across all folders of a particular type, so Inbox, for example, should show you all the messages in all of your inboxes. The sub-folders of these saved searches are the individual special folders, with the name of the account instead of the folder name. If an individual special folder has sub-folders, we will display the actual sub-folders in the folder pane, except for the inbox.

Besides verifying that these smart folders show up correctly, and contain all the messages, you should check that adding and removing accounts causes the special folders for those accounts to be added or removed from the smart folders. (If you have a smart folder selected, we won't update it when an account is added, but if you click away and click back, we should update the contents).

Comment 40

[Valery Kondakoff]

Not all of the smart folders have a mouse right-click menu option 'Mark folder read': for example top-level Local folders inbox located in Smart folders pane does not have such an option. Is it a bug, or is it by design?

There is an old bug/RFE (405627): "Mark Folder Read" should be enabled for saved searches. Is it related to my request?

Thank you.