Hi, On Saturday, 2012-10-27, Martin Klapetek wrote: > Hi, > > I'd like to move libkfacebook, the foundation for akonadi-facebook > resource, into extragear. It's been in use for a while, lots of distro ship > it bundled with akonadi-facebook resource, which is now becaming part of > kdepim-runtime for 4.10 with libkfacebook as optional compile-time > dependency. > > I'd like to ask for a review of this library, currently in kdereview - > https://projects.kde.org/projects/kdereview/libkfacebook
I had a cursory look and found a couple of things. CCing the original authors so they can provide input. - The job classes should have a QObject *parent = 0 argument and pass parent on to KJob's constructor. - Some of the constructors that can be called with only one argument are missing the explicit keyword. - FacebookJob(QString, QString) calls setCapabilities, FacebookJob(QString) does not - Is the FacebookJob(QString) constructor really needed. It seems all jobs need a path - All jobs to the same URL base setup (protocol, host, token). I am wondering if the base class could just have a KUrl member and initialize all the common stuff and derived classes just add to that. - the *Info classes seem to be normal data classes, IMHO they don't need to be QObjects but rather "value types" - The typdefs in eventinfo.h create global types with very generic names, e.g. Event Cheers, Kevin -- Kevin Krammer, KDE developer, xdg-utils developer KDE user support, developer mentoring
signature.asc
Description: This is a digitally signed message part.
