Bug 256003

Summary:	Virtual keyword missing from save() method in kdepimlibs/kcal/resourcecalendar.h
Product:	[Applications] kontact	Reporter:	Kumaran Santhanam <kumaran>
Component:	calendar	Assignee:	kdepim bugs <kdepim-bugs>
Status:	RESOLVED UNMAINTAINED
Severity:	major	CC:	kumaran, smartins, winter
Priority:	NOR
Version First Reported In:	unspecified
Target Milestone:	---
Platform:	Fedora RPMs
OS:	Linux
Latest Commit:		Version Fixed In:
Sentry Crash Report:

Description Kumaran Santhanam 2010-11-03 23:57:33 UTC

Version:           unspecified (using KDE 4.5.2) 
OS:                Linux

On line 107 of kdepimlibs/kcal/resourcecalendar.h:

bool save( Incidence *incidence = 0 );

should be:

virtual bool save( Incidence *incidence = 0 );

There are several derived objects (such as ResourceCached) which overload the save() method with the assumption that it is virtual.

Because of this bug, operations such as right-click Save do not work properly, since the derived virtual method is never called.  The consequence is that the user thinks data was saved when it wasn't, potentially resulting in data loss.


Reproducible: Always

Steps to Reproduce:
1. Turn on KDE debugging
2. Right-click a calendar and select Save


Actual Results:  
The overloaded save() method of ResourceCached is not called, causing the calendar not to be saved.


Expected Results:  
The overloaded save() method of ResourceCached should be called, immediately saving the calendar.

This is a quick fix that would be much appreciated for the next point release.

Comment 1 Allen Winter 2010-11-04 00:20:01 UTC

I'm pretty sure this is a Binary Incompatible change -- so we cannot implement your suggestion in KDE 4.5 or below.  

In KDE 4.6, we have deprecated the kcal library in favor of the kcalcore library, and kresources are history in favor of Akonadi.

So I don't think we can do what as you suggest.

Comment 2 Kumaran Santhanam 2010-11-04 00:58:55 UTC

I believe Akonadi still has compatibility for legacy kresources, so this would still be valid.  There are many resources that are still not production-ready in Akonadi, such as the KCalDAV resource (http://code.google.com/p/kcaldav).

Akonadi has been touted for years as the next best thing, but unfortunately it is maturing very slowly.  Since this change won't break anything going forward, can we just update it for 4.6 so that kresources can work better while Akonadi has its bugs ironed out?

Comment 3 Sergio Martins 2010-11-04 16:13:04 UTC

>> "Since this change won't break anything going forward"

But will break existing applications (3dparty or not) that link against kdepimlibs.

Please read http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++
about binary compatibility.

We must ensure BC for kdepimlibs and kdelibs.

Comment 4 Kumaran Santhanam 2010-11-04 16:35:52 UTC

I understand the BC issues, but what is the policy regarding BC across major revisions?  This is a long-standing bug that was recently uncovered when I was debugging some resources.  It revealed that several plugins are broken because they require overloading of the save() method.  What would be your proposal for how to fix this problem?  Will kcalcore obsolete this issue?

Comment 5 Sergio Martins 2010-11-04 17:17:56 UTC

(In reply to comment #4)
> I understand the BC issues, but what is the policy regarding BC across major
> revisions?
You can break in KDE 5 only afaik.

  This is a long-standing bug that was recently uncovered when I was
> debugging some resources.  It revealed that several plugins are broken because
> they require overloading of the save() method.  What would be your proposal for
> how to fix this problem?  
Maybe using a dynamic cast in ResourceCalendar::save() and calling the ResourceCached one?

It's really hugly and will only fix it for ResourceCached, but it's the only thing i can thing of.



> Will kcalcore obsolete this issue?
KCalCore doesn't use kresources.

Comment 6 Kumaran Santhanam 2010-11-04 18:45:50 UTC

Sergio's idea sounds like a creative solution to the problem.  I agree that it is ugly and would not implement it except for the strict BC constraints.

Since many objects derive from ResourceCached, a dynamic cast would capture a majority of situations.  The important thing is to comment it very well for future reference.

Since this won't break BC, can we get a fix in for the next point release?  If I can get a commitment from the KDE team, I volunteer to implement the fix.

Comment 7 Denis Kurz 2016-09-24 19:29:44 UTC

This bug has only been reported for versions before 4.14, which have been unsupported for at least two years now. Can anyone tell if this bug still present?

If noone confirms this bug for a Framework-based version of kontact (version 5.0 or later, as part of KDE Applications 15.08 or later), it gets closed in about three months.

Comment 8 Denis Kurz 2017-01-07 22:26:39 UTC

Just as announced in my last comment, I close this bug. If you encounter it again in a recent version (at least 5.0 aka 15.08), please open a new one unless it already exists. Thank you for all your input.