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

RESOLVED FIXED in Thunderbird 3.0b1

Status

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: wsmwk, Assigned: Bienvenu)

Tracking

Trunk
Thunderbird 3.0b1
x86
Windows Vista
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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?
Keywords: regression
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
Posted patch proposed fix (obsolete) — Splinter Review
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 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?
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.
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)
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 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+
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-
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.
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...
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.
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)
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 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+
fix checked in - changeset:   1063:d78ffa717b82
Status: ASSIGNED → RESOLVED
Closed: 11 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.