492922 - Smart folders - when creating a new account, its Sent folder doesn't show up automatically

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[David Ascher (:davida)]

I created a new gmail imap account w/ the new autoconfig dialog, and its Inbox showed up in the smart folders Inbox, but the Sent folder didn't.

Don't know if this is tied to the recent XLIST work.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090512 Shredder/3.0b3pre

(dev build, revision 2630:89fd0d3e0446)

Comment 1

[David :Bienvenu]

prior to the xlist work, we wouldn't have a sent folder for the new account, since we create it lazily.

If an OnItemAdded notification is generated, the account manager should be adding the new sent folder to the smart folder, but it's possible we're not getting an OnItemAdded notification...

Does the sent folder get added after a restart?

Comment 2

[David Ascher (:davida)]

The sent folder gets added after a switch to another folderpane mode and a return to the smart mode.

Comment 3

[David :Bienvenu]

Ah, you mean the INBOX smart folder doesn't have an account entry added as a child for the newly added account? I thought you were talking about the scope of the parent smart folder not including the newly added Sent folder.

So the newly added account adds an inbox w/o talking to the server, but for the rest of the folders, we need to connect to the server before we discover the rest of the folders. The folder pane gets rebuilt when the account is added, before we discover the rest of the folders.

The fix I did for getting rid of the top level account for the smart folders account might help here - I think I had to make a new class for smart folder items, which might help me handle new folders getting discovered specially for smart folders.

Comment 4

[David Ascher (:davida)]

Uh, it's the Sent smart folder that's missing an entry, while the Inbox smart folder has one.  

It sounds like I should just wait on getting rid of the top-level account lands, and revisit this bug then.

Comment 5

[David :Bienvenu]

ah, sorry, I meant SENT smart folder doesn't have an account entry added, as opposed to the search scope of the global SENT folder.

This bug will still exist when the top-level account removal lands, but I want to build the fix on that one.

Comment 6

[David Ascher (:davida)]

Approving for bienvenu to explore, and if it's easy to repro, should be easy to fix. (so he says)

Comment 7

[David :Bienvenu]

ok, not so easy to fix, but necessary. The issue is that various folder modes don't have a way of overriding the behavior of new folders getting added to a server. Smart Folders definitely need to override the default behavior, so I need a way to make the folder pane modes extensible w.r.t. new folders.

Comment 8

[David :Bienvenu]

Attachment: wip

This basically works, and fixes this issue, and the case of adding a new folder when all folders are sub-folders of the inbox, when in smart folder mode.

The patch needs a bit of cleaning up, and there are issues with the case of creating the first account in an empty profile, and, I think, transitioning between the first and second account. 

I may need to fix the startup code w/ new accounts to use the new auto config account setup in order to make sure that code path works...

Comment 9

[David :Bienvenu]

Attachment: proposed fix

technically, this only needs r=, but since Blake isn't a front end ui module peer, but is fairly familiar with this code, I'm going to ask him for an r= and standard for an sr=. 

The messenger.xul part fixes an issue where new profiles weren't using smart folder by default. The folderPane.js changes are to fix our handling of new folders getting added when in smart folder mode. This was made much more complicated by our desire not to show smart folders when there's only one account - it means we need to start showing smart folders when a second account is added. This also fixes an issue when new folders are added to accounts with imap servers where all folders are sub-folders of the inbox.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

>+++ b/mail/base/content/folderPane.js
> Components.utils.import("resource://gre/modules/iteratorUtils.jsm");
> Components.utils.import("resource://gre/modules/folderUtils.jsm");
>+const FLAGS = Components.interfaces.nsMsgFolderFlags;

I don't see many people saying stuff like this in reviews, but I like what
you've done here by moving the constant up to the top of the file.

