going from sort grouped to sort unthreaded or sort threaded doesn't show thread pane correctly

RESOLVED FIXED in Thunderbird 3.0b1

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: wsmwk, Assigned: Bienvenu)

Tracking

Trunk
Thunderbird 3.0b1
x86
Windows Vista

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
after bug 379806, in saved search folder going from sort grouped to sort unthreaded or sort threaded shows only 5 messages in thread pane, which coincidentally is the number of groups. I could be dreaming, but this smells familiar.

1. set grouped
2. go to ungrouped.
results: only 5 messages shown
3. leave folder
4. return to folder
results: threading is correct

Normal folder is also broken going from grouped to threaded or unthreaded. The "Today", "Yesterday", etc lines are not removed from threadpane, and threading doesn't work.  do steps 3&4 above and threading is correct - so suspect these two issues are the same bug.

Bug 462565 related?
(Reporter)

Updated

10 years ago
Keywords: regression
(Assignee)

Comment 1

10 years ago
I think technically this can't be a regression since you couldn't group or thread saved searches before today (cross folder ones, anyway). But I can reproduce this. Bug 462565 is not related, unfortunately.
Status: NEW → ASSIGNED
Keywords: regression
(Assignee)

Comment 2

10 years ago
Created attachment 345906 [details] [diff] [review]
proposed fix 

Basically, rebuilding views wasn't always cleaning up the old view contents because the view flags had changed and it didn't know it had to clean up old stuff. And enumerating the view wasn't always working, again, because we were changing the view flags too early. Now, I don't need to override RebuildView, because InternalClose knows what it has to cleanup.

Oh, and cloning group views wasn't cloning the group table, which was also causing problems.
Attachment #345906 - Flags: superreview?(neil)
Attachment #345906 - Flags: review?(bugzilla)

Comment 3

10 years ago
Comment on attachment 345906 [details] [diff] [review]
proposed fix 

