439380 - Unread counts incorrect after IMAP filter move

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Kent James (:rkent)]

When moving a message from an IMAP folder during a filter operation, the unread count is off by one (one less unread is indicated than is present) This is a regression that is not present in the 2008-06-13 nightly, but is present in 2008-06-14. Likely cause: bug 435153

STR:

Create an IMAP account, with an inbox and a subfolder (called TEST). Add at least one message to the INBOX folder, and mark it as unread. Create a message filter for the IMAP account that moves a message from the INBOX to the subfolder TEST if, for example, the subject contains "moveme".

Send a message to the IMAP account that matches the filter. The message moves to the TEST folder.

Expected result: unread count on INBOX remains unchanged.

Actual result: unread count on INBOX is decremented by one.

Comment 1

[Magnus Melin [:mkmelin]]

<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=ThunderbirdTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-06-13&maxdate=2008-06-14+05%3A00%3A00&cvsroot=%2Fcvsroot>

Comment 3

[David :Bienvenu]

the issue is that this code:

http://mxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapMailFolder.cpp#4336

which is supposed to update the pending counts, doesn't do anything if m_copyState is null.

http://mxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapMailFolder.cpp#4622

and that happens when playing back offline moves.

One way to fix this is to make filter move playback not go through the offline playback code (it's not obvious to me that filter move playback is the same kind of beast as the user pressing delete or doing a drag drop, and that starting the playback of filter moves .5 seconds later is going to make things seem faster, or be faster).

An other option is to figure out how to update the pending counts in this situation, though it's tricky because the source headers have already been deleted from the database. 

A third option is to have the offline recording of the filter move playback fix the pending counts. I'm surprised that doesn't work, but perhaps I broke it when fixing an other bug with pending counts and offine recording/playback.

Comment 4

[David :Bienvenu]

Actually, that wasn't the issue - the unread count should be handled by the fact that we have a fake offline unread header in the db. But what's happening is that nsImapMoveCoalescer, when the move finishes, tries to update the destination folder - http://mxr.mozilla.org/mozilla/source/mailnews/base/util/nsImapMoveCoalescer.cpp#258 . That causes us to Select the folder, and remove the fake header, which decrements the counts. But for some reason, we're not downloading the new header, until the next time you select the folder.  I thought it could be that the fake header's UID is the same as the new header, but I tweaked it so that wouldn't happen, but I still have the issue.  I'll debug some more.

Comment 5

[David :Bienvenu]

d'uh - so the problem is that we're doing the filter move pseudo-offline, which means that we've essentially just delayed it for 1/2 second, which means that trying to update the target folder of the move, as the imap move coalescer tries to do, is pointless because in fact, the move hasn't actually happened online yet.

One reason we were updating the target folder is to give the junk mail filters a chance to run on the new mail target folder, so I'm reluctant to remove that call. It's hard for the imap move coalescer to know when the online operation actually happens, since we pretend that it's already happened.

So I'm still left thinking that doing these filter actions pseudo-offline is a lot more trouble than its worth. It's actually likely going to delay the user from being able to interact with their inbox, instead of speeding it up.

Comment 6

[Emre Birol]

David, when the user runs a filter on a 10000 message mailbox, pseudo-offline batching would provide better experience/faster response to the user, do you think? I am just curious.

Comment 7

[David :Bienvenu]

So my belief is that the reason pseudo-offline is so nice for deleting/moving messages is that it allows the user to read the next message before the delete actually happens. And it makes the delete look faster.

The filter case is different - first of all, with imap filters on incoming mail, the user never sees the messages show up in the inbox - we run the filters first. So it's not as if they show up in the inbox and then get moved to their target folder. And it's not as if the user can do anything else with the inbox when the process of updating the inbox, downloading new headers, and applying filters to the new headers is going on. By delaying briefly the application of the message filters, we would actually make the process look slower, and delay the user from being able to do things with their inbox. And we'd be doing more work, because we'd do the offline operation, followed by the online operation.

The size of the imap folder is most likely irrelevant - the copy happens on the server. The number of headers downloaded is much more relevant, but if we're downloading 10,000 headers, the user has other problems.

If we were to rework things so that the filter moves happen long after the new mail is downloaded and the folder syncing is done (e.g, 10 or 20 seconds), then you could make things appear faster, and the user could do something useful like process new messages before the playback happens. But you'd have to fix this bug first, and figure out a way to allow the client code that's trying to copy the messages to control when offline playback is initiated, which would need to be propagated through the copy service, into the offline recording and playback operations. You'd also run into the issue that if things fail, it might be confusing for the user to find out about it so much later...

I think we've got much more low hanging fruit before we tackle this.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

This makes undoable actions go through the pseudo-offline playback, but others through normal operation, the idea being that undoable actions are user-initiated. This fixes this bug, and might fix some bugs we haven't discovered yet :-)

The alternative is enhancing the copy service in ways that would propagate throughout the code, and I'd want to think a while before doing that.

Comment 9

[Emre Birol]

I was wondering what would happen in term of UX in the edge-case where the user runs the filter on a folder explicitly. Sorry I should have been more clear. 

Comment 10

[David :Bienvenu]

(In reply to comment #9)
> I was wondering what would happen in term of UX in the edge-case where the user
> runs the filter on a folder explicitly. Sorry I should have been more clear. 
> 
Oh, sorry - that happens to be completely different code so very little of what I said was relevant :-) The size of the folder doesn't matter w.r.t. to playing back the events offline or not, however, because the size of the folder just affects the time it takes to figure out which messages match. But you're right that applying the filter hits offline first and then playing them back would look more responsive. To do this, we would still need to allow users of the copy service to specify if a copy should be done online or offline (or we could make move/copy filter actions undoable).

Comment 11

[David :Bienvenu]

fix checked in.