Differential D1702

Notify Observers before removing event from Akonadi::CalendarBase
ClosedPublic
Actions

Authored by dvratil on May 29 2016, 11:31 AM.

Details

Reviewers
smartins
Commits
R170:cbfa28102c38: Notify Observers before removing event from Akonadi::CalendarBase
Summary

KCalCore::Calendar::CalendarObserver gets notified via calendarIncidenceAboutToBeDeleted() about incidence being removed from a KCalCore::Calendar. If the incidence is removed from Akonadi::CalendarBase internal hashes before calling MemoryCalendar::deleteIncidence(), then the observer no longer has any means how to resolve the incidence-about-to-be-removed back to respective Akonadi::Item. This patch moves the call to MemoryCalendar::deleteIncidence() to happen before the incidence is removed from CalendarBase internal hashes, so that observers can still query the calendar about the event from the calendarIncidenceAboutToBeDeleted() callback.

The comment in CalendarBase says, that the call to KCalCore::MemoryCalendar::deleteIncidence has to be called last due to reentrancy, however looking at the code in CalendarBasePrivate::internalRemove() and looking at the callers of that method, the position of the call does not really seem to have any effect on reentrancy of the method. Sergio, any ideas what the comment meant?

Diff Detail

Repository
R170 Akonadi Calendar
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dvratil updated this revision to Diff 4035.May 29 2016, 11:31 AM
dvratil retitled this revision from to Remove event from KCalCore::MemoryCalendar before removing it from Akonadi::CalendarBase.
dvratil updated this object.
dvratil edited the test plan for this revision. (Show Details)
dvratil added a reviewer: smartins.
dvratil set the repository for this revision to R170 Akonadi Calendar.
dvratil added a project: KDE PIM.
Restricted Application added a subscriber: kde-pim. · View Herald TranscriptMay 29 2016, 11:31 AM
smartins edited edge metadata.May 29 2016, 9:36 PM

iirc, MemoryCalendar::deleteIncidence() triggers observer code, which is external to us, so can do anything, even try to delete the incidence again. There was a bug that was fixed this way, no idea which, very long time ago :P

I think a cleaner approach would be for CalendarBase to call Calendar::notifyIncidenceAboutToBeDeleted() before hash cleanup and notifyIncidenceDeleted() after MemoryCalendar::deleteIncidence() (and some way for MemoryCalendar::deleteIncidence() not call those)

dvratil updated this revision to Diff 4042.May 29 2016, 11:31 PM
dvratil retitled this revision from Remove event from KCalCore::MemoryCalendar before removing it from Akonadi::CalendarBase to Notify Observers before removing event from Akonadi::CalendarBase .
dvratil edited edge metadata.
dvratil removed R170 Akonadi Calendar as the repository for this revision.

Thanks! The idea with notifying the observers from CalendarBase is indeed the right approach, luckily we already have all the API we need for that - updated the review :)

smartins accepted this revision.May 30 2016, 9:07 AM
smartins edited edge metadata.
This revision is now accepted and ready to land.May 30 2016, 9:07 AM
Closed by commit R170:cbfa28102c38: Notify Observers before removing event from Akonadi::CalendarBase (authored by dvratil). · Explain WhyMay 30 2016, 7:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/calendarbase.cpp (6 lines)

Diff 4079

View Options

src/calendarbase.cpp

Loading...