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

VERIFIED FIXED in Thunderbird 3.3a3

Status

Thunderbird
Folder and Message Lists
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: wsmwk, Assigned: squib)

Tracking

({regression})

Trunk
Thunderbird 3.3a3
regression
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 4

6 years ago
Created attachment 503963 [details] [diff] [review]
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...
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
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)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 10

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 505220 [details] [diff] [review]
Now with tests

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

Comment 13

6 years ago
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)
(Assignee)

Comment 14

6 years ago
Created attachment 505241 [details] [diff] [review]
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.
Attachment #505220 - Attachment is obsolete: true
Attachment #505241 - Flags: review?(bugmail)
(Assignee)

Comment 15

6 years ago
Created attachment 505246 [details] [diff] [review]
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!
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)
(Assignee)

Comment 17

6 years ago
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.
(Assignee)

Comment 19

6 years ago
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+
(Assignee)

Comment 21

6 years ago
Adding checkin-needed.
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/3f7aeff24a30
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
Keywords: regressionwindow-wanted
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+
Checked in: http://hg.mozilla.org/releases/comm-1.9.2/rev/78b943e6833c
blocking-thunderbird3.1: ? → -
status-thunderbird3.1: --- → .10-fixed
Thanks very much Jim!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.