>@@ -60,16 +61,17 @@ let gFolderTreeView = {
>   load: function ftv_load(aTree, aJSONFile) {
>     const Cc = Components.classes;
>     const Ci = Components.interfaces;

We seem to use these two objects a few times in the file, perhaps they
should be pulled up to the top as well?

(I wouldn't block the patch for this, it would just be nice.
 Even nicer would be to change the 7 other uses of:
 let nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
 to use the new FLAGS.)

>     this._treeElement = aTree;
> 
>     let smartName = document.getElementById("bundle_messenger")
>                             .getString("folderPaneHeader_smart");
>     this.registerMode("smart", this._smartFoldersGenerator, smartName);
>+    this._mapItemAdded["smart"] = this._smartItemAdded;

It seems a little odd to be using this up here on line 69, when it's not
defined until line 1016.  Perhaps we could move the definition up to some
point above this function?

>     // the folder pane can be used for other trees which may not have these elements.
>     if (document.getElementById("folderpane_splitter"))
>       document.getElementById("folderpane_splitter").collapsed = false;
>     if (document.getElementById("folderPaneBox"))
>       document.getElementById("folderPaneBox").collapsed = false;
> 
>     try {
>       // Normally our tree takes care of keeping the last selected by itself.
>@@ -770,28 +772,17 @@ let gFolderTreeView = {
>     return folders;
>   },
> 
>   _smartFoldersGenerator: function ftv_smartFoldersGenerator(ftv)
>   {
>     let map = [];
>     let acctMgr = Components.classes["@mozilla.org/messenger/account-manager;1"]
>                     .getService(Components.interfaces.nsIMsgAccountManager);
>-    // Smart mode only makes sense if there's more than 1 incoming mail account,
>-    // so check for that and fall back to "all folders" if we don't find a 2nd.
>     let accounts = gFolderTreeView._sortedAccounts();
>-    let numMailAccounts = 0;
>-    for each (let account in accounts) {
>-      if (GetInboxFolder(account.incomingServer) != null)
>-        numMailAccounts++;
>-      if (numMailAccounts > 1)
>-        break;
>-    }
>-    if (numMailAccounts <= 1)
>-      return ftv._mapGenerators["all"](ftv);

So the new changes make smart mode make sense for 1 incoming account, but
does smart mode also now make sense for 0 accounts?

>   _addSmartFoldersForFlag: function ftv_addSmartFoldersForFlag(map, accounts, smartRootFolder,
>-                                                               flag, folderName)
>+                                                               flag, folderName, position)

Could we do something nicer with this indentation?
What about:
   _addSmartFoldersForFlag: function ftv_addSmartFoldersForFlag(map, accounts,
                                    smartRootFolder, flag, folderName, position)
?

Or:
   _addSmartFoldersForFlag: function (map, accounts, smartRootFolder, flag,
                                      folderName, position)
?

> 
>+  _mapItemAdded: {},
>   /**

A blank line below this variable would be nice.

>+    let specialFolderFlags = FLAGS.Inbox|FLAGS.Drafts|FLAGS.SentMail|FLAGS.Trash|
>+                             FLAGS.Templates|FLAGS.Archive|FLAGS.Junk;
>+    // If aItem is a special folder, we need to add it as a child of the 

Trailing space.

>+    // corresponding smart folder.
>+    // If aParentItem is a special folder other than Sent/Archives/Trash,
>+    // we need to add aItem as a top-level folder in its account.
>+    // Otherwise, we need to add it as a child of its parent.
>+    if (aItem.flags & specialFolderFlags) {

We only seem to use specialFolderFlags once, and we're inlining the
constants in the next if statement.  Maybe we should inline the constants
here too.

>+      // add as child of corresponding smart folder
>+      let acctMgr = Components.classes["@mozilla.org/messenger/account-manager;1"]
>+                      .getService(Components.interfaces.nsIMsgAccountManager);
>+      let smartServer;
>+      try {
>+        smartServer = acctMgr.FindServer("nobody", "smart mailboxes", "none");
>+      } catch (ex) {/* not much we can do without the root */ return;}

That comment and return seems a little hidden to me.  Would you mind
putting it on a line of its own?  Or at least adding a blank line after
it?

>+      let smartRootFolder = smartServer.rootFolder;
>+      // In theory, a folder can have multiple flags set, so we need to check
>+      // each flag separately.
>+      if (aItem.flags & FLAGS.Inbox)
>+        this._addSmartSubFolder(aItem, smartRootFolder, "Inbox", FLAGS.Inbox);
>+      if (aItem.flags & FLAGS.Drafts)
>+        this._addSmartSubFolder(aItem, smartRootFolder, "Drafts", FLAGS.Drafts);
>+      if (aItem.flags & FLAGS.SentMail)
>+        this._addSmartSubFolder(aItem, smartRootFolder, "Sent", FLAGS.SentMail);
>+      if (aItem.flags & FLAGS.Trash)
>+        this._addSmartSubFolder(aItem, smartRootFolder, "Trash", FLAGS.Trash);
>+      if (aItem.flags & FLAGS.Templates)
>+        this._addSmartSubFolder(aItem, smartRootFolder, "Templates", FLAGS.Templates);
>+      if (aItem.flags & FLAGS.Archive)
>+        this._addSmartSubFolder(aItem, smartRootFolder, "Archives", FLAGS.Archive);
>+      if (aItem.flags & FLAGS.Junk)
>+        this._addSmartSubFolder(aItem, smartRootFolder, "Junk", FLAGS.Junk);
>+      if (aItem.flags & FLAGS.Queue)

We aren't checking for FLAGS.Queue in the specialFolderFlags…

>+        this._addSmartSubFolder(aItem, smartRootFolder, "Outbox", FLAGS.Queue);
>+    }
>+    else if (aParentItem.isSpecialFolder(FLAGS.Inbox|FLAGS.Drafts|
>+                                         FLAGS.Templates|FLAGS.Junk, false)) {

So, why aren't these the same set of flags as specialFolderFlags?  Is there
a reason we don't want to handle some of those cases here?

>+      let parentIndex = this.getIndexOfFolder(aItem.server.rootFolder);
>+      let parent = this._rowMap[parentIndex];
>+      if (!parent)
>+         return;
>+
>+      let newChild = new ftv_SmartItem(aItem);
>+      parent.children.push(newChild);
>+      newChild._level = parent._level + 1;
>+      newChild._parent = parent;
>+      sortFolderItems(parent._children);

Will we be re-sorting this list a lot if there are a lot of children?
(I suppose there probably won't be a lot of children, but I did want to
check, because it seems like it could be a problem.)

>+  /**
>+   * This updates the rowmap and invalidates the right row(s) in the tree
>+   */
>+  _addChildToView: function ftl_addChildToView(aParent, aParentIndex, aNewChild) {
>+    // If the parent is open, add the new child into the folder pane. Otherwise,
>+    // just invalidate the parent row.

I think the "Otherwise," could go on the next line, both to keep the sentence
together, and to not go over 80 characters.

>+      // only child - go right after our parent
>+      if (newChildNum == 0)
>+        newChildIndex = Number(aParentIndex) + 1
>+      // if we're not the last child, insert ourselves before the next child.
>+      else if (newChildNum < aParent._children.length - 1)
>+        newChildIndex = this.getIndexOfFolder(aParent._children[Number(newChildNum) + 1]._folder);
>+      // otherwise, go after the last child
>+      else
>+      {

I thought the guideline was that if one of the if/else if/else's on a level
needed braces, then they all do.

And doesn't the { go on the same line as the else?

>+  _addSmartSubFolder: function ftl_addSmartSubFolder(aItem, aSmartRoot, aName, aFlag) {
[snip...]
>+        if (this._rowMap[newChildIndex]._folder.isServer)
>+          break;
>+        newChildIndex++;
>+      }
>+    } else {

I thought the "else {" was supposed to go on a new line.

>+        // From folderUtils.jsm
>+        setPropertyAtoms(this._folder, aProps);
>+        aProps.AppendElement(Components.classes["@mozilla.org/atom-service;1"]
>+                            .getService(Components.interfaces.nsIAtomService)
>+                            .getAtom("specialFolder-Smart"));

I think these two lines could have an extra space of indent.

>+    if (aItem.flags & Components.interfaces.nsMsgFolderFlags.Inbox)
>+      newChild.__defineGetter__("children", function() []);
>+     if (parent)
>+      this._addChildToView(parent, parentIndex, newChild);
>+     else {
>+      this._rowMap.splice(newChildIndex, 0, newChild);
>+      this._tree.rowCountChanged(newChildIndex, 1);
>+     }

The if, else, and } are indented one too many spaces.

>-    if ((folder.flags & FLAGS.Sent || folder.flags & FLAGS.Draft ||
>-         folder.flags & FLAGS.Template ||
>+    if ((folder.flags & FLAGS.SentMail || folder.flags & FLAGS.Drafts ||
>+         folder.flags & FLAGS.Templates ||

Good catches.


All in all, I'll be happy to give this an r+ if you can answer the
questions above about smart mode making sense for 0 accounts, why the
parent folder is checking for different flags than specialFolderFlags, and
re-sorting the _children list a lot.  And fix the nits, of course.  :)

Thanks,
Blake.

Comment 11

[David :Bienvenu]

I believe all the modes are the same with 0 accounts.

>I thought the guideline was that if one of the if/else if/else's on a level
>needed braces, then they all do.
 
I think most reviewers feel that's too strict, and you should use braces if it makes the code clearer, but not if it just clutters up the code. I know I've stopped insisting on it.

>So, why aren't these the same set of flags as specialFolderFlags?  Is there
> a reason we don't want to handle some of those cases here?

We're looking at aParentItem here, and deciding if children of the parent want to be below the parent or at the top level in the account. For Sent/Archives/Trash, we want sub-folders to appear under them in the special folders section of the folder pane. For children of the inbox, we want them to be at the top level of the account. Drafts/Templates/Junk don't tend to have sub-folders, but this could probably just be FLAGS.Inbox

+    else if (aParentItem.isSpecialFolder(FLAGS.Inbox|FLAGS.Drafts|
+                                         FLAGS.Templates|FLAGS.Junk, false)) {

New folder creation is relatively rare, so sorting the children shouldn't be a big deal. 

The rest of your comments all look reasonable, and I'll attach a new patch tomorrow morning...

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 399642 [details] [diff] [review]
proposed fix

Those all sound good to me.

Later,
Blake.

Comment 13

[Magnus Melin [:mkmelin]]

If you're touching almost all flag rows anyway, can be make FLAGS be named nsMsgFolderFlags like it's customary elsewhere?

As it's not a js module, Cc, Ci and friends shouldn't be defined on top.

Comment 14

[David :Bienvenu]

Attachment: patch addressing Blake's and mkmelin's comments

This addresses all the comments, I believe, except for the one about moving the smartItemsAdded method, since it makes sense to have load() be the first function in that class.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 400038 [details] [diff] [review]
patch addressing Blake's and mkmelin's comments

>+++ b/mail/base/content/folderPane.js

>+    // If aParentItem is a special folder other than Sent/Archives/Trash,
>+    // we need to add aItem as a top-level folder in its account.
>+    // Otherwise, we need to add it as a child of its parent.
>+    if (aItem.flags & (nsMsgFolderFlags.Inbox|nsMsgFolderFlags.Drafts|
>+                       nsMsgFolderFlags.SentMail|nsMsgFolderFlags.Trash|
>+                       nsMsgFolderFlags.Templates|nsMsgFolderFlags.Archive|
>+                       nsMsgFolderFlags.Junk | nsMsgFolderFlags.Queue)) {

I'ld like to see spaces around the |s, to match the last line.


>+  _addChildToView: function ftl_addChildToView(aParent, aParentIndex, aNewChild) {
>+    // If the parent is open, add the new child into the folder pane.
>+    //  Otherwise, just invalidate the parent row.

And one less space before the "Otherwise", please.
(To match the previous comment.)


line 1383:
>+        aProps.AppendElement(Components.classes["@mozilla.org/atom-service;1"]
>+                             .getService(Components.interfaces.nsIAtomService)
>+                             .getAtom("specialFolder-Smart"));

line 907:
>         aProps.AppendElement(Components.classes["@mozilla.org/atom-service;1"]
>                             .getService(Components.interfaces.nsIAtomService)
>                             .getAtom("specialFolder-Smart"));

Did you want to change the indentation for this one too?
(I won't mind particularly if you don't.)

r=me with at least two of these three things fixed.  ;)

Later,
Blake.

Comment 16

[David :Bienvenu]

I've fixed those issues locally...I'll attach a new patch in a minute...

Comment 17

[David :Bienvenu]

Attachment: fix addressing Blake's comments

requesting sr from Standard8 (unless he doesn't feel it's needed, which is OK too)

Comment 18

[David :Bienvenu]

Attachment: fix top level inboxes in smart folder mode not to show sub-folders

This should fix the second issue you saw. Recognizing the special folders immediately is something we only currently do for gmail, I believe...

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 400170 [details] [diff] [review]
fix top level inboxes in smart folder mode not to show sub-folders

I've done some tests on this patch (hence the additional fix) and it seems to be working fine.

A quick glance at the code and it seems to be reasonable, so I'm happy with bwinton's review here.

Comment 20

[David :Bienvenu]

fix pushed, with some more slight indentation problems bwinton noticed fixed as well.

Comment 21

[Valery Kondakoff]

I still see the scroll bar in Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.4pre) Gecko/20090912 Shredder/3.0b4pre: http://www.rugby-forum.ru/temp/scroll.jpg . Is it the right place to report this?