492764 - nsMsgSearchDBView::OnSearchHit ignores sort

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

In nsMsgXFVirtualFolderDBView::OnSearchHit:
  else if (m_sortValid)
    InsertHdrFromFolder(aMsgHdr, aFolder);
  else
    AddHdrFromFolder(aMsgHdr, aFolder);

But in nsMsgSearchDBView::OnSearchHit, just:
  return AddHdrFromFolder(aMsgHdr, folder);

InsertHdrFromFolder is introduced in nsMsgXFVirtualFolderDBView, which explains that.  I don't see any reason why we could not push the method into nsMsgSearchDBView.  We don't want to push nsMsgXFVirtualFolderDBView::OnSearchHit too because it has some header cache stuff we don't care about.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 have nsMsgSearchDBView::OnSearchHit insert at sorted position, start out with valid sort

This does as proposed above and also marks the sort valid when the view is constructed.  (This is sane because a search view starts out with no messages.)

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

pushed http://hg.mozilla.org/comm-central/rev/dd5b6a6114c0

Comment 3

[David :Bienvenu]

hmm, I wonder if this is responsible for the fact that undo of a delete in a saved search in threaded mode doesn't put the headers back in the right place. I was hoping it would fix the problem, but now I see it was pushed a while ago...

Comment 4

[David :Bienvenu]

yes, with this patch backed out, undo delete works fine - I'll figure it out, Andrew, and let you keep working on gloda search :-)

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Interesting.  I've never looked into how the XFVF caching stuff works.  Is it possible for it to bypass the active search ordering so that the binary search goes off the rails when a real insert happens?

Assuming you're creating a unit test for the problem, I'd suggest using the
DBViewWrapper testing framework because it's more clever than my first pass at
the pure view unit test stuff.  (And a cross-folder view will need proper
async handling...)  Some unification might be in order there, dunno.

Comment 6

[David :Bienvenu]

AddHdrFromFolder for nsMsgSearchDBView will insert in order, and it does some other stuff that we need, like adding the header to the viewThread. Was there something that this patch fixed (e.g., gloda backed search views?), or was it just to push code into the base class?

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Right, gloda-backed search views cram their results in via OnSearchHit.  The results were not being inserted in a sorted fashion, and this bug was the result of an attempt to make them be inserted in a sorted fashion.

I probably looked at nsMsgXFVirtualFolderDBView::OnSearchHit first, saw the explicit check for m_sortValid, assumed that normal search views had never felt the need to sort, and propagated that logic up to nsMsgSearchDBView.  I probably concurrently or after that made the change to m_sortValid, and didn't bother to check if it worked without, as otherwise the code in XFVF::OnSearchHit would make no sense.

(The m_sortValid assignment is important because nsMsgDBView::Sort bails early and does not set m_sortValid if it is empty.  That makes it hard to use .sort to get the desired result... arguably that's a bug.)

Comment 8

[David :Bienvenu]

My vague recollection is that we usually try to build up the view, and then sort, which is cheaper than keeping the view sorted as we build it up. We might be able to init xf saved search views with the cached results immediately, and then sort, which isn't the way it works today, I don't think.

In any case, I'll probably wait for the gloda search stuff to land before fixing the regression this causes in normal saved searches w/ undo of delete, to be sure I don't break gloda-backed searches.