[Kde-pim] Review Request: Maemo fixes in Kcal

[Kevin Krammer]

> On 2009-10-15 14:45:03, Allen Winter wrote:
> > there are some minor problems (like forgetting the @since 4.4 apidox tags)
> > which I fixed locally.
> > 
> > there is a major problem of binary incompatibility by adding the incidenceUpdate() pure virtual.
> > Not quite sure how to deal with 
> > the other questions is: would you be willing to use Person objects with the new Contacts methods instead of QStrings?
> 
> Alvaro Manera wrote:
>     And what is your proposal to get rid of the binary incompatibility? Because I don't want to wait to kde5 to get it in. :)
> 
> Kevin Krammer wrote:
>     One option is to create an IncidenceObserverV2 which inherits IncidenceObserver and has this additional method.
>     Then either registerObserver() or update() need to decide which of one of them is the base for the observer at hand.
>
> 
> Allen Winter wrote:
>     Alvaro, I think we should trying using signals and slots as described in http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Adding_new_virtual_functions_to_leaf_classes "Using signals instead of virtual functions".  
>     
>     Would you want to investigate doing that? 
>     
>     Else, we can commit Contacts stuff now only and also we need to deal with the recurrenceID stuff.
> 
> Alvaro Manera wrote:
>     We cannot follow the signal slot approach. The new virtual is in the IncidenceObserver class. We need to add the inheritance to QObject. And even if we could, Calendar class inherits from it (and also from QObject). We will need to change the inheritance order again. 
>     This way we cannot fix the ABI break.
>     The other option is to create the V2 class. And add the additional method. But this is valid for me, KDE will have the fix in code, but no possibility of use it. Because to do it, the inheritance change in Calendar will be needed.
>     Any other ideas?
> 
> Kevin Krammer wrote:
>     The private can inherit the new interface. it is not an exported class
> 
> Alvaro Manera wrote:
>     Yes, but the classes we are talking about are not private.
>     IncidenceObserver (the one with the new virtual method) is public. And not only that, but it is also inherited by Calendar class. That is what I meant with my other comment, even if it is not very clear.
>     
>     The other solution proposed to create the new V2 class, wouldn't bring the bug fix to KDE's KCal and only to Maemo (and other new users).
> 
> Kevin Krammer wrote:
>     Obviously one can't add new virtuals to the old interface. I thought we already agreed on that?
>     
>     I don't understand why introducing the new class does not fix KDE's KCal.
>     Isn't this approach the very reason to fix the situation while retaining binary compatibiliy for already published APIs?
> 
> Alvaro Manera wrote:
>     I think we are having a misunderstanding. Let me try to explain it better.
>     
>     Yes, we both agree that we cannot add a new virtual without breaking ABI. Two solutions were proposed:
>     
>     1) Use the signal/slot workaround
>     2) Create a new V2 class that inherits from the one that cannot be touched.
>     
>     The first one, I hope I have been already clear enough that it cannot be done. Adding signal/slot requires changing the inheritance of a public class (really it is two). So the fix doesn't fix anything. We will still have ABI break but with uglier code.
>     
>     The second one, it can be done partially. I can create the new V2 class that has the new method. And in Maemo we can use it without a problem. But not you. The class we are talking about is the "father" of other class, eg Calendar. This one cannot be modified to inherit from this new V2 class because we will break ABI again. So if we follow this approach we will end up with some new class that cannot be used by KDE, plus Maemo having to maintain the "branched" code using this new functionality. Unless of course we create also a CalendarV2 that uses the new inheritance, but a good chunk of code needs to be duplicated. And even if it is, KDE won't benefit from it.
>     
>     I hope this time it is clear enough what are the problems with the ABI break. And I am open to new proposals/ideas on how to integrate it.

Sorry for the misunderstanding, I'll probably didn't include enough context in my other reply.

Obviously we can't rebase Calendar because this would be BIC as well.

However, the public inheritance from IndicenceObserver is actually not required, it is basically just a convenience trick.
Nobody registers a calendar as an incidence observer other than the calendar itself, actually not even Calendar itself but two of its subclasses (CalendarLocal and CalendarResources).

Both of them have d-pointers, private, non-exported classes.
These privates can be changed anyway we want, we can make them inherit IncidenceObserver2.
We can make the two calendars register their private instead of themselves, forwarding the incidenceUpdated() to the parent object for the very unlikely case that somebody subclassed one of them (lxr.kde.org says no).

Anyone having their own Calendar subclass can do the same if they want the additional information provided by the new method.


- Kevin


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


On 2009-10-21 14:48:51, Alvaro Manera wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1489/
> -----------------------------------------------------------
> 
> (Updated 2009-10-21 14:48:51)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> This is a combined patch of all the changes we have in our trunk, before the implementation of the RecurrenceID.
> 
> These changes include:
> * Contacts in the incidence. (I don't know if you need this, as you have the addressbook) ¿?
> * Some small fixes in our side (like copy constructor in listbase).
> * Handling the updates. This fixes the bug that you end up having a duplicate entry in the Hash table when modifying an event. 
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepimlibs/kcal/alarm.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.h 1035467 
>   trunk/KDE/kdepimlibs/kcal/calendarlocal.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/calendarnull.h 1035467 
>   trunk/KDE/kdepimlibs/kcal/calendarnull.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/event.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/icalformat_p.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/incidence.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/incidencebase.h 1035467 
>   trunk/KDE/kdepimlibs/kcal/incidencebase.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/journal.cpp 1035467 
>   trunk/KDE/kdepimlibs/kcal/listbase.h 1035467 
>   trunk/KDE/kdepimlibs/kcal/todo.cpp 1035467 
> 
> Diff: http://reviewboard.kde.org/r/1489/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alvaro
> 
>

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