[Kde-pim] mailfilter does not see all incoming messages

[Andras Mantia]

Hi,

Wolfgang Rohdewald wrote:

> Am Mittwoch, 13. Februar 2013, 10:34:46 schrieb Wolfgang Rohdewald:
>> my filter for incoming mail does not get noticed of all new mails.
>> if I get 10 mails (sent by myself from somewhere else), between 0 and 3
>> remain in inbox. The ignored mails are always some of the last ones,
>> the first 5 mails always get through to mailfilter.
> 
> what happens:
> 
> MonitorPrivate::dispatchNotifications() deqeues a msg
> from pendingNotifications
> 
> This emits itemChanged
> processing that signal indirectly calls ChangeRecorder::changeProcessed()
> which dequeues a message from pendingNotifications. This is the
> lost message. This only happens sometimes - only if pendingNotifications
> is not empty.
> 
> There are two dequeues for one message, so one of them is too much.
> 
> I now removed one call to changeProcessed() from mailfilteragent
> by overriding
> AgentBase::Observer::itemChanged (which does call changeProcessed)
> with an empty version which does not call changeProcessed.

Congratulations! Your analysis looks good and makes a lot of sense. To 
explain it... Changerecorders can behave in two ways:
- ChangeRecorder mode
- Monitor mode

The main difference is how much do you control when is the next change 
processed (changes are basically notifications of operations coming from the 
Akonadi server that the resource has to process, like itemChanged, 
itemMoved, collectionAdded, etc.). Each Akonadi agent (and resource) has a 
change recorder object, accessible by changeRecorder(). 

setChangeRecordingEnabled() controls how the changeRecorder behaves. In 
Mailfilter agent, we call it with false(), technically turning the 
changeRecord in a Monitor. In Monitor mode, there is no need to manually 
call changeProcessed() (and thus replayNext()). But the default 
implementation of the Agent does call it, thus we need to override it, in 
order to be not called.

So as I see the problem was that *if* there was an change job running for 
any reason (e.g some filters, like spam filtering change the content of a 
mail and store in the server), that caused to switch to the next change in 
the log, thus "losing" (not processing) one change.

> This seems to work. Patch attached. Please review - if OK, I will commit
> to 4.10 and merge to master.

I will test, but the code looks pretty ok for me. :)
Nice job, thank you.

> But then I do not understand some comments in changerecorder.cpp
> like
>       // The msg is now in both pipeline and pendingNotifications.
>       // When data is available, MonitorPrivate::flushPipeline will
>       emitNotification. // When changeProcessed is called, we'll finally
>       remove it from pendingNotifications.
> 
> it seems to me that the API of Monitor and ChangeRecorder can easily be
> misunderstood and wrongly used. The API doc certainly does not say that
> slots like itemAdded must call changeProcessed.

They need to call if change recording is enabled, otherwise we don't get the 
next change. This is what is hard to get right in changerecorder mode, when 
you can have codepaths not calling changeProcessed(), and you end up with 
the resource stopping to do anything, as it doesn't get the next item.


> But then the default
> implementations in AgentBase::Monitor::itemAdded etc do call it. Which one
> is correct? If the slots should not call changeProcessed, who should in
> light of the comment above?

I hope my explanation made it clear why the default implementation has the 
changeProcessed() in there (because the default behavior is to use a real 
changerecorder, not a monitor).

Andras
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/