414038 - Replace rdf-driven folder pane with a js-driven/non-rdf treeview

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch wip

In line with http://wiki.mozilla.org/Thunderbird:Thoughts_on_Removing_RDF this is #2 on the list.  This bug is to replace the current RDF-driven folder-views with a js-driven/datasource agnostic one.

Attached is the current state of my tree.  As you can see, I've actually spliced a second tree into the same pane.  The temporary "Switch Tree" button allows me to compare behavior between the two trees.  (The js-tree has a temporary red background to make differentiation easy.)

Current status:
  -The js-tree now has basic functionality.  All 4 folder-views display the correct folders.  Clicking on a folder displays those messages in the msg pane.
  -Many of the more subtle feature of the old tree are currently missing
     -Context menus are broken
     -Drag'n'drop is broken
     -Folder open/close state is not persisted across restarts
     -A few css-styles are missing
  -There is currently a hang when closing an opened subfolder.

One big win this tree provides, besides removing rdf callers, is that it makes it extremely easy for extension authors to add new folder-views.  The plugin points (gFolderTreeModes and folderTreeView._mapGenerators) are marked.

I think the next step is going to be trying to excise the old tree out of the code, which is going to touch a lot of places.  Then work on polishing up the rough edges outlined above.

I'd really love to get some feedback on this approach, so if anyone has any comments/concerns please voice them.

Comment 1

[Joey Minta]

Attachment: reduce tree dependence

This patch lays the groundwork for replacing the rdf tree by removing many of the callers of GetFolderResource and all of the callers of GetFolderAttribute.  Both of these methods rely on the tree being rdf, and thus would break when the tree is replaced.

With this patch, we heavily reduce the dependence on the rdf interfaces on the front-end, again increasing our reliance on nsIMsgFolder.  That's as it should be in my mind.

Comment 2

[Joey Minta]

Comment on attachment 302360 [details] [diff] [review]
reduce tree dependence

Grr... I keep forgetting about this stupid localStore.rdf bug.  Ignore those changes obviously.

Comment 3

[Joey Minta]

Attachment: patch wip2

This diff includes the "reduce tree dependence" changes.  It then takes the "patch wip" changes, brings them in, excises the entire rdf tree, and fixes some of the remaining issues.  In particular, we now have working context menus!  Also, open-closed states are persisted across restarts.  Remaining open issues:
  -hang when closing some folders
  -drag and drop
  -GetFolderDatasource() needs to die.  This may require changing some interfaces.

Comment 4

[Joey Minta]

Comment on attachment 302360 [details] [diff] [review]
reduce tree dependence

This patch has bitrotted. I'm just going to fold it into the main patch.

Comment 5

[Joey Minta]

Attachment: patch wip3

This is almost there...  It includes fixes for each of the above mentioned problems.  Drag and drop now works, the freeze is gone, and a replacement for GetFolderDataSouce is in and working.  There are still some minor issues with dynamically updating the tree for changes, but this is totally dogfoodable if people want to try it.

Comment 6

[Joey Minta]

Attachment: patch v1

This patch should be ready for review.  It does however, need the patch in bug 418490 to be applied to work.  Also, if the folder-menus patch lands first (bug 413781) there is a slight conflict.  For these reasons I'm holding off on requesting actual review.

This patch should provide a drop-in replacement for the rdf tree with no regressions.  I'll note however that this patch no longer accommodates those people who have set the hidden pref to have a multi-column based tree-display.  It doesn't make such a display architecturally impossible, but it would be considerable more work.  If there's enough outcry for that option, my preference would be to work on restoring that part in another bug.

Comment 7

[Joey Minta]

Attachment: reduce tree dependence v2

This is an updated version of the "reduce tree dependence" patch which focuses on reducing usage of GetFolderResource and GetFolderAttribute, since both of these will break when the tree goes away.  In the interest of parallelizing things as much as possible while waiting for blocker reviews, I'm going to request review on this part now.  It can land as-is, with the only real change being that we have fewer RDF object flying around, and more nsIMsgFolders.

Comment 8

[Joey Minta]

Comment on attachment 312913 [details] [diff] [review]
reduce tree dependence v2

diff -r 85bf816703ca mail/base/content/mail3PaneWindowCommands.js
+      var folders = GetSelectedFolders();
+      if (!folders.length)
         return false;

That should actually be GetSelectedMsgFolders, though it won't matter once the new tree lands (x4).

Comment 9

[Phil Ringnalda (:philor)]

Comment on attachment 312913 [details] [diff] [review]
reduce tree dependence v2

