GNOME Bugzilla – Bug 674926
Show release country and date when there are many matches
Last modified: 2014-02-14 12:36:09 UTC
MB will return a match for every release where the only difference is the date and country. SJ should present this information to the user to reduce confusion.
Created attachment 218228 [details] Screenshot of single column multiple release dialog I have created a patch to add the recording label, year and country to the information displayed when choosing between multiple releases. It changes the layout so that all the information is in a single column. With more than one column the user has to scroll to the right to see all the information about a particular release. With a single column layout it is possible to see all the information for the release at once. I added the recording label as I often find that I'm ripping an album that has been released on different labels at different times and it's much easier to tell them apart from the recording label rather than reading the liner notes to find the release date. I've attached a screenshot of the new layout.
Created attachment 218229 [details] [review] Patch to change layout and add more details to multiple release dialog
Created attachment 218355 [details] [review] Updated patch to change layout and add more details to multiple release dialog This fixes some strings that were not marked for translation and adds '(Disc m/n)' to the end of the album title as requested in #666836
Great but I'm not that keen on the "in year - country" string. Then again, saying "in" again isn't great either. Maybe "during year in country"?
(In reply to comment #4) > Great but I'm not that keen on the "in year - country" string. Then again, > saying "in" again isn't great either. Maybe "during year in country"? If the “ - ” is kept, it should be changed to be an em-dash (“—”). (Sorry, pedantry getting the better of me.)
*** Bug 666836 has been marked as a duplicate of this bug. ***
*** Bug 680012 has been marked as a duplicate of this bug. ***
*** Bug 681567 has been marked as a duplicate of this bug. ***
Ross, will you take care of merging this patch? Or should I review it? As for the string, "Released in $year in $country on label $label" could work too?
Review of attachment 218355 [details] [review]: To avoid the mega-table and utter translation hell in sj-metadata, look at using iso-codes instead. I'm sure that has the country names. Apart from that looks good.
Sorry for the slow response, for some reason I wasn't subscribed to email notifications for this bug. I'll try and do a revised patch this week with new wording and just country codes rather than names (Is that what you meant? I'm afraid I don't really understand what you mean by "... look at using iso-codes instead. I'm sure that has the country names.") For the text we could have 'Released: $year in $country on $label' which gets rid of the two ins - what do you think? Best Wishes Phil
What I meant regarding iso-codes was removing sj_metadata_helper_lookup_country_code () and replacing it with an iso-codes lookup (with iso-codes you can translate using gettext a country code into a localised string). "Released: $year in $country on $label" sounds much better, yes.
Ah, I understand now, I thought there must be a library for doing the country name lookups but hadn't managed to find iso-codes. Looking at it it seems that you need to use the xml files to do the country-code to country-name lookup though, the gettext interface only does name translations. There is some code in gedit that I can cut and paste to do the lookup :-) >"Released: $year in $country on $label" sounds much better, yes. Excellent, that's what I'll change it to then.
(In reply to comment #13) > Ah, I understand now, I thought there must be a library for doing the country > name lookups but hadn't managed to find iso-codes. Looking at it it seems that > you need to use the xml files to do the country-code to country-name lookup > though, the gettext interface only does name translations. There is some code > in gedit that I can cut and paste to do the lookup :-) Don't do that. Use the GStreamer helpers (gst_tag_get_language* functions). It's LGPL code in a library that you already depend on and not GPL-only code (gedit) to link against GPL+exception code (libjuicer).
(In reply to comment #14) > Don't do that. Use the GStreamer helpers (gst_tag_get_language* functions). > It's LGPL code in a library that you already depend on and not GPL-only code > (gedit) to link against GPL+exception code (libjuicer). Unfortunately we need country names not the language names offered by the GStreamer helpers you suggest. If there's a licence problem it wont be too difficult to write something from scratch.
Totem had code to do most of the parsing using a compatible license. Pretty sure you can start from there.
(In reply to comment #16) > Totem had code to do most of the parsing using a compatible license. Pretty > sure you can start from there. Thanks for the pointer, I'll take a look at what totem does.
Created attachment 229135 [details] [review] Updated to use iso-codes and reorder release_details string. (In reply to comment #13) > > >"Released: $year in $country on $label" sounds much better, yes. > > Excellent, that's what I'll change it to then. Unfortunately it's not so excellent for countries such as United States as we need an extra 'the' to be grammatically correct. We could fix this by translating the translations we get from iso-codes but that's a bit messy. In the patch attached I've transposed $country and $year to side-step the problem and amended the sort routine to sort on the new order. I've used the code from totem as suggested by Bastien (Thanks for the tip) to do the parsing for the country codes using iso-codes. Totem was using g_atexit to register a clean-up function. Unfortunately g_atexit is now deprecated so I'm just calling the function directly from main (), after gtk_main () which I'm not so keen on but it seemed to be the simplest solution. Apart from that there are no major changes, just a small re-ordering of the patches so the disc count changes are more integrated with the rest. Let me know what you think, Phil
This is great! I have been struck by this many times. Can we please have this also include the other release information, such as the catalogue number and barcode? Where the the release has a barcode, and the user has the actual CD in front of them (as I often do), this is by far the easiest way to distinguish between multiple releases. For reference, the information that MusicBrainz shows you if you are in the same position are: Position Format Title Artist Date Country Label Catalog# Barcode
(In reply to comment #19) > This is great! Thanks! > I have been struck by this many times. Can we please have this > also include the other release information, such as the catalogue number and > barcode? Including the barcode and catalogue number is an interesting idea, my only worry is that I think we need to have enough information that the user can easily see which version of the album they have without making the interface so cluttered that they cannot find the information they need. > Where the the release has a barcode, and the user has the actual CD in front > of them (as I often do), this is by far the easiest way to distinguish between > multiple releases. This is certainly true if the barcode is actually in musicbrainz. I just tried 5 albums with multiple releases and only two of them had barcodes for all the different versions, a couple more had a barcode for one but all versions. The situation with catalogue numbers was similar. In my experience I've always been able to use the country, label or disc number to tell which one I wanted. It would probably be possible to add the barcode to the other release information in the dialog without it getting too cluttered but if we show the barcode for some releases in the dialog and not others (because it is missing from musicbrainz) that might be confusing to the user. Ultimately it's not up to me anyway. Ross what do you think?
Comment on attachment 229135 [details] [review] Updated to use iso-codes and reorder release_details string. Sorry, hadn't noticed this attachment until now ;( >From 819d8191e1dd6671841a9d00d1f5ee6c74f94907 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Sat, 30 Jun 2012 14:18:37 +0100 >Subject: [PATCH 01/13] [multiple releases] Get country name ISO code. > >Musicbrainz returns the country as a 2 letter ISO country code. Add >functions based on totem's language code handling to convert that code >to the country name and store the name, rather than the code in >AlbumDetails::country. > >Adds a dependency on iso-codes. > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 >--- > configure.in | 32 +++++++++ > libjuicer/sj-metadata-musicbrainz5.c | 3 +- > libjuicer/sj-metadata.c | 128 +++++++++++++++++++++++++++++++++++ > libjuicer/sj-metadata.h | 2 + > src/sj-main.c | 2 + > 5 files changed, 166 insertions(+), 1 deletion(-) > >diff --git a/configure.in b/configure.in >index 375e883..41cbb86 100644 >--- a/configure.in >+++ b/configure.in >@@ -79,6 +79,30 @@ if test "$have_mb5" = "yes" ; then > else > AC_MSG_ERROR([libmusicbrainz5 needs to be available for sound-juicer to build]) > fi >+# ISO-CODES >+AC_MSG_CHECKING([Whether not to check for iso-codes]) >+AC_ARG_ENABLE([iso-codes], >+ AS_HELP_STRING([--disable-iso-codes],[Whether not to check for iso-codes at build-time]), >+ [],[disable_iso_codes_check=no]) The double negation (whether not to check ... no) confuses me, I'd tend to go with --enable-iso-code=yes/no/auto >From 4a77c2a3bdfc3dad5fc2f0a529c8a6a4bcf12ff6 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Sun, 8 Jul 2012 11:13:16 +0100 >Subject: [PATCH 02/13] [multiple releases] Add disc count to AlbumDetails. > >Helps to tell multiple releases apart where one is a normal release >and the other is a special release with a bonus disc. Looks good >From 4d4ee12a28ee1152a57801a6f9d597fa3401779d Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Sun, 8 Jul 2012 11:14:07 +0100 >Subject: [PATCH 03/13] [multiple releases] Get disc count from musicbrainz. > >Helps to tell multiple releases apart where one is a normal release >and the other is a special release with a bonus disc. Looks good too >From 924a616fa6d20f8dae5391e215219e1135881165 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Mon, 18 Jun 2012 18:54:57 +0100 >Subject: [PATCH 04/13] [multiple releases] Add label to AlbumDetails. > >To disambiguate between albums with the same disc ID it would be >useful to display the recording label. NB musicbrainz provides a list >of recording labels for each release. > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 >--- > libjuicer/sj-structures.c | 10 ++++++++++ > libjuicer/sj-structures.h | 8 +++++++- > 2 files changed, 17 insertions(+), 1 deletion(-) > >diff --git a/libjuicer/sj-structures.c b/libjuicer/sj-structures.c >index 3dc29c3..72f54f3 100644 >--- a/libjuicer/sj-structures.c >+++ b/libjuicer/sj-structures.c >@@ -65,6 +65,7 @@ void album_details_free(AlbumDetails *album) > g_free (album->lyrics_url); > g_free (album->country); > g_free (album->type); >+ g_list_foreach (album->labels, (GFunc)label_details_free, NULL); > g_list_foreach (album->artists, (GFunc)artist_details_free, NULL); > g_list_free (album->artists); > g_free (album); >@@ -84,3 +85,12 @@ void artist_details_free (ArtistDetails *artist) > g_free (artist->joinphrase); > g_free (artist); > } >+ >+/* >+ * Free a LabelDetails >+ */ >+void label_details_free (LabelDetails *label) >+{ >+ g_free (label->name); >+ g_free (label->sortname); g_free (label) is missing here. >From fec2ee4bf001992d8cc4e9c53e3f90256071fa41 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Tue, 19 Jun 2012 11:14:08 +0100 >Subject: [PATCH 05/13] [multiple releases] Get labels from musicbrainz. > >To disambiguate between albums with the same disc ID it would be >useful to display the recording label. Parse the list of recording >labels provided by musicbrainz and store them in AlbumDetails. Looks good >From 2a30f96c5e441fabcae2ce77a64cb9cf7ece4af0 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Thu, 28 Jun 2012 19:01:24 +0100 >Subject: [PATCH 06/13] [multiple releases] Format recording label names. > >In order to display the list of label names that we retrieve from >musicbrainz we need to convert them into one string. > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 >--- > src/sj-main.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > >diff --git a/src/sj-main.c b/src/sj-main.c >index 7c58969..83d70b5 100644 >--- a/src/sj-main.c >+++ b/src/sj-main.c >@@ -783,6 +783,36 @@ static void selected_album_changed (GtkTreeSelection *selection, > } > > /** >+ * Utility function to format label string for multiple_album_dialog >+ */ >+static GString* format_label_text (GList* labels) >+{ >+ int count; >+ GString *label_text; >+ >+ if (labels == NULL) >+ return NULL; >+ >+ label_text = g_string_new (NULL); >+ count = g_list_length (labels); >+ while (count > 2) { >+ g_string_append (label_text, ((LabelDetails*)labels->data)->name); >+ g_string_append (label_text, ", "); wondering if some people might want to translate the ', ' and '& ', in which case using _append_printf(_("%s, ")) along with a translator comment would be better. >+ labels = labels->next; >+ count--; >+ } >+ >+ if (count > 1) { >+ g_string_append (label_text, ((LabelDetails*)labels->data)->name); >+ g_string_append (label_text, " & "); >+ } >+ >+ g_string_append (label_text, ((LabelDetails*)labels->data)->name); >+ >+ return label_text; >+} >+ >+/** > * Utility function for when there are more than one albums available > */ > AlbumDetails* multiple_album_dialog(GList *albums) >-- >1.8.0 > > >From 79d6c6f2efb557b93afd7d08863776336ba73388 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Fri, 29 Jun 2012 10:07:28 +0100 >Subject: [PATCH 07/13] [multiple releases] Format release details. > >To display the recording label, year and country on one line we need a >function to combine them into a string. > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 >--- > src/sj-main.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > >diff --git a/src/sj-main.c b/src/sj-main.c >index 83d70b5..be1dd6f 100644 >--- a/src/sj-main.c >+++ b/src/sj-main.c >@@ -813,6 +813,49 @@ static GString* format_label_text (GList* labels) > } > > /** >+ * Utility function for multiple_album_dialog to format the >+ * release label, date and country. >+ */ >+static GString* format_release_details (AlbumDetails *album) >+{ >+ GString *release_details = g_string_new (NULL); >+ >+ if (!sj_str_is_empty (album->country)){ >+ g_string_printf (release_details, _("Released: %s"), album->country); >+ if (album->release_date) { >+ g_string_append_printf (release_details, >+ _(" in %d"), g_date_get_year (album->release_date)); >+ } Building a sentence this way through string concatenation isn't going to work well with respect to translations, going with several full sentences depending on which data is available would be better. >+ if (album->labels) { >+ GString *label_text = format_label_text (album->labels); >+ g_string_append (release_details, _(" on ")); >+ g_string_append (release_details, label_text->str); >+ g_string_free (label_text, TRUE); >+ } >+ >+ } else if (album->release_date) { >+ g_string_printf (release_details, >+ _("Released in %d"), g_date_get_year (album->release_date)); >+ if (album->labels) { >+ GString *label_text = format_label_text (album->labels); >+ g_string_append (release_details, _(" on ")); >+ g_string_append (release_details, label_text->str); >+ g_string_free (label_text, TRUE); >+ } >+ >+ } else if (album->labels) { >+ GString *label_text = format_label_text (album->labels); >+ g_string_printf (release_details, _("Released on %s"), label_text->str); >+ g_string_free (label_text, TRUE); >+ >+ } else { >+ g_string_printf (release_details, _("Release label, year & country unknown")); >+ } >+ >+ return release_details; >+} >+ >+/** > * Utility function for when there are more than one albums available > */ > AlbumDetails* multiple_album_dialog(GList *albums) >-- >1.8.0 > (stopping the review there for that comment)
Comment on attachment 229135 [details] [review] Updated to use iso-codes and reorder release_details string. >From fba02adee0bf877dfee26021e99a0ca9051ed892 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Sat, 30 Jun 2012 11:18:36 +0100 >Subject: [PATCH 08/13] [multiple releases] Sort release details. > >To display the entries in the multiple release dialog in a consistent >manner we need to be able to sort them. > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 This one can be merged with "Sort displayed entries" (which is very small), this will avoid having sort_release_info() introduction separated from its actual use, and this also avoids defining a new static function and not using it (gcc can warn on that). > >From 068856445d728df5123bd33a22148345ae1da8d3 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Thu, 15 Nov 2012 11:01:07 +0000 >Subject: [PATCH 10/13] [multiple releases] Append 'Disc (m/n)' to title. > >Enables disambiguation between the same album released as a single or >part of a box set. > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 >--- > src/sj-main.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > >diff --git a/src/sj-main.c b/src/sj-main.c >index 8236f6e..159f75e 100644 >--- a/src/sj-main.c >+++ b/src/sj-main.c >@@ -1008,14 +1008,20 @@ AlbumDetails* multiple_album_dialog(GList *albums) > for (; albums ; albums = g_list_next (albums)) { > GtkTreeIter iter; > AlbumDetails *album = (AlbumDetails*)(albums->data); >+ GString *album_title = g_string_new (album->title); >+ >+ if (album->disc_number > 0 && album->disc_count > 0) >+ g_string_append_printf (album_title," (%s %d/%d)", >+ _("Disc"), album->disc_number, album->disc_count); This one is not very good translation-wise either, _(" (Disk %d/%d)") as the format string would be better > >From 05456cb9d134e288815be4696201c6f0c0c2132f Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Fri, 29 Jun 2012 10:08:15 +0100 >Subject: [PATCH 11/13] [multiple releases] Display label, year & country. > >With just the album title and artist, it is sometimes impossible to >tell the difference between all the releases listed. Adding the >recording label, year and country will help to distinguish between >different versions of the same album. > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 >--- > src/sj-main.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > >diff --git a/src/sj-main.c b/src/sj-main.c >index 159f75e..5994335 100644 >--- a/src/sj-main.c >+++ b/src/sj-main.c >@@ -956,6 +956,7 @@ AlbumDetails* multiple_album_dialog(GList *albums) > { > COLUMN_TITLE, > COLUMN_ARTIST, >+ COLUMN_YEAR, > COLUMN_DETAILS, > COLUMN_COUNT > }; >@@ -965,6 +966,7 @@ AlbumDetails* multiple_album_dialog(GList *albums) > GtkCellArea *cell_area = gtk_cell_area_box_new (); > GtkCellRenderer *title_renderer = gtk_cell_renderer_text_new (); > GtkCellRenderer *artist_renderer = gtk_cell_renderer_text_new (); >+ GtkCellRenderer *year_renderer = gtk_cell_renderer_text_new (); > > dialog = GET_WIDGET ("multiple_dialog"); > g_assert (dialog != NULL); >@@ -979,6 +981,7 @@ AlbumDetails* multiple_album_dialog(GList *albums) > gtk_tree_view_column_set_title (column, _("Albums")); > gtk_tree_view_column_pack_start (column, title_renderer, TRUE); > gtk_tree_view_column_pack_start (column, artist_renderer, TRUE); >+ gtk_tree_view_column_pack_start (column, year_renderer, TRUE); > g_object_set(title_renderer, "weight", PANGO_WEIGHT_BOLD, "weight-set", > TRUE, NULL); > g_object_set(artist_renderer, "style", PANGO_STYLE_ITALIC, "style-set", >@@ -987,12 +990,15 @@ AlbumDetails* multiple_album_dialog(GList *albums) > COLUMN_TITLE); > gtk_tree_view_column_add_attribute (column, artist_renderer, "text", > COLUMN_ARTIST); >+ gtk_tree_view_column_add_attribute (column, year_renderer, "text", >+ COLUMN_YEAR); > > g_signal_connect (albums_listview, "row-activated", > G_CALLBACK (album_row_activated), dialog); > > albums_store = gtk_list_store_new (COLUMN_COUNT, G_TYPE_STRING, >- G_TYPE_STRING, G_TYPE_POINTER); >+ G_TYPE_STRING, G_TYPE_STRING, >+ G_TYPE_POINTER); > gtk_tree_view_append_column (GTK_TREE_VIEW (albums_listview), column); > > gtk_tree_view_set_model (GTK_TREE_VIEW (albums_listview), >@@ -1009,6 +1015,7 @@ AlbumDetails* multiple_album_dialog(GList *albums) > GtkTreeIter iter; > AlbumDetails *album = (AlbumDetails*)(albums->data); > GString *album_title = g_string_new (album->title); >+ GString *release_details = format_release_details (album); > > if (album->disc_number > 0 && album->disc_count > 0) > g_string_append_printf (album_title," (%s %d/%d)", >@@ -1018,11 +1025,15 @@ AlbumDetails* multiple_album_dialog(GList *albums) > gtk_list_store_set (albums_store, &iter, > COLUMN_TITLE, album_title->str, > COLUMN_ARTIST, album->artist, >+ COLUMN_YEAR, release_details->str, > COLUMN_DETAILS, album, > -1); From a naming point of view, putting release_details in a column called "YEAR" is unexpected, maybe the naming could be improved here? All the rest looks good to me, thanks yet another great improvement!
(In reply to comment #21) > >diff --git a/configure.in b/configure.in > >index 375e883..41cbb86 100644 > >--- a/configure.in > >+++ b/configure.in > >@@ -79,6 +79,30 @@ if test "$have_mb5" = "yes" ; then > > else > > AC_MSG_ERROR([libmusicbrainz5 needs to be available for sound-juicer to build]) > > fi > >+# ISO-CODES > >+AC_MSG_CHECKING([Whether not to check for iso-codes]) > >+AC_ARG_ENABLE([iso-codes], > >+ AS_HELP_STRING([--disable-iso-codes],[Whether not to check for iso-codes at build-time]), > >+ [],[disable_iso_codes_check=no]) > > The double negation (whether not to check ... no) confuses me, I'd tend to go > with --enable-iso-code=yes/no/auto > I'd use this much simpler test: # ISO-CODES PKG_CHECK_MODULES(ISO_CODES, [iso-codes]) if $PKG_CONFIG --variable=domains iso-codes | grep 3166 >/dev/null ; then AC_DEFINE_UNQUOTED([ISO_CODES_PREFIX],["`$PKG_CONFIG --variable=prefix iso-codes`"],[ISO codes prefix]) else AC_MSG_ERROR([iso-codes database does not support iso3166 country codes]) fi as iso-codes is kind of mandatory after your patch.
(In reply to comment #21) > >diff --git a/configure.in b/configure.in > >index 375e883..41cbb86 100644 > >--- a/configure.in > >+++ b/configure.in > >@@ -79,6 +79,30 @@ if test "$have_mb5" = "yes" ; then > > else > > AC_MSG_ERROR([libmusicbrainz5 needs to be available for sound-juicer to build]) > > fi > >+# ISO-CODES > >+AC_MSG_CHECKING([Whether not to check for iso-codes]) > >+AC_ARG_ENABLE([iso-codes], > >+ AS_HELP_STRING([--disable-iso-codes],[Whether not to check for iso-codes at build-time]), > >+ [],[disable_iso_codes_check=no]) > > The double negation (whether not to check ... no) confuses me, I'd tend to go > with --enable-iso-code=yes/no/auto I propose http://cgit.freedesktop.org/~teuf/sound-juicer/commit/?id=de9833be04be605bcd1e65f4072c6fbababe6f4a though I might get convinced that an optional runtime dep could work better > >From 924a616fa6d20f8dae5391e215219e1135881165 Mon Sep 17 00:00:00 2001 > >From: Phillip Wood <phillip.wood@dunelm.org.uk> > >Date: Mon, 18 Jun 2012 18:54:57 +0100 > >Subject: [PATCH 04 [details]/13] [multiple releases] Add label to AlbumDetails. > > > >To disambiguate between albums with the same disc ID it would be > >useful to display the recording label. NB musicbrainz provides a list > >of recording labels for each release. > > > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 > >--- > >@@ -84,3 +85,12 @@ void artist_details_free (ArtistDetails *artist) > > g_free (artist->joinphrase); > > g_free (artist); > > } > >+ > >+/* > >+ * Free a LabelDetails > >+ */ > >+void label_details_free (LabelDetails *label) > >+{ > >+ g_free (label->name); > >+ g_free (label->sortname); > > g_free (label) is missing here. Fixed in http://cgit.freedesktop.org/~teuf/sound-juicer/commit/?id=b2f6bce46d6ae2e9d6f14e85f6af86bbd1db323a > >diff --git a/src/sj-main.c b/src/sj-main.c > >index 83d70b5..be1dd6f 100644 > >--- a/src/sj-main.c > >+++ b/src/sj-main.c > >@@ -813,6 +813,49 @@ static GString* format_label_text (GList* labels) > > } > > > > /** > >+ * Utility function for multiple_album_dialog to format the > >+ * release label, date and country. > >+ */ > >+static GString* format_release_details (AlbumDetails *album) > >+{ > >+ GString *release_details = g_string_new (NULL); > >+ > >+ if (!sj_str_is_empty (album->country)){ > >+ g_string_printf (release_details, _("Released: %s"), album->country); > >+ if (album->release_date) { > >+ g_string_append_printf (release_details, > >+ _(" in %d"), g_date_get_year (album->release_date)); > >+ } > > Building a sentence this way through string concatenation isn't going to work > well with respect to translations, going with several full sentences depending > on which data is available would be better. One way of doing that at http://cgit.freedesktop.org/~teuf/sound-juicer/commit/?id=7acd76f7a5981cacc434c7541612b27733734b85 > > > > >From 068856445d728df5123bd33a22148345ae1da8d3 Mon Sep 17 00:00:00 2001 > >From: Phillip Wood <phillip.wood@dunelm.org.uk> > >Date: Thu, 15 Nov 2012 11:01:07 +0000 > >Subject: [PATCH 10 [details]/13] [multiple releases] Append 'Disc (m/n)' to title. > > > >Enables disambiguation between the same album released as a single or > >part of a box set. > > > >https://bugzilla.gnome.org/show_bug.cgi?id=674926 > >--- > > src/sj-main.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > >diff --git a/src/sj-main.c b/src/sj-main.c > >index 8236f6e..159f75e 100644 > >--- a/src/sj-main.c > >+++ b/src/sj-main.c > >@@ -1008,14 +1008,20 @@ AlbumDetails* multiple_album_dialog(GList *albums) > > for (; albums ; albums = g_list_next (albums)) { > > GtkTreeIter iter; > > AlbumDetails *album = (AlbumDetails*)(albums->data); > >+ GString *album_title = g_string_new (album->title); > >+ > >+ if (album->disc_number > 0 && album->disc_count > 0) > >+ g_string_append_printf (album_title," (%s %d/%d)", > >+ _("Disc"), album->disc_number, album->disc_count); > > This one is not very good translation-wise either, _(" (Disk %d/%d)") as the > format string would be better Fixed in http://cgit.freedesktop.org/~teuf/sound-juicer/commit/?id=66ebceaf6a54e488d490c0864a2995e4a6d76e5c
In "[PATCH 10 [details]/13] [multiple releases] Append 'Disc (m/n)' to title.", I'm wondering if we should hide Disc (1/1) and only show it when one of these numbers is > 1.
Created attachment 242000 [details] [review] Revised iso-codes patch Hi Christophe, thanks for looking at the patch and taking the time to fix some of the code, I've attached a revised patch based on your comments and pulling in your suggestions. Simplifying the check for iso-codes is definitely a good idea. I'd not thought about the problems of translation with the way I had written it. It now uses your suggestion slightly modified to get rid of a spurious warning from gcc about freeing an uninitialized variable. I've renamed COLUMN_YEAR to COLUMN_RELEASE_DETAILS and done the same for year_renderer. I think that only showing disc numbers for releases with more than one disc is a good idea, I've just modified the existing test rather than add a new one as you suggested as I thought it was clearer. I hope it's in good shape now, let me know what you think.
Phillip, could you drop me an email? The address email you are using in bugzilla does not seem to be working.
Seems to have been pushed already.