New Framework Review: KDAV

[Volker Krause]

With both 20.04.2 and 5.71.0 out I think it's now time to do this move.

What extra steps do we need to take now that the framework/application 
distinction exists in Gitlab as well? I guess this is the first case of a 
post-migration move. Also, what is the impact on the 20.04.3 release when we 
move this in Gitlab?

Thanks,
Volker

On Sunday, 24 May 2020 08:52:17 CEST Volker Krause 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