> NS_IMETHODIMP nsMsgQuickSearchDBView::SetViewFlags(nsMsgViewFlagsTypeValue aViewFlags)
> {
>   nsMsgViewFlagsTypeValue saveViewFlags = m_viewFlags;
>   nsresult rv = nsMsgDBView::SetViewFlags(aViewFlags);
>   // if the grouping has changed, rebuild the view
>   if (saveViewFlags & nsMsgViewFlagsType::kGroupBySort ^
>       (aViewFlags & nsMsgViewFlagsType::kGroupBySort))
>-    RebuildView();
>+    RebuildView(saveViewFlags);
> 
>   return rv;
> }
So why is it that you can't set the view flags early in this case:
> NS_IMETHODIMP nsMsgXFVirtualFolderDBView::SetViewFlags(nsMsgViewFlagsTypeValue aViewFlags)
> {
>-  nsMsgViewFlagsTypeValue saveViewFlags = m_viewFlags;
>-  nsresult rv =  nsMsgDBView::SetViewFlags(aViewFlags);
>+  nsresult rv;
>   // if the grouping/threading has changed, rebuild the view
>-  if ((saveViewFlags & (nsMsgViewFlagsType::kGroupBySort |
>+  if ((m_viewFlags & (nsMsgViewFlagsType::kGroupBySort |
>                       nsMsgViewFlagsType::kThreadedDisplay)) !=
>       (aViewFlags & (nsMsgViewFlagsType::kGroupBySort |
>                      nsMsgViewFlagsType::kThreadedDisplay)))
>-    RebuildView();
>-
>+    rv = RebuildView(aViewFlags);
>+  else
>+    rv =  nsMsgDBView::SetViewFlags(aViewFlags);
>   return rv;
> }
Does RebuildView set the view flags?
(Assignee)

Comment 4

10 years ago
RebuildView does set the view flags, when it calls OpenWithHdrs, and passes in the new view flags.  I need to delay changing the view flags until RebuildView has had a chance to clone the old view, and clear the old view correctly based on the old view flags.

There are still issues with going in and out of grouped/threaded view for quick searches - I'll look at those today.
(Assignee)

Comment 5

10 years ago
Created attachment 346102 [details] [diff] [review]
fix single folder saved search case as well

this fixes the single folder saved search/quick search case as well.

I needed to tweak the way we keep search results, m_origKeys. This is used when expanding threads.

Loading single folder saved searches is still sub-optimal, in that it loads the whole folder and then does the search, but that's a long-standing issue.
Attachment #345906 - Attachment is obsolete: true
Attachment #346102 - Flags: superreview?(neil)
Attachment #346102 - Flags: review?(bugzilla)
Attachment #345906 - Flags: superreview?(neil)
Attachment #345906 - Flags: review?(bugzilla)
(Assignee)

Comment 6

10 years ago
oh, I should mention that RebuildView was causing m_viewFlags to get set, but wasn't always causing the view flags to get saved in the db, so I call nsMsgDBView::SetViewFlags to ensure that happens.

Comment 7

10 years ago
Comment on attachment 346102 [details] [diff] [review]
fix single folder saved search case as well

I wonder at those SetViewFlags call inconsistencies ;-)

>+  m_origKeys.InsertElementAt(insertIndex, msgKey);
Out of interest, where do these keys live? Seems to me that they (ought to) provide a cheap way of expanding all threads/groups.
Attachment #346102 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 8

10 years ago
m_origKeys lives in nsMsgQuickSearchDBView.h - it would be a cheap way of going from threaded to flat views, which does involve expanding all threads/groups, and a cheap way to rebuild the view in general - I think this would be done by making nsMsgQuickSearchDBView::GetMessageEnumerator return an iterator over the headers in m_origKeys.
Comment on attachment 346102 [details] [diff] [review]
fix single folder saved search case as well

I've set up a saved search on my imap folder. When I start TB (and then collapse and expand the Imap folders to get the saved search back, which is probably another bug), then select View -> Sort -> Grouped, I get the following assertion and a crash:

###!!! ASSERTION: nsVoidArray::FastElementAt: index out of range: '0 <= aIndex && aIndex < Count()', file ../../dist/include/xpcom/nsVoidArray.h, line 72
Attachment #346102 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 10

10 years ago
Is it a single folder saved search or a cross-folder saved search? Are there any search results? I tried this with a single folder saved search, both with results, and w/o, and didn't see this crash.
(Assignee)

Comment 11

10 years ago
What sort was the saved search in before? threaded or flat, and what did you end up grouping by? I've been grouping by date...
(Assignee)

Comment 12

10 years ago
OK, thx to Standard8, I have STR, and a fix:

1. Setup a cross-folder saved search (2 folders is fine), with all the results in one date group (e.g., old mail, by setting the search criteria to be messages older than 30 days)
2. Open the folder, sort by thread, and select a different folder.
3. Come back to the saved search, and view | sort by | grouped
4. Expand the Old Mail group - crash.

You may need to make sure that we don't select a message when the folder is open, which can mean shutting down, or turning off the select last message pref. Convincing the group to be closed on folder open can be a bit tricky.
(Assignee)

Comment 13

10 years ago
Created attachment 346517 [details] [diff] [review]
proposed fix, v2

GetInsertIndexHelper should get the EntryInfo2 msg hdr from the folder for EntryInfo2.
Attachment #346102 - Attachment is obsolete: true
Attachment #346517 - Flags: superreview+
Attachment #346517 - Flags: review?(bugzilla)
(Assignee)

Comment 14

10 years ago
disappearing imap saved search issue spun-off to Bug 463274 (I believe this is an unrelated issue, so there aren't any dependencies w/ this bug)

Comment 15

10 years ago
Comment on attachment 346517 [details] [diff] [review]
proposed fix, v2

>+nsMsgGroupView::CopyDBView(nsMsgDBView *aNewMsgDBView, nsIMessenger *aMessengerInstance, 
git-apply: warning: 1 line adds whitespace errors.
Comment on attachment 346517 [details] [diff] [review]
proposed fix, v2

+  if (m_viewFlags & (nsMsgViewFlagsType::kGroupBySort|
+                     nsMsgViewFlagsType::kThreadedDisplay))
+  {
+    nsMsgKey parentKey;
+    msgHdr->GetThreadParent(&parentKey);
+    rv = nsMsgThreadedDBView::OnNewHeader(msgHdr, parentKey, PR_TRUE);
+  }
+  else
+    rv = nsMsgDBView::AddHdr(msgHdr, resultIndex);
+  return rv;

I think we could probably skip the extra rv assignment here. Just return the first one, drop the else, and return the second.
Attachment #346517 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 17

10 years ago
fix checked in - changeset:   1063:d78ffa717b82
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b1
You need to log in before you can comment on or make changes to this bug.