Created attachment 314164 [details] [review]
added barcode and catalog number to multiple_album_dialog

When multiple musicbrainz releases match the CD it is hard to choose
the correct release in the multiple album dialog. Especially when
multiple releases of album/CD exist.

To distinguish multiple versions of the same album, I extended the
details on the possible albums shown by the multiple_album_dialog to
contain the barcode and the catalog number used by the label releasing
the album/release.

Extended the AlbumDetails struct by a field for the barcode and the
LabelDetails by a field for the catalog_number.
Furthermore I added a function format_catalog_number_text to sj-main.c
to generate a string containing a list of catalog numbers and modified
the format_release_details function to add a second line of details
to the multiple_album_dialog with the catalog numbers and barcode.

Review of attachment 314164 [details] [review]:

Hi Andreas, thanks for your patch, this would be a useful addition to sound-juicer. I think the changes to sj-main.c could be improved a bit but the other changes are fine as they are.

It's great to have such a detailed commit message but I'd change the tone to say what the patch does rather than what you have done (that is get rid of all the references to ‘I’). It should also explain why the label, country and year are insufficient to distinguish between different versions of the same album. I think the last paragraph is unnecessary - the commit message should explain why the changes were made and how they help improve the multiple album dialog, it doesn't need such a detailed list of the changes.

::: src/sj-main.c
@@ +874,3 @@
+ * catalog number and barcode of a release.
+ */
+static GString* format_catalog_number_text (GList* labels)

As you only want the text, it would be clearer to return a gchar* rather than GString*

