444815 - View unexpectedly removes message after changing tag

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[Kent James (:rkent)]

This is a followup to bug 427428, and assumes that lands.

In nsMsgQuickSearchDBView::OnHdrPropertyChanged there is legacy code that tries to detect if a message is newly classified as junk:

  if (!match && originStr.get()[0] == 'p' && flags & MSG_FLAG_NEW)
    RemoveByIndex(index); // remove hdr from view

But this can also match if a user changes the tag of a new message, so that the message no longer is in the search view. It's particularly annoying when a delayed unread is in effect, because if you change the tag to remove an item from the view, most (read) messages stay in the view, but if you're too quick and the message is still unread (and hence also new) then the message disappears from the view.

Proposed fix: Use the new PreChange functionality to truly detect if this is a new junk message, and not guess using the new flag.

Comment 1

[Kent James (:rkent)]

Sorry, wrong depends on bug.

Comment 2

[Kent James (:rkent)]

Attachment: Use pre-change to correctly detect new plugin state

Let me describe more precisely the problem this is supposed to solve. The annoying case is where there is a delayed mark-as-read, so sometimes a tagged message is read, sometimes not. But I will describe the STR with automatic marking of read off as that is easier to reproduce.

STR:

1) Setup a virtual search folder on the Local inbox, search Tag DoesntContain Later, called NotLater.
2) Ensure that automatic marking as read is non-functional, for example by turning off the message pane.
3) Make sure that junk mail processing is enabled and active for incoming messages.
4) Send yourself 2 messages, and view them as New from the NotLater folder. This requires that you have the NotLater folder open when the new message is received.
5) Mark the first message new.
6) Tag both messages Later.

Expected results: both messages stay in the view

Actual results: the unread message is removed from the view, while the read message stays in the view.

I actually don't think the expected result is ideal, but it is what is currently intended for the application. Bug 430847 proposes that some sort of indication is given that the message no longer matches, such as the strikeout that is used for IMAP deleted messages. 

The original code section is designed to immediately remove from view messages that are newly classified by the junk filter. So you could repeat the STR, but use Junk Status Is Junk instead of Tagged Later, and then the expected behaviour (after tagging as junk) would be that the message is removed from the view.

This patch modifies the expected behaviour slightly, in that messages in the view that are newly marked as junk but not NEW (typically by running the junk filter manually) will now be removed from the view, while previously they would have stayed in the view. This is a fairly rare situation (particularly since running the junk filter manually was broken in TB 2.0), and in any case I think that the behaviour should not depend on whether the message was new.

Comment 3

[David :Bienvenu]

+  nsMsgViewIndex index = FindHdr(aHdrChanged);
+  if (index == nsMsgViewIndex_None) // message does not appear in view
     return NS_OK;

What happens if the hdr property change means that the header *should* appear in the view?

Comment 4

[Kent James (:rkent)]

(In reply to comment #3)
> +  nsMsgViewIndex index = FindHdr(aHdrChanged);
> +  if (index == nsMsgViewIndex_None) // message does not appear in view
>      return NS_OK;
> 
> What happens if the hdr property change means that the header *should* appear
> in the view?
> 

My understanding is that index == nsMsgViewIndex_None would always be true, so the hdr property change does not add the message to the view. I believe this is the current behavior, and assummed it was the expected behavior. I'm not sure it is the ideal behavior, though. But I would consider that a different bug.

You can see this by setting up a saved search with criteria Junkscoreorign Is Plugin. When new messages come in and are classified, and you have the Plugin saved search open, new messages are not added until you leave and re-enter the view.

Comment 5

[David :Bienvenu]

Comment on attachment 330346 [details] [diff] [review]
Use pre-change to correctly detect new plugin state

OK, can you note the bug in a comment in the code, so it'll be a teeny bit easier for someone to fix it?

And can you add some punctuation to these comments:

+    // first call pre-change

+  // second call post-change

maybe first call - pre-change.

Otherwise it's a bit ambiguous, the other meaning being "first we're going to call pre change"

r/sr=me with those nits.

Comment 6

[Kent James (:rkent)]

Attachment: Fixed nits, patch to checkin

Fixed niots. Carrying over Bienvenu's reviews, patch to checkin.

In testing this patch, I noticed that the following line from comment 2:

"5) Mark the first message new."

should be

"5) Mark the first message read."

Comment 7

[Mark Banner (:standard8)]

Checked in, changeset 56:9dcf6fd54ed9.