Last Comment Bug 379806 - threaded/grouped-by-sort views unavailable in saved searches across multiple folders
: threaded/grouped-by-sort views unavailable in saved searches across multiple ...
Status: RESOLVED FIXED
: polish
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: P1 enhancement with 5 votes (vote)
: Thunderbird 3.0b1
Assigned To: David :Bienvenu
:
Mentors:
http://wiki.mozilla.org/Thunderbird:T...
: 267727 378724 396554 424597 (view as bug list)
Depends on: 462637 462581 462638 465011 465018 465024 539784
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-04 22:45 PDT by bugzilla.overbored
Modified: 2010-06-27 09:46 PDT (History)
22 users (show)
standard8: blocking‑thunderbird3-
standard8: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix for grouping in single folder saved searches (25.15 KB, patch)
2008-08-06 15:44 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
more wip (96.78 KB, patch)
2008-08-09 06:38 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
more wip (134.87 KB, patch)
2008-09-30 16:22 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
patch for review (186.72 KB, patch)
2008-10-07 16:46 PDT, David :Bienvenu
standard8: review-
Details | Diff | Splinter Review
fix addressing Standard8's comments (193.44 KB, patch)
2008-10-20 13:53 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
front end changes (5.82 KB, patch)
2008-10-20 13:56 PDT, David :Bienvenu
standard8: review+
Details | Diff | Splinter Review
change to nsIMsgHdr references (6.35 KB, patch)
2008-10-24 15:03 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
add convenience functions for manipulating view arrays (13.13 KB, patch)
2008-10-25 10:05 PDT, David :Bienvenu
neil: review+
neil: superreview+
Details | Diff | Splinter Review
patch addressing Neil's comments - checked in (13.13 KB, patch)
2008-10-26 14:15 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
breakout some cleanup work into its own patch (44.31 KB, patch)
2008-10-27 07:58 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
address Neil's comments - checked in (47.53 KB, patch)
2008-10-27 10:18 PDT, David :Bienvenu
neil: review+
neil: superreview+
Details | Diff | Splinter Review
[checked in] do some preparation in nsMsgGroupView (7.12 KB, patch)
2008-10-27 14:12 PDT, David :Bienvenu
neil: review+
neil: superreview+
Details | Diff | Splinter Review
remaining diffs (135.00 KB, patch)
2008-10-27 15:04 PDT, David :Bienvenu
standard8: review+
neil: superreview+
Details | Diff | Splinter Review
patch for checkin (163.34 KB, patch)
2008-10-30 15:23 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review

Description bugzilla.overbored 2007-05-04 22:45:30 PDT
User-Agent:       Opera/9.20 (Windows NT 5.1; U; en)
Build Identifier: version 2.0.0.0 (20070326)

When I create a Saved Search that includes several folders to be searched, I cannot use the Threaded view or the Grouped By Sort view. (I don't know if this makes a difference but I have only an IMAP account to try this out on, though I imagine this doesn't matter.)

Reproducible: Always

Steps to Reproduce:
1. Create a Saved Search spanning multiple folders.
2. Click on the View > Sort by.
Actual Results:  
You find that the bottom 3 menu items are disabled.

Expected Results:  
You find that the bottom 3 menu items are enabled.

A threaded view where you can see all messages in the thread - which is only possible by creating a Saved Search over both the Inbox and the Sent folders - is generally more practical and useful than one where you only see all received messages.
Comment 1 Magnus Melin 2007-05-12 10:38:28 PDT
Yes... see bug 263180 comment 8. Confirming RFE.
Comment 2 bugzilla.overbored 2007-05-12 10:42:21 PDT
I did find 263180 when I first searched. However, it's evidently still a problem in TB2. I don't understand why 263180 was marked as fixed.
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2007-05-29 13:45:42 PDT
(In reply to comment #2)
> I did find 263180 when I first searched. However, it's evidently still a
> problem in TB2. I don't understand why 263180 was marked as fixed.

what you want (multiple folders) is meant to be handled later, possibly in this bug.  bug 263180 comment 8 says it's work ends with "this allows us to thread quick search results and virtual folders with a single folder scope (but not virtual folders that search over multiple folders). That last case will require a lot of work..."
Comment 4 Magnus Melin 2007-09-27 09:13:12 PDT
*** Bug 396554 has been marked as a duplicate of this bug. ***
Comment 5 Magnus Melin 2008-02-18 09:33:24 PST
This would really vastly improve the usefulness of virtual folders...
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2008-02-18 14:34:26 PST
*** Bug 378724 has been marked as a duplicate of this bug. ***
Comment 7 Wayne Mery (:wsmwk, NI for questions) 2008-02-18 15:02:02 PST
Breaking bugzilla etiquette (and my own normal restraint) to say  YES !! 

Virtual folder is virtually of no use to me without threading. Even better, will be to see it work with view "thread with unread messages", and bubblesort extension.

bug 267727 seems to have evolved to the same issue, though afaict it didn't start that way. And it's not clear to me the reporter's original problem is gone, unless it was fixed by bug 135326.  I'll leave it to others to wade through that.
Comment 8 Magnus Melin 2008-03-25 14:27:16 PDT
*** Bug 424597 has been marked as a duplicate of this bug. ***
Comment 9 Exception e 2008-03-27 13:26:30 PDT
The severity is low now (enhancement). I would like to ask to handle this bug with greater priority.
Comment 10 Wayne Mery (:wsmwk, NI for questions) 2008-04-06 08:35:54 PDT
probably should be tested at least in an alpha (not necessarily a1) given that bug 263180 comment 8 says this "will require a lot of work..." (where attachment 197367 [details] [diff] [review] implemented threading of virtual for scope of single folder).  But no flag yet for blocking‑thunderbird3.0a2.

Perhaps Joshua would find this a worthy challenge?
Comment 11 Kent James (:rkent) 2008-07-03 14:11:32 PDT
There's another side effect of this that I want to make sure isn't missed in the fix. SyncCounts is used to correct errors in the saved folder counts due to various problems. It is called when opening the database (but not for virtual folders) and the view (but only in threaded views). So in the case of cross-folder virtual folders, SyncCounts() is never called, so the only way to correct count errors is to manually delete the database file.
Comment 12 Ben Gamari 2008-07-27 21:33:49 PDT
What is the present status of this. Coming from evolutino, search folders are of extremely little use to me without threading support.
Comment 13 David :Bienvenu 2008-07-28 11:56:51 PDT
I'm thinking of trying this...
Comment 14 David :Bienvenu 2008-07-28 13:32:20 PDT
see http://wiki.mozilla.org/Thunderbird:Threading_Cross-folder_Saved_Searches for some implementation thoughts.
Comment 15 David :Bienvenu 2008-08-06 15:44:06 PDT
Created attachment 332609 [details] [diff] [review]
fix for grouping in single folder saved searches

this adds grouping to single folder saved searches. I chose to rearrange the inheritance structure so that nsMsgThreadedDBView inherits from nsMsgGroupView, and nsMsgGroupView calls the base class when grouping is not turned on. I also made it so that grouping a saved search doesn't create&load a new view, but rather just groups the existing view, which is a lot faster. To do this, I just made nsMsgQuickSeachDBView notice when the grouping flag changed, and adjust accordingly. I also changed threadPane.js to tweak the grouping flag instead of creating new views.

Now I'm going to see if this can be extended easily to do grouping and threading in cross-folder saved searches.  I think it should be pretty straightfoward with these plumbing changes.
Comment 16 David :Bienvenu 2008-08-09 06:38:12 PDT
Created attachment 333088 [details] [diff] [review]
more wip

this gets grouping starting to work in cross-folder saved searches, and paves the way for threading in same.
Comment 17 Mark Banner (:standard8) (afk until 26th July) 2008-08-22 07:56:47 PDT
Triaging according to new policy for flags.  
https://wiki.mozilla.org/Thunderbird:Release_Driving
Comment 18 David :Bienvenu 2008-08-22 08:02:22 PDT
I think asuth is hoping to use this work with gloda and experimental views, so I'm bumping up the priority. 
Comment 19 Eugenio Perea 2008-08-29 19:05:01 PDT
I'd like to add my support to the resolution of this bug. I use a saved search
as a Global Inbox for 3 IMAP accounts and threaded messages would make it much easier to use.
Comment 20 David :Bienvenu 2008-09-30 16:22:21 PDT
Created attachment 341176 [details] [diff] [review]
more wip

this is a diff between the trunk and my repo for the relevant files, just to give a sense of what's going on with the patch.
Comment 21 David :Bienvenu 2008-10-06 11:55:31 PDT
Here's my current todo list, i.e., the things that are currently broken in my repo - http://hg.mozilla.org/users/bienvenu_nventure.com/threadview/

Improve large folder loading performance, perhaps by sorting after we've finished creating the view from the cache (might make FindHdr slow again, 
though we should be sorted by folder, then by key within folder).

