536042 - Provide a way for folder tree modes to express parent relationships and tell us which folder a message belongs to

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Rain [:rain1] (inactive)]

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?

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

(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.)

Comment 2

[Rain [:rain1] (inactive)]

(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.

Comment 3

[Rain [:rain1] (inactive)]

Attachment: Proposed API

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

Comment 4

[Rain [:rain1] (inactive)]

Attachment: all modes except smart refactored

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

Comment 5

[Rain [:rain1] (inactive)]

Attachment: [WIP] smart mode refactored, tests still due

Comment 6

[Rain [:rain1] (inactive)]

Attachment: patch, v1

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 7

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 9

[Rain [:rain1] (inactive)]

(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.

Comment 10

[David :Bienvenu]

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 11

[David :Bienvenu]

Comment on attachment 419181 [details] [diff] [review]
patch, v1

awesome!, modulo the aforementioned sleep() call.

Comment 12

[Rain [:rain1] (inactive)]

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.

Comment 13

[Rain [:rain1] (inactive)]

Attachment: patch for 3.0, v1

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 :)

Comment 14

[David :Bienvenu]

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

Comment 15

[David :Bienvenu]

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 16

[David :Bienvenu]

Comment on attachment 420124 [details] [diff] [review]
patch for 3.0, v1

minusing based on test failure (windows 7, 64 bit, debug build)

Comment 17

[Rain [:rain1] (inactive)]

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

Comment 18

[Rain [:rain1] (inactive)]

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

Comment 19

[David :Bienvenu]

Comment on attachment 420124 [details] [diff] [review]
patch for 3.0, v1

ah, ok, that test passes now.

Comment 20

[Rain [:rain1] (inactive)]

Comment on attachment 420124 [details] [diff] [review]
patch for 3.0, v1

This fixes bug 527731, and has tests for everything.

Comment 21

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/releases/comm-1.9.1/rev/70074e65dfdd

Comment 23

[jenzed]

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)