501516 - right click folder -> search causes the search to be in the displayed folder, not the selected one

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Rain [:rain1] (inactive)]

We use gFolderDisplay.displayedFolder at <http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js?rev=520d3adf13f9#3381>, which doesn't work out for the context menu.

Comment 1

[Rain [:rain1] (inactive)]

Attachment: wip without tests

Comment 2

[Rain [:rain1] (inactive)]

Attachment: patch, v1

Per discussion with Bryan, this includes a fix for multiple search windows as well.

Comment 3

[Rain [:rain1] (inactive)]

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

asking r? bienvenu for the fix, asuth for tests, and clarkbw for the UX.

Bryan, please ui-r- if this doesn't work well.

Comment 4

[Rain [:rain1] (inactive)]

Try builds:

Linux: http://s3.mozillamessaging.com/build/try-server/2009-08-15_07:[e-mail]-501516-right-click-search/[e-mail]2

Mac: http://s3.mozillamessaging.com/build/try-server/2009-08-15_07:[e-mail]-501516-right-click-search/[e-mail]

Windows coming up.

Comment 5

[Magnus Melin [:mkmelin]]

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

>     <menuitem id="folderPaneContext-searchMessages"
>               label="&folderContextSearchMessages.label;"
>               accesskey="&folderContextSearchMessages.accesskey;"
>-              command="cmd_search"/>
>+              oncommand="gFolderTreeController.searchMessages();"/>

This looks wrong.

Comment 6

[Rain [:rain1] (inactive)]

(In reply to comment #5)
> (From update of attachment 394649 [details] [diff] [review])
> >     <menuitem id="folderPaneContext-searchMessages"
> >               label="&folderContextSearchMessages.label;"
> >               accesskey="&folderContextSearchMessages.accesskey;"
> >-              command="cmd_search"/>
> >+              oncommand="gFolderTreeController.searchMessages();"/>
> 
> This looks wrong.

Why? The way I've done is is that the default for MsgSearchMessages() (accel-shift-F or anything else that triggers the cmd_search command) should be the displayed folder, while the default for gFolderTreeController.searchMessages() should be the selected one. This was the best way I could find to express this logic.

Comment 7

[Rain [:rain1] (inactive)]

Windows zip: http://s3.mozillamessaging.com/build/try-server/2009-08-15_07:[e-mail]-501516-right-click-search/[e-mail]

Windows installer: http://s3.mozillamessaging.com/build/try-server/2009-08-15_07:[e-mail]-501516-right-click-search/[e-mail]

Comment 8

[Magnus Melin [:mkmelin]]

Yeah looks good on a second look :)
Should probably add a check for canSearchmessages to the context menu setup code though. (Not that that was done earlier either.)

Comment 9

[David :Bienvenu]

The patch seems to fix the bug for me, but the mozmill test fails. I'm leery now about my mozmill setup, but the other tests you changed seem to work fine. When I run this test, I don't see the folder list expanded, nor does a window open up.

TEST-PASS |  setupModule
TEST-UNEXPECTED-FAIL |  test_open_search_window_with_nothing_selected
  EXCEPTION: Thought we would find row null at 10,0 but we found -1
    at: test-folder-display-helpers.js line 620
       Error("Thought we would find row null at 10,0 but we found -1")  0
       _row_click_helper([object XULElement],null,2) test-folder-display-helpers.js 620
       right_click_on_folder([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 731
       open_search_window_from_context_menu([object XPCWrappedNative_NoHelper]) test-search-window-helpers.js 94
       test_open_search_window_with_nothing_selected() test-right-click-to-open-search-window.js 61
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 409
            frame.js 557
            server.js 164
            server.js 168
TEST-UNEXPECTED-FAIL |  test_open_search_window_with_existing_single_selection
  EXCEPTION: Desired selection is: RightClickToOpenSearchWindowA but actual selection is: [e-mail]
    at: test-folder-display-helpers.js line 1553
       Error("Desired selection is: RightClickToOpenSearchWindowA but actual selection is: tinderbox  0
       assert_folders_selected([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1553
       assert_folders_selected_and_displayed([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1602
       test_open_search_window_with_existing_single_selection() test-right-click-to-open-search-window.js 72
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 409
            frame.js 557
            server.js 164
            server.js 168
TEST-UNEXPECTED-FAIL |  test_open_search_window_with_one_thing_selected
  EXCEPTION: Desired selection is: RightClickToOpenSearchWindowA but actual selection is: [e-mail]
    at: test-folder-display-helpers.js line 1553
       Error("Desired selection is: RightClickToOpenSearchWindowA but actual selection is: tinderbox  0
       assert_folders_selected([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1553
       assert_folders_selected_and_displayed([object XPCWrappedNative_NoHelper]) test-folder-display-helpers.js 1602
       test_open_search_window_with_one_thing_selected() test-right-click-to-open-search-window.js 85
            frame.js 452
            frame.js 504
            frame.js 546
            frame.js 409
            frame.js 557
            server.js 164
            server.js 168

Comment 10

[Rain [:rain1] (inactive)]

Attachment: patch v1.01

oops

Comment 11

[David :Bienvenu]

Comment on attachment 394909 [details] [diff] [review]
patch v1.01

test passes now...

Comment 12

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

Comment on attachment 394909 [details] [diff] [review]
patch v1.01

fantastic work on the unit tests

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 394909 [details] [diff] [review]
patch v1.01

sorry for the delay.  thanks!

Comment 14

[Rain [:rain1] (inactive)]

http://hg.mozilla.org/comm-central/rev/5a559b16fedc

Comment 15

[Rain [:rain1] (inactive)]

(In reply to comment #8)
> Should probably add a check for canSearchmessages to the context menu setup
> code though. (Not that that was done earlier either.)

Filed bug 511060.