Fix quick search (did it work before?)
Don't expand threads when loading the folder
Fix handling of case where non-root parent within thread arrives after children.
Sorting threads (e.g., by sender) doesn't seem to work
Comment 22 David :Bienvenu 2008-10-06 16:46:35 PDT
I've made about a 3x improvement in large view loading performance, fixed a few issues with parents coming in after children, etc. I've defaulted threads to collapsed, and made sorting threads work, though it could be faster, and it forgets which threads were expanded.

I'll look at quick sort, and deal with the non-root parent coming in after children next.
Comment 23 David :Bienvenu 2008-10-06 19:59:38 PDT
quick sort is working now...
Comment 24 David :Bienvenu 2008-10-07 10:19:22 PDT
I've fixed handling parents coming in out of order, in the thread object, but I need to communicate this back to the view object, if the thread is expanded.  I need to solve this coordination in general, either by passing back information about what changed, so the view can fix itself up, or by just rebuilding the part of the view that needs to change, based on the thread object.

I've also found a bug where sorting once you're in grouped mode in an xf saved search doesn't do the right thing. I need to sort that out as well.
Comment 25 Andrew Sutherland [:asuth] 2008-10-07 10:30:20 PDT
(In reply to comment #24)
> I've also found a bug where sorting once you're in grouped mode in an xf saved
> search doesn't do the right thing. I need to sort that out as well.
                                               ^^^^
I see what you did there.

I have a feature enhancement request while you are still dealing with the views.  It would be nice for the tab UI logic to be able to persist multiple-selections across sessions and have a way to efficiently restore them.  Right now, the tab code is able to use SelectFolderMsgByKey to select as _single_ message, but you can't accumulate multiple messages into the selection.  I think the only way for the js code to do restore multi-selection is to walk the entire set of messages in the view, building the selection explicitly.  (I discount repeatedly using SelectFolderMsgByKey to find the view indices because I believe it has non-trivial event side-effects.)  This represents a potentially horrendous XPCOM transit cost, so it would be good to at least cut that out, even if the C++ path isn't fully optimized.  In fact, just having SelectFolderMsgByKey have a mode of operation where it adds to the selection rather than forcing a single-select would be perfect.
Comment 26 David :Bienvenu 2008-10-07 10:54:47 PDT
Heh, I couldn't resist.

Your request reminded me, for xf saved searches, we can't assume that message keys are unique, so we need an other way of identifying selections.  Message keys + folders or message uri's would work, as long as compaction doesn't come along and invalidate the message keys. Message-ids are stable, but not quite unique.
Comment 27 Andrew Sutherland [:asuth] 2008-10-07 11:41:48 PDT
SelectFolderMsgByKey does take an nsIMSgFolder and an nsMsgKey... but nsMsgSearchDBView and nsMsgXFVirtualFolderDBView don't override it to do something, and nsMsgDBView's implementation doesn't use the folder passed-in...
Comment 28 Dan Mosedale (:dmose) 2008-10-07 12:50:50 PDT
We really need UUIDs for messages that are entirely stable across compaction, moves etc.  Would trying to leverage GloDa IDs for this somehow be sane?
Comment 29 Kent James (:rkent) 2008-10-07 13:30:03 PDT
(In reply to comment #28)
> We really need UUIDs for messages that are entirely stable across compaction,
> moves etc.  Would trying to leverage GloDa IDs for this somehow be sane?

I'm really struggling with this in some of my stuff too, for example bug 449768. asuth has proposed message id + folder as the identifier, but that is not persistent as you suggest.

One proposal I would suggest would be to add a copy counter to a message whenver it is copied, then extend the message id with that. So <200810071950.m97Josv1001820@mrapp52.mozilla.org> becomes <200810071950.m97Josv1001820@mrapp52.mozilla.org#1> if copied. We would maintain both the original message id and the copy counter in mork, but the "message id" field would use this extended format. Then I could recover message metadata from copies.

The reason to do this, rather than a freshly created GUID, is that in cases where the relationship between global id and message copy is lost, you could always revert to the unadorned message id to restore some connection.
Comment 30 Andrew Sutherland [:asuth] 2008-10-07 15:02:45 PDT
Gloda's indexing process now stores a "gloda-id" on messages as it indexes them.  In the event the "gloda-id" field disappears, gloda's next indexing of that message will attempt to map the message back to the same underlying gloda-id, although ambiguity happens.

The tab persistence code I speak of can use the gloda identifier to store the mapping, but still wants the underlying nsMsgDBView method to be talking about a (folder and) message key.

In regards to rkent's proposal, I do not think it acceptable to mutate the actual message-id; doing so would cause in-reply-to/references to be wrong.  Using the existing message-id as a base is fine, but a copy-counter could lead to collisions, meaning we'd have to append a real GUID in the first place.  At that point, there's no benefit appending to the message-id since we can just track them separately.

gloda could benefit from the existence of a real GUID, so I'm fine with one being added.  There is no way in the TB3 time-frame for gloda to provide an always-there unique identifier; we can only provide one as a best effort.  Keep in mind that the gloda-id is at best a locally unique identifier and can never be exposed to a shared medium, such as actual IMAP messages; that would need a real GUID.
Comment 31 Kent James (:rkent) 2008-10-07 15:31:58 PDT
Another reason that I am uncomfortable using some portion of gloda for something as fundamental as a message GUID is that gloda has been viewed as an indexer on top of underlying databases. We would need to support the GUID in the C++ database code, which so far has not needed to rely on gloda. Why would we introduce that extra dependency unless we really had to?

If you need a GUID, let's define it at the nsIMsgDBHeader level, created when a message is added to the database. Then we can all use it to have a common way of describing a unique message. At this point I think we would be shortsighted to make it a local identifier instead of a GUID.

I still think you are going to have problems unless that GUID is closely related to the message id, as the message id is the only identifer that we can reasonably be sure will virtually always be available.
Comment 32 Joshua Cranmer [:jcranmer] 2008-10-07 15:43:27 PDT
(In reply to comment #29)
> One proposal I would suggest would be to add a copy counter to a message
> whenver it is copied, then extend the message id with that. So
> <200810071950.m97Josv1001820@mrapp52.mozilla.org> becomes
> <200810071950.m97Josv1001820@mrapp52.mozilla.org#1> if copied. We would
> maintain both the original message id and the copy counter in mork, but the
> "message id" field would use this extended format. Then I could recover message
> metadata from copies.

The problem with that is I may be receiving the same message from different places, e.g., a newsgroup message also sent out by email to a few people, or a message that a family member sends to two of your email accounts.

Message IDs may be unique in certain contexts, but, quite frankly, they're not going to be unique in an entire email profile, and trying to make them so is not a good idea.
Comment 33 David :Bienvenu 2008-10-07 16:46:53 PDT
Created attachment 342170 [details] [diff] [review]
patch for review

let's get the review party started - this was generated by doing a diff between my repo and a comm-central repo of the mailnews dir - I'm not sure if there's a better way, or a way to diff across repo's using hg directly.  There's some whitespace cleanup, but not a ton, so I left it in the diff.

This changes the inheritance structure of the view classes somewhat. Now, everyone inherits from nsMsgGroupView so all the views can do grouping. This means that more methods need to do their own dispatching to the right class based on the view flags, but it allows us to do grouping without creating a new view object, which is a win.

I've made a few more methods virtual, and added some new virtual methods so we can better deal with cross-folder views. The non cross-folder views shouldn't be noticeably affected.

I've added a new class, nsMsgXFViewThread, which is like nsMsgGroupThread, in that it implements nsIMsgThread, instead of using the db's implementation. It's closely tied to nsMsgSearchDBView, so I've made them friends.

If you want to run the code, I'd recommend using my repo, http://hg.mozilla.org/users/bienvenu_nventure.com/threadview/, which has the latest changes, and has been merged with the trunk very recently.
Comment 34 Kent James (:rkent) 2008-10-08 09:00:57 PDT
After compiling the code from the repository, I'm seeing quite a few:

WARNING: NS_ENSURE_SUCCESS(err, err) failed with result 0x80004005: file s:/tb/
hreadview/src/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 5054

which is at:

  nsresult err = GetSearchResultsTable(aSearchFolderUri, PR_FALSE, getter_AddRefs(table));
  NS_ENSURE_SUCCESS(err, err);
Comment 35 Kent James (:rkent) 2008-10-08 11:17:36 PDT
I have a test profile with an existing XF virtual folder "Tags Contains Important" searching over 2 local folders.

If I do any selections in View/Threads (such as select All for example) then the thread pane goes blank, and stays blank until I select a different folder, then reselect the XF virtual folder.

There are no console or debug error messages.
Comment 36 David :Bienvenu 2008-10-08 13:23:36 PDT
I'm inclined to disable things like view | threads with unread for xf saved searches for the time being.
Comment 37 Dan Mosedale (:dmose) 2008-10-08 14:35:28 PDT
Since David's already got a patch here, should we spin off the ID issue to another bug?  (I have some responses to the comments folks have made here, but I suspect this isn't really the best venue...)
Comment 38 David :Bienvenu 2008-10-08 14:39:24 PDT
yes, a new bug is the way to go. I would not block this work on being able to support a global id - for one thing, getting imap servers to universally support keywords or some other annotations shouldn't block this.
Comment 39 David :Bienvenu 2008-10-08 17:22:26 PDT
I have not seen this - WARNING: NS_ENSURE_SUCCESS(err, err) failed with result 0x80004005: file s:/tb/
hreadview/src/mailnews/db/msgdb/src/nsMsgDatabase.cpp, line 5054

But I wonder if you were using a profile where you had been rebuilding databases. Blowing away databases will remove the cached hits in that folder for all the saved searches that have that folder in their search scope. It's harmless in the sense that we will rebuild the cache, but it shouldn't happen unless db's have been deleted.
Comment 40 Mark Banner (:standard8) (afk until 26th July) 2008-10-20 07:34:23 PDT
Comment on attachment 342170 [details] [diff] [review]
patch for review

So this totally didn't apply to my tree - I think it may be because you have the diff directories miss-balanced:

>diff -uNrp8 ./base/public/nsIMsgHdr.idl /threadview/mailnews/base/public/nsIMsgHdr.idl
>--- ./base/public/nsIMsgHdr.idl	Fri Aug 29 08:26:29 2008
>+++ /threadview/mailnews/base/public/nsIMsgHdr.idl	Mon Sep 15 11:43:53 2008

On second thoughts, its probably because I was trying to qimport it and that expects a diff from one above the source directory.

Anyway, I've taken a reasonably quick first-pass through the patch and made some comments.

>@@ -96,17 +96,17 @@ interface nsIMsgDBHdr : nsISupports
>     readonly attribute unsigned long dateInSeconds;
>     attribute string messageId;
>     attribute string ccList;
>     attribute string author;
>     attribute string subject;
>     attribute string recipients;
>     
>     /* anything below here still has to be fixed */
>-    void setReferences(in string references);
>+    attribute string references;

Although it still needs to be fixed (does it?), please can you document it. Can we make it an ACString? getStringReference implies that these can be treated as ACString.

>     readonly attribute unsigned short numReferences;
>     ACString getStringReference(in long refNum);

>-  var dbviewContractId = "@mozilla.org/messenger/msgdbview;1?type=group";
>-  gDBView = Components.classes[dbviewContractId].createInstance(Components.interfaces.nsIMsgDBView);
>+  gDBView = Components.classes["@mozilla.org/messenger/msgdbview;1?type=group"].
>+                                createInstance(Components.interfaces.nsIMsgDBView);

nit: . before the createInstance, not on the end of the previous line.

>--- ./base/src/Makefile.in	Sun Sep 14 18:58:28 2008
>+++ /threadview/mailnews/base/src/Makefile.in	Mon Sep 15 11:43:53 2008
>@@ -117,26 +117,27 @@ CPPSRCS		= \
> 		nsMsgPrintEngine.cpp \
> 		nsStatusBarBiffManager.cpp \
> 		nsMsgDBView.cpp \
> 		nsMsgThreadedDBView.cpp \
> 		nsMsgSpecialViews.cpp \
> 		nsMsgQuickSearchDBView.cpp \
> 		nsMsgSearchDBView.cpp \
> 		nsMsgXFVirtualFolderDBView.cpp \
>+		nsMsgXFViewThread.cpp \
> 		nsMsgGroupThread.cpp \
> 		nsMsgGroupView.cpp \
> 		nsMsgOfflineManager.cpp \
> 		nsMsgProgress.cpp \
> 		nsMessengerContentHandler.cpp \
> 		nsSpamSettings.cpp \
> 		nsCidProtocolHandler.cpp \
> 		nsMsgContentPolicy.cpp \
> 		nsMsgTagService.cpp\
>-                nsMsgFolderNotificationService.cpp\
>+        nsMsgFolderNotificationService.cpp\

nit: this should be two tabs.

>diff -uNrp8 ./base/src/nsMsgDBView.cpp /threadview/mailnews/base/src/nsMsgDBView.cpp
>--- ./base/src/nsMsgDBView.cpp	Mon Oct  6 15:56:31 2008
>+++ /threadview/mailnews/base/src/nsMsgDBView.cpp	Tue Oct  7 07:42:57 2008
>@@ -65,17 +65,16 @@
> #include "nsIMsgSearchSession.h"
> #include "nsIMsgCopyService.h"
> #include "nsMsgBaseCID.h"
> #include "nsISpamSettings.h"
> #include "nsIMsgAccountManager.h"
> #include "nsITreeColumns.h"
> #include "nsTextFormatter.h"
> #include "nsIMutableArray.h"
>-
> nsrefcnt nsMsgDBView::gInstanceCount  = 0;

I don't think we need to remove this line.

>+nsMsgViewIndex nsMsgDBView::GetThreadIndex(nsMsgViewIndex msgIndex)
>+{
>+  if (!IsValidIndex(msgIndex))
>+    return NS_MSG_INVALID_DBVIEW_INDEX;

nit: blank line after here please.

>+  // scan up looking for level 0 message.
>+  while (m_levels[msgIndex] && msgIndex)
>+    msgIndex--;

nit: --msgIndex is slightly more efficient for the compiler to think about.

>+  return msgIndex;
>+}

>+// can this be combined with GetIndexForThread??
>+nsMsgViewIndex 
>+nsMsgDBView::GetThreadRootIndex(nsIMsgDBHdr *msgHdr)

If you're not going to address this for now, please add XXX at the start of the comment.

>-nsresult nsMsgDBView::GetFolders(nsISupportsArray **aFolders)
>+nsresult nsMsgDBView::GetFolders(nsCOMArray <nsIMsgFolder> **aFolders)

Since most of the instances don't check the nsresult, why not change this to:

nsCOMArray<nsIMsgFolder>* nsMsgDBView::GetFolders()

Also no space before the < please (generally throughout the patch as well)

>+NS_IMETHODIMP nsMsgDBView::nsMsgViewHdrEnumerator::GetNext(nsISupports **aItem)
>+{
>+  NS_ENSURE_ARG_POINTER(aItem);
>+
>+  if ((m_curHdrIndex + 1) >= m_view->GetSize())
>+    return NS_ERROR_FAILURE;
>+  nsCOMPtr <nsIMsgDBHdr> nextHdr;
>+
>+  nsresult rv = m_view->GetMsgHdrForViewIndex(++m_curHdrIndex, getter_AddRefs(nextHdr));

Hmm, can you put the blank line before the nsCOMPtr line please.

>+struct IdDWord

Why two ds?
>+{
>+  nsMsgKey    id;
>+  PRUint32    bits;
>+  PRUint32    dword;
>+  nsIMsgFolder* folder;
>+};
>+
>+struct IdKey : public IdDWord
>+{
>+  PRUint8     key[1];
>+};

Why an array of one?

>+  if (! (m_viewFlags & nsMsgViewFlagsType::kGroupBySort))
>+    return nsMsgDBView::OnHdrDeleted(aHdrDeleted, aParentKey, aFlags, aInstigator);

nit: no space after the ! please (ditto in other places)

>+nsresult nsMsgQuickSearchDBView::AddHdr(nsIMsgDBHdr *msgHdr, nsMsgViewIndex *resultIndex)
>+{
>+  if (m_viewFlags & nsMsgViewFlagsType::kGroupBySort)
>+    return nsMsgGroupView::OnNewHeader(msgHdr, nsMsgKey_None, PR_TRUE);
>+  else
>+    return nsMsgDBView::AddHdr(msgHdr, resultIndex);

No need for the else here.

>+}
>+
>+
>+NS_IMETHODIMP nsMsgQuickSearchDBView::OpenWithHdrs(nsISimpleEnumerator *aHeaders, nsMsgViewSortTypeValue aSortType,
>+                                        nsMsgViewSortOrderValue aSortOrder, nsMsgViewFlagsTypeValue aViewFlags,
>+                                        PRInt32 *aCount)

This indentation is wrong.

>-  nsresult rv = NS_MSG_INVALID_DBVIEW_INDEX;
>-  nsCOMPtr<nsIMsgFolder> folder = do_QueryElementAt(m_folders, index);
>-  if (folder)
>+  if (m_viewFlags & nsMsgViewFlagsType::kGroupBySort)
>+  {
>+    return nsMsgGroupView::OnHdrDeleted(aHdrDeleted, aParentKey, 
>+                                        aFlags, aInstigator);
>+  }
>+  else if (m_viewFlags & nsMsgViewFlagsType::kThreadedDisplay)

no need for the else here.

>+    else
>+    {
>+      // get the thread root index before we add the header, because adding
>+      // the header can change the sort position.
>+      nsMsgViewIndex threadIndex = GetThreadRootIndex(threadRoot);
>+      NS_ASSERTION(!m_levels[threadIndex], "threadRoot incorrect, or level incorrect");
>+      viewThread->AddHdr(msgHdr, msgIsReferredTo, posInThread, 
>+                         getter_AddRefs(parent)); 

nit: these two lines have unnecessary whitespace on the end (and I noticed some other lines throughout the patch like this).
Comment 41 David :Bienvenu 2008-10-20 12:04:41 PDT
>>+struct IdDWord

>Why two ds?

DWord is a double word - believe it or not, when this code was written, 32 bits really was thought of as a dword by old timers :-)

I could change it to use Uint32 in the name, at the risk of making the patch even bigger :-) but while I'm here, sure.

>+
>+struct IdKey : public IdDWord
>+{
>+  PRUint8     key[1];
>+};

Why an array of one?

It's to make the compiler happy. It's a variable length array, actually; when we allocate our IdKeys, we allocate enough memory for the key itself in the struct.

This code was heavily optimized not to fragment memory, and to avoid doing lots of little allocations as possible when sorting by strings, so it's a bit opaque. I'll add a comment.
Comment 42 David :Bienvenu 2008-10-20 13:53:07 PDT
Created attachment 343966 [details] [diff] [review]
fix addressing Standard8's comments

This fix addresses Standard8's comments, and also has a few bug fixes that were in my repo.

I'll attach a patch for the TB front end changes in a minute.
Comment 43 David :Bienvenu 2008-10-20 13:56:06 PDT
Created attachment 343968 [details] [diff] [review]
front end changes

one caveat - that previous patch should from the top level, which should make it easier to apply. I haven't compiled it, though, as I was cleaning up nsCOMPtr < throughout the patch while my tree was building...
Comment 44 Kent James (:rkent) 2008-10-20 17:56:04 PDT
Using the latest patches, I don't see the unread counts working. That is, while viewing a threaded XF view in a virtual folder, if I change unread status of a message, the unread counts do not change in the folder tree for the XF virtual folder.
Comment 45 Kent James (:rkent) 2008-10-20 20:31:49 PDT
I now see different threading in the non-XF case than I see in the normal case and XF case.

STR:

Create a multi-level reply thread on the inbox, looking like this:

test 1
  Re: test1
    Re: test1

Create both XF and non-XF saved searches on the inbox, with for example "Tags DoesntContain Important".

The Inbox and the XF search thread as shown above, that is correctly. But the non-XF search threads as:

test1
  Re: test1
  Re: test1
Comment 46 Kent James (:rkent) 2008-10-20 20:50:13 PDT
Here's more unread count problems:

1) In a threaded XF virtual folder, select a message in a thread that has several unread messages.

2) Select (Message context, ie right click)/Mark/Thread as read.
3) The messages are marked read
BUT in the original non-search folder, the unread counts do not change, and remain out of sync even if the original folder is selected. This lack of sync even survives restart. It eventually resynced, though I am not sure what caused the resync.

