Closed Bug 544983 Opened 14 years ago Closed 13 years ago

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

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(blocking-thunderbird3.1 -, thunderbird3.1 .10-fixed)

VERIFIED FIXED
Thunderbird 3.3a3
Tracking Status
blocking-thunderbird3.1 --- -
thunderbird3.1 --- .10-fixed

People

(Reporter: wsmwk, Assigned: squib)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
Wayne did it work after the great UI rewrite from last june ?
(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.
blocking-thunderbird3.1: --- → ?
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)
Summary: select thread (ctrl+shift+A) not working in cross folder virtual/saved searched → select thread (ctrl+shift+A) not working in cross folder virtual/saved searched/unified folders
Attached patch Fix this (obsolete) — Splinter Review
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...
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
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. :)
Attachment #503963 - Flags: review?(bugmail)
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.
Attachment #503963 - Flags: review?(bugmail)
(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.
(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).
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.
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.
(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.
Attached patch Now with tests (obsolete) — Splinter Review
Ok, here are some Mozmill tests. I think this should be sufficient, but if not, let me know.
Attachment #503963 - Attachment is obsolete: true
Attachment #505220 - Flags: review?(bugmail)
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...
Attachment #505220 - Flags: review?(bugmail)
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.
Attachment #505220 - Attachment is obsolete: true
Attachment #505241 - Flags: review?(bugmail)
Today is not my day. :( This version removes a stowaway letter "r" in test-folder-display-helpers.js. Sorry about all the bugspam!
Attachment #505241 - Attachment is obsolete: true
Attachment #505246 - Flags: review?(bugmail)
Attachment #505241 - Flags: review?(bugmail)
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.
Attachment #505246 - Flags: review?(bugmail)
I don't currently have try server access (I'm working on it, though). Could you push to try for me?
(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 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
Attachment #505246 - Flags: review?(bugmail)
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.
Attachment #505246 - Flags: review?(bugmail) → review+
Adding checkin-needed.
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/3f7aeff24a30
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
Hardware: x86 → All
Version: unspecified → Trunk
blocking-thunderbird3.1: ? → ---
blocking-thunderbird3.1: --- → ?
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.
Attachment #505246 - Flags: approval-thunderbird3.1.10+
Thanks very much Jim!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: