516923 - Some folders do not appear in Smart folder view

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect, P1)

Description

[Gervase Markham [:gerv]]

Setup:

Thunderbird 3.0b4pre on Ubuntu Linux 9.04. Two IMAP accounts - "[e-mail]" and "[e-mail]", plus two feed accounts and two newsgroup accounts. 

In "All" folder view:

[e-mail] account has folders "Sent" (empty) and "sent-mail" (full; defined in prefs as sent mail folder), both with stamp icon. It has folders "archive" (full; defined in prefs as archive folder) and "Archives" (empty), both with belt buckle icon.

[e-mail] account has only "sent-mail" with Stamp icon, but has same arrangement with archive folders.

The context menu does not allow me to delete or rename any of the above-mentioned folders. 

The user prefs in prefs.js seem correct, so I don't know where this "two special folders" thing is coming from.

In "Smart" folder view:

"Sent" top-level folder picks up empty "Sent" folder from [e-mail] and calls it "[e-mail]".

"Archives" top-level folder similarly picks up empty "Archives" folders from both accounts.

"Drafts" top-level folder has picked up the [e-mail] defined Drafts folder ("[e-mail]", which is actually in Local Folders) but for some reason the parallel "[e-mail]" folder remains a top-level folder in this view, and is not subsumed under the "Drafts" top level.

Neither [e-mail]'s "sent-mail" or either "archive" folder is visible anywhere else in the folder tree. As these folders contain important messages, this could be considered dataloss. 

A screenshot is attached of the two views. Do let me know what other information I can provide.

Gerv
Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Screenshot of both views, side-by-side

Comment 2

[David :Bienvenu]

we need to figure out how to handle having multiple special folders in the same account.

Comment 3

[David :Bienvenu]

Attachment: get all special folders in each account

This makes it so we look for all special folders in an account. 

I've cc'ed clarkbw because I think we need some UI advice about how to present multiple special folders in the same account. For example, I can have the default Sent folder in an account, and then specify some other random folder in that same account as the sent folder for a different account. Prior to this patch, the second folder wouldn't appear at all in the smart folders view. With this patch, there will be two sub-folders of the Smart sent folder with the same account name, so the user can't distinguish between them (but at least they can see both of them). We could try to detect having multiple special folders in an account and do something like adding the folder name in parenthesis (e.g., [e-mail](Sent) and [e-mail](random sent))

Comment 4

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