STR:
1. Apply patch
2. Right-click folder, Rename
3. Click OK (don't bother changing it)
4. Listen to the console screech about how "folderTree is not defined" in widgetglue.js's RenameFolder()

(At least folder Properties and the isCommandEnabled call from selecting a message are broken, too, rename was just the first thing I tried.)

Comment 10

[Phil Ringnalda (:philor)]

On second thought, do bother changing it, since on OS X otherwise we try to raise a "folder with the same name exists" alert() with a sheet already up, and fail miserably.

Comment 11

[Joey Minta]

Attachment: remove GetFolderAttribute [Checkin: Comment 14]

This is a simplified/scaled back version of the reduce tree dependence idea.  This patch just removes the GetFolderAttribute function, and doesn't worry about cleaning up the surrounding code, ugly though it is.

Comment 12

[Joey Minta]

Comment on attachment 325114 [details] [diff] [review]
remove GetFolderAttribute
[Checkin: Comment 14]

I just realized bienvenu needs to look at this anyway, because of the dnd stuff.

Comment 13

[David :Bienvenu]

Comment on attachment 325114 [details] [diff] [review]
remove GetFolderAttribute
[Checkin: Comment 14]

looks ok, thx! I'll keep running with this and see if I find any regressions.

Comment 14

[Joey Minta]

GetFolderAttribute patch checked in.

Checking in mail/base/content/mail3PaneWindowCommands.js;
/cvsroot/mozilla/mail/base/content/mail3PaneWindowCommands.js,v  <--  mail3PaneWindowCommands.js
new revision: 1.49; previous revision: 1.48
done
Checking in mail/base/content/mailContextMenus.js;
/cvsroot/mozilla/mail/base/content/mailContextMenus.js,v  <--  mailContextMenus.js
new revision: 1.34; previous revision: 1.33
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.207; previous revision: 1.206
done
Checking in mail/base/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mail/base/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.155; previous revision: 1.154
done
Checking in mailnews/base/resources/content/messengerdnd.js;
/cvsroot/mozilla/mailnews/base/resources/content/messengerdnd.js,v  <--  messengerdnd.js
new revision: 1.66; previous revision: 1.65
done
Checking in mailnews/base/resources/content/msgSynchronize.js;
/cvsroot/mozilla/mailnews/base/resources/content/msgSynchronize.js,v  <--  msgSynchronize.js
new revision: 1.24; previous revision: 1.23
done

Comment 15

[Joey Minta]

Attachment: clean up folder selection

This was another part of the reduce tree dependence patch.  The function GetSelectedMsgFolders currently walks the folder-pane's nsITreeView selection property, and then converts this data into folders using GetFolderResource.  A bunch of people who want these folders copy the same code, however, instead of just calling this function.  Because GetFolderResource will need to disappear when a js-driven folder pane appears, it'd be helpful to have all of these consumers call into the same function.  Not to mention this just makes all the code cleaner.  This patch converts these consumers to use the normal function.

Comment 16

[Joey Minta]

Comment on attachment 325490 [details] [diff] [review]
clean up folder selection

Note to self: GetMsgFolderFromResource is unused after this patch (yay!)

Comment 17

[Phil Ringnalda (:philor)]

(In reply to comment #13)
> see if I find any regressions.

Ooh, ooh, I found one! Bug 439839.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 325490 [details] [diff] [review]
clean up folder selection

>+function SetupNewMenuItem(folder, numSelected, isServer, serverType,specialFolder)

Add a space please.

In IsPropertiesEnabled, would appear folderResource.Value would cause errors. And do we still need the try/catch?

Didn't get around to testing yet, there is also some (minor) bitrot.

Comment 19

[Joey Minta]

Attachment: clean up folder selection v2 [Checkin: Comment 21]

Fixes mkmelin's comments.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 325891 [details] [diff] [review]
clean up folder selection v2
[Checkin: Comment 21]

+  var folder =  folders[0];

Drop the extra space. Other than that, looks good! r=mkmelin

Comment 21

[Joey Minta]

Clean up folder selection v2 patch checked in

Checking in mail/base/content/commandglue.js;
/cvsroot/mozilla/mail/base/content/commandglue.js,v  <--  commandglue.js
new revision: 1.96; previous revision: 1.95
done
Checking in mail/base/content/mail3PaneWindowCommands.js;
/cvsroot/mozilla/mail/base/content/mail3PaneWindowCommands.js,v  <--  mail3PaneWindowCommands.js
new revision: 1.51; previous revision: 1.50
done
Checking in mail/base/content/mailContextMenus.js;
/cvsroot/mozilla/mail/base/content/mailContextMenus.js,v  <--  mailContextMenus.js
new revision: 1.35; previous revision: 1.34
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.208; previous revision: 1.207
done
Checking in mail/base/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mail/base/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.157; previous revision: 1.156
done
Checking in mail/base/content/widgetglue.js;
/cvsroot/mozilla/mail/base/content/widgetglue.js,v  <--  widgetglue.js
new revision: 1.30; previous revision: 1.29
done

Comment 22

[Joey Minta]

Attachment: patch v2

davida asked me to prioritize this bug, so rather than wait for the js-tree stuff, I went ahead and updated the patch. This patch still depends on iteratorUtils.jsm, but otherwise it's independent and could land. I don't know when iteratorUtils is going to get reviewed, but I wanted to at least start the review process here.

Comment 23

[David Ascher (:davida)]

I've been poking around w/ patch v2, and it's certainly much, much easier to work with than the RDF stuff.  Nice!

Note that we want to move to a world where we can display things other than folders in the folderpane (calendar-related things for example), maybe spacer rows, or maybe even non-selectable "headers" grouping different sets of folders.  See https://wiki.mozilla.org/Image:New-folder-pane-w-calendar-tasks.png for example.

The current patch assumes all of the tree entries have _URI attributes, which is probably not correct in those expanded use cases.  Also, I suspect we want to come up with a framework allowing extensions to register entries to show up in the tree at various places in the hierarchy, without having to introspect the gFolderTreeView structure too much.

Also, while this patch maintains the different mode views, I'm not sure that just filtering contents of the folder pane is necessarily the right model -- I suspect that if we keep the mode views, that it'll make sense to make it more generic -- not just filtering, for example.

It's probably easier to defer those discussion points to smaller patches that can be discussed after this has landed though.

Comment 24

[Joey Minta]

(In reply to comment #23)
> Note that we want to move to a world where we can display things other than
> folders in the folderpane (calendar-related things for example), maybe spacer
> rows, or maybe even non-selectable "headers" grouping different sets of
> folders.  See
> https://wiki.mozilla.org/Image:New-folder-pane-w-calendar-tasks.png for
> example.
The aim of the ftvTreeItem object was to try and provide a generic wrapper for these kinds of objects. The interface defined above that object can be wrapped around things like calendars/events, and the tree should still be able to display them.  But see comments below for further nuances.

> 
> The current patch assumes all of the tree entries have _URI attributes, which
> is probably not correct in those expanded use cases.  
In one version of this patch, I wrapped the URI in a id() property of the ftvTreeItem, which is probably the way to go, and would eliminate this problem. The catch is that there are lots of consumers of the tree (This patch makes those consumers easier to identify, because more of them now explicitly call gFolderTreeView), and these all assume that things like the selected item are l msgFolders. We'll need to audit them obviously. (I have some more plans here, specifically further consolidating folder commands/calls that will help, but are beyond the scope of this patch.)

> Also, I suspect we want
> to come up with a framework allowing extensions to register entries to show up
> in the tree at various places in the hierarchy, without having to introspect
> the gFolderTreeView structure too much.
Agreed. One can easily tweak the _mapGenerators to check a "additionalObjects" array, which would generate more tree-items.

> 
> Also, while this patch maintains the different mode views, I'm not sure that
> just filtering contents of the folder pane is necessarily the right model -- I
> suspect that if we keep the mode views, that it'll make sense to make it more
> generic -- not just filtering, for example.
Also agreed, and the idea with this patch was to make it really easy for an extension to expose this functionality. Obviously, core code could expose it too.

> 
> It's probably easier to defer those discussion points to smaller patches that
> can be discussed after this has landed though.
I'd appreciate that. To summarize, this patch should be modified to not depend on .URI explicitly. Further patches potentially would include (1) add ability for extensions to add items (to the "all" mode) (2) Audit tree-consumers to not be folder specific (this may involve the folder-command consolidation I mentioned above) (3) Expose flexibility in creation of modes.

Comment 25

[David :Bienvenu]

I'm running with these patches now. Two problems off the bat; one is that one of my accounts appears twice in the folder pane (that may be a corruption of my prefs that rdf "hides"). The second is that whenever I receive new mail, all my accounts in the folder pane collapse away, other than my first account. This is rather disconcerting. I am running with gloda, but I doubt it's responsible - I will try and verify. Thirdly (of the two problems :-) ), my default imap account shows up initially with Inbox as a container, and then as online folder discovery runs, realizes that I've told it to show children of the Inbox as peers, which happens in rather slow chunks. I'll try to figure out what's going on there.

Comment 26

[David :Bienvenu]

I forgot I had a saved search under the imap inbox, which is why it was showing a sub-folder of the inbox. What's happening is that the startup imap folder discovery, where we're just iterating over the .msf files on disk, isn't triggering a sufficient notification for the js folder pane to add the folders, apparently.

Comment 27

[David :Bienvenu]

ok, the collapse on new mail issue is because we rebuild the folder pane even when it's a header that's been added, and when we rebuild, we don't realize that the server has been expanded.  That's easy enough to fix.

Comment 28

[David :Bienvenu]

I've fixed the collapsing of accounts on new mail arriving, though there still seem to be some cases that cause collapsing. I've also found that we don't call PerformExpand on servers when expanding a server, and drag drop doesn't work - both of these were handled by the folderObserver in msgMail3PaneWindow.js, so presumably that isn't hooked up or getting notified...

Comment 29

[David :Bienvenu]

The reason my imap server shows up initially with just an inbox, without any of the .msf files on disk, is that the new folder pane is not forcing the local profile imap folder discovery, by calling getSubfolders. The old RDF folder pane did this before it painted anything, which avoids all the itemAdded notifications. I need to figure out a way to force the initial folder discovery before the js folder view is up and running, and before we load the inbox and do the online discovery.

Comment 30

[David :Bienvenu]

actually, the problem is more that the new folder pane isn't populating the rdf datasource with the folders it discovers, so the imap folder discovery code, which uses rdf, doesn't know about the folders. Maybe converting to the folder lookup service will help; I'll have a look at that other patch.

But the scary thing is that I'm seeing a lot of duplication of .msf and .sbd files on disk, e.g., foo.msf, foo-1.msf, foo-2.msf, folder-1.sbd, folder-2.sbd, etc, and I believe they started showing up when I started running with the folder pane. This is for imap folders. I need to figure out what's going on there as well.

Comment 31

[David :Bienvenu]

Attachment: saving off my changes to folderPane.js

saving off a few changes to folderPane.js, particularly to get performExpand to be called, and not rebuild so often.

Comment 32

[Kent James (:rkent)]

I'm trying to apply "patch v2", but I get a failure caused by this part of the patch at line 1450:

-function loadFolderViewForTree(aNewFolderView, aFolderTree)
+function loadFolderViewForTree(aNewFolderView)
 {
   var folderPaneHeader = document.getElementById('folderpane-title');
   var database = aFolderTree.database;
   var nsIRDFDataSource = Components.interfaces.nsIRDFDataSource;

aFolderTree is no longer defined, but is used in the routine. Am I doing something wrong?

Comment 33

[David :Bienvenu]

I can't remember if I had to fix that locally or not :-( But it looks like aFolderTree needs to be an arg.

Comment 34

[Joey Minta]

It doesn't need to be in arg. With this patch, it always refers to the toolbar-menu-popup that can be added when someone customizes their toolbar. Just replace aFolderTree with the relevant getElementById call.

Comment 35

[Kent James (:rkent)]

Let me list the problems I have seen so far. I am testing with attachment 336838 [details] [diff] [review] and the replaced folderPane.js from attachment 338666 [details].

1. Various js errors occur if there is a folderTree view in the toolbar that prevent anything from showing, such as my comment 32 as well as others.

2. "Show expanded columns in the folder pane" do not work.

3. IMAP folders with virtual subfolders are appearing twice, once as "test" and a second time as "INBOX.test"

4. After I created a new IMAP folder, that new folder and all Inbox subfolders are showing as "INBOX.ANewFolder" "INBOX.Junk" etc instead of "ANewFolder" and "Junk"

5. I have one profile (POP and local folders only) that only shows Local Folders (and children) when run under trunk, but shows another parent (and children) as well under these patches. The extra mail folders are there in the profile, so I'm not sure why they are different. On another profile that uses a global inbox, I see a separate parent server for the POP account (with Inbox and Trash folders) that I don't see when running under trunk.

6. Upon startup, the IMAP portion of the folder tree adds and removes folder items a few times before it settles down.

7. The displayed unread count for an NNTP folder jumps from 632 (which is what trunk always shows) to 3067 as the folder is expanded. When the folder is selected, the count goes back down to 632.

8. Creating a new folder collapsed the Local Folders tree.

9. If I open a folder in a new tab, switching back and forth between two open tabs that point to different tabs folders does not switch the folder selected, or the messages displayed in the message pane (This is a regression from trunk).

10. As reported earlier, drag and drop does not work for messages to a folder.

This represents about one hour of experimenting. There are console errors occurring as well, possibly associated with some of the above issues.

Comment 36

[Joshua Cranmer [:jcranmer]]

(In reply to comment #35)
> 5. I have one profile (POP and local folders only) that only shows Local
> Folders (and children) when run under trunk, but shows another parent (and
> children) as well under these patches. The extra mail folders are there in the
> profile, so I'm not sure why they are different. On another profile that uses a
> global inbox, I see a separate parent server for the POP account (with Inbox
> and Trash folders) that I don't see when running under trunk.

I suspected as much when I first read the latest patch that global Inbox would be broken; that saves me from having to confirm so when I do my experimenting bout.

> 7. The displayed unread count for an NNTP folder jumps from 632 (which is what
> trunk always shows) to 3067 as the folder is expanded. When the folder is
> selected, the count goes back down to 632.

I don't think that this is a regression, because I recall seeing jumps of this kind occurring on the builds I've spun. I don't know anything of specifics, but newsgroup counts are known to be often inaccurate; see bug 71728 and its dependencies.

I'll need to read the code carefully and do testing before I can give an accurate assessment on this point, though...

Comment 37

[[e-mail]]

Comment on attachment 325891 [details] [diff] [review]
clean up folder selection v2
[Checkin: Comment 21]

>+//xxx this function should go away
> function GetSelectedFolders()
That shouldn't bee too hard, it only has one caller...

Comment 38

[Joshua Cranmer [:jcranmer]]

Some start on the batch of regressions I've uncovered based on revision 397:8dcf88e8d2cc of the kill-rdf repo.

1. All accounts seem to claim to have new messages (except news which never claims this).

2. When loading one IMAP server, it added some of the IMAP folders, deleted them, added some more, deleted, etc., for several seconds.

3. The Drafts and Sent folder of said IMAP server do not have the special icons. Other IMAP servers do have the icons.

4. One of my RSS folders changed its name to "CSS0dc231a4." My attempts to debug this, however, seem to show that this was actually regressed earlier.

5. The Outbox in my Local Folders does not have the special icon.

6. Newsgroup names have changed: I see "fr.comp.lang.java" where I should be seeing "f.c.l.java"

7. Many folders also have the claim to be new which goes away when clicking on them.

8. I see this assertions:

[ looks like during rebuild ]
WARNING: NS_ENSURE_TRUE(identity) failed: file /home/source/kill-rdf/mailnews/base/util/nsMsgIncomingServer.cpp, line 1835

[ Something about incorporating messages... ]
###!!! ASSERTION: shouldn't have any listeners: 'm_ChangeListeners.IsEmpty()', file /home/source/kill-rdf/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 887

[ Subsequently, during usage, at various points ]
WARNING: resource was never registered: file /home/source/kill-rdf/mozilla/rdf/base/src/nsRDFService.cpp, line 1307

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80550006 [nsIMsgFolder.getMsgDatabase]"  nsresult: "0x80550006 (<unknown>)"  location: "JS frame :: chrome://messenger/content/mailWidgets.xml :: parseFolder :: line 1911"  data: no]

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80550006: file /home/source/kill-rdf/mailnews/base/util/nsMsgDBFolder.cpp, line 3995

failed to get view & sort values.  ex = [Exception... "Component returned failure code: 0x80550006 [nsIMsgFolder.getMsgDatabase]"  nsresult: "0x80550006 (<unknown>)"  location: "JS frame :: chrome://messenger/content/commandglue.js :: FolderPaneSelectionChange :: line 824"  data: no]

This is a copy of the profile I've been using since TB 1.5. I did not test anything like subscribing, movemail, adding/removing new accounts, etc., just opening folders. Since this is the first time I've actually used it in full on TB 3.0, I may be noticing regressions from other changes in this list as well.

The following is my userChrome:
#folderTree > treechildren::-moz-tree-cell-text(hasUnreadMessages-true) {
	color: red !important;
}
#folderTree > treechildren::-moz-tree-cell-text(isServer-true,biffState-NewMail) {
	font-weight: bold !important;
	color: red !important;
}

Comment 39

[David :Bienvenu]

This happens on the trunk as well:

[ Something about incorporating messages... ]
###!!! ASSERTION: shouldn't have any listeners: 'm_ChangeListeners.IsEmpty()',
file /home/source/kill-rdf/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 887

#2 is the problem I'm dealing with now. I'm fairly close to making folder object creation not go through RDF at all, but until I finish that, there will be some weirdness, espcially with special folders.

Re the userchrome stuff, I'm not sure we're exposing the same attributes to the tree that we used to; jminta would know more about that.

Comment 40

[Joey Minta]

We should be exposing the same attributes (though there's an argument to be made that the code would be much cleaner if we changed some of them). I didn't understand jcranmer to be saying his userChrome didn't work, but if some css is indeed not working with the new tree, let me know.

Comment 41

[Joshua Cranmer [:jcranmer]]

What I was mentioning was that the userChrome was showing that certain attributes (e.g. biffState-NewMail) were being set when they should not have been set. I don't have enough data to determine why they were incorrectly set.

Comment 42

[Joshua Cranmer [:jcranmer]]

(In reply to comment #38)
> 4. One of my RSS folders changed its name to "CSS0dc231a4." My attempts to
> debug this, however, seem to show that this was actually regressed earlier.

Indeed, I have a rough regression range for this... 2005-08-12 (when Mozilla 1.8 branched) to 2007-06-01. I'd reduce it a bit more, but I run into a thorny problem in that I don't have easy access to a box with both X and libstdc++.so.5. I'm filing (or looking for) a different bug on this issue.

So it's not your fault.

Comment 43

[Magnus Melin [:mkmelin]]

The name change can happen if the folder has a name which isn't valid (on all platforms). Does the real name end with "." or by any chance? That's one fix i recall, there may be others.

Comment 44

[David :Bienvenu]

In jminta's kill-rdf repo, I finally figured out how to fix the restoring of expanded state of servers and folders on startup, so it's a lot more usable now.

Comment 45

[David :Bienvenu]

At this point, I'm not seeing the problems jcranmer saw. Here are the problems I see:

1. Add/Deleting a folder causes us to rebuild the view, which causes us to do the stuff we do on server expand, like refresh imap folders, and news counts.
2. Options | advanced is blank (no idea how we could have broken that)
3. When we select a folder on startup, we don't pass in the msg window, which leads to an assertion in nsMsgBDFolder::AutoCompact.
4. Subscribe ui doesn't work (more precisely, toggling the subscribe checkmarks doesn't do anything)
5. I'm seeing an assertion in nsMsgDBFolder::ApplyRetentionSettings, when we fail to open the db. Not sure if it's related or not.
6. Account settings shows "mdn" and "smime" lines instead of Return Receipts and Security.

I'll try to start knocking things off the list. The other issue is that someone needs to build SeaMonkey and verify that it still works.

Comment 46

[Joshua Cranmer [:jcranmer]]

Pedantically, the account manager bugs should go on that bug itself, but I'll list them here anyways for the sake of having a unified list.

The following problems are reproducible by just going through the list page by page:
1. JS warnings on opening:
JavaScript strict warning: chrome://messenger/content/AccountManager.js, line 116: assignment to undeclared variable addAccountButton
JavaScript strict warning: chrome://messenger/content/AccountManager.js, line 117: assignment to undeclared variable removeButton
JavaScript strict warning: chrome://messenger/content/AccountManager.js, line 118: assignment to undeclared variable setDefaultButton
2. JS warning on various pages (server settings?):
Error in enabling/disabling server.TrashFolderPicker
3. JS warning on some pages (probably a synchronization settings one):
Error: folder is not defined
Source File: chrome://messenger/content/msgSynchronize.js
Line: 175
4. Offline settings page:
Chrome file doesn't exist: /home/source/build/kill-rdf/mozilla/dist/bin/chrome/classic/skin/classic/communicator/icons/offline.png
5. I hit this on one of the Local Folders pages (movemail has a similar one, probably same thing):
###!!! ASSERTION: This should probably never be called!: '0', file /home/source/kill-rdf/mailnews/local/src/nsNoneService.cpp, line 184
6. Local folders identity page:
JavaScript error: chrome://messenger/content/AccountManager.js, line 813: account.defaultIdentity is null
7. The default charset encoding picker is not initialized to anything.

Creating accounts:
1. RSS and movemail accounts cannot be created, asserting out in nsMsgIncomingServer.cpp (CreateRootFolderFromUri is not overridden).
2. Adding a news account (not sure if valid):
srcServer.ServerType-nntp = undefined
3. The account manager tree doesn't update. Extremely serious regression that I'm going to have to fix.
4. When removing an account, it only gets the correct account to remove if I select something other than the server settings pages.

Removing an identity:
JavaScript error: chrome://global/content/bindings/listbox.xml, line 615: this.currentItem._fireEvent is not a function

When I closed out of the dialog, I got on two different occasions:
The newsgroup Trash does not appear to exist on the host news.mozilla.org.  Would you like to unsubscribe from it?

This was immediately followed by a crash on both occasions, appearing in nsMsgKeySet.

Comment 47

[David :Bienvenu]

I've fixed #6 in my list, 6. Account settings shows "mdn" and "smime" lines instead of Return Receipts and Security.

Comment 48

[David :Bienvenu]

I've fixed most of the account settings issues, I believe (a few of them, I couldn't reproduce, one of them was not specific to the kill-rdf branch, but I fixed anyway...). I'll keep working on them, especially the account creation and identity ones.

Comment 49

[David :Bienvenu]

Creating accounts seems fine - I have no problems creating rss accounts and CreateRootFolderFromUri is overridden in nsMailboxServer, which both movemail and rss both inherit from (which makes me wonder if you rebuilt everything, jcranmer). 

>2. Adding a news account (not sure if valid):
>srcServer.ServerType-nntp = undefined
This happens on the trunk as well, so it's not related to the kill-rdf work.

The account manager tree seems to update fine. I'll look at the identity stuff next...but as things stand now, I'm not seeing nearly as many issues - adding/removing accounts causing a rebuild is the only big issue I'm seeing at the moment.

Comment 50

[Joshua Cranmer [:jcranmer]]

(In reply to comment #49)
> Creating accounts seems fine - I have no problems creating rss accounts and
> CreateRootFolderFromUri is overridden in nsMailboxServer, which both movemail
> and rss both inherit from (which makes me wonder if you rebuilt everything,
> jcranmer). 

I fixed that already. And it turns out that only movemail had the problem.

Only problems I still see are 4 and 7; I haven't checked the removing identity one again, though. I don't see number 3 anymore, but I didn't try to fix it, so I don't know if that's merely something harder to reproduce or not...

Comment 51

[David :Bienvenu]

I fixed 3 yesterday. I also just checked in fixes for editing saved searches, and fixed add folder not to do a rebuild. I'll try to recreate 4 and 7...

Comment 52

[David :Bienvenu]

I was not able to reproduce 7. The default charset encoding picker is not initialized to anything - that's apparently from the server settings on an nntp server. Mine is set to my locale's default, Western.

I don't see the problem with the offline settings (syncing & disk space?), but I'm using the default windows theme.

The current problem I'm fighting is that changing an imap server settings, like get messages on startup, doesn't persist. I don't see anything interesting in the console either.

Comment 53

[David :Bienvenu]

Also, empty trash isn't removing imap sub-folders in the kill-rdf repo, but it seems to be OK on the trunk. I believe it's not even trying to remove them on the server, though it does ask for confirmation from the user, so it gets that far. I don't see anything interesting on the console.

Comment 54

[David :Bienvenu]

the reason changing imap server settings doesn't work is also related to the trash folder. For servers where all folders are under the INBOX, the trash should be INBOX/Trash, but we try to first create it as Trash (I think), which fails, and then if I pick Inbox/Trash in the picker, it creates Inbox/Trash instead of INBOX/Trash, and we fail to create the parent resource, and bail out. In any case, the code is not prepared for GetMsgFolderFromUri to fail, since it never used to fail on the trunk.

I think the picker is not returning the right folder name for the INBOX - at some point, we have to force imap uri's starting with Inbox to INBOX, but I'm not sure where to do that...

Comment 55

[David :Bienvenu]

I've changed the trash picker code to always upper case a leading INBOX, which fixes the problem.

I've also fixed the empty trash folder pane code - it was checking the return value of confirmEx incorrectly.

Comment 56

[David :Bienvenu]

I've made clicking on checkboxes in imap subscribe work. It was changed not to be a cycler, and making it a cycler didn't help (which was probably why it was made not a cycler :-) ). So I added an onclick handler like the search subscribe view has. But there are definitely issues with the handling of unsubscribed folders - they don't all show up in the subscribe UI, but they get added to the folder pane :-) And collapsing folders in the subscribe UI can get into an infinite js loop.

Comment 57

[David :Bienvenu]

We have a bit of a problem with the folder lookup service semantics, with the subscribe ui. With RDF, we could create folders w/o having them get added to the folder pane, because creating folders didn't necessarily cause an OnItemAdded notification. But the folder lookup service requires that we send an OnItemAdded notification as a result of AddSubfolder. But, unfortunately, the folder pane gets this notification as well, and tries to add the unsubscribed folder to the folder pane.

So, one possibility is to make the folder pane smart enough to ignore unsubscribed folders for imap servers that are using subscribe (and make it ignore news folders that aren't subscribed as well, if needs be). An other would be to use some other mechanism besides onItemAdded to notify the folder lookup service of folders - one possible motivation for this would be to avoid sending onItemAdded notifications to things like gloda, win search, spotlight, etc, for unsubscribed imap folders when the user brings up the subscribe UI. News apparently doesn't have this issue, so we could probably live with the imap problem.

Or I could make the imap subscribe UI not create folder objects at all...

Comment 58

[Joshua Cranmer [:jcranmer]]

When I first did the subscribe changes, I noted why IMAP subscribe was failing. My first instinct was to rush to fix the code to not use folders. I didn't because too much plumbing looked like it had to be changed, as IMAP subscription involves protocol work, which involves folders. NNTP doesn't need protocol work to subscribe to a newsgroup, so it never had this problem.

Another possibility that might be worth investigating is to create folder objects directly and eschew the entire folder service and notification stuff or maybe holding off on creation until the actual subscribe/unsubscribe time, where in the first case it will be added momentarily anyways, and in the second it should already exist.

Comment 59

[David :Bienvenu]

Thx for the comments, Joshua - yes, it looks like subscription involves folders, but the imap service really just wants a folder for the hierarchy delimiter. The actual folder name is passed into the subscribe calls. I think it's sufficient to find the most immediate ancestor we can for that purpose (though this whole subscribe UI process is really turning out to be much more complicated than I thought, so I could be wrong)

Comment 60

[David :Bienvenu]

I've got imap subscribe working better, though there are issues when I try to subscribe to both a parent and child - it's almost as if the subscribe window is closing before the subscription finishes, which causes strange context assertions since it's listening for backend events.

Next, I'm going to work on getting rid of as many of the new warnings I'm seeing, and also, fix this bug:

1. have IMAP inbox with sub-folders, collapse inbox, select inbox
2. Expand inbox

We end up selecting the last sub-folder of Inbox (in my case), instead of keeping the selection on the inbox.

Comment 61

[David :Bienvenu]

I've fixed where we insert new folders (we weren't taking into account expanded children ). I've also fixed the bug described in #60 - we were telling the tree the new rows were inserted at the parent row, not the parent row + 1, so it adjusted the selection needlessly.

Next, gonna see why drag drop of folders within folder pane is not working.

Comment 62

[David :Bienvenu]

ok, I've fixed drag drop of folders and messages (except that the drop target for messages isn't right - it shows between the folders, which isn't so helpful)

Comment 63

[David :Bienvenu]

Attachment: some core changes to start paving the way

I'm trying to break out some of the work on the repo that can be done independently in order to reduce the size of the final patch. This patch does three things:

1. Quiets a warning when bringing up the account settings - the account datasource was whining quite a bit.

2. Makes the local stores (movemail, local folders, rss) share a common class, nsMailboxServer, so we can start getting rid of code duplication.

3. Makes it so we don't have to go through rdf to create the appropriate kinds of folders. The intent is for methods like CreateFolderFromUri to get called from AddSubfolder, which is how the folder lookup service tries to create folders. None of these functions are called yet, however (with this patch, that is; they're called in the kill-rdf repo).

I'm building the suite from the kill-rdf repo right now to see how it works. I don't know if SM is going to want to use the new non-rdf folder pane and folder lookup service right away, which may mean an awkward period of #ifdefs, or some sort of wrapper function that hides rdf from as much code as possible.

Comment 64

[Joey Minta]

(In reply to comment #63)
> I'm building the suite from the kill-rdf repo right now to see how it works. I
> don't know if SM is going to want to use the new non-rdf folder pane and folder
> lookup service right away, which may mean an awkward period of #ifdefs, or some
> sort of wrapper function that hides rdf from as much code as possible.
One option would be to use the folder-lookup-server itself as the wrapper. In SeaMonkey it could just return RDF.GetResource, while in Thunderbird it takes the non-rdf path.

Comment 65

[[e-mail]]

Comment on attachment 344524 [details] [diff] [review]
some core changes to start paving the way

>+  nsCOMPtr<nsIMsgFolder> folder = do_QueryInterface(aSource);
>+  if (!folder)
>+    return NS_OK;
>   nsCOMPtr<nsIMsgIncomingServer> server;
>   nsresult rv = getServerForFolderNode(aSource, getter_AddRefs(server));
getServerForFolderNode already does this.

>+  // all children will override this to create the right class of object.
>+  virtual nsresult CreateChildFromURI(const nsCString &uri, nsIMsgFolder **folder);
Why isn't this pure virtual?

>+  virtual nsresult CreateRootFolderFromUri(const nsCString &serverUri,
>+                                           nsIMsgFolder **rootFolder);
Or this?

>+  nsImapMailFolder *newRootFolder = new nsImapMailFolder;
>+  if (!newRootFolder)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  newRootFolder->Init(serverUri.get());
>+  NS_ADDREF(*rootFolder = newRootFolder);
Nit: should AddRef the folder *before* calling any methods on it.

>+		nsMailboxServer.cpp \
Forgot to hg add before hg diff?

Comment 66

[David :Bienvenu]

(In reply to comment #65)
> (From update of attachment 344524 [details] [diff] [review])
> >+  nsCOMPtr<nsIMsgFolder> folder = do_QueryInterface(aSource);
> >+  if (!folder)
> >+    return NS_OK;
> >   nsCOMPtr<nsIMsgIncomingServer> server;
> >   nsresult rv = getServerForFolderNode(aSource, getter_AddRefs(server));
> getServerForFolderNode already does this.

true, I guess I should just get rid of the NS_WARNING after the call site.

> 
> >+  // all children will override this to create the right class of object.
> >+  virtual nsresult CreateChildFromURI(const nsCString &uri, nsIMsgFolder **folder);
> Why isn't this pure virtual?
> 
> >+  virtual nsresult CreateRootFolderFromUri(const nsCString &serverUri,
> >+                                           nsIMsgFolder **rootFolder);
> Or this?

Consistency with the other methods that probably should be pure virtual ;-) But I do like having them pure virtual better.

> 
> >+  nsImapMailFolder *newRootFolder = new nsImapMailFolder;
> >+  if (!newRootFolder)
> >+    return NS_ERROR_OUT_OF_MEMORY;
> >+  newRootFolder->Init(serverUri.get());
> >+  NS_ADDREF(*rootFolder = newRootFolder);
> Nit: should AddRef the folder *before* calling any methods on it.
> 
> >+		nsMailboxServer.cpp \
> Forgot to hg add before hg diff?

I'll fix these and add a new patch in a few...

Comment 67

[David :Bienvenu]

Attachment: patch addressing Neil's comments [Checkin: Comment 82]

this addresses Neil's comments.

I've got the kill-rdf repo SeaMonkey busy importing my TB profile; I'll work on making it work next.

Comment 68

[David :Bienvenu]

SM is working reasonably well in the kill-rdf repo. One problem - the location bar is broken, which gives SM problems on startup, so I had to catch an exception in OnLoadLocationTree.

Comment 69

[David :Bienvenu]

I've fixed the SM location bar by restoring the widget that was removed from mailwidgets.xml - it should be harmless for TB. At this point, it would be helpful if SM folks could check out a SM build built from the kill-rdf repo and see if everything is working OK for them.

Comment 70

[David :Bienvenu]

the next issue I'm working on is the search messages folder picker, which doesn't scroll at all, or handle keypresses - one error is in onKeyPressMenuList,           var box = this._tree.treeBoxObject;

gives this._tree is undefined.

Comment 71

[David :Bienvenu]

I've fixed the key handling problem. Still looking at the scrolling problem. (attempting to scroll closes the widget)

Comment 72

[David :Bienvenu]

Attachment: separating from kill-rdf work...

This attempts to separate out this work from the kill-rdf work, because this is really the thing blocking an extensible folder pane. Things work reasonably well in my source tree, other than the folder location picker, and empty Junk.

I may have messed up making this patch - I had to remove some other diffs in my tree (I know; I need to learn how to use hg queues). And I'm sure jminta can spot some changes I should have included, or not included :-)

But I'd appreciate if you could look at this, Mark, and see what you think.

Comment 73

[Mark Banner (:standard8)]

Comment on attachment 345724 [details] [diff] [review]
separating from kill-rdf work...

This doesn't apply cleanly (as I think you expected) and there are a quite a few hunks that don't apply, so I'm not going to try and do it manually. Also, now I've looked through the patch, I think as you've got the threading patches in now, it'll probably sort most of it out.

This first comment I'm doing is on the modified files. I'll do a second comment later for the new files that have been added.

> function SwitchView(command)
> {

The changes here seem to be from a different bug, but I think that's been committed now.

>+  var Ci = Components.interfaces;

I think these should use const throughout (same applies to Cc).

>@@ -114,20 +114,29 @@ var FolderPaneController =
>     // really disabled. kick out if the command should be disabled.
>     if (!this.isCommandEnabled(command)) return;
> 
>     switch ( command )
>     {
>       case "cmd_delete":
>       case "cmd_shiftDelete":
>       case "button_delete":
>-        MsgDeleteFolder();
>+        // Even if the folder pane has focus, don't do a folder delete if
>+        // we have a selected message, but delete the message instead.
>+        if (GetNumSelectedMessages() == 0)
>+          gFolderTreeController.deleteFolder();
>+        else
>+          DefaultController.doCommand(command);
>+        break;
>+        break;

Two breaks.

>   // Never show "Undelete" in the 3-pane for folders, when delete would
>   // apply to the selected folder.
>-  if (this.WhichPaneHasFocus && WhichPaneHasFocus() == GetFolderTree()
>+  if (this.WhichPaneHasFocus
>+      && WhichPaneHasFocus() == document.getElementById("folderTree")
>       && GetNumSelectedMessages() == 0)

nit: operators on the end of line please (I know it was wrong before, but I'd still like it fixed).

>@@ -1417,26 +1344,27 @@ let mailTabType = {
>     aTab.messenger = messenger;
> 
>     aTab.msgSelectedFolder = gMsgFolderSelected;
> 
>   // clear selection, because context clicking on a folder and opening in a new
>   // tab needs to have SelectFolder think the selection has changed.
>   // We also need to clear these globals to subvert the code that prevents
>   // folder loads when things haven't changed.
>-  GetFolderTree().view.selection.clearSelection();
>-  GetFolderTree().view.selection.currentIndex = -1;
>+  var folderTree = document.getElementById("folderTree");
>+  folderTree.view.selection.clearSelection();
>+  folderTree.view.selection.currentIndex = -1;
>   gMsgFolderSelected = null;
>   msgWindow.openFolder = null;
> 
>   // clear thread pane selection - otherwise, the tree tries to impose 
>   // the current selection on the new view.
>   gDBView = null; // clear gDBView so we won't try to close it.
>-    SelectFolder(aTab.uriToOpen);
>-    aTab.dbView = gDBView;
>+  gFolderTreeView.selectFolder(GetMsgFolderFromUri(aTab.uriToOpen));
>+  aTab.dbView = gDBView;
>   },

Something is wrong with the indentation here. 

> function IsCompactFolderEnabled()
> {
>-  var server = GetServer(GetSelectedFolderURI());
>-  return (server &&
>-      (server.type != 'nntp') && // compact news folder is not supported
>-      ((server.type != 'imap') || server.canCompactFoldersOnServer) &&
>-      isCommandEnabled("cmd_compactFolder"));   // checks e.g. if IMAP is offline
>+  var folder = GetSelectedMsgFolders()[0];
>+  if (!folder)
>+    return;
>+
>+  return ((folder.server.type != 'nntp') && // no compact for news folders
>+      ((folder.server.type != 'imap') ||
>+        folder.server.canCompactFoldersOnServer) &&
>+      isCommandEnabled("cmd_compactFolder")); // checks e.g. if IMAP is offline

These need re-aligning. More like:

return ((folder.server.type != 'nntp') && // no compact for news folders
        ((folder.server.type != 'imap') ||
         folder.server.canCompactFoldersOnServer) &&
         isCommandEnabled("cmd_compactFolder")); // checks e.g. if IMAP is offline

>--- a/mail/base/content/messenger.xul
>+++ b/mail/base/content/messenger.xul

>-<script type="application/x-javascript" src="chrome://messenger/content/messengerdnd.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/accountUtils.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/searchBar.js"/>
>+<script type="application/x-javascript;version=1.8" src="chrome://messenger/content/folderPane.js"/>

This version=1.8 shouldn't be needed as we are chrome code. Though I think I did see several bugs on replacing application/x-javascript with application/javascript (but that doesn't actually change anything afaict).

>-      <tree id="folderTree"
>-            class="plain focusring"
>-            treelines="true"
>-            flex="1"
>+      <tree id="folderTree" class="plain focusring" flex="1"
>+            hidecolumnpicker="true" persist="mode" mode="all"
>             context="folderPaneContext"
>             disableKeyNavigation="true"
>-            datasources="rdf:null"
>-            statedatasource="rdf:mailnewsfolders"
>-            flags="dont-build-content"
>-            ondraggesture="BeginDragFolderTree(event);"
>+            ondraggesture="gFolderTreeView._onDragStart(event);"
>+            ondragover="gFolderTreeView._onDragOver(event);"
>+            ondblclick="gFolderTreeView.onDoubleClick(event);"
>+            onmousedown="TreeOnMouseDown(event);"
>             onselect="FolderPaneSelectionChange();">

What happened to treelines? Is that intentional?

>           <treecol id="folderNameCol"
>                    flex="5"
>                    crop="center"
>                    persist="width"
>                    hideheader="true"
>                    ignoreincolumnpicker="true"
>                    primary="true"
>-                   sort="?folderTreeNameSort"
>                    sortActive="true"
>                    sortDirection="ascending"/>
>-          <splitter class="tree-splitter"/>
>-          <treecol id="folderUnreadCol"
...
>-          <splitter class="tree-splitter"/>
>-          <treecol id="folderTotalCol"
...
>-          <splitter class="tree-splitter"/>
>-          <treecol id="folderSizeCol"

So we are changing the UI capabilities with this patch? (rather than a like-for-like replacement that I was expecting).

If we are, then folderSizeColumn.label can be removed from the dtd files that it is in (its in two).


>diff --git a/mail/installer/windows/packages-static b/mail/installer/windows/packages-static
>--- a/mail/installer/windows/packages-static
>+++ b/mail/installer/windows/packages-static
>@@ -96,16 +96,17 @@ bin\extensions\{972ce4c6-7e08-4474-a285-
> ; Mail Extensions (smime, etc.)
> ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> bin\MapiProxy.dll
> bin\mozMapi32.dll
> bin\components\mapihook.xpt
> bin\components\nsSetDefaultMail.js
> bin\components\offlineStartup.js
> bin\components\nsMailDefaultHandler.js
>+bin\components\folderLookupService.js

This isn't part of this patch.

Comment 74

[David :Bienvenu]

Let me try to get a patch that applies cleanly to the current trunk.

Re changing the folder pane, yes, the current patch removes the extra columns (total messages and folder size). I believe the thinking was that once we start putting things that aren't folders in the folder pane, those columns make less sense, and that some mechanism other than columns would be better. It will be interesting to get feedback on this, though.

Comment 75

[Kent James (:rkent)]

(In reply to comment #74)
>
> Re changing the folder pane, yes, the current patch removes the extra columns
> (total messages and folder size). I believe the thinking was that once we start
> putting things that aren't folders in the folder pane, those columns make less
> sense, and that some mechanism other than columns would be better. It will be
> interesting to get feedback on this, though.

I've commented on this before, but since you are asking for feedback I'll just reiterate that I don't like removing these columns. Both total and unread are important parts of what I look for in a folder. Unread is the immediate action, total is the backlog of unprocessed items. I was tickled when I first saw these available though buried deep in the UI to enable, and I will miss them if you remove them. I don't understand this philosophy of removing some feature, then hoping you will add it back later.

Perhaps if you gave some examples of these new items in the folder pane where "unread" and/or "total" are undefinable, I would be more sympathetic.

Comment 76

[David :Bienvenu]

Thx for the feedback - I'm really just the messenger - I sorely miss those columns myself. But you can imagine calendaring items in the folder pane for which size made no sense, and total not so much sense.

Comment 77

[David :Bienvenu]

Attachment: patch addressing comments; should apply

I've left the extra column strings in - jminta said he hoped to have time tomorrow to try to add them back, but I'd still like to drive this patch forward.

This should apply to a clean tree.

Comment 78

[David :Bienvenu]

I've fixed the treelines in my local tree. I'm looking at the drag drop highlighting of folder pane targets - it's very strange that leaf folders don't get highlighting, only parent folders, expanded or collapsed.

Comment 79

[David :Bienvenu]

it's a bit worse than I thought - drag drop no longer checks if we can drop messages onto the folder, e.g., newsgroups, readonly folders, or accounts shouldn't be drop targets.

Comment 80

[David :Bienvenu]

ftv_canDrop needs to do all the same kinds of things that CanDropOnFolderTree does. I've started coding this up...

Comment 81

[Mark Banner (:standard8)]

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

I believe this patch is obsolete now there is a newer folderPane.js in attachment 346275 [details] [diff] [review]

Comment 82

[Mark Banner (:standard8)]

Comment on attachment 344546 [details] [diff] [review]
patch addressing Neil's comments
[Checkin: Comment 82]

http://hg.mozilla.org/comm-central/rev/7d07094f7940
http://hg.mozilla.org/comm-central/rev/92046e4f9f91
http://hg.mozilla.org/comm-central/rev/031112deacdf

Comment 83

[Mark Banner (:standard8)]

(In reply to comment #77)
> Created an attachment (id=346275) [details]
> patch addressing comments; should apply
...
> This should apply to a clean tree.

Here's a few comments from general testing:

1) First time I start Thunderbird (with this patch) everything is collapsed, apart from the default account. Obviously we're going to loose expansion/collapse settings from 2.x (unless we add some sort of migration in), but could it be a slightly better idea to have the top-level of each account open?

2) Tabs don't work correctly:

a) Start Thunderbird
b) Select default account
c) Right Click, Open in new Tab 
d) Select Inbox (ok so far).
e) Select first tab - inbox still shown although account name is in tab title.

By selecting different folders I seem to get out of this cycle. The current nightlies don't have this problem.

3) I'm also seeing a problem with the message pane listing, although this may not be to do with this bug, with the two tabs in the non-above-problem state, going between inbox & sent folder the columns change on the message pane listing (from changing to recipient), however when I switch tabs, the columns stay the same.

I couldn't reproduce on my current nightly as the columns always seemed to stay the same and wouldn't restore to default.

4) "Error -> ReferenceError: GetServer is not defined" is output on the console every time I select a top level account.

