Moving libkfacebook to extragear

[Martin Klapetek]

On Mon, Nov 5, 2012 at 7:52 PM, Pino Toscano <pino at kde.org> wrote:

> Some notes:
>
> - as others have pointed out, public classes should be better use
> d-pointers; for the jobs hierarchy, you could use a shared d-pointer,
> see [1] for example
>

The data classes are finally done - all of them now use d-pointers and none
are qobject; I've added the qobject-parsers as Kevin suggested, please give
it a look.


> - GetCommentsJob::commentCount() could need the const qualifier
>
> - LikeInfo::setCount() does not need const& for the int parameter
>
> - ListJobBase::numEntries() (and in subclasses) could be better named
> as entriesCount()
>

All fixed.


>
> - ListJobBase, instead of a bool, could use an enum for readability
>
> - in CommentInfo, NotificationInfo and PostInfo some methods return
> *Info objects (or QList of them) while their setters take
> QVariantMap/QVariantList; better just make them take the object too,
> to not rely on the implementation (i.e. the parsing done with qjson)?
> also some of those getters have a *Map() version too, maybe those
> should be removed for the same reason above?
>

Well this classes are/should be writeable only from the QJson parser (which
passes the QVariant* stuff) as it reflects the server-side object. To all
the rest of the library & co. it's just a read-only container, so there is
no need for setters taking the *Info objects as they will never be used.
I'm wondering if the API should better reflect this by eg. making the
setters private and making the qobject-parsers friend classes....


>
> - in UserInfo there's birthdayAsString() while in other classes methods
> like that are named fooString()
>
> - UserInfo has no getters for city and country
>

Both fixed.


>
> - in UserInfo picture is a QUrl but website a QString?
>

Fixed all links to be QUrl everywhere.


> - remember to remove the qjson includes from public headers
>

Done.

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