Another issue, though this is an edge case:

Compare the behaviour of a XF and a non-XF virtual folder to Mark/Thread as read in cases where one of the messages in the thread is not in the current search. The non-XF marks all of the messages in the thread read, even those not in the view. The XF search only marks messages that match the search criteria, so other messages in the thread remain marked as unread.
Comment 47 neil@parkwaycc.co.uk 2008-10-24 04:46:14 PDT
Comment on attachment 343966 [details] [diff] [review]
fix addressing Standard8's comments

ListIdsInThread doesn't seem to call RemoveRows?

Does AdjustReadFlag normally need to get the db from the folder e.g. don't bother if GetFolders() returns null?

I think nsMsgThreadEnumerator should use a m_nextHdrIndex member ranging from 0 to the view size.

Consider getting quicker reviews by splitting this up into separate patches. I'm not sure which changes are functional changes and which are convenience changes, such as:
* Change references into an ACString
* Add convenience functions e.g. for inserting/deleting rows
* deCOMtaminate GetFolders
Comment 48 David :Bienvenu 2008-10-24 07:39:14 PDT
(In reply to comment #47)
> (From update of attachment 343966 [details] [diff] [review])
> ListIdsInThread doesn't seem to call RemoveRows?

No, it only adds things to the view. It's called when expanding threads, or building up a view with threads expanded.
> 
> Does AdjustReadFlag normally need to get the db from the folder e.g. don't
> bother if GetFolders() returns null?

That's a thought - my recollection is that AdjustReadFlag shouldn't be needed, except when we get confused about the read state of a message, e.g., in news when the newsrc file changes out from under us.

> 
> I think nsMsgThreadEnumerator should use a m_nextHdrIndex member ranging from 0
> to the view size.

instead of starting at -1...I'll see about changing that.

> 
> Consider getting quicker reviews by splitting this up into separate patches.
> I'm not sure which changes are functional changes and which are convenience
> changes, such as:
> * Change references into an ACString
> * Add convenience functions e.g. for inserting/deleting rows
> * deCOMtaminate GetFolders

I can work on breaking out the convenience changes - they're relatively small but I guess every bit helps. You've identified the major ones, the ones I remember, anyway.
Comment 49 David :Bienvenu 2008-10-24 15:03:30 PDT
Created attachment 344689 [details] [diff] [review]
change to nsIMsgHdr references

the motivation for this is to be able to read the whole reference string, which the xf threading needs, but the patch does stand by itself...
Comment 50 neil@parkwaycc.co.uk 2008-10-24 16:08:34 PDT
Comment on attachment 344689 [details] [diff] [review]
change to nsIMsgHdr references

IMHO this patch is solving the wrong problem.
Comment 51 David :Bienvenu 2008-10-25 10:05:27 PDT
Created attachment 344755 [details] [diff] [review]
add convenience functions for manipulating view arrays

This patch adds some convenience methods for manipulating the view arrays. These will be overridden by the search db view & xf folder views in a later patch.

These lines were removed because they won't work in cross-folder views, and the same error should be handled downstream, e.g., GetThreadCount() will return an error.

-  nsCOMPtr <nsIMsgDBHdr> msgHdr;
-  rv = m_db->GetMsgHdrForKey(firstIdInThread, getter_AddRefs(msgHdr));
-  if (NS_FAILED(rv) || msgHdr == nsnull)
-  {
-    NS_ASSERTION(PR_FALSE, "error collapsing thread");
-    return NS_MSG_MESSAGE_NOT_FOUND;
-  }
-
Comment 52 neil@parkwaycc.co.uk 2008-10-25 13:24:42 PDT
Comment on attachment 344755 [details] [diff] [review]
add convenience functions for manipulating view arrays

>+void nsMsgDBView::InsertMsgHdrAt(nsIMsgDBHdr *hdr, nsMsgViewIndex index, 
>+                              nsMsgKey msgKey, PRUint32 flags, PRUint32 level)

>+void nsMsgDBView::SetMsgHdrAt(nsIMsgDBHdr *hdr, nsMsgViewIndex index, 
>+                              nsMsgKey msgKey, PRUint32 flags, PRUint32 level)
I think it would make more sense for index to be the first parameter.

>-    m_keys.RemoveElementsAt(index + 1, threadCount);
>-    m_flags.RemoveElementsAt(index + 1, threadCount);
>-    m_levels.RemoveElementsAt(index + 1, threadCount);
>+    RemoveRows(index + 1, threadCount);
ListIdsInThread also removes rows. r+sr=me with that fixed.
Comment 53 David :Bienvenu 2008-10-26 14:15:05 PDT
Created attachment 344831 [details] [diff] [review]
patch addressing Neil's comments - checked in

this is what I'll check in.
Comment 54 David :Bienvenu 2008-10-27 07:58:03 PDT
Created attachment 344901 [details] [diff] [review]
breakout some cleanup work into its own patch

this patch breaks out some of the cleanup work:

1. IdDWord -> IdUint32, and these defs moved to the .h file, which I'll need later
2. GetFolders returns a pointer to a com array, and corresponding changes, including stopping using nsISupports when we know it's an nsIMsgFolder
3. GetInsertIndexHelper allows the caller to pass in a folder array, which I'll need later.
4. small whitespace cleanup, move var decls to where they're used, fix arg name for idl methods, etc.
Comment 55 neil@parkwaycc.co.uk 2008-10-27 09:21:59 PDT
Comment on attachment 344901 [details] [diff] [review]
breakout some cleanup work into its own patch

>+            nsIMsgFolder *bottomFolder = folders->ObjectAt(bottomIndex);
>+            nsIMsgFolder *topFolder = folders->ObjectAt(topIndex);
>+            folders->ReplaceObjectAt(topFolder, bottomIndex);
>+            folders->ReplaceObjectAt(bottomFolder, topIndex);
[We could use a SwapObjectsAt method ;-)]

