505967 - Threaded view is broken when a view is in effect

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Can Koy]

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

Comment 1

[Can Koy]

Attachment: Broken threaded view of 3.0b3

Comment 2

[Can Koy]

Attachment: Threaded view of 2.0.0.22

Comment 3

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

Weird.  Definitely happens.

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

Comment 4

[Ludovic Hirlimann [:Usul]]

(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

Comment 6

[David :Bienvenu]

yes, this is bad...

Comment 7

[Jens Müller (:tessarakt)]

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

Comment 8

[David :Bienvenu]

I'll figure this out...

Comment 9

[David :Bienvenu]

Attachment: proposed fix - checked in.

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

Comment 10

[David :Bienvenu]

Attachment: add to view unit test

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

Comment 11

[[e-mail]]

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.

Comment 12

[[e-mail]]

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 13

[Mark Banner (:standard8)]

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.

Comment 14

[David :Bienvenu]

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 15

[[e-mail]]

Comment on attachment 395127 [details] [diff] [review]
proposed fix - checked in.

r+sr if you follow this up with an ordering fix.

Comment 16

[David :Bienvenu]

Attachment: cumulative 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...

Comment 17

[David :Bienvenu]

Attachment: fix for the child view ordering in qs

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

Comment 18

[David :Bienvenu]

Attachment: test case

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

Comment 19

[[e-mail]]

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?

Comment 20

[David :Bienvenu]

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 21

[Mark Banner (:standard8)]

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.

Comment 22

[[e-mail]]

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

Comment 23

[[e-mail]]

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

Comment 24

[David :Bienvenu]

Attachment: fix addressing comments

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

Comment 25

[David :Bienvenu]

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

Comment 26

[David :Bienvenu]

pinging for sr.

Comment 27

[eberry]

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 28

[[e-mail]]

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.

Comment 29

[David :Bienvenu]

fix checked in, along with test case changes.