Created attachment 267656 [details] [review]
Print musicbrainz errors

Add g_warning if there are any errors when performing musicbrainz
queries.

Created attachment 267657 [details] [review]
Use localized artist aliases from musicbrainz

Musicbrainz returns artist names in their native locale so it returns
"Пётр Ильич Чайковский" instead of "Pyotr Ilyich Tchaikovsky" but
provides localized versions of the artist name as aliases. Use the
alias so we get "Pyotr Ilyich Tchaikovsky" in English locales and
"Piotr Ilitch Tchaïkovski" in French locales etc.

Although GLib supports fallback locales we do not use them as
MusicBrainz does not provide locale information for the canonical
artist name, only it's aliases. This means that it is not possible to
fallback to other locales as the canonical name may actually be in a
higher priority locale than the alias that matches one of the fallback
locales.

Created attachment 268558 [details] [review]
I've reworked these patches slightly and split them up a bit to try

and make reviewing them easier.

Add instance member to MusicBrainz callbacks.

To use the artist alias information for composers sometimes we're
going to need to do a query from one of the callbacks and cache the
result. Add an instance member to the callbacks so they can access the
cache and MusicBrainz query object.

Created attachment 268559 [details] [review]
Print MusicBrainz errors.

Add a convenience function for performing MusicBraniz queries and
print a warning if there are any errors.

Created attachment 268560 [details] [review]
Cache MusicBrainz artist details.

This will avoid performing unnecessary composer alias queries and
scanning an artist's alias list more than once.

Remove 'joinphrase' from ArtistDetails structures to enable caching
just one ArtistDetails per artist. Create ArtistCredit structure to
hold the join phrase and an ArtistDetails* when creating artist
lists. Remove the artist lists from AlbumDetails and TrackDetails
structures as they are only used to generate the artist text and not
having them simplifies things a little.

Created attachment 268561 [details] [review]
Use artist cache for composers.

MusicBrianz only returns artist name, sortname & id for
work-level-rels so we need to perform another query if we want the
alias list. Cache the result so the query only needs to be performed
once per composer.

Created attachment 268566 [details] [review]
Use localized artist aliases from MusicBrainz.

Use localized artist aliases from MusicBrainz.

Musicbrainz returns artist names in their native locale so it returns
"Пётр Ильич Чайковский" instead of "Pyotr Ilyich Tchaikovsky" but
provides localized versions of the artist name as aliases. Use the
alias so we get "Pyotr Ilyich Tchaikovsky" in English locales and
"Piotr Ilitch Tchaïkovski" in French locales etc.

Although GLib supports fallback locales we do not use them as
MusicBrainz does not provide locale information for the canonical
artist name, only it's aliases. This means that it is not possible to
fallback to other locales as the canonical name may actually be in a
higher priority locale than the alias that matches one of the fallback
locales.

Review of attachment 268559 [details] [review]:

Looks good .

::: libjuicer/sj-metadata-musicbrainz5.c
@@ +119,3 @@
+  g_free (message);
+}
+

This could also build a GError describing the error which occurred, but this can probably be done when actually needed ;)

@@ +138,3 @@
+                                1, inc, &includes);
+  if (metadata == NULL)
+    print_musicbrainz_query_error (self, entity, id);

metadata will only be NULL in error cases right ?

Review of attachment 268560 [details] [review]:

::: libjuicer/sj-metadata-musicbrainz5.c
@@ +189,3 @@
     Mb5NameCredit name_credit;
     Mb5Artist artist;
+    ArtistCredit *artist_credit = g_slice_new0 (ArtistCredit);

Could probably be a g_new0 as the rest of the allocations in the ArtistCredit/ArtistDetails structures. No big preference here.

::: libjuicer/sj-structures.c
@@ +82,2 @@
 /*
+ * GDestoryNotify callback to free a ArtistDetails*

GDestroyNotify (small typo)

@@ +87,3 @@
+  artist_details_free (artist);
+}
+

Not strictly needed as you can pass (GDestroyNotify)artist_details_free to functions which need it.

@@ +91,3 @@
+ * Free an ArtistCredit*
+ */
+void artist_credit_free (ArtistCredit *credit, gboolean free_details)