I expect that as a result, we have incorrect listing of items on the top-level summary page (e.g. newsgroups for an imap account) and the links don't work.

5) All folders/accounts (regardless of their type) have a subscribe option in the context menu leading to RSS Subscribe dialog. Imap and newsgroup folders have an additional subscribe option leading to the expected dialog.

6) Right-click on a folder, select delete - it doesn't work, error on console:

JavaScript error: chrome://messenger/content/folderPane.js, line 1181: toXPCOMIterator is not defined

7) I think compact isn't working for imap accounts, but I'm not entirely sure that isn't a different bug.

8) With zero unread messages, receive a new mail with saved searches (on imap, or local folders, probably cross-folder ones as well) and they go blue and show the correct unread count. Select the message in the inbox, that folder name goes black and clears count, the saved searches clear the count but all stay blue.

9) I have two imap saved searches that are appearing with the patch and aren't there without the patch (just below inbox level).

10) If I select two Inbox folders as favourites then go to the Favourites folder view, they are both named "Inbox". With current trunk, they have "Inbox" and the account name.

11) Only one Inbox appears on the Recent Folders view - and it doesn't have the account name.

12) Sort orders across the folder views seem random:

a) Recent folders seem random (as opposed to, I think, special folder type and name)
b) Unread folders seems to do sort on account and then name, but one of my Imap saved searches (SavedSearch) appears before Inbox.
c) Favourite folders also seems to be a random order.