>+    EntryInfo2.folder = (folders) ? folders->ObjectAt(tryIndex) :  m_folder.get();
Nit: double space after :  [I also don't like (folders)]

>-#ifdef DEBUG_bienvenu
>-  NS_ASSERTION(isRead == (*msgFlags & MSG_FLAG_READ != 0), "msgFlags out of sync");
>+#ifdef DEBUG_David_Bienvenu
>+  NS_ASSERTION(isRead == ((*msgFlags & MSG_FLAG_READ) != 0), "msgFlags out of sync");
[You really need to stick with the same username across computers ;-)]

>   nsresult rv = NS_MSG_INVALID_VIEW_INDEX;
>+  if (index == nsMsgViewIndex_None || index > (PRUint32) m_folders.Count())
>+    return rv;
>+  nsCOMPtr<nsIMsgFolder> folder = m_folders[index];
Could you get away with an nsIMsgFolder* here (and other similar places)?

> NS_IMETHODIMP nsMsgSearchDBView::GetFolderForViewIndex(nsMsgViewIndex index, nsIMsgFolder **aFolder)
> {
>-  return m_folders->QueryElementAt(index, NS_GET_IID(nsIMsgFolder), (void **) aFolder);
>+  NS_ENSURE_ARG_POINTER(aFolder);
>+  NS_IF_ADDREF(*aFolder = m_folders[index]);
You need to range-check this (as per the previous method).

>+  return aFolder ? aFolder->GetMsgDatabase(nsnull, db) 
>+                 : NS_MSG_INVALID_DBVIEW_INDEX;
After range-checking, will aFolder ever be null?

>   //we want to set imap delete model once the search is over because setting next
>   //message after deletion will happen before deleting the message and search scope
>   //can change with every search.
Heh, I never knew this code existed. I dread to think what happens across accounts, but for a simple search result can you always use the root folder?

>+     if (m_uniqueFoldersSelected->IndexOf(curFolder) < 0)
>+       m_uniqueFoldersSelected->AppendElement(curFolder); 
Ooh, a trailing space ;-)
[Should m_uniqueFoldersSelected be an nsCOMArray of some sort?]

