User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.12) Gecko/2009070811 Ubuntu/9.04 (jaunty) Firefox/3.0.12 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1) Gecko/20090715 In 3.0b3, messages of a thread are not cascaded when View filter is set to Unread. See attached screenshots from 2.0.0.22 and 3.0b3. Reproducible: Always

Weird. Definitely happens. Fixing the bug reference to the bug we are actually using for 474701 related regressions.

(In reply to comment #3) > Weird. Definitely happens. > > Fixing the bug reference to the bug we are actually using for 474701 related > regressions. Oups sorry

yes, this is bad...

Can you please update the title? It's independent from the kind of filtering, I guess ...

I'll figure this out...

gonna go see what I have to add to the view xpcshell tests to catch this.

Neil would prefer someone else look at the test patch -> Standard8.

Comment on attachment 395127 [details] [diff] [review] proposed fix - checked in. While this does display messages at their original level, they are sorted within the thread by original order rather than by thread order.

Threaded Example: 1 Foo 2 Re: Foo 4 Re: Foo 3 Re: Foo Filtered version: 1 Foo 2 Re: Foo 3 Re: Foo 4 Re: Foo

Comment on attachment 395182 [details] [diff] [review] add to view unit test Ok, this looks fine, but you may want to extend it to cover Neil's comments on the patch when you address those.

nsMsgQuickSearchDBView.cpp needs to use the equivalent of nsMsgDBView::ListIdsInThreadOrder in order to fix the ordering issue. That shouldn't be too hard. And I need to tweak one of the test threads to be slightly out of order (probably by flipping the order of a couple of the synthetic messages in the array before adding them to the db.

Comment on attachment 395127 [details] [diff] [review] proposed fix - checked in. r+sr if you follow this up with an ordering fix.

this fixes the thread ordering - comments in patch explain all the fun. I need to add test cases for these two issues in the xpcshell tests...I'll do that before asking for review just because sometimes doing the unit tests turns up bugs...

this fixes it by using the thread enumerator...test case coming up...

this adds explicit tests for this bug to the view tests.

Comment on attachment 396306 [details] [diff] [review] fix for the child view ordering in qs >+ if (msgKey == keyToSkip || m_origKeys.BinaryIndexOf(msgKey) == -1) >+ continue; I understand the keyToSkip bit but what does the checking m_origKeys achieve?

m_origKeys is the list of messages that matched the search criteria in the folder, which is usually a subset of the messages in the folder/db. But the enumerator gives us all messages in the thread, not just ones in the view. The thread in this case is the real db thread object, not the simulation we use in cross-folder threaded views.

Comment on attachment 396306 [details] [diff] [review] fix for the child view ordering in qs >+ nsresult rv = NS_OK; >+ >+ nsCOMPtr <nsISimpleEnumerator> msgEnumerator; >+ threadHdr->EnumerateMessages(parentKey, getter_AddRefs(msgEnumerator)); This can fail, so we should be checking the result or msgEnumerator. >+ if (rootKey != parentKey) >+ rv = ListIdsInThreadOrder(threadHdr, rootKey, level, parentKey, viewIndex, >+ pNumListed); nit: pNumListed needs to be aligned under threadHdr.

I was trying to test this, but I hit some unrelated memory corruption when I tried to quicksearch mozilla.dev.apps.firefox for "(OT)" :-(

(In reply to comment #22) > I was trying to test this, but I hit some unrelated memory corruption when I > tried to quicksearch mozilla.dev.apps.firefox for "(OT)" :-( OK, that was easily figured out - I had some totally bogus debug code that I'm surprised didn't crash before as it was potentially corrupting memory all over. (In reply to comment #20) > m_origKeys is the list of messages that matched the search criteria in the > folder, which is usually a subset of the messages in the folder/db. But the > enumerator gives us all messages in the thread, not just ones in the view. The > thread in this case is the real db thread object, not the simulation we use in > cross-folder threaded views. Right, but you don't want to stop enumerating children just because the current message doesn't match the search criteria. Otherwise searching on an author will only ever usually find their first message in each thread. (There are some edge cases that this code will let through, such as two replies to the same message by the same author, or the author following up on his own message, or the virtual root not being the thread root.)

This addresses Neil's comments, I believe, and Standard8's. Thx for catching that, Neil. This fixes it for me.

I've fixed the pNumListed indentation issue in my local tree.

pinging for sr.

There is another fairly recent oddity in the threaded view when a view is active, but I'm not sure if this patch would also fix that, or if they are even related. It used to be the case that if I had the unread view active and selected a newsgroup with several unread messages, the threads would be fully expanded so that all messages would be visible. Now with recent TB 3.x nightlies under Vista, I select the newsgroup for the first time after new posts arrive, all threads are collapsed, but if I just select a second newsgroup, and go back to the original one, the threads are then properly expanded, as they were in TB 2.x. Relevant Settings: View, Threads, All; View, Messages, Unread; View, Sort By, Order Received, Threaded; Active View: Unread. This happens to be a SSL newsgroup requiring password authentication, but I'm not sure that is required.

Comment on attachment 396459 [details] [diff] [review] fix addressing comments >+ childLevel++; >+ } I couldn't find a thread quite complicated enough to thoroughly test this out but it coped well enough on the threads that I did try it on.

fix checked in, along with test case changes.