New CoverProvider for Discogs

[davidsansome, 2012-01-23 12:08:27]

Looks good.  Comments below are mostly style issues.  Could you add copyright
headers to the new files too?  You can just copy and paste them from any
existing file, changing the author to your name and the date to 2012.

You usually don't need to worry about lifetimes of Qt value types like QList -
they're implicitly shared and reference counted, so "copies" are cheap and
memory is managed automatically.  See
http://developer.qt.nokia.com/doc/qt-4.8/implicit-sharing.html

If you want to be able to add all the images, you could collect them up in a
struct that you allocate on the heap in StartSearch and then pass the pointer
around between the callbacks.  If you do that you can put the ID and
QVariantList of results in that struct too.

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider.cpp
File src/covers/discogscoverprovider.cpp (right):

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.cpp:67: // FIXME: is it safe to pass the results
map here? when is it destroyed??
It's on the stack so it's fine - it'll be destroyed at the end of this function,
after FollowBestReply returns.  NewClosure makes a (shallow, reference counted)
copy of the whole thing so the underlying data will survive to
QueryResourceReply.

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.cpp:76: foreach (QVariant result, results) {
const QVariant&

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.cpp:140: foreach (QVariant image, images) {
const QVariant&

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.cpp:145: CoverSearchResult cover_result;
2-space indent

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.cpp:159: foreach (QVariant image, images) {
const QVariant&

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.cpp:162: CoverSearchResult cover_result;
2-space indent

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider.h
File src/covers/discogscoverprovider.h (right):

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.h:15: static const char* search_url;
kSearchUrl

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.h:21: void QueryResourceReply(QNetworkReply*
reply, int id, QList<QVariant>& results);
const QVariantList&

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.h:25: void QueryResource(QString resource_url,
int id, QList<QVariant>& results);
const QString& and const QVariantList&

http://codereview.appspot.com/5562050/diff/1/src/covers/discogscoverprovider....
src/covers/discogscoverprovider.h:26: void FollowBestResult(QNetworkReply*
reply, int id, QList<QVariant>& results);
Since you're modifying results, pass it as a pointer to make it really obvious.

[mbj4668, 2012-01-23 21:34:55]

Hi,

Thanks for your comments, I have fixed the code locally.

On Mon, Jan 23, 2012 at 1:08 PM, <davidsansome@gmail.com> wrote:

> If you want to be able to add all the images, you could collect them up
> in a struct that you allocate on the heap in StartSearch and then pass
> the pointer around between the callbacks.  If you do that you can put
> the ID and QVariantList of results in that struct too.
>

You mean send one request at the time?  If I do this, where do I delete the
heap allocated data?   I can of course do that before I emit
SearchFinished, but that doesn't work if the search timer triggers and
CancelSearch is called, right?  So I need a map from 'id' to this heap
allocated data?


/martin