>+  nsCOMPtr <nsIMsgFolder> curFolder = m_folders[0];
>   if (curFolder)
>     GetImapDeleteModel(curFolder);
"Range" check / SafeObjectAt?
Comment 56 David :Bienvenu 2008-10-27 10:18:23 PDT
Created attachment 344936 [details] [diff] [review]
address Neil's comments - checked in

I've added range checking where it was missing, changed m_uniqueFoldersSelected to an nsCOMArray, used an nsIMsgFolder instead of an nsCOMPtr where possible, and fixed some formatting. 

>Heh, I never knew this code existed. I dread to think what happens across
>accounts, but for a simple search result can you always use the root folder?

In this code, m_folders will always have at least one folder if there were any hits, so I don't think we need to look at the root folder. Leaving aside the cross-account case :-)

I left in some of the null folder checking, just to be defensive.
Comment 57 neil@parkwaycc.co.uk 2008-10-27 10:37:54 PDT
Comment on attachment 344936 [details] [diff] [review]
address Neil's comments - checked in

>+    info->folder = (folders) ? folders->ObjectAt(numSoFar) : m_folder.get();
(folders) ;-)

>+  nsCOMPtr<nsIMsgFolder> curFolder = m_folders.SafeObjectAt(0);

>+  nsCOMPtr <nsIMsgFolder> curFolder = m_folders.SafeObjectAt(0);
Interesting spacing difference ;-) Also another possible nsIMsgFolder*

