On Sat, Oct 27, 2012 at 6:05 PM, Kevin Krammer <span dir="ltr"><<a href="mailto:krammer@kde.org" target="_blank">krammer@kde.org</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

I had a cursory look and found a couple of things. CCing the original authors<br>
so they can provide input.<br>
<br>
- The job classes should have a QObject *parent = 0 argument and pass parent<br>
on to KJob's constructor.<br>
<br>
- Some of the constructors that can be called with only one argument are<br>
missing the explicit keyword.<br></blockquote><div><br></div><div>Will fix.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- FacebookJob(QString, QString) calls setCapabilities, FacebookJob(QString)<br>
does not<br>
<br>
- Is the FacebookJob(QString) constructor really needed. It seems all jobs<br>
need a path<br></blockquote><div><br></div><div>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.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- All jobs to the same URL base setup (protocol, host, token). I am wondering<br>
if the base class could just have a KUrl member and initialize all the common<br>
stuff and derived classes just add to that.<br></blockquote><div><br></div><div>Will check.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- the *Info classes seem to be normal data classes, IMHO they don't need to be<br>
QObjects but rather "value types"<br></blockquote><div><br></div><div>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.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- The typdefs in eventinfo.h create global types with very generic names, e.g.<br>
Event<br></blockquote><div><br></div><div>Good point, will add those into namespace.</div><div><br></div></div>-- <br><div><span style="color:rgb(102,102,102)">Martin Klapetek | KDE Developer</span></div><br>