Closed Bug 379806 Opened 17 years ago Closed 16 years ago

threaded/grouped-by-sort views unavailable in saved searches across multiple folders

Categories

(Thunderbird :: Mail Window Front End, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: bugzilla.overbored, Assigned: Bienvenu)

References

(Depends on 1 open bug, )

Details

(Keywords: polish)

Attachments

(6 files, 8 obsolete files)

5.82 KB, patch
standard8
: review+
Details | Diff | Splinter Review
13.13 KB, patch
Details | Diff | Splinter Review
47.53 KB, patch
neil
: review+
Details | Diff | Splinter Review
7.12 KB, patch
neil
: review+
Details | Diff | Splinter Review
135.00 KB, patch
standard8
: review+
Details | Diff | Splinter Review
163.34 KB, patch
Details | Diff | Splinter Review
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.
Yes... see bug 263180 comment 8. Confirming RFE.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
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.
(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..."
Version: unspecified → Trunk
This would really vastly improve the usefulness of virtual folders...
Assignee: mscott → nobody
Flags: blocking-thunderbird3?
Blocks: 272759
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.
The severity is low now (enhancement). I would like to ask to handle this bug with greater priority.
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?
Keywords: polish
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.
What is the present status of this. Coming from evolutino, search folders are of extremely little use to me without threading support.
I'm thinking of trying this...
Assignee: nobody → bienvenu
see http://wiki.mozilla.org/Thunderbird:Threading_Cross-folder_Saved_Searches for some implementation thoughts.
Status: NEW → ASSIGNED
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.
Attached patch more wip (obsolete) — Splinter Review
this gets grouping starting to work in cross-folder saved searches, and paves the way for threading in same.
Attachment #332609 - Attachment is obsolete: true
Triaging according to new policy for flags.  
https://wiki.mozilla.org/Thunderbird:Release_Driving
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3-
Priority: -- → P3
Target Milestone: --- → Thunderbird 3.0b2
I think asuth is hoping to use this work with gloda and experimental views, so I'm bumping up the priority. 
Priority: P3 → P1
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.
Attached patch more wip (obsolete) — Splinter Review
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.
Attachment #333088 - Attachment is obsolete: true
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
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.
quick sort is working now...
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.
(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.
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.
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...
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?
(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.
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.
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.
(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.
Attached patch patch for review (obsolete) — Splinter 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.
Attachment #341176 - Attachment is obsolete: true
Attachment #342170 - Flags: review?(bugzilla)
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);
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.
I'm inclined to disable things like view | threads with unread for xf saved searches for the time being.
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...)
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.
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.
Whiteboard: [needs review standard8]
Attachment #342170 - Flags: review?(bugzilla) → review-
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).
>>+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.
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.
Attachment #342170 - Attachment is obsolete: true
Attachment #343966 - Flags: superreview?(neil)
Attachment #343966 - Flags: review?(bugzilla)
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...
Attachment #343968 - Flags: review?(bugzilla)
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.
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
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 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
(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.
Attached patch change to nsIMsgHdr references (obsolete) — Splinter Review
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...
Attachment #344689 - Flags: superreview?(neil)
Attachment #344689 - Flags: review?(neil)
Attachment #344689 - Flags: superreview?(neil)
Attachment #344689 - Flags: review?(neil)
Comment on attachment 344689 [details] [diff] [review]
change to nsIMsgHdr references

IMHO this patch is solving the wrong problem.
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;
-  }
-
Attachment #344755 - Flags: superreview?(neil)
Attachment #344755 - Flags: review?(neil)
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.
Attachment #344755 - Flags: superreview?(neil)
Attachment #344755 - Flags: superreview+
Attachment #344755 - Flags: review?(neil)
Attachment #344755 - Flags: review+
this is what I'll check in.
Attachment #344755 - Attachment is obsolete: true
Attachment #344831 - Attachment description: patch addressing Neil's comments → patch addressing Neil's comments - checked in
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.
Attachment #344901 - Flags: superreview?(neil)
Attachment #344901 - Flags: review?(neil)
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?
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.
Attachment #344901 - Attachment is obsolete: true
Attachment #344936 - Flags: superreview?(neil)
Attachment #344936 - Flags: review?(neil)
Attachment #344901 - Flags: superreview?(neil)
Attachment #344901 - Flags: review?(neil)
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.
Attachment #344936 - Flags: superreview?(neil)
Attachment #344936 - Flags: superreview+
Attachment #344936 - Flags: review?(neil)
Attachment #344936 - Flags: review+
Attachment #344936 - Attachment description: address Neil's comments → address Neil's comments - checked in
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.
Attachment #344969 - Flags: superreview?(neil)
Attachment #344969 - Flags: review?(neil)
Attached patch remaining diffsSplinter Review
this is the remaining diffs (they don't include the previous nsMsgGroupThread patch I attached earlier, and called nsMsgGroupView by mistake)
Attachment #344988 - Flags: superreview?(neil)
Attachment #344988 - Flags: review?(bugzilla)
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 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
(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...
Neil, see previous comment...
Attachment #344969 - Flags: superreview?(neil)
Attachment #344969 - Flags: superreview+
Attachment #344969 - Flags: review?(neil)
Attachment #344969 - Flags: review+
Comment on attachment 344969 [details] [diff] [review]
[checked in] do some preparation in nsMsgGroupView

Thanks for explaining it.
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 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
Attachment #344969 - Attachment description: do some preparation in nsMsgGroupView → [checked in] do some preparation in nsMsgGroupView
Attachment #343966 - Attachment is obsolete: true
Attachment #343966 - Flags: superreview?(neil)
Attachment #343966 - Flags: review?(bugzilla)
Attachment #344689 - Attachment is obsolete: true
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).
Attachment #343968 - Flags: review?(bugzilla) → review+
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.
Attachment #344988 - Flags: superreview?(neil) → superreview+
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.
Attachment #344988 - Flags: review?(bugzilla) → review+
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.
Whiteboard: [needs review standard8] → will checkin when tree is green
fix checked in. We'll file follow on bugs for issues that arise. Thx for all the reviews!
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(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.
(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. ;)
I've filed a new bug for this issue -  Bug 462555
Depends on: 462581
Depends on: 462637
Depends on: 462638
Depends on: 465011
Depends on: 465018
Depends on: 465024
No longer blocks: 272759
Whiteboard: will checkin when tree is green
Depends on: 539784
You need to log in before you can comment on or make changes to this bug.