The 'free_details' arg does not seem to ever be TRUE ?

@@ +100,3 @@
+
+/*
+ * GDestoryNotify callback to free a ArtistCredit*

Same typo

Review of attachment 268561 [details] [review]:

ACK, why do this only for composers? I guess regular artists could have aliases as well?

Review of attachment 268566 [details] [review]:

Ah, previous commit is a bit confusing as it talks about aliases, but they are only added in this commit.

::: libjuicer/sj-metadata-musicbrainz5.c
@@ +957,3 @@
+     matches one of the fallback locales. */
+  l = g_get_language_names ();
+  if (strlen (l[0]) > 1) { /* Ignore "C" locale */

Why not skip to the next item if l[0] is the C locale ?

Created attachment 269896 [details] [review]
fixup! Print MusicBrainz errors.

(In reply to comment #8)
>@@ +138,3 @@
>+                                1, inc, &includes);
>+  if (metadata == NULL)
>+    print_musicbrainz_query_error (self, entity, id);
>
>metadata will only be NULL in error cases right ?

Good point, I had forgotten to check a disc that wasn't in
MusicBraniz, we don't want to print a error when a discid isn't found.

(In reply to comment #9)
> Review of attachment 268560 [details] [review]:
> 
> ::: libjuicer/sj-metadata-musicbrainz5.c
> @@ +189,3 @@
>      Mb5NameCredit name_credit;
>      Mb5Artist artist;
> +    ArtistCredit *artist_credit = g_slice_new0 (ArtistCredit);
> 
> Could probably be a g_new0 as the rest of the allocations in the
> ArtistCredit/ArtistDetails structures. No big preference here.

I might leave it as g_slice and change the others in the future?

> ::: libjuicer/sj-structures.c
> @@ +82,2 @@
>  /*
> + * GDestoryNotify callback to free a ArtistDetails*
> 
> GDestroyNotify (small typo)

Thanks, I'll correct it before I commit the patch
 
> @@ +87,3 @@
> +  artist_details_free (artist);
> +}
> +
> 
> Not strictly needed as you can pass (GDestroyNotify)artist_details_free to
> functions which need it.

I know that works but it's actually undefined behaviour as the function pointer is not cast back to a compatible type in g_list_free_full

> @@ +91,3 @@
> + * Free an ArtistCredit*
> + */
> +void artist_credit_free (ArtistCredit *credit, gboolean free_details)
> 
> The 'free_details' arg does not seem to ever be TRUE ?

No as the ArtistDetails is always owned by the hash table at the moment but I
wanted to make it clear for the future that artist_credit_free didn't free all
the members.

> @@ +100,3 @@
> +
> +/*
> + * GDestoryNotify callback to free a ArtistCredit*
> 
> Same typo

The perils of copy and paste!

Created attachment 269900 [details] [review]
fixup! Use localized artist aliases from MusicBrainz.

(In reply to comment #11)
>::: libjuicer/sj-metadata-musicbrainz5.c
>@@ +957,3 @@
>+     matches one of the fallback locales. */
>+  l = g_get_language_names ();
>+  if (strlen (l[0]) > 1) { /* Ignore "C" locale */
>
>Why not skip to the next item if l[0] is the C locale ?

For the same reason that we don't use any fallback locales. I think what it
should do is set the alias locale to "en"

*** Bug 616760 has been marked as a duplicate of this bug. ***

(In reply to comment #13)
> > Not strictly needed as you can pass (GDestroyNotify)artist_details_free to
> > functions which need it.
> 
> I know that works but it's actually undefined behaviour as the function pointer
> is not cast back to a compatible type in g_list_free_full

This would probably break a lot of code if this changed :-/ But ok.

Attachment 268558 [details] pushed as 86b6f50 - Add instance pointer to MusicBrainz callbacks. 
Attachment 268559 [details] and attachment 269896 [details] [review] pushed as d2a288a - Print MusicBrainz errors.
Attachment 268560 [details] pushed as 0ecb20f - Cache MusicBrainz artist details.
Attachment 268561 [details] pushed as e952077 - Use artist cache for composers.
Attachment 268566 [details] and attachment 269900 [details] [review] pushed as e574be2 - Use localized artist aliases from MusicBrainz.