Comment 84

[Mark Banner (:standard8)]

(In reply to comment #83)
> 5) All folders/accounts (regardless of their type) have a subscribe option in
> the context menu leading to RSS Subscribe dialog. Imap and newsgroup folders
> have an additional subscribe option leading to the expected dialog.

This is apparently due to the recent RSS move, a clobber of mozilla/dist fixed it.

Comment 85

[David :Bienvenu]

Re 1, I think this will be less important as we go to the new folder pane design where we have "smart, special" folders at top, and the accounts down below, and I think we want those accounts collapsed by default.

I don't see 4 in my tree, though I've seen similar errors. I'll keep trying to recreate it.

Compact imap folders works for me - I look in the protocol log and we are issuing the expunge.

Deleting folders works fine in my tree, which means I must have screwed up the patch.

Comment 86

[David :Bienvenu]

Recent folders are sorted by MRU time, i.e., how recently you used them, when the view was created.  That's perhaps worth trying to see what people think.

Other folder views are in account, then child order, i.e., whatever order they happen to be in the subfolders array. I think we should try to do what the old folder pane did here.

The unification of folder names in the folder views was lost. I think I did that in RDF, so that explains how they got lost.

In my tree, I fixed a couple places where we were using ToXPCOMIterator instead of ToXPCOMArray.

