New Framework Review: KDAV

[Aleix Pol]

On Sun, May 24, 2020 at 8:52 AM Volker Krause <vkrause at kde.org> wrote:
>
> The remaining issues that didn't change ABI anymore (movable value types, hide
> private methods/slots inside the private classes, etc) have long since been
> addressed.
>
> I think there's two possible time slots to actually execute the move to
> frameworks now:
> * ASAP, for the June release.
> * For the July release, just in time for the 20.08 dependency freeze.
>
> Opinions?
>
> Thanks,
> Volker
>
> On Saturday, 4 April 2020 17:32:19 CEST Volker Krause wrote:
> > 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
>
>
>
>

If it's only to be used in 20.08 then it makes sense that it's by then
when it gets adopted? You can do all the moves beforehand so if you
realise there's some ABI changes or anything of sorts you can still do
it by then.

Aleix