@@ +888,3 @@
+   count = 0;
+   while (labels) {
+     if (((LabelDetails*)labels->data)->catalog_number)

Generally when iterating over a single list I'm trying to standardize on
GList *l;
for (l = labels; l != NULL; l = l->next) {
  ..
}

@@ +898,3 @@
+   labels = labels_begin;
+
+   /* You could make a distinction between singular and plural Catalog# here */

There's a gettext function to do just that - https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html - we should use it

@@ +901,3 @@
+   /* Translators: this string is a list of catalog number(s) used by the
+    *  label(s) to identify the release */
+   catalog_text = g_string_new (_("Catalog#: "));

Catalog# is a bit ugly, can we just say Catalog number or something else?

@@ +902,3 @@
+    *  label(s) to identify the release */
+   catalog_text = g_string_new (_("Catalog#: "));
+   while (labels) {

Again use a for loop please

I'm a bit concerned that catalog numbers from different labels all end up together with no way to tell which label they came from but maybe that doesn't matter too much.

@@ +903,3 @@
+   catalog_text = g_string_new (_("Catalog#: "));
+   while (labels) {
+     if (((LabelDetails*)labels->data)->catalog_number) {

As you're using the catalog number again later on it would be clearer to do
gchar *catalog_number = ((LabelDetails*) labels->data)->catalog_number;
if (catalog_number != NULL) {
  ...
}

Also note the explicit NULL check

@@ +907,3 @@
+           g_string_append (catalog_text,
+                            ((LabelDetails*)labels->data)->catalog_number);
+           g_string_append (catalog_text, ", ");

g_string_append_printf ("%s, ", catalog_number);
would be clearer

@@ +917,3 @@
+   }
+
+   return catalog_text;

To return a gchar* rather than GString* do
return g_string_free(catalog_text, FALSE);

@@ +927,3 @@
 {
   gchar *details;
+  gchar *first_line;

I'd change details to be a GString* and then you can just call g_string_append_printf() and friends to build the whole string with the catalog number and barcode at the end.

@@ +930,2 @@
   GString *label_text = NULL;
+  GString *catalog_number_text = NULL;

Make this a gchar* (Sometime I ought to change the label formatting to do the same)

@@ +1004,3 @@
+                             album->barcode, NULL);
+    } else {
+      details = g_strconcat (first_line, NULL);

Making details a GString* simplifies this as it isn't needed. (For future reference this would be better as g_strdup(first_line);)

@@ +1013,3 @@
+  if (catalog_number_text)
+    g_string_free (catalog_number_text, TRUE);
+

This can be just 
g_free(catalog_number_text);
if we make it a gchar*

@@ +1014,3 @@
+    g_string_free (catalog_number_text, TRUE);
+
+  g_free (first_line);

This isn't needed if you change details to be a GString*

@@ +1017,1 @@
   return details;

Change this to
return g_string_free (details, FALSE);

Hi Philipp,

thanks for reviewing the patch and for your suggestions.
They were very helpful.
> 
> It's great to have such a detailed commit message but I'd change the tone to
> say what the patch does rather than what you have done (that is get rid of
> all the references to ‘I’). It should also explain why the label, country
> and year are insufficient to distinguish between different versions of the
> same album. I think the last paragraph is unnecessary - the commit message
> should explain why the changes were made and how they help improve the
> multiple album dialog, it doesn't need such a detailed list of the changes.

Right, reading the commit message again it sounds a bit weird. I wrote it right after the bug report and apparently forgot to switch from bug report tone to commit message tone. :)

> 
> I'm a bit concerned that catalog numbers from different labels all end up
> together with no way to tell which label they came from but maybe that
> doesn't matter too much.
> 

Yes, that's right. However, I can tell from my musicbrainz experience that there's hardly any release having more than one catalog number. In fact I think I never saw it.
I implemented the multiple catalog number string just in case... You never know.
I also thought about mixing it with the existing year, label & country string and append it to the right label, but this sentence is so nice to look at and I didn't want to destroy it with some ugly numbers.
So I propose to keep it separate. What do you think?

Other than that I will implement your improvements and submit a new patch.

(In reply to Phillip Wood from comment #2)

While improving the patch, a question concerning your suggestions came up:

> 
> As you're using the catalog number again later on it would be clearer to do
> gchar *catalog_number = ((LabelDetails*) labels->data)->catalog_number;
> if (catalog_number != NULL) {
>   ...
> }
> 
> Also note the explicit NULL check
> 

I assume be explicit NULL check you mean "if(catalog_number != NULL)" instead of "if(catalog_number)". Scanning through the rest of the code it is unclear to me in when to prefer an explicit "!= NULL"-form over a simple if(ptr).
Could you tell me your guideline there?

(In reply to Andreas H from comment #4)
> (In reply to Phillip Wood from comment #2)
> 
> While improving the patch, a question concerning your suggestions came up:
> 
> > 
> > As you're using the catalog number again later on it would be clearer to do
> > gchar *catalog_number = ((LabelDetails*) labels->data)->catalog_number;
> > if (catalog_number != NULL) {
> >   ...
> > }
> > 
> > Also note the explicit NULL check
> > 
> 
> I assume be explicit NULL check you mean "if(catalog_number != NULL)"
> instead of "if(catalog_number)". Scanning through the rest of the code it is
> unclear to me in when to prefer an explicit "!= NULL"-form over a simple
> if(ptr).
> Could you tell me your guideline there?

Good question - for new code always use an explicit NULL check please. As you've seen there is quite a lot of old code that does not follow this convention.

(In reply to Andreas H from comment #3)
> > 
> > I'm a bit concerned that catalog numbers from different labels all end up
> > together with no way to tell which label they came from but maybe that
> > doesn't matter too much.
> > 
> 
> Yes, that's right. However, I can tell from my musicbrainz experience that
> there's hardly any release having more than one catalog number. In fact I
> think I never saw it.
> I implemented the multiple catalog number string just in case... You never
> know.
> I also thought about mixing it with the existing year, label & country
> string and append it to the right label, but this sentence is so nice to
> look at and I didn't want to destroy it with some ugly numbers.
> So I propose to keep it separate. What do you think?

I think that's probably best. Part of me wonders if we really need to show both the catalog number and the barcode as all the CD covers I've looked at show both but there will be some albums in MusicBrainz that only have one or the other so it's probably best to leave it as it is. The barcode information will enable us to avoid showing the multiple album dialog in some cases - see bug 757247.

> Other than that I will implement your improvements and submit a new patch.
That's great, thanks again for your work on this.

Created attachment 314770 [details] [review]
added barcode and label catalog number to multiple_album_dialog

Extended the details on the possible albums shown by the
multiple_album_dialog by the barcode and the catalog number used by
the label releasing the album/release.
This eases the selection of the correct musicbrainz release, when an
album was released several times and the release label and country
do not offer enough information to identify the release.

Review of attachment 314770 [details] [review]:

Thanks for reworking this, it will be a useful addition to sound-juicer. The patch now looks fine apart from a couple of formatting issues that I can fix before I push it. Congratulations on you first patch for sound-juicer!

Patch 314770 has been pushed as commit d6315bf - Add barcode and catalog number to multiple_album_dialog

Thanks for your contribution, it will be available in the 3.20 release
of sound-juicer.