previous fix in monitoredItemRemoved() was incorrect
ClosedPublic

Authored by mkoller on May 4 2017, 6:49 PM.

Details

Summary

In one of my previous commits, I added the lines you see in the diff to avoid the problem with lazy population and the warning.

However I have seen that when I move 2 or more mails with kmail into a different folder, then this method is called with an item
which already has the NEW (target) collection as parentCollection set and then the check seems wrong.
What happens is that this method returns but leaves the items in the old collection and kmail still shows them.

I'm not really sure about how this works here, but at least removing the if-return block again solves the issue and kmail no longer
shows the already moved mails in the old folder.

(I did not analyze why a single mail move did work)

Diff Detail

Repository
R165 Akonadi
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.May 4 2017, 6:49 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptMay 4 2017, 6:49 PM
Restricted Application added a subscriber: KDE PIM. · View Herald Transcript
dvratil edited edge metadata.May 5 2017, 8:20 AM

The difference comes likely from the Monitor. Better solution (which allows us keeping the if () check) is to pass the parent collection as a second argument to monitoredItemRemoved() (with Collection() as default value) - when called from monitoredItemMoved, you pass the sourceCollection, when called from elsewhere, you won't pass anything and then you just check if the collection is valid and use it in the if(), or fallback to item.parentCollection().

mkoller updated this revision to Diff 14162.May 5 2017, 3:49 PM

pass parentCollection explicitely on move

dvratil requested changes to this revision.May 5 2017, 11:14 PM

Phab seems to be unable to display proper context for any of your patches, how are you generating/publishing them?

src/core/models/entitytreemodel_p.cpp
4

This won't work - just exclude the Akonadi::Collection parameter here and Qt will correctly use the default parameter value for the collection.

23

Not sure if it's Phabricator, but the patch looks incomplete or broken - there seems to be missing chunk here.

This revision now requires changes to proceed.May 5 2017, 11:14 PM
mkoller updated this revision to Diff 14186.May 6 2017, 8:46 AM
mkoller edited edge metadata.

I just use "git diff > diff" and upload the file.

dvratil accepted this revision.May 6 2017, 9:11 AM

Hm, weird that Phab does not show context properly then. Anyway, thanks for the fix, I can confirm it fixes the mass-move issue.

This revision is now accepted and ready to land.May 6 2017, 9:11 AM
This revision was automatically updated to reflect the committed changes.