GNOME Bugzilla – Bug 661646
Add support for composer tags
Last modified: 2012-09-06 18:43:34 UTC
Created attachment 198927 [details] [review] Adds basic support for composer tag and parsing classical album titles For classical albums it would be nice if sound juicer supported setting the composer tag in the ripped tracks. The patch (against the current git head) attached adds basic support for doing this. There are a few things to note: 1 - At the moment the parsing of the track title from MusicBrainz to determine if there is a classical artist in the title is quite simplistic and will probably give false positives for some non-classical albums. 2 - The artist sortname is not set 3 - The artist ID is wrongly set to the ID of the composer 4 - The composer sortname is not set 5 - There is no way to use the composer in the path for the saved files I intend to address these issues soon. Fixing 1 by checking the string against known MusicBrainz artist will make 2 & 3 easy to fix. 4 & 5 should be reasonably simple as well. I hope this is useful to the project, any feedback on the patch and it's implementation would be very welcome.
Created attachment 206564 [details] [review] Basic support for composer tags Updated patch to current head which addresses issues 2-5 above. It allows the user to manually enter the composer for the album and tracks in the user interface. The composer can be used in the path/filename of the saved file. There is support for a sortable composer name but it is not currently possible to set this from the UI (I plan on setting it from MusicBrainz metadata in the future). This patch does not attempt to set the composer from the MusicBrainz metadata, this area still needs more work to be reliable as MusicBrainz does not have a composer field. For simple classical albums it uses the artist field for the composer and puts the artist in parentheses in the title, but we need to be careful of false positives where the album title has something in parentheses but is not a classical album. Classical compilations etc. have more complicated rules. Now that we have libmusicbrainz4 support we can also try using work relationships which have composer and arranger information. Unfortunately not many releases have work relationships in the database at the moment. This patch is very usable and I would really like it to be considered for inclusion in the main codebase. It is no effort at all to enter the composer in the UI, all the tags are set correctly and the composer can be used in the path/filename. Please let me know if you think it is good enough for inclusion. (The first two commits in the patchfile are fixes for two minor bugs which I noticed but are unrelated to the composer tag support)
Can you also attach a screenshot of this patch in action?
Created attachment 206582 [details] Screenshot showing new UI with composer fields
Created attachment 206583 [details] Preferences dialog showing a couple of naming options which use composer data
With that UI, not by default. Is there a way of detecting a classical CD from Musicbrainz metadata? You're impacting the experience for everyone who doesn't listen to classical, which more people than do listen to classic.
Just so I'm clear your objection is to displaying the composer fields for non classical albums? Musicbrainz does not store genre information. AFIK the only way to determine if an album is classical (or more accurately has composer information stored in the artist tag) is to look for an artist name in parentheses in the title. I have a very basic implementation of this (not included in the patch I submitted) using libmusicbrainz3 but it needs to be more robust and the Musicbrainz queries need to be ported to libmusicbrainz4. Musicbrainz also allows composers to be related to 'works' which are in turn related to recordings. I think libmusicbrainz4 allows access to those relationships but they are not present for many recordings at the moment and it may need several queries to get from the recording to the composer information which might be a bit slow if we have to do it for every track. I'm biased but I'm not sure displaying the composer fields really negatively impacts the experience for non classical albums. All albums have composers, it is just that they are often forgotten. If you want the composer fields to be optional then there are several possibilities: 1 - Only show the composer fields if the user sets the genre to 'Classical' 2 - Implement a command-line switch or user preference to toggle the display (this is probably not really in the spirit of gnome) 3 - Wait until I can reliably determine if an album has composer information in musicbrainz and only display the fields for those albums. 4 - A combination of 1 & 3 where we only display the composer fields and look for composer information in the musicbrainz metadata if the user sets the genre to 'Classical'. This has the advantage that we know that the album is definitely classical when we parse the metadata for composer information. On the down side it would be better to find a way to robustly determine the genre automatically Thanks for your interest, let me know if any of the options above appeal as a way forward.
Created attachment 212754 [details] [review] Updated composer support patch for libmusicbrainz4 Updated patch: * Modify UI so that composer fields are only shown if the user enters a classical genre or there is composer metadata from one of the metadata getters. Otherwise the normal UI is shown. This should address previous concerns about distracting users encoding non-classical albums. * Use libmusicbrainz4 to retrieve composer metadata using composer relationships. To do this reliably required an extension of the libmusicbrainz4 API. Many thanks to Andy Hawkins, the libmusicbrainz maintainer, for making these changes. As a consequence of this libmusicbrainz >= 4.0.1 is required to compile this patch. This library should be released soon, in the meantime it is available via git at https://github.com/musicbrainz/libmusicbrainz.git The composer support is usable now and should not impact on other users. I hope this can be considered for inclusion, if you've got any questions just let me know
Interesting. Before I look at the code, could you attach a screenshot of it in "classical" mode?
Created attachment 212787 [details] Screenshot of UI Hi, the screenshot shows the normal UI at startup at the top and then then "classical" mode once a classical album has been detected from the composer relationships in musicbrainz below. (This is implemented by hiding the composer labels & fields by default and only setting them visible if they should be shown.)
Is it expected that Artist/Composer are the same on all tracks on this screenshot? I thought these changes were about Performer VS Composer I had a quick look over the patch series, just a few comments, a deeper look is still needed :) - one of the patch changes code using strcasecmp to compare artist/song names, it would be better to use UTF8-aware comparison functions (g_utf8_casefold and friends). - "Clear album composer sortname when name is edited" removes a call to remove_musicbrainz_ids, without looking more into the code I don't know if it's intentional - The whitespaces cleanups in 2 of the commits are a bit annoying (especially in the one mentioning the whitespace changes in the ChangeLog) - (very minor nit, it's perfectly fine to ignore) "Add composer field to tracks" removes 2 unused variables from on_cell_edited, I tend to move such changes to a separate commit to make it clear that it's unrelated to what the commit is doing I'd tend to reorder the commits to something like Add composer field to tracks Add composer field to album data Add composer sortname to album & track data Bump libmusicbrainz requirement to 4.0.1 Get composer metadata from musicbrainz relations Fill album composer using track composers Remove deprecated libmusicbrainz methods. Set composer sortname tag when extracting Initalise track composers in UI from track data Add album composer entry to UI Disable album composer entry when extracting Support editing of album composer Clear album composer sortname when name is edited Unset track composer/artist sortname/id if edited Add album & track composer to schema for file name and path Support composer in prefs for files & paths Expand composer format specifiers in files & paths Update genre_entry if album->genre is set Only show composer fields when needed to make the progression more logical. I've tested that they can be rebased this way without conflicts, but I haven't compile-tested them. I wasn't sure if "Update genre_entry if album->genre is set" could be moved last or not, and "Only show composer fields when needed" can probably be moved near the UI bits ("Add album composer entry to UI" and the next commit), but if there are bad conflicts that's not worth it. If this order makes sense to you, the 1st commit would need to be split though, with the tag setting going with the composer_sortname tag setting, and the UI bits moving after the libmb4 changes.
(In reply to comment #10) Hi Christophe, thanks for your comments, this is the first patch I've submitted to a project like this so any guidance is greatly appreciated. > Is it expected that Artist/Composer are the same on all tracks on this > screenshot? I thought these changes were about Performer VS Composer The changes are to get composer metadata from musicbrianz and set the appropriate tags, they do not change the way artists are handled. The composers & artists are the same in the screenshot as they are the same in musicbrainz (see http://musicbrainz.org/release/ed5e82e7-5502-45b2-b042-3bb321c6b499). This is unfortunately quite common, they are rewriting the classical style guidelines at the moment which will hopefully address this. I guess we could work around this by comparing the track artist to the track composer and if they are the same use the album artist for the track artist instead but that might cause problems with other albums. I don't think my changes make things worse than they are already as you would get the composer as the artist at the moment. At least with the changes you get the correct data in the composer field. > I had a quick look over the patch series, just a few comments, a deeper look is > still needed :) > - one of the patch changes code using strcasecmp to compare artist/song names, > it would be better to use UTF8-aware comparison functions (g_utf8_casefold and > friends). I can change that. (I used strcasecmp as it is already used in similar places in the code) > - "Clear album composer sortname when name is edited" removes a call to > remove_musicbrainz_ids, without looking more into the code I don't know if it's > intentional That was intentional, I should have said something in the changelog. My thinking was that if I make a minor change to the formatting of the composer or artist name I don't want to loose all the musicbrainz tags as it is still the same album. I can put it back if you think that would be better. > - The whitespaces cleanups in 2 of the commits are a bit annoying (especially > in the one mentioning the whitespace changes in the ChangeLog) Yes ,I agree it's annoying - do you know of a good way of dealing with this? I left them as they are because I couldn't think of anything better to do. Assuming that removing excess whitespace is a good thing I could add an extra commit to separate whitespace changes from code changes? > - (very minor nit, it's perfectly fine to ignore) "Add composer field to > tracks" removes 2 unused variables from on_cell_edited, I tend to move such > changes to a separate commit to make it clear that it's unrelated to what the > commit is doing I'll try and remember for the future > > I'd tend to reorder the commits to something like > Add composer field to tracks > Add composer field to album data > Add composer sortname to album& track data > Bump libmusicbrainz requirement to 4.0.1 > Get composer metadata from musicbrainz relations > Fill album composer using track composers > Remove deprecated libmusicbrainz methods. > Set composer sortname tag when extracting > Initalise track composers in UI from track data > Add album composer entry to UI > Disable album composer entry when extracting > Support editing of album composer > Clear album composer sortname when name is edited > Unset track composer/artist sortname/id if edited > Add album& track composer to schema for file name and path > Support composer in prefs for files& paths > Expand composer format specifiers in files& paths > Update genre_entry if album->genre is set > Only show composer fields when needed > > to make the progression more logical. I've tested that they can be rebased this > way without conflicts, but I haven't compile-tested them. I wasn't sure if > "Update genre_entry if album->genre is set" could be moved last or not, and > "Only show composer fields when needed" can probably be moved near the UI bits > ("Add album composer entry to UI" and the next commit), but if there are bad > conflicts that's not worth it. If this order makes sense to you, the 1st commit > would need to be split though, with the tag setting going with the > composer_sortname tag setting, and the UI bits moving after the libmb4 changes. > I'll have a think about the best way of reordering them. Thanks again for your feedback Phil
(In reply to comment #11) > > - The whitespaces cleanups in 2 of the commits are a bit annoying (especially > > in the one mentioning the whitespace changes in the ChangeLog) > > Yes ,I agree it's annoying - do you know of a good way of dealing with this? I > left them as they are because I couldn't think of anything better to do. > Assuming that removing excess whitespace is a good thing I could add an extra > commit to separate whitespace changes from code changes? > Yes, when you want to make such changes, a separate commit is a good thing when there are too many of them. A few such changes mixed in an unrelated commit is ok with me (other people may disagree), but when there are too many, it just makes the review more difficult. It's easy to generate 2 distinct commits using git once you notice that git show -w will show the commit without the whitespace changes. If the commit you want to split is $SHA1, then something like: git rebase -i $SHA1^ mark $SHA1 with edit git reset --hard HEAD^ (when it stops at $SHA1 for editing) git show -w $SHA1 |patch -p1 git commit -c $SHA1 (the non-whitespace changes are committed) git checkout $SHA1 (this will readd the whitespace changes in the working tree and index) git commit -m "remove whitespace" git rebase --continue Since you made me look at this commit again, another nit ;) In "Expand composer format specifiers in files & paths" the commit message is composed of 3 different items, this is a very good hint that the commit could be split in 3 different commits. The reason I'm mentioning this is that the smaller the commits, the easier the review is. But no need to split this patch, this is mainly a comment in passing for future patches ;)
(In reply to comment #12) > (In reply to comment #11) > Yes, when you want to make such changes, a separate commit is a good thing when > there are too many of them. A few such changes mixed in an unrelated commit is > ok with me (other people may disagree), but when there are too many, it just > makes the review more difficult. > It's easy to generate 2 distinct commits using git once you notice that git > show -w will show the commit without the whitespace changes. If the commit you > want to split is $SHA1, then something like: > > git rebase -i $SHA1^ > mark $SHA1 with edit > git reset --hard HEAD^ (when it stops at $SHA1 for editing) > git show -w $SHA1 |patch -p1 > git commit -c $SHA1 (the non-whitespace changes are committed) > git checkout $SHA1 (this will readd the whitespace changes in the working tree > and index) > git commit -m "remove whitespace" > git rebase --continue > > Since you made me look at this commit again, another nit ;) In "Expand composer > format specifiers in files & paths" the commit message is composed of 3 > different items, this is a very good hint that the commit could be split in 3 > different commits. The reason I'm mentioning this is that the smaller the > commits, the easier the review is. But no need to split this patch, this is > mainly a comment in passing for future patches ;) Thanks for the tip, hopefully I'll have some revised patches ready by the beginning of next week.
I wanted to spend some time reviewing this series today, and instead ending up filing https://bugzilla.gnome.org/show_bug.cgi?id=675984 https://bugzilla.gnome.org/show_bug.cgi?id=675990 because of runtime warnings that showed up when I wanted to test your patch :-/
(In reply to comment #14) > I wanted to spend some time reviewing this series today, and instead ending up > filing > https://bugzilla.gnome.org/show_bug.cgi?id=675984 > https://bugzilla.gnome.org/show_bug.cgi?id=675990 > because of runtime warnings that showed up when I wanted to test your patch :-/ Oh dear, things are never straight forward when you want them to be are they. I've put another patch set together incorporating the changes that you suggested and a couple of others. * Caseless string comparisons are now UTF-8 safe. * Re-ordered and split some commits to (hopefully) be more logical. * Clarified wording of a couple of commit messages * Merged all the changes in sound-juicer.ui into one commit for clarity. * Separated whitespace changes from code changes. In the end as there were several files affected by this I rebased onto a branch with all the trailing whitespace striped. This gives a rather large commit at the start of the patch set but it contains only trailing whitespace changes. I can split it if you think that would be better. At the moment it strips all trailing whitespace from all *.[ch], if you think this might cause problems for others I can restrict it to just the files that have code changes as well. * I've moved is_empty which I had defined as static in two different files out into sj-utils and renamed it sj_str_is_empty. In "Clear album composer sortname when name is edited" I've not reinstated the call to remove_musicbrainz_ids yet but can do in the future. Thanks for looking at the patch. I'll be offline for a week starting tomorrow but will happily answer any queries you have on my return :-)
Created attachment 214076 [details] [review] Revised patches for composer support for libmusicbrainz4 Here are the patches I should have attached to my last message....
Hi Phillip, I've started reviewing your patches, but stopped at [PATCH 09/24] Remove deprecated libmusicbrainz methods. for now. I've made a few changes which I'll detail below, once we agree on (not) including these changes, they can get pushed to git as they are self-contained and should not break anything. I've pushed my changes to http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=composer The interesting bits are: - "Use named initialisers in sj-prefs.c": without this, when the composer fields are added to the AlbumDetails/TrackDetails structures in the next commits, the initialization of these structures in sj-prefs.c gets wrong and causes warnings (and probably runtime crashes if this code was run) - "Factor code iterating over relations": instead of having 2 very similar fill_work_relations and fill_recording_relations functions, this introduces a more generic function doing the iteration, and then 2 small callbacks are doing the actual work - "Use relationlist_list_foreach_relation in fill_relations" builds on this factorization commit and on the "Remove deprecated libmusicbrainz methods" commit and also use the new helper for fill_relations. It could be even better, but I've been lazy and I've only mentioned that in the commit log - I've also made some minor changes to "Fill album composer using track composers", I think I've only added a NULL check compared to your commit Also, I have only compile-tested this code, my CD reader is 300 km away these days ;)
Hi Christophe > Also, I have only compile-tested this code, my CD reader is 300 km away these > days ;) Thanks for looking at the patches, I like the idea of using callbacks to avoid duplication. I haven't looked at it yet but I think we might be able to do something to reduce the duplication between on_composer_edit_changed and on_artist_edit_changed in sj-main.c. I've compiled your code and ripped a CD and it works fine. libmuicbrainz4.0.3 gives a deprecation warning: sj-metadata-musicbrainz4.c:461:5: warning: 'mb4_releasegroup_get_type' is deprecated (declared at /usr/include/musicbrainz4/mb4_c.h:2686): Use 'mb4_releasegroup_get_primarytype' instead [-Wdeprecated-declarations] I'll have a look at the new documentation this week and see what changes are needed. I'm also getting a runtime warning from libmusicbrainz (Unrecognised relation element: 'ended') which seems to be it not completely understanding the new schema yet - I'll follow that up with Andy Hawkins. Best Wishes Phillip
Created attachment 215457 [details] [review] Bump libmusicbrainz requirement to v4.0.3 & remove calls to deprecated methods. Hi Christophe > sj-metadata-musicbrainz4.c:461:5: warning: 'mb4_releasegroup_get_type' is > deprecated (declared at /usr/include/musicbrainz4/mb4_c.h:2686): Use > 'mb4_releasegroup_get_primarytype' instead [-Wdeprecated-declarations] This patch applies to the head of your composer branch and removes the call to mb4_releasegroup_get_type in favour of mb4_releasegroup_get_primarytype
Created attachment 215544 [details] [review] Revised patches for composer support for libmusicbrainz4 This patch set includes the following changes: * Fix error introduced when previously re-ordering patches which caused code that should have been in 'Update genre_entry if album->genre is set' to be included in 'Show composer fields when needed' * Modify 'Support editing of album composer' so that the callback can be used to edit the album artist as well as the composer. * Modify 'Add album composer entry to UI' to use the new callback. * Add 'Use generic callback for editing album artist' to use the new callback. * The changes that Christophe has in his composer branch. * Use libmusicbrainz>=4.0.3 & remove deprecated methods
Ross, I'd like to merge the libmusicbrainz bits to git master so that we can move forward with the libmb5 changes, and then we can go on with the review of the rest of this series, is it ok with you?
Yes, the purely libmb changes are good to be merged now.
I've pushed the libmusicbrainz part of these changes
NB: I've also pushed the libmusicbrainz5 changes from bug #676749, hopefully this won't break things for you
> I've pushed the libmusicbrainz part of these changes That's great, thanks for letting me know. > NB: I've also pushed the libmusicbrainz5 changes from bug #676749, hopefully > this won't break things for you Everything still compiles and works fine, thanks
Created attachment 218014 [details] [review] Rebased patchset Rebased patchset now that the libmusicbrainz stuff has been merged: * Changed "Show composer fields when needed" to allow the genre names to be translated. * Add bugzilla url to comment of each commit.
(In reply to comment #26) > * Add bugzilla url to comment of each commit. This can be done automatically with git bz ( http://git.fishsoup.net/cgit/git-bz/ )
Comment on attachment 218014 [details] [review] Rebased patchset I've made a few changes to your patchset (mentioned below) and pushed these at http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=composer Apart from the gettext issues mentioned below, this looks good! I've tested it and it works as expected (no composer in the UI unless it has the expected genre or has a composer set in musicbrainz medatada), so if Ross does not object, this can go in once this issue is solved. Sorry it took me so long to finish this review :( >From c1ae0b10b1d93b9828457b44a528bc06045a68f6 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Sun, 13 May 2012 13:03:53 +0100 >Subject: [PATCH 03/15] Add composers to track listview > >Add track composers to the listview and support for editing them in >on_cell_edited. > >Remove unused variables from on_cell_edited. The fact that you need to list 2 different things in the commit log is a clear indication that the patch could be split in 2 independent commits (no need to modify this patch, I'm just mentioning it for the next time you are in this situation) Update: I've ended up splitting them, you can get this from my repo >@@ -130,9 +131,9 @@ extern gboolean open_finished; > extern gboolean autostart; > > /** >- * Toggle, Title and Artist Renderers >+ * Toggle, Title Artist and Composer Renderers Very uninteresting nitpick, missing comma (Toggle, Title, Artist and Composer Renderers) (I've changed this locally). >From 4b4f98e7e784796f719ac0cd688cd23632bac7b7 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Sun, 3 Jun 2012 17:58:31 +0100 >Subject: [PATCH 08/15] Show composer fields when needed > >To avoid cluttering the UI with unecessary fields, only show composer >fields when there is composer metadata or when the user sets the genre >to one of 'classical', 'lieder', 'opera', 'chamber' or 'musical'. > >https://bugzilla.gnome.org/show_bug.cgi?id=661646 >--- > src/sj-main.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 113 insertions(+), 4 deletions(-) > >diff --git a/src/sj-main.c b/src/sj-main.c >index 18ebff8..6fad27f 100644 >--- a/src/sj-main.c >+++ b/src/sj-main.c >@@ -343,7 +346,7 @@ set_info_bar_text_and_icon (GtkInfoBar *infobar, > > ally_target = gtk_widget_get_accessible (button); > >- hbox_content = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 8); >+ hbox_content = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 8); > gtk_widget_show (hbox_content); > > image = gtk_image_new_from_stock (icon_stock_id, GTK_ICON_SIZE_DIALOG); >@@ -351,7 +354,7 @@ set_info_bar_text_and_icon (GtkInfoBar *infobar, > gtk_box_pack_start (GTK_BOX (hbox_content), image, FALSE, FALSE, 0); > gtk_misc_set_alignment (GTK_MISC (image), 0.5, 0); > >- vbox = gtk_box_new (GTK_ORIENTATION_VERTICAL, 6); >+ vbox = gtk_box_new (GTK_ORIENTATION_VERTICAL, 6); 2 unrelated whitespace changes as far as I can tell (no big deal since there are only 2 of them, they can stay in this commit) > gtk_widget_show (vbox); > gtk_box_pack_start (GTK_BOX (hbox_content), vbox, TRUE, TRUE, 0); > >+/* >+ * Determine if the composer fields should be shown based on genre, >+ * always show composer fields if they are non-empty. >+ */ >+static void >+composer_show_hide (const char* genre) >+{ >+ const static char *composer_genres[] = { >+ N_("classical"), N_("lieder"), N_("opera"), N_("chamber"), N_("musical") >+ }; /* Genres for which the composer fields should be shown. */ >+#define COUNT (G_N_ELEMENTS (composer_genres)) >+ static char *genres[COUNT]; /* store localized genre names */ >+ static gboolean init = FALSE; /* TRUE if localized genre names initalized*/ >+ gboolean composer_show = FALSE; >+ int i; >+ GList* l; >+ >+ if (composer_column == NULL) >+ return; >+ >+ if (!init) { >+ for (i = 0; i < COUNT; i++) >+ genres[i] = g_utf8_strdown (gettext (composer_genres[i]), -1); Nope, this part is wrong, N_() will make sure the strings in composer_genres are translated, you don't need to do it yourself. At least 'classical' is already translated in sj-genres.c, but when spelt 'Classical', this way the existing translation will be automatically reused. Do we want to add 'lieder', 'opera', 'chamber' and 'musical' to the list of known genres? I'd say no, but better to ask ;) >+ >+ init = TRUE; >+ } >+ >+ composer_show = !sj_str_is_empty (current_album->composer); >+ for (l = current_album->tracks; l; l = g_list_next (l)) { >+ if (!sj_str_is_empty (((TrackDetails*) (l->data))->composer) == TRUE) { >+ composer_show = TRUE; >+ break; >+ } >+ } >+ >+ for (i = 0; i < COUNT; i++) { >+ char *lower_genre = g_utf8_strdown (genre, -1); >+ if (g_str_equal (lower_genre, genres[i])) { >+ g_free (lower_genre); >+ composer_show = TRUE; For a case-unsensitive comparison, I'd look at g_utf8_casefold It seems albums from musicbrainz with one of the known genres but no composer set will not show the composer field, did I miss something? (this is not a blocking point as it's unlikely that the composer will be missing from mb if there's one imo). >From 93e0d3b95cbd561711d2bc4a55fb2deca6b65b57 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Wed, 25 Jan 2012 14:10:00 +0000 >Subject: [PATCH 09/15] Support editing of album composer > >Add callback to support editing of album composer. This basically does >the same thing as the callback for the album artist but on different >fields of AlbumDetails & TrackDetails so make it generic enough to do >both. > >Use UTF8 safe case comparison for composer and artist editing >callbacks. > Same comment about listing 2 unrelated things in the log hinting at the need for 2 separate commits (git add -p makes this easy to achieve). Split in my repo >https://bugzilla.gnome.org/show_bug.cgi?id=661646 >--- >diff --git a/src/sj-main.c b/src/sj-main.c >index 6fad27f..b2bb6cf 100644 >--- a/src/sj-main.c >+++ b/src/sj-main.c > /** >- * Callback when the title or artist cells are edited in the list. column_data >- * contains the column number in the model which was modified. >+ * Callback when the title, artist or composer cells are edited in the list. >+ * column_data contains the column number in the model which was modified. > */ This change belongs to a much earlier commit(Subject: [PATCH 03/15] Add composers to track listview), I moved it there in my branch > static void on_cell_edited (GtkCellRendererText *renderer, > gchar *path, gchar *string, >@@ -1517,6 +1523,113 @@ G_MODULE_EXPORT void on_title_edit_changed(GtkEditable *widget, gpointer user_da >+G_MODULE_EXPORT void on_person_edit_changed(GtkEditable *widget, >+ gpointer user_data) { >+ GtkTreeIter iter; >+ gboolean ok; /* TRUE if iter is valid */ >+ TrackDetails *track; >+ gchar *former_album_person = NULL; >+ /* Album person name and sortname */ >+ gchar **album_person_name, **album_person_sortname; >+ /* Offsets for track person name and sortname */ >+ int off_person_name, off_person_sortname; >+ int column; /* column for person in listview */ >+ >+ g_return_if_fail (current_album != NULL); >+ >+ if (widget == GTK_EDITABLE (artist_entry)) { >+ column = COLUMN_ARTIST; >+ album_person_name = ¤t_album->artist; >+ album_person_sortname = ¤t_album->artist_sortname; >+ off_person_name = G_STRUCT_OFFSET (TrackDetails, artist); >+ off_person_sortname = G_STRUCT_OFFSET (TrackDetails, >+ artist_sortname); >+ } else if (widget == GTK_EDITABLE (composer_entry)) { >+ column = COLUMN_COMPOSER; >+ album_person_name = ¤t_album->composer; >+ album_person_sortname = ¤t_album->composer_sortname; >+ off_person_name = G_STRUCT_OFFSET (TrackDetails, composer); >+ off_person_sortname = G_STRUCT_OFFSET (TrackDetails, >+ composer_sortname); >+ } else { >+ g_warning (_("Unknown widget calling on_person_edit_changed.")); >+ return; >+ } I think it makes more sense to change the artist edition to use this first, and then add support for composer edition, this way this function addition + the artist function removal end up in the same commit. In this case, this does not result in a very readable diff, but sometimes it makes it much easier to see what was changed in the genericized function and to make sure nothing was missed. >From 72403a2b5b33012156db4ed1e24f2677a92a2b83 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Tue, 1 Nov 2011 10:22:25 +0000 >Subject: [PATCH 12/15] Add album & track composer to schema for file name and > path >From 9499573e7ec751a278e72de3b292a548ddf6f9fd Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Wed, 2 Nov 2011 12:52:14 +0000 >Subject: [PATCH 13/15] Support composer in prefs for files & paths > >Added album and track composer options in the preferences for path and >filename for ripped tracks. > >https://bugzilla.gnome.org/show_bug.cgi?id=661646 >From 868fc7e83e0b5b91aa5a10583e0e260033a8d970 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Wed, 2 Nov 2011 12:54:08 +0000 >Subject: [PATCH 14/15] Expand composer format specifiers in files & paths > >Added support for the various %tc, %tp, %ac & %ap specifiers in output >pathnames in filename_parse_pattern which actually generates the >filenames. I've merged this part in the 2 previous patches, and split the rest of the commit in 2 separate ones > >Added default names for empty path and filename elements where the >metadata is missing. > >Modified sanitize_path so it does not segfault when passed a null >pointer. (Shouldn't need to now there are default names for empty >elements but safer to do it anyway.) > Did these things start happening when you added optional composer support? Or can they also be triggered with sound-juicer from master? >https://bugzilla.gnome.org/show_bug.cgi?id=661646 >--- > >From e6f3282b3da29dce5d7e15445dcc711389d7fde3 Mon Sep 17 00:00:00 2001 >From: Phillip Wood <phillip.wood@dunelm.org.uk> >Date: Wed, 25 Jan 2012 14:14:26 +0000 >Subject: [PATCH 15/15] Update genre_entry if album->genre is set > >If genre metadata is present display it in the UI when the CD is >changed. I don't understand exactly what this patch is doing :( Is it fixing a bug? improving something? I think a more detailed commit log would do the trick in enlightening me.
Hi Christophe Thanks for reviewing the code. I'm away for a week from tomorrow, I'll look and your comments and get back to you when I return.
Created attachment 221993 [details] [review] Updated composer support patch (In reply to comment #28) > (From update of attachment 218014 [details] [review]) > I've made a few changes to your patchset (mentioned below) and pushed these at > http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=composer > > Apart from the gettext issues mentioned below, this looks good! I've tested it > and it works as expected (no composer in the UI unless it has the expected > genre or has a composer set in musicbrainz medatada), so if Ross does not > object, this can go in once this issue is solved. > Sorry it took me so long to finish this review :( Thanks for reviewing the code and taking the time to make the corrections that you have. It's great to know that things are mostly ok, sorry it's taken me so long to reply. I'm a bit confused by the gettext comments as you'll see below. I think the current implementation is now useful and usable but here are a couple of potential future improvements to be aware of: i) Some classical albums that have composer metadata also have the composer in the artist credit or even worse the artist. At the moment we display the information as it is given to us by musicbrainz which means that for some albums the composer appears in the artist as well as the composer fields. It would be possible to use each tracks performer relationships to check if the composer is really a performer and remove them from the artist if they're not but I haven't done anything to implement this at the moment. ii) Some classical albums without composer metadata have the artist set to the composer and the artist details are in parentheses at the end of the album title. In principle it would possible to do a reasonable job of parsing this information and setting the composer and artist correctly but we don't at the moment. Note that no more albums should be added to musicbrainz in this format now that it supports composer relationships so over time this problem should disappear as the musicbrainz metadata is updated. Note that these improvements would be primarily if not wholly implemented in the musicbrainz5 class so would not affect the code in the current patches. One further thing to note is that a lot of non-classical albums have composer metadata in musicbrainz and so will show the composer with these changes as they are. > >From 4b4f98e7e784796f719ac0cd688cd23632bac7b7 Mon Sep 17 00:00:00 2001 > >From: Phillip Wood <phillip.wood@dunelm.org.uk> > >Date: Sun, 3 Jun 2012 17:58:31 +0100 > >Subject: [PATCH 08 [details]/15] Show composer fields when needed > > > >To avoid cluttering the UI with unecessary fields, only show composer > >fields when there is composer metadata or when the user sets the genre > >to one of 'classical', 'lieder', 'opera', 'chamber' or 'musical'. > > > >https://bugzilla.gnome.org/show_bug.cgi?id=661646 > >--- > > src/sj-main.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 113 insertions(+), 4 deletions(-) > > > >diff --git a/src/sj-main.c b/src/sj-main.c > >index 18ebff8..6fad27f 100644 > >--- a/src/sj-main.c > >+++ b/src/sj-main.c > >@@ -343,7 +346,7 @@ set_info_bar_text_and_icon (GtkInfoBar *infobar, > > > > ally_target = gtk_widget_get_accessible (button); > > > >- hbox_content = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 8); > >+ hbox_content = gtk_box_new (GTK_ORIENTATION_HORIZONTAL, 8); > > gtk_widget_show (hbox_content); > > > > image = gtk_image_new_from_stock (icon_stock_id, GTK_ICON_SIZE_DIALOG); > >@@ -351,7 +354,7 @@ set_info_bar_text_and_icon (GtkInfoBar *infobar, > > gtk_box_pack_start (GTK_BOX (hbox_content), image, FALSE, FALSE, 0); > > gtk_misc_set_alignment (GTK_MISC (image), 0.5, 0); > > > >- vbox = gtk_box_new (GTK_ORIENTATION_VERTICAL, 6); > >+ vbox = gtk_box_new (GTK_ORIENTATION_VERTICAL, 6); > > 2 unrelated whitespace changes as far as I can tell (no big deal since there > are only 2 of them, they can stay in this commit) Arrgh! I thought I had sorted the whitespace issues, it should be gone now. > > gtk_widget_show (vbox); > > gtk_box_pack_start (GTK_BOX (hbox_content), vbox, TRUE, TRUE, 0); > > > >+/* > >+ * Determine if the composer fields should be shown based on genre, > >+ * always show composer fields if they are non-empty. > >+ */ > >+static void > >+composer_show_hide (const char* genre) > >+{ > >+ const static char *composer_genres[] = { > >+ N_("classical"), N_("lieder"), N_("opera"), N_("chamber"), N_("musical") > >+ }; /* Genres for which the composer fields should be shown. */ > >+#define COUNT (G_N_ELEMENTS (composer_genres)) > >+ static char *genres[COUNT]; /* store localized genre names */ > >+ static gboolean init = FALSE; /* TRUE if localized genre names initalized*/ > >+ gboolean composer_show = FALSE; > >+ int i; > >+ GList* l; > >+ > >+ if (composer_column == NULL) > >+ return; > >+ > >+ if (!init) { > >+ for (i = 0; i < COUNT; i++) > >+ genres[i] = g_utf8_strdown (gettext (composer_genres[i]), -1); > > Nope, this part is wrong, N_() will make sure the strings in composer_genres > are translated, you don't need to do it yourself. Maybe I've misunderstood something, but are you sure about this? The glib documentation (http://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS) says "N_() - Only marks a string for translation. This is useful in situations where the translated strings can't be directly used, e.g. in string array initializers. To get the translated string, call gettext() at runtime." Haven't the initializers have got to be compile-time constants here (and so cannot be translated on initalization)? Looking at sj-prefs.c path_patterns and file_patterns are initialized with N_() and then we call gettext via _() when these strings are used in populate_pattern_combo (line 421). We also do the same thing for the list of genres in sj-genres.c. > At least 'classical' is > already translated in sj-genres.c, but when spelt 'Classical', this way the > existing translation will be automatically reused. I've given them all capitals to start. > Do we want to add 'lieder', 'opera', 'chamber' and 'musical' to the list of > known genres? I'd say no, but better to ask ;) Having better granularity in the genre names can be useful, otherwise 'classical' becomes a very diverse category (Much more diverse than the other non-classical categories). Note I'm not suggesting that these should be added to the list in sj-genres.c, just that we should recognize them here. It would add a little to the translation overhead though. > >+ > >+ init = TRUE; > >+ } > >+ > >+ composer_show = !sj_str_is_empty (current_album->composer); > >+ for (l = current_album->tracks; l; l = g_list_next (l)) { > >+ if (!sj_str_is_empty (((TrackDetails*) (l->data))->composer) == TRUE) { > >+ composer_show = TRUE; > >+ break; > >+ } > >+ } > >+ > >+ for (i = 0; i < COUNT; i++) { > >+ char *lower_genre = g_utf8_strdown (genre, -1); > >+ if (g_str_equal (lower_genre, genres[i])) { > >+ g_free (lower_genre); > >+ composer_show = TRUE; > > For a case-unsensitive comparison, I'd look at g_utf8_casefold Ok, done. > It seems albums from musicbrainz with one of the known genres but no composer > set will not show the composer field, did I miss something? You're right but musicbrainz does not supply genre information so the case you are worried about does not exist in practice. > (this is not a blocking point as it's unlikely that the composer will be > missing from mb if there's one imo). As explained at the top, classical metadata in musicbrainz could be better, there are a lot of classical albums where the artist is still set to the composer and the artist is at the end of the album title. Also a surprising number of pop albums have composer information. This means that the composer fields are shown for a lot of non-classical albums as they have composer metadata. We might want to change the logic so that the composers are only shown if the user sets an appropriate genre if people don't want composers cluttering the UI for non-classical albums. (Of course the fact that people have entered all these non-classical composers implies that there is some demand for this information so maybe we should show it.) > > static void on_cell_edited (GtkCellRendererText *renderer, > > gchar *path, gchar *string, > >@@ -1517,6 +1523,113 @@ G_MODULE_EXPORT void on_title_edit_changed(GtkEditable *widget, gpointer user_da > > >+G_MODULE_EXPORT void on_person_edit_changed(GtkEditable *widget, > >+ gpointer user_data) { > >+ GtkTreeIter iter; > >+ gboolean ok; /* TRUE if iter is valid */ > >+ TrackDetails *track; > >+ gchar *former_album_person = NULL; > >+ /* Album person name and sortname */ > >+ gchar **album_person_name, **album_person_sortname; > >+ /* Offsets for track person name and sortname */ > >+ int off_person_name, off_person_sortname; > >+ int column; /* column for person in listview */ > >+ > >+ g_return_if_fail (current_album != NULL); > >+ > >+ if (widget == GTK_EDITABLE (artist_entry)) { > >+ column = COLUMN_ARTIST; > >+ album_person_name = ¤t_album->artist; > >+ album_person_sortname = ¤t_album->artist_sortname; > >+ off_person_name = G_STRUCT_OFFSET (TrackDetails, artist); > >+ off_person_sortname = G_STRUCT_OFFSET (TrackDetails, > >+ artist_sortname); > >+ } else if (widget == GTK_EDITABLE (composer_entry)) { > >+ column = COLUMN_COMPOSER; > >+ album_person_name = ¤t_album->composer; > >+ album_person_sortname = ¤t_album->composer_sortname; > >+ off_person_name = G_STRUCT_OFFSET (TrackDetails, composer); > >+ off_person_sortname = G_STRUCT_OFFSET (TrackDetails, > >+ composer_sortname); > >+ } else { > >+ g_warning (_("Unknown widget calling on_person_edit_changed.")); > >+ return; > >+ } > > I think it makes more sense to change the artist edition to use this first, and > then add support for composer edition, this way this function addition + the > artist function removal end up in the same commit. In this case, this does not > result in a very readable diff, but sometimes it makes it much easier to see > what was changed in the genericized function and to make sure nothing was > missed. It certainly makes for a messier diff :-) but is safer as you say. One thing that bothers me a bit is that we add a reference to on_person_edit_change back in patch 6 "Add composer entry to UI" but only implement it for the composer entry in patch 12. Having said that everything should compile and run at the intermediate commits but you just cannot edit the composer successfully. > >From 72403a2b5b33012156db4ed1e24f2677a92a2b83 Mon Sep 17 00:00:00 2001 > >From: Phillip Wood <phillip.wood@dunelm.org.uk> > >Date: Tue, 1 Nov 2011 10:22:25 +0000 > >Subject: [PATCH 12 [details]/15] Add album & track composer to schema for file name and > > path > > >From 9499573e7ec751a278e72de3b292a548ddf6f9fd Mon Sep 17 00:00:00 2001 > >From: Phillip Wood <phillip.wood@dunelm.org.uk> > >Date: Wed, 2 Nov 2011 12:52:14 +0000 > >Subject: [PATCH 13 [details]/15] Support composer in prefs for files & paths > > > >Added album and track composer options in the preferences for path and > >filename for ripped tracks. > > > >https://bugzilla.gnome.org/show_bug.cgi?id=661646 > > >From 868fc7e83e0b5b91aa5a10583e0e260033a8d970 Mon Sep 17 00:00:00 2001 > >From: Phillip Wood <phillip.wood@dunelm.org.uk> > >Date: Wed, 2 Nov 2011 12:54:08 +0000 > >Subject: [PATCH 14 [details]/15] Expand composer format specifiers in files & paths > > > >Added support for the various %tc, %tp, %ac & %ap specifiers in output > >pathnames in filename_parse_pattern which actually generates the > >filenames. > > I've merged this part in the 2 previous patches, and split the rest of the > commit in 2 separate ones Cool, I've edited the commit messages slightly as there were a couple of typos. > > > >Added default names for empty path and filename elements where the > >metadata is missing. > > > >Modified sanitize_path so it does not segfault when passed a null > >pointer. (Shouldn't need to now there are default names for empty > >elements but safer to do it anyway.) > > > > Did these things start happening when you added optional composer support? Or > can they also be triggered with sound-juicer from master? I've deleted this. They stared happening when I first added composer support because album->composer and track->composer can be NULL. Before if there was no artist or title metadata it was passed as an empty string so messed up the filename but did not segfault. Now that we have implemented defaults for missing names sanitize_path should never be called with a NULL pointer. > >https://bugzilla.gnome.org/show_bug.cgi?id=661646 > >--- > > > > >From e6f3282b3da29dce5d7e15445dcc711389d7fde3 Mon Sep 17 00:00:00 2001 > >From: Phillip Wood <phillip.wood@dunelm.org.uk> > >Date: Wed, 25 Jan 2012 14:14:26 +0000 > >Subject: [PATCH 15 [details]/15] Update genre_entry if album->genre is set > > > >If genre metadata is present display it in the UI when the CD is > >changed. > > I don't understand exactly what this patch is doing :( Is it fixing a bug? > improving something? I think a more detailed commit log would do the trick in > enlightening me. I've deleted this. In an earlier incarnation of composer support I tried to guess the genre from the musicbrainz metadata an this then displayed it in the UI but it was not possible to do the guessing reliably.
Very quick answer as I'm on holidays until next week, I'll make a more detailed answer later :) (In reply to comment #30) > (In reply to comment #28) > > (From update of attachment 218014 [details] [review] [details]) > > Nope, this part is wrong, N_() will make sure the strings in composer_genres > > are translated, you don't need to do it yourself. > > Maybe I've misunderstood something, but are you sure about this? The glib > documentation (http://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS) > says I've reread this doc for another project recently, and I'm indeed wrong :) I'll have to reread this part of the patch again now that my brain is fixed :) > > Did these things start happening when you added optional composer support? Or > > can they also be triggered with sound-juicer from master? > > I've deleted this. They stared happening when I first added composer support > because album->composer and track->composer can be NULL. Before if there was no > artist or title metadata it was passed as an empty string so messed up the > filename but did not segfault. Now that we have implemented defaults for > missing names sanitize_path should never be called with a NULL pointer. > Can't hurt to at least have a g_return_if_fail or something like this to make this kind of issues more obvious/easier to diagnose.
Created attachment 222208 [details] [review] A couple of extra changes (applies on top of composer-mb5-2.patch) (In reply to comment #31) > Very quick answer as I'm on holidays until next week, I'll make a more detailed > answer later :) Thanks, hope your enjoying your holidays. I'll be off-line next week. > (In reply to comment #30) > > (In reply to comment #28) > > > (From update of attachment 218014 [details] [review] [details] [details]) > > > Nope, this part is wrong, N_() will make sure the strings in composer_genres > > > are translated, you don't need to do it yourself. > > > > Maybe I've misunderstood something, but are you sure about this? The glib > > documentation (http://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS) > > says > > I've reread this doc for another project recently, and I'm indeed wrong :) I'll > have to reread this part of the patch again now that my brain is fixed :) Thanks - glad I'm not going mad! > > > Did these things start happening when you added optional composer support? Or > > > can they also be triggered with sound-juicer from master? > > > > I've deleted this. They stared happening when I first added composer support > > because album->composer and track->composer can be NULL. Before if there was no > > artist or title metadata it was passed as an empty string so messed up the > > filename but did not segfault. Now that we have implemented defaults for > > missing names sanitize_path should never be called with a NULL pointer. > > > > Can't hurt to at least have a g_return_if_fail or something like this to make > this kind of issues more obvious/easier to diagnose. Good plan, I've included this in the attached patch. Also in the attached patch is a change to make sure that when the user selects "strip special characters" and file names of the type "Disc 1 - 1 track title" we actually strip the spaces from "Disc 1 - " part of the file name.
There were some minor conflicts rebasing this to master, I've pushed a rebased branch to http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=composer
I've squashed the patch below in "Show composer fields when needed", even I didn't miss something, things are better this way. diff --git a/src/sj-main.c b/src/sj-main.c index 7a07e10..1ecd8e5 100644 --- a/src/sj-main.c +++ b/src/sj-main.c @@ -576,6 +576,7 @@ composer_show_hide (const char* genre) gboolean composer_show = FALSE; int i; GList* l; + char *folded_genre; if (composer_column == NULL) return; @@ -595,15 +596,14 @@ composer_show_hide (const char* genre) } } + folded_genre = g_utf8_casefold (genre, -1); for (i = 0; i < COUNT; i++) { - char *folded_genre = g_utf8_casefold (genre, -1); if (g_str_equal (folded_genre, genres[i])) { - g_free (folded_genre); composer_show = TRUE; break; } - g_free (folded_genre); } + g_free (folded_genre); if (composer_show) show_composer_fields (); [
Apart from this, I think this series has spent enough time maturing in this bug report and that we should push it. Is it fine with you Ross?
Yeah, totally pushable now.
(In reply to comment #33 & comment #34) >There were some minor conflicts rebasing this to master, I've pushed a rebased >branch to http://cgit.freedesktop.org/~teuf/sound-juicer/log/?h=composer >I've squashed the patch below in "Show composer fields when needed", even I >didn't miss something, things are better this way. Thanks Christophe, squashing that one makes sense. I've ripped a couple of CD's with your new branch and it works fine for me. Thanks for all the time and effort you've put into reviewing and editing the patches and the hints you've given me along the way, it's great to see these patches in a state where they can be merged.
(In reply to comment #36) > Yeah, totally pushable now. Thanks Ross, that's fantastic.
(In reply to comment #37) > Thanks for all the time and > effort you've put into reviewing and editing the patches and the hints you've > given me along the way, it's great to see these patches in a state where they > can be merged. Well thanks a lot for your patience, these patches stayed in bugzilla for a long time :( But yeah, great to have these in!