413781 - XBLify folder-selection menus

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch wip

I feel like I need some dramatic Thucididean line to kick off my war on RDF.  Alas, tis not to be...

In line with http://wiki.mozilla.org/Thunderbird:Thoughts_on_Removing_RDF this begins with re-implementing the folder/account menus to be JS-driven instead of RDF templates (#4 on the wiki).  While RDF is still the underlying data-source, this significantly reduces the direct consumers, making an eventual replacement of the datasource easier.

This bug is to consolidate all the existing places where folder-selection is chosen with a menu into consumers of a single xbl binding.  In simple terms, this will replace the Move To, Copy To, Get New Messages, etc menus with new implementations (that should behave identically).

In a later patch, we can do something similar for the tree implementation(s).

This patch is where I got to today.  It works reasonably well in my basic testing.  I think there's still some bugs in the update listeners, which I hope to fix tomorrow.  Additionally, there are a few consumers that I don't even know what they are. filterListDialog.xul what?  I'll track those down and replace those callers too.

On the whole, we're getting rid of a significant number of datasources= instances here, which is a good thing in my book.

"Any data model which claims to be able to represent everything is either completely wrong or hopelessly complex. RDF happens to be both."
~bsmedberg

Comment 1

[Joey Minta]

Comment on attachment 298857 [details] [diff] [review]
patch wip

Obviously ignore the localStore.rdf and mimeTypes.rdf changes, but do enjoy the fact that this is actually a code-size win, if we're counting lines. :)  The remaining changes will only increase that win.

Comment 2

[Joey Minta]

Attachment: patch v1

This patch fixes all of the Thunderbird consumers I found, with the exception of feed-properties.xul.  That menu is really pretty messed up, and should probably have something more like what appears in the filter-dialog, since it seems to want to allow sub-folder selection.

Otherwise, this patch should fix all consumers, and behavior should remain largely consistent.  Some small ui-changes will be noticable, as all folder-menus will now be consistently styled.  That is, some of the more buried folder-selection instances previously just listed names, while now icons will be next to those folder-names in many cases.

The trees are next on my hit-list.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> Created an attachment (id=298962) [details]
> patch v1

You'll want to update your patch because of the Cv1-TB patch check-in in bug 68174 :-|

It would be nice if you could use the standard cvs diff format;
so BugZilla could show the proper paths and add a link on them, ...

> This patch fixes all of the Thunderbird consumers I found, with the exception

What about <ABSearchDialog.xul> ? See bug 68174 Ev1-TB patch.

I think we should coordinate what we do in these two bugs...

Comment 4

[Joey Minta]

