Moving libkfacebook to extragear

[Martin Klapetek]

On Sat, Oct 27, 2012 at 6:05 PM, Kevin Krammer <krammer at kde.org> wrote:

> 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.
>

Will fix.


>
> - FacebookJob(QString, QString) calls setCapabilities, FacebookJob(QString)
> does not
>
> - Is the FacebookJob(QString) constructor really needed. It seems all jobs
> need a path
>

This is used (only) from FacebookGetJob, assumingly for querying multiple
items, where just the ids are specified and no path is needed. I'll see
what can be done about this.


> - 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.
>

Will check.


> - the *Info classes seem to be normal data classes, IMHO they don't need
> to be
> QObjects but rather "value types"
>

This is for the parsing purposes - the library uses QJson parser/mapper,
which automagically maps the received json data to qobjects, otherwise
there would have to be manual parsing everywhere (and the facebook jsons
are huge), which means more code, more error possibilities, more
maintaining requirement and worse readability (compared to two lines QJson
mapper). So I'd like to leave this one as is.


> - The typdefs in eventinfo.h create global types with very generic names,
> e.g.
> Event
>

Good point, will add those into namespace.

-- 
Martin Klapetek | KDE Developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121028/1c23406b/attachment.htm>