New Framework Review: KDAV

Sat Apr 4 16:32:19 BST 2020

Thanks for the review! We are cutting it close again with the 20.04 deadline, 
but fortunately most of these findings aren't ABI-breaking :)

The result was discussed in more detail at the (virtual) PIM sprint, summary 
below for the record.

On Saturday, 4 April 2020 16:20:21 CEST Kevin Ottens wrote:
> Hello,
> 
> On Saturday, 9 November 2019 12:33:54 CEST Volker Krause wrote:
> > during Akademy there was a request to promote KDAV from KDE PIM to
> > Frameworks for use by Plasma Mobile. KDAV is a framework that implements
> > the CalDav/ CardDav/GroupDav protocol on top of KIO's WebDav support. It
> > would be classified as a functional tier 3 framework.
> > 
> > So far we have fixed a number of obvious ABI-compatibility issues, removed
> > QtXml[Patterns] usage from the public interface and relicensed GPL parts
> > (apart from a bit of test code) to LGPL. The next step would be a more
> > thorough review to identify changes necessary before becoming a Framework.
> > 
> > To avoid the last minute invasive changes we ended up doing for
> > KCalendarCore, I'd propose the following timeline:
> > 
> > - identify and implement all necessary changes to the API and ABI until
> > the
> > 20.04 Application release (that includes the still necessary move to the
> > KF5 library namespace).
> 
> I'm likely late to the party, but here is what I found by looking at KDAV
> master today (first day of the KDE PIM sprint):
>  * There's a few private methods or Q_SLOTS that I'd hide completely by
> moving them to the d-pointer, for the slots we're using type safe connects
> so they don't even need to be marked as slots at all;

Cosmetic with no ABI impact, we can do that post 20.04 still.

>  * Is it worth making DavCollection moveable? It's only copyable right now;

Probably yes, that's new API with no ABI break, so we can do that post 20.04 
as well.

>  * We might want to do something about "ctag" in DavCollection it's a bit
> obscure as a name (and the API doc doesn't help), also it seems to not be an
> official standard (while being widely supported) and there's the sync-token
> mechanism which has a RFC (RFC6578);

I have no idea what ctag is (I am only doing the technical work needed to turn 
this into a framework, I didn't write this library).

>  * Why isn't DavCollectionModifyJob using DavCollection somehow? (might just
> be my ignorance but I find it surprising that it is solely based on a
> property mechanism);

I think this is to be able to control which properties get changed, rather 
than sending the full set of them.

>  * DavCollections(Multi)FetchJob has a mysterious "protocol" parameter on
> its collectionDiscovered signal, is it really necessary? if it has to stay,
> shouldn't be at least documented? or at least a safer type than int? 

Fixed in https://phabricator.kde.org/D28564 and https://phabricator.kde.org/
D28566

> * DavCollectionsMultiFetchJob is inconsistent as it's not using
> Q_DECLARE_PRIVATE;

That's due to using KJob as a base directly.

Subsequent discussion suggested this should be a KCompositeJob, David is 
taking care of this.

>  * KDAV::Error would benefit from more apidox;

Yes, not blocked by the 20.04 freeze though.

>  * Is it worth making DavItem moveable? It's only copyable right now;

See above, same as DavCollection.

>  * Same comment about etag for DavItem than the ctag one for DavCollection

See above, same as ctag.

>  * I'd be tempted to move all the protected methods of DavJobBase on its d-
> pointer, the job subclasses would have access to them anyway, it'd make
> sense to put them protected in the header only if we expect subclasses
> outside of the lib (and I doubt this is actually supported);

ABI impact mitigated by https://phabricator.kde.org/D28562 so we can clean 
this up after 20.04.

>  * It needs to decide between Qt smart pointers or STL ones I think, found a
> bit of both so far (I'd lean toward STL ones but maybe that's just me); 

Also fixed by https://phabricator.kde.org/D28562.

> * Make DavUrl moveable?

See above, same as DavCollection and DavItem.

>  * EtagCache probably shouldn't have anything protected, also, why is it a
> QObject at all?

This is why: https://lxr.kde.org/source/kde/pim/kdepim-runtime/resources/dav/
resource/akonadietagcache.cpp

>  * Are we sure we want to return a QLatin1String in ProtocolInfo? this
> strike me as an odd choice.

Fixed in https://phabricator.kde.org/D28563.

> Overall apidox would likely need a big pass of cleanups as well.

> I think that's it from me.

I hope we managed to address everything on short notice that would require ABI 
breaks after the 20.04 release (and thus cause a delay of the frameworks move 
Volker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20200404/148e8a01/attachment.sig>