(In reply to comment #3)
> It would be nice if you could use the standard cvs diff format;
> so BugZilla could show the proper paths and add a link on them, ...
Yeah, I'm using a local hg copy of the mozilla tree in line with http://wiki.mozilla.org/Using_Mercurial_locally_with_CVS  I find the work-flow to be infinitely better there, so I'm not really sure what the solution is...

> What about <ABSearchDialog.xul> ? See bug 68174 Ev1-TB patch.
I should clarify.  This patches fixes all consumers using the folder and account rdf datasources.  (That's what the folder-selection menus let you select from.) As I read it, the ABSearchDialog uses |datasources="rdf:addressdirectory"|, so it isn't impacted by this patch at all.  Note also that the removing-rdf outline I'm working from differentiates between removing rdf from the addressbook and from the other widgets, I think with good reason.  Don't get me wrong, removing rdf from the address-book is on my list, it's just not in the scope of this bug.

Let me know if I misunderstand anything here.

Comment 5

[Joey Minta]

(In reply to comment #3)
> You'll want to update your patch because of the Cv1-TB patch check-in in bug
> 68174 :-|
> 
Crap.  You're absolutely right here.  This patch is going to bit-rot like crazy, so I might like to see at least a bird's eye architecture review before going around updating every week.  David, if you really want a clean patch to review, though, just give me 48hrs notice for when you think you'll get to this and I should be able to turn things around.

Comment 6

[David :Bienvenu]

Joey, overall, this looks very neat. I'm not an expert on the xml stuff, so I can't tell you if there's a better or cleaner way to do what you've done, but it certainly looks cleaner than what it replaces.

Maybe a comment as to why you're doing this:
+  var serverMenu = document.getElementById("serverMenu");
+  serverMenu.menupopup.showPopup();
+  serverMenu.menupopup.hidePopup();
+

I'd really need a clean patch that I could easily apply so I could run with the changes. I may not be able to test the patch immediately, but I should be able to apply the patch within 48 hours and then run with it.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> http://wiki.mozilla.org/Using_Mercurial_locally_with_CVS

(Oh. I'm not familiar with that :-()

> I should clarify.  This patches fixes all consumers using the folder and
> account rdf datasources.

(My bad.)

Comment 8

[[e-mail]]

Comment on attachment 298962 [details] [diff] [review]
patch v1

>+  var serverMenu = document.getElementById("serverMenu");
>+  serverMenu.menupopup.showPopup();
>+  serverMenu.menupopup.hidePopup();
That's really lame...

Personally I'd prefer for menulists to switch to using the tree popups, which we can subsequently switch from RDF to a JS or C++-based tree view.

Failing that, why not make the constructor call _ensureInitialised automatically if the parent node is a menulist?

Comment 9

[[e-mail]]

Comment on attachment 298962 [details] [diff] [review]
patch v1

Note: this does not constitue a review or approval of any of the code!

>+        session.RemoveFolderListener(this._listener, Ci.nsIFolderListener.all);
RemoveFolderListener only takes one parameter.

>+        // We don't care about the rest of these...
I think you do, otherwise you'll miss renames, biff changes etc.

>+          session.AddFolderListener(this._listener, Ci.nsIFolderListener.all);
Note that this listens to all folders...

>+          switch (aFolder.biffState) {
>+            case Ci.nsIMsgFolder.nsMsgBiffState_NewMail:
>+              aMenuNode.setAttribute("BiffState", "NewMail");
>+              break;
>+            case Ci.nsIMsgFolder.nsMsgBiffState_NoMail:
>+              aMenuNode.setAttribute("BiffState", "NoMail");
>+              break;
>+            default:
>+              aMenuNode.setAttribute("BiffState", "UnknownMail");
>+          }
Could use an array: ["NewMail", "NoMail", "UnknownMail"][aFolder.biffState]

>+          try {
>+            server.QueryInterface(Ci.nsINntpIncomingServer);
instanceof doesn't need try/catch

Comment 10

[Joey Minta]

Thanks for the comments! I'll incorporate them into the next patch.

(In reply to comment #8)
> (From update of attachment 298962 [details] [diff] [review])
> Personally I'd prefer for menulists to switch to using the tree popups, which
> we can subsequently switch from RDF to a JS or C++-based tree view.
I agree, and the tree-popups are probably next on my list.

> 
> Failing that, why not make the constructor call _ensureInitialised
> automatically if the parent node is a menulist?
That's a good idea I didn't think of.

(In reply to comment #9)
> >+        // We don't care about the rest of these...
> I think you do, otherwise you'll miss renames, biff changes etc.
Hmm, can you point me to any documentation about which is fired when?

> 
> >+          session.AddFolderListener(this._listener, Ci.nsIFolderListener.all);
> Note that this listens to all folders...
That was intentional, so that I could catch all the changes.  I realize we're doing some superfluous rebuilds at the moment as a result. (see the xxx optimization comments)

> Could use an array: ["NewMail", "NoMail", "UnknownMail"][aFolder.biffState]
Nice idea.

> 
> >+          try {
> >+            server.QueryInterface(Ci.nsINntpIncomingServer);
> instanceof doesn't need try/catch
I thought I ran into issues where instanceof would return false if the object had never been QI'd to the desired interface, but I could be wrong.  I'll try it.

Comment 11

[Joey Minta]

Serge,
  I'm really swamped with schoolwork at the moment, meaning this bug probably isn't going to see much movement for a week or two.  If that impacts your blocked bug's workpath, I'd suggest making your changes and I'll deal with the bitrot when I have some free time.

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> If that impacts your
> blocked bug's workpath, I'd suggest making your changes and I'll deal with the
> bitrot when I have some free time.

Bug 68174 "revealed" some issues in the existing code / UI behavior:
I'm working on that bug, I'll let you know when I'm done with the files involved in your patch here.

*****

Bug 350962 seems to be dependent/duplicate of the current bug, is it not ?

Comment 13

[Joey Minta]

Attachment: patch v2

This is a patch updated to fix the bitrot from other landings and to take into account Neil's comments.  David, it should be ready for you to apply and play with! :-)

Comment 14

[David :Bienvenu]

OK, thx, I've applied this, and I'll give it a shot as soon as I can. 

Comment 16

[David :Bienvenu]

two problems I saw right away - it seemed to think a ton of folders were recent, including newsgroups. Running 2.0x against the same profile showed the expected small set of recent folders. And the menus are no longer sorted with special folders like Inbox, Sent, etc, at the top of an account.

Comment 17

[Joey Minta]

(In reply to comment #16)
> two problems I saw right away - it seemed to think a ton of folders were
> recent, including newsgroups. Running 2.0x against the same profile showed the
> expected small set of recent folders. 
Ahh yep.  Forgot to include the CanFileMessages test when compiling the Recent list.

> And the menus are no longer sorted with
> special folders like Inbox, Sent, etc, at the top of an account.
> 
I was going based on sortResource="<blah>#folderTreeName". This means the folders were (should be) sorted in alphabetical order.  Is there a different sorting algorithm somewhere that I missed?

Comment 18

[David :Bienvenu]

> Is there a different sorting algorithm somewhere that I missed?

I'm not sure if you're asking if there's code that'll do "the right thing" or at least what we used to do, or if there used to be a different algorithm :-) But what we used to do was sort special folders like INBOX to the top, and then do a case-insensitive sort of the rest of the folders - your sort is case-sensitive.

Comment 19

[David :Bienvenu]

Comment on attachment 302841 [details] [diff] [review]
patch v2

minusing based on sort issues.

Comment 20

[Joey Minta]

Attachment: patch v3

This patch includes a couple of small changes.
(1) The main sorting function now uses sortKey comparisons so that special folders should end up first
(2) Recent folders are now filtered based on canFileMessages
(3) Css styles are now properly applied to menus, not just menuitems

Note on recent folders: This patch *does* change the sorting behavior for recent folders on trunk.  It currently seems to sort much like the regular menu (based on sort-key and name).  This patch changes it to sort based on recency, which seems to make more sense to me.

This patch is slightly bigger due to increased context in my hg setup. (-U 8)

Comment 21

[David :Bienvenu]

Joey, before I apply this patch, does it sort folder case-insensitively, so that ab comes after Aa and before Ba?

Comment 22

[Joey Minta]

It should.
+          /**
+           * Sorts the list of folders. We give first priority to the sortKey
+           * property, and then via a case-insensitive comparison of names
+           */
+          function nameCompare(a, b) {
+            var sortKey = a.compareSortKeys(b);
+            if (sortKey)
+              return sortKey;
+            return a.prettyName.toLowerCase() > b.prettyName.toLowerCase();
+          }

Comment 23

[David :Bienvenu]

Comment on attachment 304030 [details] [diff] [review]
patch v3

Joey, sorry it took me so long to try this patch - the ordering looks great, but the recent folders aren't limited to a small number the way they were before (I think 15 or so initially).

Comment 24

[Joey Minta]

Oh, yeah, that's a simple scoping problem.  Saving the tree's context outside of the addIfRecent function, and using it to access the _MAXRECENT variable should fix it.  (see below) Are there any other changes you want to see while I'm fixing that one?

+          /**
+           * This function will add a folder to the recentFolders array if it
+           * is among the 15 most recent.  If we exceed 15 folders, it will pop
+           * the oldest folder, ensuring that we end up with the right number
+           *
+           * @param aFolder the folder to check
+           */
+          function addIfRecent(aFolder) {
+            if (!aFolder.canFileMessages)
+              return;
+
+            var time = aFolder.getStringProperty("MRUTime") || 0;
+            if (time <= oldestTime)
+              return;
+
+            if (recentFolders.length == this._MAXRECENT) {
+              recentFolders.sort(sorter);
+              recentFolders.pop();
+              oldestTime = recentFolders[recentFolders.length-1].getStringProperty("MRUTime");
+            }
+            recentFolders.push(aFolder);
+          }

Comment 25

[David :Bienvenu]

The other issue I noticed is that servers appear in the move/copy/file menu even if you can't copy messages to them - they shouldn't (e.g., news servers).  They have a pull right indicator, and when you mouse to them, a tiny box shows up.

Comment 26

[Karsten Düsterloh]

Comment on attachment 304030 [details] [diff] [review]
patch v3

>diff -r 8282afdef81e mail/base/content/mail-folder-bindings.xml
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/mail/base/content/mail-folder-bindings.xml	Mon Feb 18 08:51:35 2008 -0500

Is this so TB-centric that it justifies to be put under /mail, not /mailnews?
Especially when compared to /mailnews/base/search/resources/content/searchWidgets.xml and /mailnews/base/resources/content/mailWidgets.xml?

If not so, consider folderWidgets.xml as the name, to "keep the pattern".

Comment 27

[Joey Minta]

Attachment: patch v4

Both fixes ended up being pretty trivial.  I already noted that the recent-count problem was just an issue of scoping.  The news-server issue simply required re-ordering the checks in the filter-function. (Don't ask me why the news-server was passing those other checks.)

Comment 28

[Joey Minta]

Just checking the status here, since I'm a bit concerned about bitrot.  (Bug 420614 is coming which makes a couple of my calls here deprecated.)

Comment 29

[David :Bienvenu]

Comment on attachment 307870 [details] [diff] [review]
patch v4

thx for the ping, sorry it took so long:

two problems I've seen so far:

The copy menu doesn't display "File Here" for container folders, though the move menu does. I don't know if that broke recently or if it was always broken. Should be trivial to fix.

The recent folders menu doesn't distinguish between folders of the same name, e.g., Inbox on user@foo, Inbox on user@bar. That's pretty critical if you have multiple accounts. That might be a bit harder since we did that in the datasource before.

If you can turn around a fix for those two issues quickly, I promise to try it out quickly.

Comment 30

[David :Bienvenu]

I also noticed that Edit Draft of a local drafts folder didn't work - I got this xul error:

XML Parsing Error: undefined entity
Location: chrome://messenger/content/messengercompose/messengercompose.xul
Line Number 380, Column 13:            <menupopup type="folder" mode="filing" showFileHereLabel="true"
------------^

Can you try that as well?

Comment 31

[Joey Minta]

Attachment: patch v5

This patch adds in the logic to check for duplicate names in the recent menu, and also fixes the minor thinko that led to the blank label in the main menubar's copy menu.

Comment 32

[David :Bienvenu]

Comment on attachment 312843 [details] [diff] [review]
patch v5

thx, Joey, r/sr=bienvenu. But this patch has bit-rotted. GetSubFolders no longer exists, so things are completely broken. Either convert to using .subFolders, which is a simple enumerator, or (much less desirable) switch to subFoldersObsolete.

Note that because the folder cache seems to be completely broken, it takes a long time for the menus to get populated, but that's not your fault.

Comment 33

[Joey Minta]

Patch checked in, with the switch to .subFolders.  In terms of testing, this patch will impact the following menus:
(main menubar)'s Message:Move and Message:Copy
(message context menu)'s Move To and Copy To
(filter dialog)'s server/account menu
(search dialog)'s server/account menu
(main toolbar)'s Get Mail dropdown menu
(main toolbar)'s File here menu
(subscribe dialog)'s server menu
(compose window)'s carbon copy menu

One should check that these menus are properly populated and that selecting various options in them functions properly

Checking in mail/base/jar.mn;
/cvsroot/mozilla/mail/base/jar.mn,v  <--  jar.mn
new revision: 1.107; previous revision: 1.106
done
Checking in mail/base/content/FilterListDialog.js;
/cvsroot/mozilla/mail/base/content/FilterListDialog.js,v  <--  FilterListDialog.js
new revision: 1.15; previous revision: 1.14
done
Checking in mail/base/content/FilterListDialog.xul;
/cvsroot/mozilla/mail/base/content/FilterListDialog.xul,v  <--  FilterListDialog.xul
new revision: 1.7; previous revision: 1.6
done
Checking in mail/base/content/SearchDialog.xul;
/cvsroot/mozilla/mail/base/content/SearchDialog.xul,v  <--  SearchDialog.xul
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/mail/base/content/mail-folder-bindings.xml,v
done
Checking in mail/base/content/mail-folder-bindings.xml;
/cvsroot/mozilla/mail/base/content/mail-folder-bindings.xml,v  <--  mail-folder-bindings.xml
initial revision: 1.1
done
Checking in mail/base/content/mailWindow.js;
/cvsroot/mozilla/mail/base/content/mailWindow.js,v  <--  mailWindow.js
new revision: 1.63; previous revision: 1.62
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.194; previous revision: 1.193
done
Checking in mail/base/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v  <--  mailWindowOverlay.xul
new revision: 1.236; previous revision: 1.235
done
Checking in mail/base/content/messenger.css;
/cvsroot/mozilla/mail/base/content/messenger.css,v  <--  messenger.css
new revision: 1.22; previous revision: 1.21
done
Checking in mail/base/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mail/base/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.146; previous revision: 1.145
done
Checking in mail/base/content/subscribe.js;
/cvsroot/mozilla/mail/base/content/subscribe.js,v  <--  subscribe.js
new revision: 1.14; previous revision: 1.13
done
Checking in mail/base/content/subscribe.xul;
/cvsroot/mozilla/mail/base/content/subscribe.xul,v  <--  subscribe.xul
new revision: 1.20; previous revision: 1.19
done
Checking in mail/components/compose/content/MsgComposeCommands.js;
/cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.131; previous revision: 1.130
done
Checking in mail/components/compose/content/messengercompose.xul;
/cvsroot/mozilla/mail/components/compose/content/messengercompose.xul,v  <--  messengercompose.xul
new revision: 1.119; previous revision: 1.118
done

Comment 34

[Richard Marti (:Paenglab)]

When I want to create a new message a window with this error text appears:

XML Parsing Error: undefined entity
Location: chrome://messenger/content/messengercompose/messengercompose.xul
Line Number 385, Column 13:            <menupopup type="folder" mode="filing" showFileHereLabel="true"

Comment 35

[Phil Ringnalda (:philor)]

Checked in a bustage fix - for some reason, messengercompose.dtd doesn't have a fileHereMenu.accesskey, so using it in messengercompose.xul didn't work out very well. I'd guess the reason it's not there is "the menu already uses up every letter of the alphabet" but in any case I didn't feel like checking for a free one and adding it at this time of night. If anyone has ever missed having it there, there should already be a bug about adding one, and we can add it there.

Comment 36

[Joe Sabash [:JoeS1]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041103 Thunderbird/3.0a1pre ID:2008041103

Get Mail dropdown menu works fine, but it has been my habit to select an inbox in the folder pane, then click 'get mail' without using the dropdown selection.

This no longer works in today's build. For whatever reason I see:
Error: gSearchInput.setSearchCriteriaText is not a function
Source File: chrome://messenger/content/searchBar.js
Line: 606

Comment 37

[Joe Sabash [:JoeS1]]

I'm not sure what caused this error, though not connected with the 'get mail' problem, at some point I'm getting:
Error: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://messenger/content/mail-folder-bindings.xml :: act_add :: line 132"  data: no]
Source File: chrome://messenger/content/mail-folder-bindings.xml
Line: 132

I'll try to isolate exactly when this occurs.

Comment 38

[Phil Ringnalda (:philor)]

(In reply to comment #36)
> I see:

Please file a new bug, and mark it as blocking this bug if you believe this bug caused it.


(In reply to comment #37)
> I'm getting:

Please file a new bug, and mark it as blocking this bug if you believe this bug caused it.

Comment 39

[walter sheets]

I'm seeing the same problem with 'Get Mail' on linux.  Have you filed a new bug on this yet?

Comment 40

[Joe Sabash [:JoeS1]]

(In reply to comment #39)
> I'm seeing the same problem with 'Get Mail' on linux.  Have you filed a new bug
> on this yet?
> 
See dependent bug 428620

Comment 41

[[e-mail]]

Comment on attachment 312843 [details] [diff] [review]
patch v5

>+      <field name="_parentFolder">null</field>
>+      <property name="parentFolder"
>+                onget="return this._parentFolder;"
>+                onset="return this._parentFolder = val;"/>
What's the difference between this and <field name="parentFolder"/>?

>+      <field name="_listener"><![CDATA[({
>+        _menu: this,
>+        OnItemAdded: function act_add(aRDFParentItem, aItem) {
>+          aItem.QueryInterface(Components.interfaces.nsIMsgFolder);
Optimisation: check the folder's parent as being this.parentFolder?

>+          //xxx I'm not quite sure why this isn't always a function
Was this due to your listener not getting removed issue?

>+          var child = this._getChildForItem(aItem);
>+          if (child) {
>+            child._folder = aItem;
What causes the _folder to change?

>+              this._menu._teardown();
>+              this._menu._ensureInitialized();
Why don't the other cases reinitialise?

>+           var item = aItem.QueryInterface(Components.interfaces.nsIMsgFolder);
>+           for (var i = 0; i < this._menu.childNodes; i++) {
>+             var folder = this._menu.childNodes[i]._folder;
>+             if (folder && folder.URI == item.URI)
if (this._menu.childNodes[i]._folder == aItem)
[xpconnect does COM identity comparison for ==]

>+          if (!this._parentFolder) {
>+            var acctMgr = Cc["@mozilla.org/messenger/account-manager;1"].
>+                          getService(Ci.nsIMsgAccountManager);
>+            var count = acctMgr.accounts.Count();
>+            for (var i = 0; i < count; i++) {
>+              var acct = acctMgr.accounts.GetElementAt(i).QueryInterface(Ci.nsIMsgAccount);
>+              folders.push(acct.incomingServer.rootFolder);
>+            }
Maybe someone should implement nsIMsgAccountManager::GetSubFolders?

>+          if (this.getAttribute("showFileHereLabel") == "true" &&
>+              this._parentFolder && 
>+              (this._parentFolder.noSelect || this._parentFolder.canFileMessages)) {
Why bother showing the file here label for a folder that can't file here?
I don't see the RDF generating file here for noselect folders.

>+          if (mode && mode != "") {
>+            var filterFunction = this._filters[mode];
if (mode) implies mode != "" but consider if (mode in this._filters)

>+          function nameCompare(a, b) {
>+            var sortKey = a.compareSortKeys(b);
>+            if (sortKey)
>+              return sortKey;
>+            return a.prettyName.toLowerCase() > b.prettyName.toLowerCase();
compareSortKeys already returns the correct answer, see msgViewNavigation.js

>+              var popup = this.cloneNode(true);
>+              popup._teardown();
Eww :-\

>+            var time = aFolder.getStringProperty("MRUTime") || 0;
>+            if (time <= oldestTime)
This will fail around Sat Nov 20 2286 17:46:40 (UTC)

>+            addIfRecent(acct.incomingServer.rootFolder);
Hmm, accounts can count as recent?

>+          function sorter(a, b) {
>+             return a.getStringProperty("MRUTime") < b.getStringProperty("MRUTime");
No, sort functions return -1 or 1, not true or false.

>+          recentFolders.sort(sorter);
Note that if you have too many recent folders then they're already sorted!

>+          for (var i = 0; i < recentFolders.length; i++) {
>+            for (var j = i + 1; j < recentFolders.length; j++) {
>+              // We can end up with the same name in dupeNames more than once,
>+              // but that's ok.
>+              if (recentFolders[i].prettyName == recentFolders[j].prettyName)
>+                dupeNames.push(recentFolders[i].prettyName);
>+            }
>+          }
This feels inefficient to me, but I don't have any better ideas offhand

>+          // Now create the Recent folder and its children
So although I might never use the recent folder it always gets built?

>+          // Sigh... why aren't these in an IDL somewhere public?
They are now ;-)

>+          var biffStates = ["NewMail", "NoMail", "UnknownMail"];
>+          for each (var state in biffStates) {
>+            if (aFolder.biffState == Ci.nsIMsgFolder["nsMsgBiffState_" + state]) {
>+              aMenuNode.setAttribute("BiffState", state);
aMenuNode.setAttribute("BiffState", biffStates[aFolder.biffState]);

>+            if (child._folder.URI == aFolder.URI || (this._parentFolder &&
>+                this._parentFolder.isAncestorOf(aFolder))) {
if (child._folder == aFolder || child._parentFolder.isAncestorOf(aFolder)) ?

>+          while(this.hasChildNodes())
Space after while ;-)

>+         - @note _ensureInitialized can be called repeatedly without issue, so
>+         -       don't worry about it here.
>+        -->
>+      <handler event="popupshowing" phase="capturing">
Whenever you open subfolders, for instance; phase="target" would be better.

Comment 42

[Joey Minta]

(In reply to comment #41)
> What's the difference between this and <field name="parentFolder"/>?
In my mental model, fields are private and properties are public, but maybe that's just me?

> 
> >+          //xxx I'm not quite sure why this isn't always a function
> Was this due to your listener not getting removed issue?
Quite possibly, haven't had a chance to go back and look.

> 
> >+          var child = this._getChildForItem(aItem);
> >+          if (child) {
> >+            child._folder = aItem;
> What causes the _folder to change?
If I understand the question, it changes because I didn't want to end up with stale copies of the folders (with incorrect properties) hanging around.

> 
> >+              this._menu._teardown();
> >+              this._menu._ensureInitialized();
> Why don't the other cases reinitialise?
As far as I know, those cases don't impact the display of the folders, or are handled elsewhere.

> 
> >+           var item = aItem.QueryInterface(Components.interfaces.nsIMsgFolder);
> >+           for (var i = 0; i < this._menu.childNodes; i++) {
> >+             var folder = this._menu.childNodes[i]._folder;
> >+             if (folder && folder.URI == item.URI)
> if (this._menu.childNodes[i]._folder == aItem)
> [xpconnect does COM identity comparison for ==]
I've run into double-wrapping problems with == on XPCOM objects before.  Then a single-wrapped and double-wrapped copy of the same object show up as different.  dmose can tell you more.

> Maybe someone should implement nsIMsgAccountManager::GetSubFolders?
Go for it.  STEEL has something like this.

> Why bother showing the file here label for a folder that can't file here?
> I don't see the RDF generating file here for noselect folders.
The rule I saw was
<rule nc:NoSelect="true" iscontainer="true" isempty="false"> (SearchDialog.xul)

> 
> >+          if (mode && mode != "") {
> >+            var filterFunction = this._filters[mode];
> if (mode) implies mode != "" but consider if (mode in this._filters)
I intended unidentified/unimplemented modes to throw.

> 
> >+          function nameCompare(a, b) {
> >+            var sortKey = a.compareSortKeys(b);
> >+            if (sortKey)
> >+              return sortKey;
> >+            return a.prettyName.toLowerCase() > b.prettyName.toLowerCase();
> compareSortKeys already returns the correct answer, see msgViewNavigation.js
Cool!

> 
> >+            var time = aFolder.getStringProperty("MRUTime") || 0;
> >+            if (time <= oldestTime)
> This will fail around Sat Nov 20 2286 17:46:40 (UTC)
I think I'm ok with that.

> 
> >+            addIfRecent(acct.incomingServer.rootFolder);
> Hmm, accounts can count as recent?
That's up to the people who label their MRU times.

> 
> >+          function sorter(a, b) {
> >+             return a.getStringProperty("MRUTime") < b.getStringProperty("MRUTime");
> No, sort functions return -1 or 1, not true or false.
Doh.

> 
> >+          // Now create the Recent folder and its children
> So although I might never use the recent folder it always gets built?
This function is wrapped in
  if (!this._parentFolder && this.getAttribute("showRecent") == "true") {
so if the recent menu isn't being used in this instance, it won't get created.

> 
> >+          // Sigh... why aren't these in an IDL somewhere public?
> They are now ;-)
Is there a bug for updating callers?

> 
> >+          var biffStates = ["NewMail", "NoMail", "UnknownMail"];
> >+          for each (var state in biffStates) {
> >+            if (aFolder.biffState == Ci.nsIMsgFolder["nsMsgBiffState_" + state]) {
> >+              aMenuNode.setAttribute("BiffState", state);
> aMenuNode.setAttribute("BiffState", biffStates[aFolder.biffState]);
That seems like an inappropriate reliance on someone not changing the implementation numbers.

> 
> >+            if (child._folder.URI == aFolder.URI || (this._parentFolder &&
> >+                this._parentFolder.isAncestorOf(aFolder))) {
> if (child._folder == aFolder || child._parentFolder.isAncestorOf(aFolder)) ?
See the above note on wrapping.

> 
> >+          while(this.hasChildNodes())
> Space after while ;-)
Doh.

> Whenever you open subfolders, for instance; phase="target" would be better.
Can you explain the difference to me?

Comment 43

[[e-mail]]

(In reply to comment #42)
> (In reply to comment #41)
> > What's the difference between this and <field name="parentFolder"/>?
> In my mental model, fields are private and properties are public, but maybe
> that's just me?
Then why bother with the _ prefix?

> > >+          var child = this._getChildForItem(aItem);
> > >+          if (child) {
> > >+            child._folder = aItem;
> > What causes the _folder to change?
> If I understand the question, it changes because I didn't want to end up with
> stale copies of the folders (with incorrect properties) hanging around.
copies? Who's copying nsMsgFolder instances?

> > >+              this._menu._teardown();
> > >+              this._menu._ensureInitialized();
> > Why don't the other cases reinitialise?
> As far as I know, those cases don't impact the display of the folders, or are
> handled elsewhere.
You misunderstood - the other cases that tear down don't reinitialise.

> I've run into double-wrapping problems with == on XPCOM objects before.  Then a
> single-wrapped and double-wrapped copy of the same object show up as different.
Ah, so you're planning on rewriting nsMsgFolder in JS then?

> > Maybe someone should implement nsIMsgAccountManager::GetSubFolders?
> Go for it.
I'll file the bug for you then ;-)

> > Why bother showing the file here label for a folder that can't file here?
> > I don't see the RDF generating file here for noselect folders.
> The rule I saw was
> <rule nc:NoSelect="true" iscontainer="true" isempty="false"> (SearchDialog.xul)
Maybe all the menus had it, but this one got overlooked. Or maybe Seth was just not thinking straight at the time. Who can tell?

> > >+          if (mode && mode != "") {
> > >+            var filterFunction = this._filters[mode];
> > if (mode) implies mode != "" but consider if (mode in this._filters)
> I intended unidentified/unimplemented modes to throw.
OK, so what about the mode != ""?

> > >+            var time = aFolder.getStringProperty("MRUTime") || 0;
> > >+            if (time <= oldestTime)
> > This will fail around Sat Nov 20 2286 17:46:40 (UTC)
> I think I'm ok with that.
Did you work out why it will fail?

> > >+          // Now create the Recent folder and its children
> > So although I might never use the recent folder it always gets built?
> This function is wrapped in
>   if (!this._parentFolder && this.getAttribute("showRecent") == "true") {
> so if the recent menu isn't being used in this instance, it won't get created.
Available != used

> > >+          // Sigh... why aren't these in an IDL somewhere public?
> > They are now ;-)
> Is there a bug for updating callers?
I don't think there's a separate bug, but then I haven't worked out how best to write them in JS yet (one const nsMsgFolderFlags somewhere? where?)

> > >+          var biffStates = ["NewMail", "NoMail", "UnknownMail"];
> > >+          for each (var state in biffStates) {
> > >+            if (aFolder.biffState == Ci.nsIMsgFolder["nsMsgBiffState_" + state]) {
> > >+              aMenuNode.setAttribute("BiffState", state);
> > aMenuNode.setAttribute("BiffState", biffStates[aFolder.biffState]);
> That seems like an inappropriate reliance on someone not changing the
> implementation numbers.
Perhaps I could add a big scary comment to nsMsgFolder.idl ;-)

> > Whenever you open subfolders, for instance; phase="target" would be better.
> Can you explain the difference to me?
phase="bubbling" (default) = "please tell me about this event as late as possible, i.e. after all my children"
phase="capturing" = "please tell me about this event as soon as possible, i.e. before aby of all my children"
phase="target" = "please only tell me about this event if it's mine"

Comment 44

[[e-mail]]

Comment on attachment 312843 [details] [diff] [review]
patch v5

>+          function nameCompare(a, b) {
>+            var sortKey = a.compareSortKeys(b);
>+            if (sortKey)
>+              return sortKey;
>+            return a.prettyName.toLowerCase() > b.prettyName.toLowerCase();
>+          }
>+          folders = folders.sort(nameCompare);
Wouldn't it be handy if someone fixed compareSortKeys to work on accounts ;-)

Comment 45

[Joey Minta]

(In reply to comment #44)
> Wouldn't it be handy if someone fixed compareSortKeys to work on accounts ;-)

I don't understand why nsIMsgFolder::subFolders and nsIAccountManager::accounts doesn't just return the list sorted by default.  Does someone actually need an unsorted list? Are some of these calls on such a perf-critical path that sorting is harmful?

Comment 46

[David :Bienvenu]

Aren't they're currently in the order that they'll be displayed in the folder pane? If we were to allow the user to order their accounts (which I really think we should do), the simplest implementation would be to simply order the accounts list in the account pref. We also write out the accounts based on that same data, I believe, so if you were to sort m_accounts based on some other criteria, then we'd need an other way to order them. Similarly, for news accounts, the m_folders are sorted by newsrc order, which can be ordered with drag drop, so resorting that based on some other criteria might mess that feature up.

Which is not to say that you couldn't sort them, but you might have to be a little careful.

Comment 47

[[e-mail]]

(In reply to comment #46)
> Aren't they're currently in the order that they'll be displayed in the folder pane?
No, the RDF sorts them using the FolderTreeNameSort resource, implemented in nsMsgAccountManagerDataSource::GetTarget and nsMsgDBFolder::GetSortKey.