Created attachment 207399 [details] [review]
Use libmusicbrainz4

Quickly patch

Created attachment 208287 [details] [review]
A better patch

Comment on attachment 207399 [details] [review]
Use libmusicbrainz4

Supposedly obsoleted by Vadim's patch.

Review of attachment 208287 [details] [review]:

Some comments below.
I don't think it's a good idea to just depend on libmusicbrainz4 though, since some distributions (e.g. Fedora) don't have it packaged yet.

::: src/libsushi/sushi-cover-art.c
@@ +221,3 @@
+  gchar *param_names[2];
+  gchar *param_values[2];
+  gchar *query_string = NULL;

You need to g_free the strings in param_names and param_values.
Alternatively, you can declare them as gchar **, allocate them with g_new or g_malloc and call g_strfreev on the whole array when done.

@@ +227,1 @@
+  param_names[0] = g_strdup("query");

Missing space after g_strdup

@@ +230,2 @@
+  param_names[1] = g_strdup("limit");
+  param_values[1] = g_strdup("1");

Ditto

@@ +233,1 @@
+  metadata = mb4_query_query (query, "release", "" , "",

Extra space after the first ""

@@ +238,3 @@
+  if (metadata) {
+    release_list = mb4_metadata_get_releaselist (metadata);
+    if (mb4_release_list_size(release_list) > 0) {

Missing space after mb4_release_list_size

@@ +241,3 @@
+      gchar asin[255];
+
+      release = mb4_release_list_item(release_list, 0);

Missing space

Created attachment 208686 [details] [review]
Even better patch

Cosimo, I've updated the patch. However I'm not sure about g_strfreev part - maybe this can be reworked.

On the whole purpose of the patch - doesn't Fedora have

Review of attachment 208686 [details] [review]:

Thanks for the updated patch. Other comments below.

::: src/libsushi/sushi-cover-art.c
@@ +226,2 @@
+  param_names = g_new (gchar*, 2);
+  param_values = g_new (gchar*, 2);

Since g_strfreev() expects a NULL-terminated array (and it's generally a good idea to always NULL-terminate string arrays anyway), you should allocate three pointers here instead of two, and set

param_names[2] = NULL;
param_values[2] = NULL;

@@ +239,3 @@
+
+  if (metadata) {
+    release_list = mb4_metadata_get_releaselist (metadata);

It's not really clear to me from the libmusicbrainz4 documentation, but are we supposed to call mb4_release_list_delete() on this? (and similarly, mb4_release_delete() on the Mb4Release object later?)

@@ +272,3 @@
+    g_strfreev (param_names);
+  if (param_values != NULL)
+    g_strfreev (param_values);

Since you always allocate these with g_new, you can avoid checking for != NULL here and just call g_strfreev().

Created attachment 209078 [details] [review]
Slightly better patch

Thanks, Cossimo, I've updated the patch.
param_names[2] = NULL seems to solve unnecessary NULL check in the function end (otherwise it crashed).
Added mb4_release_delete and mb4_release_list_delete also, I'll try to use valgrind to seem if we are leaking memory anywhere else in this function.

I suppose, we should wait for all major distributions to adopt libmusicbrainz4 and commit this patch. Another option would be committing to a separate branch

Hello Cosimo,

Could this get another review?

Its at least in F16, F17 and F18 now: http://koji.fedoraproject.org/koji/packageinfo?packageID=13459

Review of attachment 209078 [details] [review]:

Thanks, this looks good to me.
Feel free to commit after we branch for GNOME 3.4.

(In reply to comment #8)

> Its at least in F16, F17 and F18 now:
> http://koji.fedoraproject.org/koji/packageinfo?packageID=13459

Yeah it recently got into Fedora; I don't think pushing this for 3.4 is a good idea though, so let's wait after branching.

Comment on attachment 209078 [details] [review]
Slightly better patch

sushi branched, commited in commit 9afbcc1ed6cbb3181b2ad1a2e1abcd771a0aac17