Closed Bug 505967 Opened 12 years ago Closed 12 years ago

Threaded view is broken when a view is in effect

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: cankoy, Assigned: Bienvenu)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [no l10n impact])

Attachments

(6 files, 2 obsolete files)

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
Blocks: 474701
Version: unspecified → 2.0
Version: 2.0 → Trunk
Weird.  Definitely happens.

Fixing the bug reference to the bug we are actually using for 474701 related regressions.
Blocks: gloda-ui-regressions
No longer blocks: 474701
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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
Keywords: regression
Duplicate of this bug: 510044
Flags: blocking-thunderbird3?
yes, this is bad...
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b4
Can you please update the title? It's independent from the kind of filtering, I guess ...
Summary: Threaded view is broken when Unread filter in effect → Threaded view is broken when a filter is in effect
I'll figure this out...
Assignee: nobody → bienvenu
OS: Linux → All
Summary: Threaded view is broken when a filter is in effect → Threaded view is broken when a view is in effect
gonna go see what I have to add to the view xpcshell tests to catch this.
Attachment #395127 - Flags: superreview?(neil)
Attachment #395127 - Flags: review?(neil)
Neil would prefer someone else look at the test patch -> Standard8.
Attachment #395182 - Flags: review?(bugzilla)
Whiteboard: [has patch for review, neil, standard8]
Whiteboard: [has patch for review, neil, standard8] → [has patch for review, neil, standard8][no l10n impact]
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.
Attachment #395182 - Flags: review?(bugzilla) → review+
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.
Attachment #395127 - Flags: superreview?(neil)
Attachment #395127 - Flags: superreview+
Attachment #395127 - Flags: review?(neil)
Attachment #395127 - Flags: review+
Attached patch cumulative fix (obsolete) — Splinter Review
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...
Attachment #395127 - Attachment description: proposed fix → proposed fix - checked in.
this fixes it by using the thread enumerator...test case coming up...
Attachment #396306 - Flags: superreview?(neil)
Attachment #396306 - Flags: review?(bugzilla)
Attached patch test caseSplinter Review
this adds explicit tests for this bug to the view tests.
Attachment #396307 - Flags: review?(bugzilla)
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.
Attachment #396307 - Flags: review?(bugzilla) → review+
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.
Attachment #396306 - Flags: review?(bugzilla) → review+
Whiteboard: [has patch for review, neil, standard8][no l10n impact] → [has patch for review, neil][no l10n impact]
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.
Attachment #396082 - Attachment is obsolete: true
Attachment #396306 - Attachment is obsolete: true
Attachment #396459 - Flags: superreview?(neil)
Attachment #396306 - Flags: superreview?(neil)
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.
Attachment #396459 - Flags: superreview?(neil) → superreview+
fix checked in, along with test case changes.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch for review, neil][no l10n impact] → [no l10n impact]
You need to log in before you can comment on or make changes to this bug.