[Kde-pim] Review Request: CalendarResources::endChange() only saves when an incidence was added or modified, not when deleted

[Kevin Krammer]

> On 2009-02-15 14:17:29, Volker Krause wrote:
> > Good catch!
> 
> Kevin Krammer wrote:
>     I am kind of missing (or not seeing) the place where the new variable is initialized
> 
> Sergio Martins wrote:
>     It is set to false in beginChange(), I was going to put it only in the constructor, but what happens if deleteEvent() returns an error? Will endChange() still get called (and clear the variable)?.

In beginChange() is definitely necessary, but I think it is always good to initialize members to a known value.
Actually, at work we have a static code analyzer that complains about that. Calls it a Severe Violation :)


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/96/#review123
-----------------------------------------------------------


On 2009-02-15 14:01:58, Sergio Martins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/96/
> -----------------------------------------------------------
> 
> (Updated 2009-02-15 14:01:58)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> When an incidence is added or modified CalendarResources::endChange() saves it, but when one is deleted it doesn't because it exits immediately with an error.
> 
>   ResourceCalendar *r = resource( incidence );
>   if ( !r ) {
>     return false;
>   }
> 
> It doesn't know which resource the incidence belongs to because deleteEvent() already removed it from the mapping.
> 
> Not only it doesn't save, it also doesn't clear the lock created by beginChange(), so the next change won't be saved either, even if it's a move or add, (it will only save when KOrganizer exits).
> 
> This fix will allow the "save after 5 second timeout" workaround to be removed from ResourceAkonadi because now save is called after a delete.
> 
> This review is related to Kevin's http://reviewboard.kde.org/r/94/ and probably should go in the same commit.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepimlibs/kcal/calendarresources.h 926606 
>   trunk/KDE/kdepimlibs/kcal/calendarresources.cpp 926606 
> 
> Diff: http://reviewboard.kde.org/r/96/diff
> 
> 
> Testing
> -------
> 
> Added, modified, deleted incidences.
> Unchecked/checked the resource to see if the changes were permanent.
> 
> 
> Thanks,
> 
> Sergio
> 
>

_______________________________________________
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/