Moving libkfacebook to extragear

Albert Astals Cid aacid at kde.org
Wed Nov 7 19:11:12 GMT 2012


El Dilluns, 5 de novembre de 2012, a les 22:59:43, Martin Klapetek va 
escriure:
> 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.

You sure? I still see a lots of d-pointers missing (AllEventsListJob, 
AllNotesListJob, etc).

Also you have some classes without the export thingie, like AttendeeInfo or 
the facebookTimeToKDateTime function

Cheers,
  Albert

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




More information about the kde-core-devel mailing list