Attica moved to kdereview

[Frederik Gladhorn]

Hola Albert,
thanks a lot for the review!
I hope that we addressed your points by now.

Albert Astals Cid wrote:
> A Dimarts, 3 de novembre de 2009, Frederik Gladhorn va escriure:
>> Hi all,
>> after spending some time with libattica, we decided that now is a good
>>  point to get a review for it and eventually have it used in KDE.
> 
> The licensing is unclear, activity.h is GPL2+ while downloaditem.h is
> LGPL2+
Done, we forgot to make it the same everywhere, it's relicensed to LGPL2+ in 
agreement with all authors now.

> You have one foreach without const & in the "iterator"
/me hides :D

> Maybe it would make sense to d-pointify Attica:BaseJob::Metadata,
> Attica::DownloadUrlDescription, Attica::GetJob, Attica::PostJob
Mostly done, will be finished tomorrow.

> In Attica::DownloadUrlDescription if you put the two bools together your
> struct will use less memory
Done.
> Should downloadUrlDescription(), previewPicture(), license(), author() of
> Attica::Content be const?
Done.
> Should url() in Attica::DownloadItem be const?
Yes. 
> Any reason to make Attica::DownloadItem use a
> QExplicitlySharedDataPointer?
Total nonsense and an oversight by me.
Seems like I wasn't quite awake when writing that class...

> From the application point of view, it would make sense to me that if for
> example it makes no sense that the application ever calls
> Folder::setMessageCount it should be private and called by a friend class.
> But maybe it makes sense calling Folder::setMessageCount and it's ok :D
Since we support creating items and sending them to the server again, most 
setters make sense. The case you asked about is indeed questionable.

> 
> After a quick look at the API i'm not sure if the Jobs are supposed to be
> part of the public API or not. Their headers are installed an the classes
> exported, but
> 
> GetJob(const QSharedPointer<Internals>& internals, const QNetworkRequest&
> request);
> seems a bit weird, what's that QSharedPointer<Internals>?
> 
Yep, this is supposed to be used only internally. You just get instances of 
these classes back from Attica::Provider. Constructors changed to private or 
protected now.

Thanks!

Greetings
Frederik