This is going to be needed to fix bug 524821 -- right now we assume that the folder.parent is also its parent in the view. For smart folders this is definitely not true. Currently, the registerMode API accepts a callback <http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#165> that generates the view. I'm thinking of changing the API to accept an object implementing an interface (IFolderTreeMode?), maybe with a back-compat fallback. asuth or dmose, could you make a call on whether the fallback so that Tb3.0 extensions keep working is necessary?

(In reply to comment #0) > I'm thinking of changing the API to accept an object implementing an interface > (IFolderTreeMode?), maybe with a back-compat fallback. asuth or dmose, could > you make a call on whether the fallback so that Tb3.0 extensions keep working > is necessary? It sounds like the assumption fundamentally limits the existing API to providing filtered subsets of the full folder pane. This does not sounds particularly useful to extensions and I don't see any add-ons that look like they would be using the hook. (Lots of extensions seem to clobber the global sortFolderItems though.) As such, the existing API does not sound worth maintaining. So, assuming this fix is targeted at 3.1 and not 3.0.x, then I think it is fine to lose the fallback. However, I would change the registration function name to avoid confusion; registerFolderTreeMode or registerModeProvider sound like reasonable options but anything different is fine by me. If we want this on 3.0.x I think we need fallback support. I would still do the different function name and have the old function adapt the old interface to the new. Also, I don't think you need to define an XPCOM interface proper (not sure if you were proposing that). I think it would be desirable to document what the interface is, and if you think the easiest way to do that is to create a JS object to serve as a place to hang your doxygen/jsdoc, that's fine. (Or if having the interface be something you can subclass to get reasonable default behaviour for some methods, that's also a good reason to do that.)

(In reply to comment #1) > > Also, I don't think you need to define an XPCOM interface proper (not sure if > you were proposing that). I think it would be desirable to document what the > interface is, and if you think the easiest way to do that is to create a JS > object to serve as a place to hang your doxygen/jsdoc, that's fine. Right, that was what I was thinking -- this would be similar to dbViewWrapper.js's IDBViewWrapperListener.

This is what I'm looking at right now. This should cover the cases both in bug 524821/bug 527731 and in bug 520330.

Smart folders are currently implemented separately, but I plan to bring them together with the other builtin modes.

This is on top of the one in bug 536543. bienvenu, could you review this, and asuth, could you sign off on the tests? I haven't implemented getFolderForMsgHdr yet -- I'll do it, along with tests, in bug 520330. I'd like to keep that separate because this doesn't require ui-review and that does.

Comment on attachment 419181 [details] [diff] [review] patch, v1 I don't like the 10000ms sleep in test_display_message_in_smart_folder_mode_works, remove that. As noted on the other bug, I am not in a great way from a mozmill perspective right now. However, inspection of the tests looks good, and I am going to assume that bienvenu runs the tests and they pass as part of his review. If you have any spare cycles, I would love a patch (in a new bug) that has folder-display-helpers implements mozmill teardown helpers so that at the conclusion of every test file we have exactly one 3-pane window open with exactly one tab open. That would make it less dangerous to add new mozmill tests to the repo without fear that their breakage would corrupt the results of all the other tests.

By the way, thank you for the tremendous level of commenting/function documentation in that patch.

(In reply to comment #7) > (From update of attachment 419181 [details] [diff] [review]) > I don't like the 10000ms sleep in > test_display_message_in_smart_folder_mode_works, remove that. > Oops, I think I was attaching the debugger there. Removed.

I will run the mozmill tests if I'm the r?, but my mozmill isn't working so well at the moment - I should be able to get it working tomorrow and run the tests.

Comment on attachment 419181 [details] [diff] [review] patch, v1 awesome!, modulo the aforementioned sleep() call.

http://hg.mozilla.org/comm-central/rev/20c2d9e8e9b4 This needs a dev doc for the new interface and about how 3.1 will break compatibility for extension developers. Actually shifting to the new interface from the 3.0 callback is simple -- the callback maps to generateMap directly, and the other functions should be easy enough to define.

This is basically a reduced version of the trunk patch, and contains all the new, non-refactored code there -- the tests are the same, but the solution is not nearly as elegant :)

I ran the folder-display tests, and had one failure with this patch applied. I'll try again, and try without the patch applied, but here's the failure: TEST-PASS | test_display_message_with_folder_present_in_current_folder_mode TEST-UNEXPECTED-FAIL | test_display_message_in_smart_folder_mode_works EXCEPTION: Folder: mailbox://nobody@Local%20Folders/Inbox/DisplayMessageWithFo lderModesA should not be visible, but is at: test-folder-display-helpers.js line 804 Error("Folder: mailbox://nobody 0 assert_folder_not_visible([object XPCWrappedNative_NoHelper]) test-folder -display-helpers.js 804 test_display_message_in_smart_folder_mode_works() test-display-message-wi th-folder-modes.js 134 frame.js 452 frame.js 504 frame.js 546 frame.js 400 frame.js 551 server.js 164 server.js 168 TEST-PASS | test_switch_to_all_folders

everything passes when I backout the patch - just to be sure, I didn't touch the computer while running the tests, because sometimes that interferes with the tests.

Comment on attachment 420124 [details] [diff] [review] patch for 3.0, v1 minusing based on test failure (windows 7, 64 bit, debug build)

Do you have the patch in bug 536552 applied? I bet it's a toggleOpenState call that's failing there.

I've pushed bug 536552 to comm-1.9.1, so it shouldn't be a problem any more.

Comment on attachment 420124 [details] [diff] [review] patch for 3.0, v1 ah, ok, that test passes now.

Comment on attachment 420124 [details] [diff] [review] patch for 3.0, v1 This fixes bug 527731, and has tests for everything.

Broken compatibility with previous versions has been noted on the "Thunderbird 3.1 for developers" page (https://developer.mozilla.org/en/Thunderbird_3.1_for_developers)