delay filtering to the point when a new item gets its remote id
ClosedPublic

Authored by mkoller on Apr 30 2017, 2:23 PM.

Details

Summary

I'm not sure about this approach, therefore I'd like comments.

What I see is troubles (duplicate mails, empty remote ids in DB) which has its root cause in timing of messages between mailfilter_agent and maildir_agent.
What this patch does is doing the mail filtering not at the very first point when a new item is added, but only after
the (maildir) resource sets the RID onto the item.
First tests are very promising. I don't get empty RIDs any longer.

I'm not using IMAP. Would this approach work there as well ?

Diff Detail

Repository
R206 KMail
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mkoller created this revision.Apr 30 2017, 2:23 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 30 2017, 2:23 PM
Restricted Application added a subscriber: KDE PIM. · View Herald Transcript
dvratil requested changes to this revision.Apr 30 2017, 2:54 PM

This isn't going to work for other resources and has much higher overhead than itemAdded.

What I would suggest is to check if the Item has an RID and if it does not then register to it to a dedicated Monitor (Monitor::setMonitoredItem) and listen for this Monitor's itemChanged signal to check for RID change and then send the Item into the filtering pipeline and unregister it from the Monitor.

This revision now requires changes to proceed.Apr 30 2017, 2:54 PM
mkoller updated this revision to Diff 14031.Apr 30 2017, 3:58 PM
mkoller edited edge metadata.

changed waiting for remote id to using a Monitor.
It's not a perfect solution since I can still get duplicate mails, but it improves the situation.

BTW: I'm unsure about the parentCollection() usage.
Can I assuem that it's valid in these cases ?
Why does then itemAdded() pass the collection explicitely ?

dvratil requested changes to this revision.EditedApr 30 2017, 4:27 PM

Looks good, just some minor details.

Regarding the parent collection being passed separately to itemAdded, I believe that's because of the semantics of the slot. When item is created, it technically does not belong to a collection yet (because the Resource did not store it there remotely yet).

Preferably you should pass the collection to filterItem() as a second argument, I'm not sure if you can rely on Item::parentCollection() in the itemAdded slot. You can use Item::parentCollection() in slotItemChanged() safely though.

For the itemMonitor, you should ensure via ItemFetchScope that the remote identifiers and ancestor are retrieved (the ancestor Retrieval policy should correspond with the the policy of the Resource's ChangeRecorder).

This revision now requires changes to proceed.Apr 30 2017, 4:27 PM
mkoller updated this revision to Diff 14034.Apr 30 2017, 5:02 PM
mkoller edited edge metadata.

set fetch scope and pass collection

knauss added a subscriber: knauss.Apr 30 2017, 10:10 PM
knauss added inline comments.
agents/mailfilteragent/mailfilteragent.cpp
208

use syntax with explicit { and } also for one-line statements.

mkoller updated this revision to Diff 14049.May 1 2017, 10:29 AM

fix coding style

dvratil accepted this revision.May 3 2017, 9:10 AM

Looks good, thanks!

agents/mailfilteragent/mailfilteragent.cpp
129

There's also a non-const Monitor::itemFetchScope() overload that returns a non-const reference, so you can do

itemMonitor->itemFetchScope().setFetchRemoteIdentification(true)

Not a big issue, you can fix it before committing if you want to.

This revision is now accepted and ready to land.May 3 2017, 9:10 AM
mkoller added inline comments.May 3 2017, 6:09 PM
agents/mailfilteragent/mailfilteragent.cpp
129

It's your code, so I follow your rules.
However from an OO point of view, I do not like classes which return non-const refs to members.
That's changing things behind the back of the owner class.
In that case the member could also just be public, which also circumvents the OO mechanism.

Please tell me if I shall do it like you proposed or if I shall commit as is.

dvratil added inline comments.May 3 2017, 6:15 PM
agents/mailfilteragent/mailfilteragent.cpp
129

You are right, it's not a good API design and it's very non-standard in Qt, but since we already have it, let's use it (consistency ftw).

Killing the API could be a nice junior-job (hinthint ;-))

woebbe added a subscriber: woebbe.May 3 2017, 6:19 PM
woebbe added inline comments.
agents/mailfilteragent/mailfilteragent.h
28

Is this include really needed or could you just forward Monitor?

BTW, thanks for working on this :-)

This revision was automatically updated to reflect the committed changes.
mkoller marked 2 inline comments as done.