544983 - select thread (ctrl+shift+A) not working in cross folder virtual/saved searched/unified folders

Status: VERIFIED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Wayne Mery (:wsmwk)]

select thread (ctrl+shift+A) not working in cross folder virtual/saved searched

1. click message in saved search whose thread has >1 message
2. ctrl+shift+A

expected results: all messages of the thread are selected
actual results: no change

Comment 1

[Ludovic Hirlimann [:Usul]]

Wayne did it work after the great UI rewrite from last june ?

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to comment #1)
> Wayne did it work after the great UI rewrite from last june ?

you mean is that when it regressed? 
No, it regressed in the week(s) prior to comment 0.
And still doesn't work.

Fails also of course in unified folders, so requesting wanted flag.

Comment 3

[Wayne Mery (:wsmwk)]

standard8, I had nominated this for "wanted" status because of our push of unified folders, and because it fails even if all the messages in the thread to select are from the same real folder - thus select thread is quite broken.

Jim, would you be up for taking a crack at this?

(it is hard to believe, but I did not find any matching topics in getsatisfaction)

Comment 4

[Jim Porter (:squib)]

Attachment: Fix this

Here we go. Are there any other commands that are missing that people actually want? It seems that a lot of them are excluded from search folders...

Comment 5

[Jim Porter (:squib)]

Comment on attachment 503963 [details] [diff] [review]
Fix this

Ok, I tested this with conversations/gloda results and everything works. :asuth, do you think I need tests for this? There don't seem to be tests for saved searches to begin with...

I'll write tests, but I'd rather not do more than actually necessary for this bug, since it's only a 2-line patch and one of those is a whitespace fix. :)

Comment 6

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

Comment on attachment 503963 [details] [diff] [review]
Fix this

Sadly, tests are required in this case since they are within reach and nsMsgDBView and its descendants are combinatorially complex and so likely to break without us noticing it.

Existing test(s) from closest to the code to furthest:

Direct nsMsgDBView tests:
http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_nsMsgDBView.js

View wrapper tests which exercise that code:
http://mxr.mozilla.org/comm-central/source/mail/base/test/unit/test_viewWrapper_virtualFolder.js

The folder-display tests in mozmill.

Comment 7

[David :Bienvenu]

(In reply to comment #5)
> There don't seem to be tests for saved searches to begin with...

Smart/Unified folders are saved searches, and we have various tests for those.

Comment 8

[Jim Porter (:squib)]

(In reply to comment #7)
> (In reply to comment #5)
> > There don't seem to be tests for saved searches to begin with...
> 
> Smart/Unified folders are saved searches, and we have various tests for those.

Mozmill tests? I assume I'd need to do this via Mozmill, since it's testing a keyboard shortcut. I didn't see anything in the Mozmill directory except search-window/ but that looks like something different (I really don't know a whole lot about this part of the code).

Comment 9

[David :Bienvenu]

folder-display/test-deletion-from-virtual-folders.js and folder-display/test-savedsearch_reload_after_compact.js are a couple mozmill tests that have to do with saved searches.

Comment 10

[Jim Porter (:squib)]

Ok, I'm probably going to do this via Mozmill, since I can't figure out how to convince xpcshell tests to actually select a message in the thread pane. If anyone has ideas for that, I can do xpcshell tests too.

Comment 11

[David :Bienvenu]

(In reply to comment #10)
> Ok, I'm probably going to do this via Mozmill, since I can't figure out how to
> convince xpcshell tests to actually select a message in the thread pane. If
> anyone has ideas for that, I can do xpcshell tests too.

er, no, mozmill is definitely the way to go for selection tests.

Comment 12

[Jim Porter (:squib)]

Attachment: Now with tests

Ok, here are some Mozmill tests. I think this should be sufficient, but if not, let me know.

Comment 13

[Jim Porter (:squib)]

Comment on attachment 505220 [details] [diff] [review]
Now with tests

There's no way this test can be right, since it passes even when it should fail. Hang on...

Comment 14

[Jim Porter (:squib)]

Attachment: Now with tests that actually work

Ok, here we go. It turns out that in the cross-folder case, the threads were collapsed so the whole thread was selected already. This fixes that. Also, I got rid of some trailing whitespace in nsMsgSearchDBView.cpp.

Comment 15

[Jim Porter (:squib)]

Attachment: Once more but this time with feeling

Today is not my day. :( This version removes a stowaway letter "r" in test-folder-display-helpers.js. Sorry about all the bugspam!

Comment 16

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

Comment on attachment 505246 [details] [diff] [review]
Once more but this time with feeling

Thanks very much for the test!

mozmill tests can be brittle when faced with the actual tinderbox machines or on other platforms, so please push this to the comm-central try server and make sure the test and its containing group end up green, then request review again.  If the test fails or its group fails, let me know and I can help figure out what's going wrong too.

Comment 17

[Jim Porter (:squib)]

I don't currently have try server access (I'm working on it, though). Could you push to try for me?

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> I don't currently have try server access (I'm working on it, though). Could you
> push to try for me?

Pushed - look for revision a4a3b2b76f57 on try server.

Comment 19

[Jim Porter (:squib)]

Comment on attachment 505246 [details] [diff] [review]
Once more but this time with feeling

It looks like this is good (the failures look unrelated), so requesting review again: http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=a4a3b2b76f57

Comment 20

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

Comment on attachment 505246 [details] [diff] [review]
Once more but this time with feeling

Yes, not a lucky try-server push :)  Now that someone (aka you) is actively touching chrome more frequently, the oranges are becoming much more troublesome.  I will try and allocate some time to help squash them if sid0/Standard8/friends don't get to them first.  Right now this is looking like mid-February.

Thanks for the patch and super thanks for the mozmill test.

Comment 21

[Jim Porter (:squib)]

Adding checkin-needed.

Comment 22

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/3f7aeff24a30

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 505246 [details] [diff] [review]
Once more but this time with feeling

I had a chat to drivers today and we decided we could take this for 3.1.10.

Comment 24

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/comm-1.9.2/rev/78b943e6833c

Comment 25

[Wayne Mery (:wsmwk)]

Thanks very much Jim!