Comment 87

[David :Bienvenu]

actually, the child folders are sorted alphabetically, so favorites and unread views are sorted by account, and then alphabetically with the account.

Comment 88

[Joey Minta]

(In reply to comment #83)
> 4) "Error -> ReferenceError: GetServer is not defined" is output on the console
> every time I select a top level account.

isCompactFolderEnabled needs to change to look like this: http://hg.mozilla.org/users/jminta_gmail.com/kill-rdf/file/4db07560cb26/mail/base/content/mailWindowOverlay.js#l2040

Comment 89

[David :Bienvenu]

Unless I'm missing something subtle, that's how it looks in the patch and in my tree...
(In reply to comment #88)
> (In reply to comment #83)
> > 4) "Error -> ReferenceError: GetServer is not defined" is output on the console
> > every time I select a top level account.
> 
> isCompactFolderEnabled needs to change to look like this:
> http://hg.mozilla.org/users/jminta_gmail.com/kill-rdf/file/4db07560cb26/mail/base/content/mailWindowOverlay.js#l2040

Comment 90

[Mark Banner (:standard8)]

Comment on attachment 346275 [details] [diff] [review]
patch addressing comments; should apply

>diff --git a/mail/base/content/folderPane.js b/mail/base/content/folderPane.js

+/**
+ * This file contains the controls and functions for the folder pane
+ */

I think we could do with some terminology definitions here. In particular: view, pane, mode, ftv


>+let gFolderTreeView = {
>+  /**
>+   * Called when the window is initially loaded.  This function initializes the
>+   * folder-pane to the view last shown before the application was closed.
>+   */
>+  load: function ftv_load(aTree, aJSONFile) {
>+    let Cc = Components.classes;
>+    let Ci = Components.interfaces;

nit: please use const (multiple places)

>+    try {
>+      // Normally our tree takes care of keeping the last selected by itself.
>+      // However older versions of TB stored this in a preference, which we need
>+      // to migrate
>+      let prefB = Cc["@mozilla.org/preferences-service;1"].
>+                  getService(Ci.nsIPrefBranch);

dots on the start of the following line please (multiple places).

>+    if (document.getElementById('folderpane-title')) {
>+      let key = "folderPaneHeader_" + this.mode;
>+      let string = document.getElementById("bundle_messenger").getString(key);
>+      document.getElementById('folderpane-title').value = string;
>+    }

Why would folderpane-title not exist?

>+  /**
>+   * This is an array of all possible modes for the folder tree.  Extensions are
>+   * free to add to this pane, but you must make a corresponding change to the
>+   * _mapGenerators object.

I think replace "change" by "addition"

>+  selectFolder: function ftv_selectFolder(aFolder) {
>+    let tree = this;

I don't see the point of this let, especially when you revert to this later on in the function.

>+    function openIfNot(aFolderToOpen) {
>+      let index = tree.getIndexOfFolder(aFolderToOpen);
>+      if (index) {
>+        if (!tree._rowMap[index].open)
>+          tree._toggleRow(index, false);
>+        return;
>+      }
>+
>+      // not found, so open the parent
>+      openIfNot(aFolderToOpen.parent);
>+
>+      // now our parent is open, so we can open ourselves
>+      tree._toggleRow(tree.getIndexOfFolder(aFolderToOpen), false);
>+    }
>+    if (aFolder.parent)
>+      openIfNot(aFolder.parent);

Why is this function internally defined? IMHO this is more complex than having a simple if statement.

>+  canDrop: function ftv_canDrop(aIndex, aOrientation) {
>+  drop: function ftv_drop(aRow, aOrientation) {
>+  _onDragStart: function ftv_dragStart(aEvent) {
>+  _onDragOver: function ftv_onDragOver(aEvent) {

skipped due to known drag and drop issues (comment 80).

>+  /**
>+   * If we have a child-list with at least one element, we are a containe.  This

nit: container.

>+  /**
>+   * Opens or closes a folder with children.  The logic here is a bit hairy, so
>+   * be very careful about changing anything.
>+   */
>+  toggleOpenState: function ftv_toggleOpenState(aIndex) {
>+    this._toggleRow(aIndex, true);
>+    },

nit: incorrect indentation of closing bracket.

>+  _toggleRow: function toggleRow(aIndex, aExpandServer)
>+  {
>+    // Ok, this is a bit tricky.
>+    this._rowMap[aIndex].open = !this._rowMap[aIndex].open;
>+    if (!this._rowMap[aIndex].open) {
>+      // We're closing the current container.  Remove the children
>+
>+      // Note that we can't simply splice out children.length, because some of
>+      // them might have children too.  Find out how many items we're actually
>+      // going to splice
>+      let count = 0;
>+      let i = 2;
>+      let row = this._rowMap[aIndex + 1];
>+      while (row && row.level > this._rowMap[aIndex].level) {
>+        count++;
>+        row = this._rowMap[aIndex + i++];
>+      }
>+      this._rowMap.splice(aIndex + 1, count);

As i doesn't appear to be used elsewhere in this function, I think it would be much simpler to let i = aIndex + 1, and then replace the [aIndex + 1] or [aIndex + i++] as appropriate.

>+    let curLevel = 0;
>+    let tree = this;
>+    function openLevel() {
...
>+      if (goOn)
>+          openLevel();

nit : incorrect indentation.

>+    all: function ftv__mg_all() {

>+      // Bug 41133 workaround
>+      accounts = accounts.filter(function fix(a) { return a.incomingServer; });
>+
>+      // Don't show deferred pop accounts
>+      accounts = accounts.filter(function isNotDeferred(a) {
>+        let server = a.incomingServer;
>+        return !(server instanceof Ci.nsIPop3IncomingServer &&
>+                 server.deferredToAccount);
>+      });

This feels like we need a follow-up bug to deferredToAccount it more generic across account types and hence easier for extensions to account types to re-use global inbox if they wish.

>+  /**
>+   * This is our implementation of nsIMsgFolderListener to watch for changes
>+   */
>+  OnItemAdded: function ftl_add(aParentItem, aItem) {
>+    //only rebuild if we didn't know about the folder

nit: missing space after //

>+  // Believe it or not, this is the simplest way to watch for account creation
>+  observe: function ftv_observe(aSubject, aTopic, aPrefName) {
>+    if (aPrefName != "mail.accountmanager.accounts")
>+      return;
>+    let view = this;
>+    function wrapper() {
>+      view._rebuild();
>+    }
>+    setTimeout(wrapper, 0);

Why are we doing this with a timeout?

>+  sortChildren: function () {
>+      function sorter(a, b) {
>+        let sortKey = a._folder.compareSortKeys(b._folder);
>+        if (sortKey)
>+          return sortKey;
>+        return a.text.toLowerCase() > b.text.toLowerCase();
>+      }
>+      this._children.sort(sorter);
>+  },

nit: incorrect indentation.

>+    //xxx useless param

Let's file a follow-up bug for these useless params.

>+      var msgDB = folder.getMsgDatabase(msgWindow);
>+      msgDB.summaryValid = false;
>+      try {
>+            folder.closeAndBackupFolderDB("");

nit: incorrect indentation

>+  deleteFolder: function ftc_delete(aFolder) {
>+    let Ci = Components.interfaces;
>+    let folder = aFolder || gFolderTreeView.getSelectedFolders()[0];
>+
>+    const FLAGS = Ci.nsMsgFolderFlags;
>+    if (folder.flags & FLAGS.Inbox || folder.flags & FLAGS.Trash)
>+      return;
>+
>+     let prefix = "@mozilla.org/messenger/protocol/info;1?type=";
>+     let info = Components.classes[prefix + folder.server.type]
>+                          .getService(Ci.nsIMsgProtocolInfo);

nit: incorrect indentation

>+    if (showPrompt) {
>+      let checkbox = {value:false};
>+      let promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"].
>+                          getService(Ci.nsIPromptService);
>+      let bundle = document.getElementById("bundle_messenger");
>+      let ok = promptService.confirmEx(window,
>+                                       bundle.getString(aCommand + "Title"),
>+                                       bundle.getString(aCommand + "Message"),
>+                                       promptService.STD_YES_NO_BUTTONS,
>+                                       null, null, null,
>+                                       bundle.getString(aCommand + "DontAsk"),
>+                                       checkbox) == 0;
>+     if (checkbox.value)
>+       pref.setBoolPref("mail." + aCommand + ".dontAskAgain", true);
>+     if (!ok)
>+       return false;
>+    }

nit: indentation wrong (of the ifs)

Comment 91

[Joey Minta]

(In reply to comment #90)
> Why would folderpane-title not exist?
We recycle folderPane.js for other folder-trees (such as virtual-folder-picker), where those elements won't exist.

> 
> >+    function openIfNot(aFolderToOpen) {
> >+      let index = tree.getIndexOfFolder(aFolderToOpen);
> >+      if (index) {
> >+        if (!tree._rowMap[index].open)
> >+          tree._toggleRow(index, false);
> >+        return;
> >+      }
> >+
> >+      // not found, so open the parent
> >+      openIfNot(aFolderToOpen.parent);
> >+
> >+      // now our parent is open, so we can open ourselves
> >+      tree._toggleRow(tree.getIndexOfFolder(aFolderToOpen), false);
> >+    }
> >+    if (aFolder.parent)
> >+      openIfNot(aFolder.parent);
> 
> Why is this function internally defined? IMHO this is more complex than having
> a simple if statement.
Notice that it's called recursively from inside itself, so it's not just an if() check.

> >+  // Believe it or not, this is the simplest way to watch for account creation
> >+  observe: function ftv_observe(aSubject, aTopic, aPrefName) {
> >+    if (aPrefName != "mail.accountmanager.accounts")
> >+      return;
> >+    let view = this;
> >+    function wrapper() {
> >+      view._rebuild();
> >+    }
> >+    setTimeout(wrapper, 0);
> 
> Why are we doing this with a timeout?
Because the account isn't completely set up when the pref changes.

Comment 92

[David :Bienvenu]

I'll put up a patch that addresses Standard8's comments, other than the ones jminta just addressed. I've got drag drop working quite a bit better now as well, as well, and fixed sorting of favorite and folders with unread views (I didn't put the sort code in folderUtils.jsm because we're sorting ftvItems, not folders...but I'm open to suggestions)

Comment 93

[David :Bienvenu]

Attachment: address review comments and fix a host of issues

This addresses a bunch of the review comments, and fixes a bunch of issues, including:

recent folders is now most recently used folders, not least recently used folders
we sort favorites and folders with unread by special flags, then name
we append the account name to all folders in the special folder views (not optimal, but good enough for now)
drag drop of messages now works to all folders

Comment 94

[Joey Minta]

Attachment: with multiple colums

This is the patch bienvenu attached, but with support for multiple columns, which I still say are silly...

Comment 95

[David Ascher (:davida)]

Multiple columns won't make sense for heterogeneous contents (non-folder things), as the value of the column won't apply across item types.

Wouldn't the approach in bug 441414 be a better way for us to show more for folders w/o having to use columns?

Comment 96

[Mark Banner (:standard8)]

(In reply to comment #95)
> Multiple columns won't make sense for heterogeneous contents (non-folder
> things), as the value of the column won't apply across item types.
>
> Wouldn't the approach in bug 441414 be a better way for us to show more for
> folders w/o having to use columns?

My comment about columns was originally aimed at confirming what we were doing. I would be quite happy for this to go in without columns as long as we then filed blocking follow-up bugs to restore some level of capability.

For beta 1 I don't know if we'd block on restoring that capability, its a hidden pref and its the same as TB 2. There are various hits for "mail.showFolderPaneColumns" on google (including mozillazine), so doing a preview release without equivalent functionality may cause complaints.

There is one other option I have thought of, land two separate changesets for beta 1: a changeset with the base work for this bug, plus a changeset with just the column additions in. We can then do whatever work we want, and post beta 1 we can decide what functionality we really want and then back the second changeset and replace it with the new functionality.

Comment 97

[Mark Banner (:standard8)]

Comment on attachment 346823 [details] [diff] [review]
with multiple colums

Just from a quick test:

a) top-level accounts have unread count of -1, total of -1 and size of 0.
b) sizes aren't in MB/KB
c) Column selection isn't persisted.

Given the current discussion re columns, and I'm assuming these are the only changes over David's patch, then I'm going to revert to testing David's patch for the time being.

If we do decide to re-implement columns, then we should keep these as a separate patch to make the review easier.

Comment 98

[Mark Banner (:standard8)]

(In reply to comment #93)
> Created an attachment (id=346780) [details]
> address review comments and fix a host of issues
> 
> This addresses a bunch of the review comments, and fixes a bunch of issues,
> including:

Something Neil pointed out to me on irc, the folder picker widget on the
toolbar is now broken, the update code for it has been removed.

Comment 99

[[e-mail]]

Comment on attachment 346823 [details] [diff] [review]
with multiple colums

>-    var folderTree = GetFolderTree();
>-    var folderSelection = folderTree.view.selection;
>+    var folderSelection = document.getElementById("folderTree").view.selection;
Why not gFolderTreeView.selection?

>+    if (document.getElementById("folderpane_splitter"))
>+      document.getElementById("folderpane_splitter").collapsed = false;
>+    if (document.getElementById("folderPaneBox"))
>+      document.getElementById("folderPaneBox").collapsed = false;
This makes no sense.

>+        let str = sstream.read(4096);
available()?

>+      
"blank" line ;-)

>+  onDoubleClick: function ftv_onDoubleClick(aEvent) {
>+    if (pref.getBoolPref("mailnews.reuse_thread_window2") ||
>+        aEvent.button != 0 || aEvent.originalTarget.localName == "twisty" ||
>+        aEvent.originalTarget.localName == "slider" ||
>+        aEvent.originalTarget.localName == "scrollbarbutton")
>+      return;
>+
>+    let folder = gFolderTreeView.getFolderAtCoords(aEvent.clientX,
>+                                                   aEvent.clientY);
Except you're not adding it as an event listener, so "this" works.

>+        aEvent.button != 0 || aEvent.originalTarget.localName == "twisty" ||
>+        aEvent.originalTarget.localName == "slider" ||
>+        aEvent.originalTarget.localName == "scrollbarbutton")
Check for treechildren instead perhaps?

>+    let folder = gFolderTreeView.getFolderAtCoords(aEvent.clientX,
>+                                                   aEvent.clientY);
>+    folder.command();
What if there is no folder at those coordinates?

>+  _mode: null,
>+  get mode() {
>+    if (!this._mode)
>+      this._mode = this._treeElement.getAttribute("mode");
Seems hardly worth shadowing the attribute.

>+  /**
>+   * If the next item in our list has the same level as us, it's a sibling
>+   */
>+  hasNextSibling: function ftv_hasNextSibling(aIndex, aNextIndex) {
>+    return this._rowMap[aIndex].level == this._rowMap[aNextIndex].level;
>+  },
I'm not sure that's true.

>+  /**
>+   * If we have a child-list with at least one element, we are a container.  This
>+   * means that we don't display empty containers as containers.
>+   */
This comment doesn't reflect reality.

>+  isContainerEmpty: function ftv_isContainerEmpty(aIndex) {
>+    // We don't count you as a container if you don't have children
Neither does this. It's quite normal for containers to be empty.

>+  /**
>+   * This is a javaascript map of which folders we had open, so that we can
>+   * persist their state over-time.  It is designed to be used as a JSON object.
>+   */
>+  _persistOpenMap: {},
We actually have a folder flag to persist this, so presumably on first switch
everyone's folders will collapse and they'll have to open them again?

>+      // Bug 41133 workaround
Oh, I see now, this is copied from msgViewNavigation.js

>+      function sortAccounts(a, b) {
This is also copied ;-)
We already have a function that compares folders. Perhaps we need one for
accounts.

>+      // force each root folder to do its local subfolder discovery.
>+      for each (acct in accounts)
>+        acct.incomingServer.rootFolder.subFolders;
Any reason we do it now and not in isContainerEmpty?

>+        if (folder.flags & 0x80000000) // Come on, why isn't this in an IDL?
It is. Look harder.

>+  // Believe it or not, this is the simplest way to watch for account creation
No, I still don't believe it ;-)

>+        // Bah, silly advanced option makes this more difficult
>+        if (gFolderTreeView._showingCols)
>+          return text;
Not true, you could only be showing the "Size" column, in which case you do
want to include the num unread in the name.

>+  get open() {
>+    return this._open;
>+  },
>+
>+  set open(aOpen) {
>+    return this._open = aOpen;
>+  },
This works the same as open: false; except that it's slower.

>+  set useServerName(aUseServerName) {
>+    return this._useServerName = aUseServerName;
>+  },
Write-only property?

>+/**
>+ * This handles the invocation of most commmands dealing with folders, based off
>+ * of the current selection, or a passed in folder.
>+ */
Was it necessary to include this rewrite?

>+    //xxx useless param
>+    function newFolderCallback(aName, aFolder) {
The New Folder dialog allows you to change the parent folder.

>+    // Can't compact imap folders
Is this a different Compact Folder to the one I use on imap folders?

>+function sortFolderItems (aFtvItems) {
>+  function sorter(a, b) {
>+    let sortKey = a._folder.compareSortKeys(b._folder);
>+    if (sortKey)
>+      return sortKey;
>+    return a.getText("folderNameCol").toLowerCase() > b.getText("folderNameCol").toLowerCase();
Strangely enough compareSortKeys already accounts for the name (although you're
actually sorting on the name here, you're sorting on the text). The one time
you should have copied code from msgViewNavigation.js and didn't...

> // Controller object for folder pane
> var FolderPaneController =
Hmm, we now have two controllers? That's going to get confusing!

Comment 100

[David Ascher (:davida)]

On columns: The two use cases that I've been able to identity for columns is "managing inbox-zero style", where unread items are important because they represent pending tasks; and "size management", looking for folders that are too large.  

For the former, I think that we could have a spinoff bug that offers to display (unread/total) instead of (unread) as the count in the existing column.  That's just an example of a UI we could use to allow the same task w/o columns.

For the latter, we can do something that is a much more accurate view of how big folders are in e.g. the server pane, showing big folders using disk space measurements.  

If that sounds reasonable I'll file spinoff bugs.

Comment 101

[Wayne Mery (:wsmwk)]

I was thinking ~ the same solution.  

I use those counts and I wouldn't want to lose them.  And, although it's an edge case and only one example, total message count is important for those of us who live with the possibility of losing mail for various reasons - it is a poor method but one can judge whether significant numbers of messages have gone missing.

Comment 102

[David :Bienvenu]

yes, we're dropping the folder picker widget for now, until we either come up with a better replacement, or redo it.

>We actually have a folder flag to persist this, so presumably on first switch
>everyone's folders will collapse and they'll have to open them again?

Right - I guess we could read the folder flag - I'd forgotten that we actually persist that.

> Why not gFolderTreeView.selection?

I might have screwed up the merge with the whitespace cleanup.

I'll fix the container comments; I fixed the code, but not the comments.

Comment 103

[David :Bienvenu]

I'm still struggling trying to get the new folder pane not to select a folder on right click, which would be handy for opening folders in tabs. I've tried doing an onclick handler, to no avail.

Comment 104

[David Ascher (:davida)]

David: you probably want an onmousedown even that does an event.stopPropagation or event.preventDefault, depending on how the selection is happening.

Comment 105

[David :Bienvenu]

yes, we have that, but it seems to get called after the selection has already happened in the tree. Maybe we've attached the handler to the wrong place - in the old folder pane, that handler is attached by adding an observer to the folder tree, instead of directly defined in the xul.

Comment 106

[David :Bienvenu]

adding observers to the folder tree in the js onload handler seems to work fine. I don't know why adding them to the xul didn't work.

Comment 107

[David :Bienvenu]

Attachment: fixes right click, double click, and a few other issues

This fixes the right click context menu on the folder pane, so we can open tabs without switching the current folder. It also fixes double click, which was broken (confusion between ftv item, and folders, i.e., nsIMsgFolders). It also fixes a problem with account central that was causing the GetServer error. And reduces the number of expected exceptions we throw so that I can make Venkman stop on exceptions w/o pulling the rest of my hair out.

There are still problems switching to tabs open on an account, but the exact same problem happens on the trunk and has to do with a strange problem in this code:

    var startpage = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
                              .getService(Components.interfaces.nsIURLFormatter)
                              .formatURLPref(startPageUrlPref());

the url formatter service claims not to support the nsIURLFormatter interface. I'm investigating that.

Comment 108

[David :Bienvenu]

I also fixed compact of imap folders...

Comment 109

[David :Bienvenu]

Attachment: fix some more of Neil's comments

this fixes a few more of Neil's comments, in particular the _open and _useServerName var definitions, using the Favorite flag from idl, and also removes deleted folders from the persistMap (this is less than perfect since it only removes them from the current "mode", but normally only the full mode will have "open" folders, and users will normally only delete folders from the full mode. And it's pretty harmless to have non-existent folders in the persistMap)

Comment 110

[Mark Banner (:standard8)]

Comment on attachment 347723 [details] [diff] [review]
fix some more of Neil's comments

+function ftvItem(aFolder) {
+  this._folder = aFolder;
+  this._level = 0;
+  this._useServerName = false;
+}

You can get rid of the _useServerName assignment now.

+ftvItem.prototype = {
+  _open: false,
+  _useServerName: false,

These shouldn't have the _ before the names.

+  get text() {
+    let text = this._folder.abbreviatedName;
+    if (this._useServerName)

This also needs changing not to have _

With the above fixed, Unread/Favourites/Recent panes:

- right click and select open. New window opens up, no folder selected, error in console:

Error: gMsgFolderSelected is undefined
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 212

- Similar problem with Open in new tab, different effect:

Error: aFolder is null
Source File: chrome://messenger/content/folderPane.js
Line: 277

- Renaming a folder causes the folder selection to revert to the top of the list, and the folder doesn't get renamed in the list

Comment 111

[Mark Banner (:standard8)]

Comment on attachment 347723 [details] [diff] [review]
fix some more of Neil's comments


>+    if (document.getElementById("folderpane_splitter"))
>+      document.getElementById("folderpane_splitter").collapsed = false;
>+    if (document.getElementById("folderPaneBox"))
>+      document.getElementById("folderPaneBox").collapsed = false;

We should add a comment saying that the folder pane can be used for other trees in other places, and hence these may not exists.

>+        let str = sstream.read(4096);
>+        while (str.length > 0) {
>+          data += str;
>+          str = sstream.read(4096);
>+        }

So now I look at this again, using available as Neil suggested would make this more concise:

while (sstream.available()) {
  data += sstream.read(4096);
}

>+    // Listen for account creation/deletion
>+    let ps = Cc["@mozilla.org/preferences-service;1"]
>+                .getService(Ci.nsIPrefService);
>+    let branch = ps.getBranch("").QueryInterface(Ci.nsIPrefBranch2);
>+    branch.addObserver("mail.accountmanager.", this, false);

we should just change this to:

Cc[...]
  .getService(Ci.nsIPrefBranch2)
  .addObserver(...)

>+
>+    let ps = Cc["@mozilla.org/preferences-service;1"]
>+                .getService(Ci.nsIPrefService);
>+    let branch = ps.getBranch("").QueryInterface(Ci.nsIPrefBranch2);
>+    branch.removeObserver("mail.accountmanager.", this, false);

similar here

>+  drop: function ftv_drop(aRow, aOrientation) {
>+    const Cc = Components.classes;
>+    const Ci = Components.interfaces;
>+    let targetFolder = gFolderTreeView._rowMap[aRow]._folder;
>+
>+    let dt = this._currentTransfer;
>+    let count = dt.mozItemCount;
>+    let cs = Cc["@mozilla.org/messenger/messagecopyservice;1"]
>+                .getService(Ci.nsIMsgCopyService);
>+             

nit: unnecessary whitespace on the "blank" line.

>+    else if (Array.indexOf(types, "text/x-moz-message") != -1) {
>+      let array = Cc["@mozilla.org/array;1"]
>+                    .createInstance(Components.interfaces.nsIMutableArray);
>+      let sourceFolder;       

nit: whitespace on end of line

>+  /**
>+   * Opens or closes a folder with children.  The logic here is a bit hairy, so
>+   * be very careful about changing anything.
>+   */
>+  toggleOpenState: function ftv_toggleOpenState(aIndex) {
>+    this._toggleRow(aIndex, true);
>+  },
>+    

nit: whitespace on blank line

Comment 112

[Mark Banner (:standard8)]

(In reply to comment #99)
> >+  /**
> >+   * This is a javaascript map of which folders we had open, so that we can
> >+   * persist their state over-time.  It is designed to be used as a JSON object.
> >+   */
> >+  _persistOpenMap: {},
> We actually have a folder flag to persist this, so presumably on first switch
> everyone's folders will collapse and they'll have to open them again?

David and I have discussed this, and we've decided to leave it as it is. The reason is that the folder pane will move over time to being not just for folders, so we don't want to force new implementations to have to save their own flags. Additionally with the way we are heading with the folder pane it currently having all the accounts collapsed on start up will make sense for Thunderbird 3.

Comment 113

[David :Bienvenu]

Attachment: address a few more issues [Checkin: See comment 116]

Comment 114

[Mark Banner (:standard8)]

Comment on attachment 347858 [details] [diff] [review]
address a few more issues
[Checkin: See comment 116]

r=me without the calendar/providers/gdata/install.rdf
 and mailnews/mailnews.js
 changes.

Comment 115

[Joey Minta]

Comment on attachment 347858 [details] [diff] [review]
address a few more issues
[Checkin: See comment 116]

+    // Listen for account creation/deletion
+    let ps = Cc["@mozilla.org/preferences-service;1"]
+                .getService(Ci.nsIPrefBranch2)
+                .addObserver("mail.accountmanager.", this, false);
If you're doing this all on one line, there's no need to assign it to a variable.

Comment 116

[David :Bienvenu]

fix checked in - changeset:   1089:1de58e1d7549 - thx for all the work, jminta, and thx to Neil and Standard8 for the reviews.

Comment 117

[Joey Minta]

Attachment: cleanup [Checkin: Comment 119]

This cleans up a few extra functions that can now be removed (they moved into the new gFolderTreeController object). sr needed for the msgAccountCentral change.

Comment 118

[David :Bienvenu]

Comment on attachment 347957 [details] [diff] [review]
cleanup
[Checkin: Comment 119]

thx, jminta

Comment 119

[Joey Minta]

Comment on attachment 347957 [details] [diff] [review]
cleanup
[Checkin: Comment 119]

Landed as rev 1097:7e582f51f799

Comment 120

[Przemyslaw Bialik]

looks like this patch broke std msg window.
Error: AddDataSources is not defined
Source File: chrome://messenger/content/messageWindow.js
Line: 227
removing this call should be ok ?

Comment 121

[David :Bienvenu]

thx, it should be - I'll try it and land it if is ok

Comment 122

[David :Bienvenu]

Attachment: patch as suggested [Checkin: Comment 123]

I'll land this; thx for diagnosing it.

Comment 123

[David :Bienvenu]

Comment on attachment 347971 [details] [diff] [review]
patch as suggested
[Checkin: Comment 123]

changeset:   1098:c63398e7c21c

Comment 124

[Richard Marti (:Paenglab)]

It looks like the landing of this bug has broken the cross folder next unread message read.

Comment 125

[Joey Minta]

(In reply to comment #124)
> It looks like the landing of this bug has broken the cross folder next unread
> message read.
Can you please file this as a new bug, and include any errors you see in the Error Console? Thanks!

Comment 126

[Richard Marti (:Paenglab)]

Already filed by Cory Albrecht: bug 464714

Comment 127

[Joey Minta]

Attachment: basic tests

Basic mozmill tests for the folder pane and related folder-functions.

Comment 128

[Joey Minta]

Comment on attachment 348304 [details] [diff] [review]
basic tests

Just for fun, I managed to get the debugger to give me a (mostly accurate) list of all the lines these tests hit: http://tinyurl.com/62sdz8

Comment 129

[Mark Banner (:standard8)]

Comment on attachment 348304 [details] [diff] [review]
basic tests

This currently fails somewhere around doing things with local folders in account manager (on Mac at least). On the console I see:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Error: javascript assert was not succesful' when calling method: [nsITimerCallback::notify]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "<unknown>"  data: no]
************************************************************
WARNING: No valid default account found, just using first (FIXME): file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgAccountManager.cpp, line 776

Please can we also locate the tests in mail/tests/mozmill for the time being.

Comment 130

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #83)
> 1) First time I start Thunderbird (with this patch) everything is collapsed,
> apart from the default account. Obviously we're going to loose
> expansion/collapse settings from 2.x (unless we add some sort of migration in),
> but could it be a slightly better idea to have the top-level of each account
> open?

I still can see this. The first account is expanded but only the first level. Subfolders are collapsed too. It's bad to have to expand the whole hierarchy step by step again in manual mode. Is there any way we can avoid that or do any user has to do those steps?

Comment 131

[Chris]

I've looked everywhere and can't find someone who has the same problem as myself.  I hope you can help...

Error: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 [nslMsgFolder.compareSortKeys]
Source File: chrome://messenger/content/folderPane.js  Line 2426

Symptoms: My folder pane is completely empty and my address book doesn't show anything either.  But, I do see the data there in the folders.  I'm using MAC OS vs. 10.6.8 and tried uninstalling the most current version of TB and even went back to several versions older, but still having the same symptoms.