git-apply tells me that three of your new lines still end in whitespace.
Comment 58 David :Bienvenu 2008-10-27 14:12:38 PDT
Created attachment 344969 [details] [diff] [review]
[checked in] do some preparation in nsMsgGroupView

this is some preparation for cross-folder grouping - the code that will use it is in nsMsgGroupView.cpp, but teasing out those changes is a bit trickier. 

Basically, cross-folder groups will keep track of the folders of the messages, and then use the view code that knows how to get insert locations for messages based on message key and folder.
Comment 59 David :Bienvenu 2008-10-27 15:04:27 PDT
Created attachment 344988 [details] [diff] [review]
remaining diffs

this is the remaining diffs (they don't include the previous nsMsgGroupThread patch I attached earlier, and called nsMsgGroupView by mistake)
Comment 60 neil@parkwaycc.co.uk 2008-10-27 17:22:47 PDT
Comment on attachment 344936 [details] [diff] [review]
address Neil's comments - checked in

> nsMsgViewIndex nsMsgDBView::GetInsertIndexHelper(nsIMsgDBHdr *msgHdr, nsTArray<nsMsgKey> &keys,
>+                                                 nsCOMArray<nsIMsgFolder> *folders,
OK, so all the callers seem to pass GetFolders(), so why is this needed?

>+  return GetInsertIndexHelper(msgHdr, m_keys, GetFolders(), m_sortOrder, m_sortType);
Well, that's clear enough

>+    insertIndex = view->GetInsertIndexHelper(child, m_keys, nsnull, threadSortOrder, nsMsgViewSortType::byDate);
I think GetFolders() always returns null in this view

>+      nsMsgViewIndex insertIndex = GetInsertIndexHelper(newHdr, m_origKeys, nsnull,
>                       nsMsgViewSortOrder::ascending, nsMsgViewSortType::byId);

>+      threadRootIndex = GetInsertIndexHelper(rootHdr, threadRootIds, nsnull,
>+                                             nsMsgViewSortOrder::ascending, 
>+                                             nsMsgViewSortType::byId);
And also in this view
Comment 61 neil@parkwaycc.co.uk 2008-10-27 17:24:44 PDT
Comment on attachment 344969 [details] [diff] [review]
[checked in] do some preparation in nsMsgGroupView

>+   return view->GetInsertIndexHelper(child, m_keys, nsnull, threadSortOrder, nsMsgViewSortType::byDate);
And here too

>+   return view->GetInsertIndexHelper(child, m_keys, &m_folders, threadSortOrder, nsMsgViewSortType::byDate);
And here GetFolders() returns &m_folders anyway
Comment 62 David :Bienvenu 2008-10-27 19:36:21 PDT
(In reply to comment #61)
> (From update of attachment 344969 [details] [diff] [review])
> >+   return view->GetInsertIndexHelper(child, m_keys, nsnull, threadSortOrder, nsMsgViewSortType::byDate);
> And here too
> 
> >+   return view->GetInsertIndexHelper(child, m_keys, &m_folders, threadSortOrder, nsMsgViewSortType::byDate);
> And here GetFolders() returns &m_folders anyway

"this" is not the view, but the group thread object. In other words, view->m_folders is not the same as this->m_folders. "this is the group thread object, and m_folders are the folders for the messages in the group, e.g., messages from today.  So I can't just make nsMsgDBView::GetInsertIndexHelper call GetFolders on itself...
Comment 63 David :Bienvenu 2008-10-27 19:37:25 PDT
Neil, see previous comment...
Comment 64 neil@parkwaycc.co.uk 2008-10-28 06:21:45 PDT
Comment on attachment 344969 [details] [diff] [review]
[checked in] do some preparation in nsMsgGroupView

Thanks for explaining it.
Comment 65 neil@parkwaycc.co.uk 2008-10-28 06:57:06 PDT
Comment on attachment 344988 [details] [diff] [review]
remaining diffs

>   // We may have added too many elements (i.e., subthreads were cut)
>+  // ### fix for cross folder view case.
>   if (*pNumListed < numChildren)
>-  {
>-    m_keys.RemoveElementsAt(viewIndex, numChildren - *pNumListed);
>-    m_flags.RemoveElementsAt(viewIndex, numChildren - *pNumListed);
>-    m_levels.RemoveElementsAt(viewIndex, numChildren - *pNumListed);
>-  }
>+    RemoveRows(viewIndex, numChildren - *pNumListed);
I did ask about this before...
It's not like you to remove the {}s ;-)

>+// ### Can this be combined with GetIndexForThread??
It probably could, since the only difference is that one returns highIndex if the hdr was found and the other if it was not found.

>+  for (nsMsgViewIndex i = 0; i < m_keys.Length();)
>+  {
>+    // ignore non threads
>+    if (m_levels[i])
>+      continue;
This isn't going to work ;-)

A better idea would be to set up EntryInfo1 at the beginning of the loop, then start the loop at 1 looking for a thread; whenever you find a thread you set up EntryInfo2 for that thread, then save that as EntryInfo1 for the next loop, or maybe do some fancy trickery with the pValue pointers. Don't forget to free the last key at the end of the loop ;-)

> NS_IMETHODIMP
> nsMsgDBView::GetSupportsThreading(PRBool *aResult)
> {
>   NS_ENSURE_ARG_POINTER(aResult);
>-  *aResult = PR_FALSE;
>+  *aResult = PR_TRUE;
>   return NS_OK;
> }
The most important part of the patch ;-) Don't forget to remove the overrides. Does this mean we can clean up some of the UI code too?

>+  NS_ENSURE_ARG_POINTER(m_view);
Ideally this would be NS_ENSURE_STATE if you still think you need it.

>diff -uwNp8 /xfviewland/mailnews/base/src/nsMsgGroupView.cpp ./nsMsgGroupView.cpp
bbl
Comment 66 Mark Banner (:standard8) (afk until 26th July) 2008-10-29 07:47:07 PDT
Comment on attachment 344969 [details] [diff] [review]
[checked in] do some preparation in nsMsgGroupView

This was checked in a few days ago:

http://hg.mozilla.org/comm-central/rev/4275223036ad
Comment 67 Mark Banner (:standard8) (afk until 26th July) 2008-10-29 08:41:01 PDT
Comment on attachment 343968 [details] [diff] [review]
front end changes

>diff -uwrNp8 mail/base/content/commandglue.js /threadview/mail/base/content/commandglue.js

>   // now switch views
>   var oldSortType = gDBView ? gDBView.sortType : nsMsgViewSortType.byThread;
>   var oldSortOrder = gDBView ? gDBView.sortOrder : nsMsgViewSortOrder.ascending;
>   var viewFlags = gDBView ? gDBView.viewFlags : gCurViewFlags;
>+  var viewType = gDBView ? gDBView.viewType : nsMsgViewType.eShowAllThreads;
>+  var db = (gDBView) ? gDBView.db : null;
> 
>   // close existing view.
>   if (gDBView) {
>     gDBView.close();
>     gDBView = null; 
>   }

I'm thinking that 6 checks for gDBView warrants being reduced to just one.

>diff -uwrNp8 mail/base/content/searchBar.js /threadview/mail/base/content/searchBar.js

This part already seems to be in the codebase (or at least doesn't apply properly).
Comment 68 neil@parkwaycc.co.uk 2008-10-29 17:29:32 PDT
Comment on attachment 344988 [details] [diff] [review]
remaining diffs

>+NS_IMETHODIMP nsMsgQuickSearchDBView::OnHdrDeleted(nsIMsgDBHdr *aHdrDeleted, nsMsgKey aParentKey, PRInt32 aFlags,
>+                                        nsIDBChangeListener *aInstigator)
>+{
>+  return (m_viewFlags & nsMsgViewFlagsType::kGroupBySort) ? 
>+    nsMsgGroupView::OnHdrDeleted(aHdrDeleted, aParentKey, aFlags, aInstigator) :
>+    nsMsgDBView::OnHdrDeleted(aHdrDeleted, aParentKey, aFlags, aInstigator);
>+}
nsMsgGroupView already does this, no need to override it

>+  PRBool hasSelection =  mTreeSelection && mTree && (mTreeSelection->GetCount(&selCount), selCount);
I don't see the point of this (it could actually be wrong in an edge case).

sr=me with those removed and my previous comments addressed.
Comment 69 Mark Banner (:standard8) (afk until 26th July) 2008-10-30 06:36:49 PDT
Comment on attachment 344988 [details] [diff] [review]
remaining diffs

>@@ -4968,16 +4981,243 @@ PRInt32 nsMsgDBView::FindLevelInThread(n
>       // need to update msgKey so the check for a msgHdr with matching
>       // key+parentKey will work after first time through loop
>       curMsgHdr->GetMessageKey(&msgKey);
>     }
>   }
>   return 1;
> }
> 
>+  nsresult rv;
>+  PRUint16  maxLen;

nit: you only need one space here (ditto in InitEntryInfoForIndex)

>+}
>+
>+
>+NS_IMETHODIMP 
>+nsMsgQuickSearchDBView::OpenWithHdrs(nsISimpleEnumerator *aHeaders, 

nit: did you mean for two blank lines here?

>+  PRBool hasMore;
>+  nsCOMPtr<nsISupports> supports;
>+  nsCOMPtr<nsIMsgDBHdr> msgHdr;
>+  nsresult rv = NS_OK;
>+  while (NS_SUCCEEDED(rv) && NS_SUCCEEDED(rv = aHeaders->HasMoreElements(&hasMore)) && hasMore)
>+  {
>+    rv = aHeaders->GetNext(getter_AddRefs(supports));
>+    if (NS_SUCCEEDED(rv) && supports)
>+    {
>+      msgHdr = do_QueryInterface(supports);
>+      AddHdr(msgHdr); 
>+    }

I'm not totally sure you need the check for NS_SUCCEEDED twice here in the loop parameters.

>+NS_IMETHODIMP nsMsgQuickSearchDBView::SetViewFlags(nsMsgViewFlagsTypeValue aViewFlags)
>+{
>+  nsMsgViewFlagsTypeValue saveViewFlags = m_viewFlags;
>+  nsresult rv =  nsMsgDBView::SetViewFlags(aViewFlags);

nit: double space

>+NS_IMETHODIMP nsMsgSearchDBView::Open(nsIMsgFolder *folder, 
>+                                      nsMsgViewSortTypeValue sortType, 
>+                                      nsMsgViewSortOrderValue sortOrder, 
>+                                      nsMsgViewFlagsTypeValue viewFlags, 
>+                                      PRInt32 *pCount)
> {
...
>+  nsCOMPtr<nsIPrefBranch> prefBranch (do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));

nit: no bracket after prefBranch

>+    {
>+      viewThread =  static_cast<nsMsgXFViewThread*>(thread.get());
>+      thread->GetChildAt(0, getter_AddRefs(threadRoot));
>+    }

nit: double space.

>+// This method removes the thread at threadIndex from the view 
>+// and puts it back in its new position, determined by the sort order.
>+// And, if the selection is affected, save and restore the selection.
>+void nsMsgSearchDBView::MoveThreadAt(nsMsgViewIndex threadIndex)
>+{
...
>+  PRBool hasSelection =  mTreeSelection && mTree && (mTreeSelection->GetCount(&selCount), selCount);

nit: double space

> nsresult nsMsgSearchDBView::RemoveByIndex(nsMsgViewIndex index)
...
>+      nsMsgXFViewThread *viewThread =  static_cast<nsMsgXFViewThread*>(thread.get());

nit: double space

> NS_IMETHODIMP nsMsgSearchDBView::Sort(nsMsgViewSortTypeValue sortType, nsMsgViewSortOrderValue sortOrder)
> {
...
>+    if (m_viewFlags & (nsMsgViewFlagsType::kThreadedDisplay |
>+                      nsMsgViewFlagsType::kGroupBySort))
>+    {
>+      // this forgets which threads were expanded, and is sub-optimal
>+      // since it rebuilds the thread objects.  But it might be good
>+      // enough for landing.
>+      m_sortType = sortType;
>+      m_sortOrder = sortOrder;
>+      return RebuildView();

Can you make this an XXX comment and file a bug on possible perf improvement?

>+nsresult nsMsgSearchDBView::GetXFThreadFromMsgHdr(nsIMsgDBHdr *msgHdr, 
>+                                                  nsIMsgThread **pThread,
>+                                                  PRBool *foundByMessageId)
>+{
...
>+  msgHdr->GetMessageId(getter_Copies(messageId));
>+  CopyASCIItoUTF16(messageId, hashKey);
...
>+      msgHdr->GetStringReference(i, reference);
>+      if (reference.IsEmpty())
>+        break;
>+
>+      CopyASCIItoUTF16(reference, hashKey);
...
>+    msgHdr->GetSubject(getter_Copies(subject));
>+    CopyASCIItoUTF16(subject, hashKey);

I can understand Message Ids and reference being ASCII, but can you just confirm we do expect the subject to be ascii here?

>+nsresult 
>+nsMsgSearchDBView::ListIdsInThread(nsIMsgThread *threadHdr, 
>+                                   nsMsgViewIndex startOfThreadViewIndex, 
>+                                   PRUint32 *pNumListed)
>+{
>+  NS_ENSURE_ARG(threadHdr);
>+  // these children ids should be in thread order.
>+  nsresult rv = NS_OK;

rv isn't actually used in this function, expect to return at the end...

>+PRBool nsMsgXFViewThread::IsHdrAncestorOf(nsIMsgDBHdr *possibleAncestor,
>+                                          nsIMsgDBHdr *possibleDescendent)
>+{
>+  PRBool result;
>+  possibleDescendent->RefersTo(possibleAncestor, &result);
>+  return result;
>+}
>+NS_IMETHODIMP nsMsgXFViewThread::GetNewestMsgDate(PRUint32 *aResult) 

nit: missing blank line and unnecessary space at the end of the last line here.

>diff -uwNp8 /xfviewland/mailnews/base/src/nsMsgXFViewThread.h ./nsMsgXFViewThread.h

>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Please make both these numbers 2 (and in the c++ file as well.

r=me with those & Neil's comments fixed.
Comment 70 David :Bienvenu 2008-10-30 15:23:41 PDT
Created attachment 345590 [details] [diff] [review]
patch for checkin

this is what I'm planning on landing. It addresses all the review comments, I believe - I did change the ValidateSort stuff to be just #ifdef DEBUG_David_Bienvenu, since obviously we weren't hitting that code very often, and I'll just remove it once this has had wider testing.
Comment 71 David :Bienvenu 2008-10-30 15:51:58 PDT
fix checked in. We'll file follow on bugs for issues that arise. Thx for all the reviews!
Comment 72 Hartmut Figge 2008-10-31 12:13:56 PDT
(In reply to comment #71)

> fix checked in. 

After in my last self-compiled build of SM2 View-Threads->Threads with Unread and View-Threads->Threads->Watched Threads wit Unread were greyed out, i wrote a comment to de.comm.software.mozilla.nightly-builds.

A reply of Jens Hatlak, thanks, pointed to this bug as a possible cause. Backing out the patch solved the problem. Verified by an immediatedly following new build wich contained the patch and the error was back again.
Comment 73 Hartmut Figge 2008-10-31 12:23:48 PDT
(In reply to comment #72)

> After in my last self-compiled build of SM2 View-Threads->Threads with Unread
> and View-Threads->Threads->Watched Threads wit Unread were greyed out,

Should be
View->Threads->Threads with Unread and View->Threads->Watched Threads with Unread

Hopefully this time without errors. ;)
Comment 74 David :Bienvenu 2008-10-31 12:38:22 PDT
I've filed a new bug for this issue -  Bug 462555
Comment 75 David :Bienvenu 2008-11-24 08:12:25 PST
*** Bug 267727 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.