(In reply to comment #3)
> We could try to detect having multiple special
> folders in an account and do something like adding the folder name in
> parenthesis (e.g., [e-mail](Sent) and [e-mail](random sent))

We could at least make it the tooltip, I think the additional text is likely to be cropped off and out of view.

Is there some way to get out of this situation? i.e. right click, [x] Sent Folder, uncheck.

Comment 5

[David :Bienvenu]

Right now we use the tooltip to give the new msg summary for the folder. I suppose we could show the folder name if there aren't any new messages, which would be the common case for everything but the Inbox.

The way out of this situation for the user is not intuitive - for every folder that has a special folder flag inappropriately set, they must first set it as the special folder in account settings, then set a different folder as the special folder.

We could add code to verify that every folder that has various special folder flags set is actually a special folder of that type, i.e., verify the flags against the prefs.

Comment 6

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

Hmm, I'm a little scared of a solution like that this far into the release schedule but that really depends on how you feel about it.

Perhaps we could always insert the additional text before the summary for the folder so that it is always visible.

Comment 7

[David :Bienvenu]

(In reply to comment #6)
> Hmm, I'm a little scared of a solution like that this far into the release
> schedule but that really depends on how you feel about it.
It's a little scary.
> 
> Perhaps we could always insert the additional text before the summary for the
> folder so that it is always visible.
I'll code that up.

Comment 8

[Magnus Melin [:mkmelin]]

Maybe i'm missing something, but why wouldn't we just have it show which account it is a special folder for.

So

 Sent 
     Account1          <-- Sent on Account1
     Account2          <... e.g. MoreSent on Account1 (special sent for account2)
     ...

Or why would that be a problem (ui-wise)?

Comment 9

[David :Bienvenu]

We could do that if there are duplicates on the same account, though Bryan was worried it would be too long and the account name might not be completely visible.

I kinda prefer that solution to the tooltip one, personally.

Comment 10

[Magnus Melin [:mkmelin]]

I guess my question was more of: why would it look any different than normal?

Comment 11

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

(In reply to comment #10)
> I guess my question was more of: why would it look any different than normal?

Yeah, I was worried it wouldn't be seen at all because of the length but you make a good point.  We don't need to do the tooltip change if we make this change instead.

Comment 12

[David :Bienvenu]

Attachment: show "folder - account" for duplicate folders w/ special flags

fortunately, the code already knew how to do this for unread/favorites views

Comment 14

[Magnus Melin [:mkmelin]]

I still don't understand why this needs special casing.

With a pop and an imap account, i set the Sent folder of two pop-identities to be on the imap account (folders otheraccount_sent and other2_account_sent)

With patch: http://imagebin.ca/view/M0rY-o5.html
Without: http://imagebin.ca/view/TP1yDRm.html

Why is it a problem like as is? (Except that it only lists one of the folders.)
With the patch it says on which server the folder is - but that's irrelevant imo. You just want to know which account it belongs to. The only case the "folder - account" name is needed is if (due to multiple identity sent settings) end up with several folders named the same.

Comment 15

[David :Bienvenu]

If I have two sent folders on the same account, whether because of a bug, or deliberately, we will show just the account twice (assuming the first patch), and I won't know what folder it is. In other words, take my first patch, apply it, and then go to account 2, set it's sent folder to some random folder on account 1, and the sub-folders of the smart sent folder will show acount1 twice.

Comment 16

[David :Bienvenu]

let me try a little ascii art. If I have two sent folders in an account foo@bar, from two identities, I'll see the following, with my first patch:

Sent
  foo@bar
  foo@bar

With the second patch, I'll see

Sent
  Sent - foo@bar
  random sent - foo@bar

So the user can tell that the folder name is, as well as the account it's in (though not the account/identity it's for).

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 402183 [details] [diff] [review]
show "folder - account" for duplicate folders w/ special flags

Ah, i see now.

>+     let foldersWithFlag = acct.incomingServer.rootFolder.getFoldersWithFlags(aFolderFlag);
>+     if (foldersWithFlag.length > 0) {
>+       for each (folderWithFlag in fixIterator(foldersWithFlag.enumerate(),
>+                                               Components.interfaces.nsIMsgFolder)) {
>+         folders.push(folderWithFlag);
>+        // add sub-folders of Sent and Archive to the result.
>+        if (deep && (aFolderFlag & (nsMsgFolderFlags.SentMail | nsMsgFolderFlags.Archive)))
>+          this.addSubFolders(folderWithFlag, folders);
>+        }

Indention is off (and please capitalize Add).

But why is it only sent and archive, not the other special folders?

>       }
>     }
>     return folders;
>   },
> 
>   _smartFoldersGenerator: function ftv_smartFoldersGenerator(ftv)
>   {
>     let map = [];
>@@ -890,26 +893,35 @@ let gFolderTreeView = {
>       map.push(smartFolderItem);
>     else
>       map[position] = smartFolderItem;
>     // Add the actual special folders as sub-folders of the saved search.
>     // By setting _children directly, we bypass the normal calculation
>     // of subfolders.
>     smartFolderItem._children = [new ftvItem(f) for each (f in subFolders)];
> 
>-    // sortFolderItems(this._children);
>+    let prevChild = null;
>     // Each child is a level one below the smartFolder
>     for each (let child in smartFolderItem._children) {
>       child._level = smartFolderItem._level + 1;
>       child._parent = smartFolderItem;
>       // don't show sub-folders of the inbox, but I think Archives/Sent, etc
>       // should have the sub-folders.
>       if (flag & nsMsgFolderFlags.Inbox)
>         child.__defineGetter__("children", function() []);
>-      child.useServerNameOnly = true;
>+      // If we have consecutive children with the same server, then both
>+      // should display as folder - server.
>+      if (prevChild && (child._folder.server == prevChild._folder.server)) {
>+        child.addServerName = true;
>+        prevChild.addServerName = true;
>+        prevChild.useServerNameOnly = false;
>+      }
>+      else
>+        child.useServerNameOnly = true;

Braces for else too.

r=mkmelin with that sorted out

Comment 18

[David :Bienvenu]

fix checked in. Re why it's only sent and archive, not the other special folders, I haven't changed anything there. It doesn't affect the folders you see in smart folders mode, but it will affect folders are included in the top level smart folder contents, i.e., the scope of the saved search.  Sent/Archives are the ones where I think it's important to see the contents of the sub-folders in the top-level smart folder.