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
signature.asc
Description: This is a digitally signed message part.
