Moving libkfacebook to extragear

[Kevin Krammer]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121027/5e5239c1/attachment.sig>