After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723297 - Use localized artist aliases from MusicBrainz
Use localized artist aliases from MusicBrainz
Status: RESOLVED FIXED
Product: sound-juicer
Classification: Applications
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: Sound Juicer Maintainers
Sound Juicer Maintainers
: 616760 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-01-30 15:58 UTC by Phillip Wood
Modified: 2014-04-02 10:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Print musicbrainz errors (4.30 KB, patch)
2014-01-30 15:58 UTC, Phillip Wood
none Details | Review
Use localized artist aliases from musicbrainz (20.41 KB, patch)
2014-01-30 15:58 UTC, Phillip Wood
none Details | Review
I've reworked these patches slightly and split them up a bit to try (10.34 KB, patch)
2014-02-09 11:14 UTC, Phillip Wood
committed Details | Review
Print MusicBrainz errors. (4.30 KB, patch)
2014-02-09 11:14 UTC, Phillip Wood
committed Details | Review
Cache MusicBrainz artist details. (11.11 KB, patch)
2014-02-09 11:15 UTC, Phillip Wood
committed Details | Review
Use artist cache for composers. (3.03 KB, patch)
2014-02-09 11:15 UTC, Phillip Wood
committed Details | Review
Use localized artist aliases from MusicBrainz. (5.33 KB, patch)
2014-02-09 11:22 UTC, Phillip Wood
committed Details | Review
fixup! Print MusicBrainz errors. (1.32 KB, patch)
2014-02-21 11:45 UTC, Phillip Wood
committed Details | Review
fixup! Use localized artist aliases from MusicBrainz. (1.14 KB, patch)
2014-02-21 11:58 UTC, Phillip Wood
committed Details | Review

Description Phillip Wood 2014-01-30 15:58:29 UTC
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.
Comment 1 Phillip Wood 2014-01-30 15:58:31 UTC
Created attachment 267656 [details] [review]
Print musicbrainz errors

Add g_warning if there are any errors when performing musicbrainz
queries.
Comment 2 Phillip Wood 2014-01-30 15:58:36 UTC
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.
Comment 3 Phillip Wood 2014-02-09 11:14:43 UTC
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.
Comment 4 Phillip Wood 2014-02-09 11:14:54 UTC
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.
Comment 5 Phillip Wood 2014-02-09 11:15:04 UTC
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.
Comment 6 Phillip Wood 2014-02-09 11:15:15 UTC
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.
Comment 7 Phillip Wood 2014-02-09 11:22:41 UTC
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.
Comment 8 Christophe Fergeau 2014-02-14 14:18:27 UTC
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 ?
Comment 9 Christophe Fergeau 2014-02-14 14:51:48 UTC
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
Comment 10 Christophe Fergeau 2014-02-14 15:00:15 UTC
Review of attachment 268561 [details] [review]:

ACK, why do this only for composers? I guess regular artists could have aliases as well?
Comment 11 Christophe Fergeau 2014-02-14 15:15:06 UTC
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 ?
Comment 12 Phillip Wood 2014-02-21 11:45:29 UTC
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.
Comment 13 Phillip Wood 2014-02-21 11:55:36 UTC
(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!
Comment 14 Phillip Wood 2014-02-21 11:58:16 UTC
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"
Comment 15 Phillip Wood 2014-02-21 12:00:34 UTC
*** Bug 616760 has been marked as a duplicate of this bug. ***
Comment 16 Christophe Fergeau 2014-03-24 16:01:42 UTC
(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.
Comment 17 Phillip Wood 2014-04-02 10:04:53 UTC
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.