GNOME Bugzilla – Bug 133444
Sort band list without 'The
Last modified: 2011-04-13 08:35:42 UTC
*** Bug 133445 has been marked as a duplicate of this bug. ***
Created attachment 24124 [details] [review] Fix sorting of artists to ignore leading 'the'.
Hi, Jon. I do appreciate the patch, but unfortunately it's not that simple. There could very well be a language where "The" means something entirely different from English - this patch would break things for them. Moreover, after adding this, this would imply we should do it for other languages as well. The set of all prefix words from all languages is certain to have conflicts with words in a different language. Now, if someone has written (or writes) a sorting library which does this right, and is known and proven to work for many languages and many prefix words, we can talk about using it. Certainly, other free software applications before Rhythmbox must have encountered this issue - it would be good to do some research on what they do.
Testing bug 139195, please ignore if you're cc'd on the bug.
I doubt that it's possible to do this automatically, at least if you want to handle more than just "the". Anyhow, adding musicbrainz.org support to rhythmbox (see bug 104203) would be a partial solution. Files with musicbrainz tags have an MUSICBRAINZ_SORTNAME tag: E.g. ARTIST=David Bowie -- MUSICBRAINZ_SORTNAME=Bowie, David ARTIST=The Kills -- MUSICBRAINZ_SORTNAME=Kills, The ARTIST=Die Ärzte -- MUSICBRAINZ_SORTNAME=Ärzte, Die
You could at least add it for when the current language is English, and add more languages later.
I don't think this would work as generic as mentioned above. Example? Take these two: "Die Aerzte" (the German band called above) "Die Happy" (don't know where they are from, but their name is english) -- "Die Bart, Die." -- Sideshow Bob from the Simpsons
Sven, i don't know what you are referring to with "above." Anyway, this bug is not about "die," it's about "the."
Yes, and who says that "The" is not being used in any other language as a completely different word? I was just showing an example with the German "die" (which is the German female version of the) and how it can be misunderstood as the English "die". This can happen with any word, so this bug is definitely something that would break i18n massively. So the only reasonable solution is the one proposed by Juergen above, we would have a set of replace patterns, but don't do anything automatically (as this would possibly break lots of things).
Sven, at this point I think this request is not for any other language than English. For English users, this feature would work perfectly as described in previous comments.
Mailing list thread(s) that are relevant: http://mail.gnome.org/archives/rhythmbox-devel/2006-February/thread.html#00055 In particular: http://mail.gnome.org/archives/rhythmbox-devel/2006-February/msg00089.html
Unassigning: nobody appears to be actively working on this.
*** Bug 336805 has been marked as a duplicate of this bug. ***
As Juergen says, a better solution would be to use the "sortname" musicbrainz tag of the artist. Ignoring words is inflexible and rather arbitrary. Musicbrainz already maintains a sortname for each artist and adds it to the tags of the file. If RB sorted by Sortname instead of Artist Name, it could assume Sortname = Artist for those files missing a Sortname tag. Additionally, if any file had such a tag, that sortname should be inferred for all other files with an identical Artist field. This second feature would mean that one well-tagged "The Beatles" (Sortname = "Beatles, The") file would cause all (including non-sortnamed) "The Beatles" songs to be listed together under B. If no The Beatles songs had a sortname, they would all be filed under "The".
*** Bug 346137 has been marked as a duplicate of this bug. ***
*** Bug 346440 has been marked as a duplicate of this bug. ***
Perhaps make a plugin as a temporary solution for this?
*** Bug 349141 has been marked as a duplicate of this bug. ***
I thought tis was an easy problem, but ready mailing messages and the bug comment, I realize that it's not. My view of this now is that there are 2 options: 1) Do the sorting automatically, with a preference UI where the user can enter the list of words to ignore (e.g. ['The', 'Les', 'Die']) and also a list of exceptions (e.g. 'Die Happy', 'The ou café']). Let the user simply add a song, album or artist name to the list of exceptions (e.g. with a right-click popup menu). I think this solution is relatively simple to implement and covers quite a lot of usecases, and works without specials tags (that we can't set for DAAP shares and over read-only sources) 2) Do that with sort-tags. This is surely more powerful, for example I like this idea of being able to put side by side the french group 'Fredericks, Goldman, Jones' and 'Jean-Jacques Goldman'. But again it won't work for sources that are not "sort-tags aware". The other problem is filling all those sort-tags: either it implies having musicbrainz support (surely a lot of MB querying for a several thousands songs library...), either some sort of automatic technique similar to 1). I guess the best would be to do 2), so that we have the flexibility of sort-tags and implement a layer upon it doing the functionnality of 1) for auto-filling sort-tags and maybe also simulating them for non sort-tags aware sources.
Its not elegant, but the suggestion made back in Comment 6 would satisfy the bulk of users (assuming that most of Rhythmbox's users run in English): If the system language is set to some type of English (including variants like en-GB, en-US etc), then use a special case for "The XXX" to be sorted as "XXX, The" (except for "The The"). Would an "English Systems Only" version of Jon Trowbridge's patch on comment 2 be acceptable as a short term solution? I agree that long term using artist sort-tags would probably be best (maybe using MusicBrainz where possible).
I think it's a mistake to think of it as an internationalization problem, because no matter what the current locale is, the particles 'The' and 'Les' should be ignored. I mean if there exist a list of words to be ignored, every instance of RB in the world should share the same.
It would be nice to a have version of comment #20 (i.e. enabling ignoring of "The" in the artist name, but *not* changing the display) done as a plugin, that way it wouldn't have to be enabled by default. It would also mean 1) we would keep about 99% of all users happy, 2) that we would stop getting duplicate bugs being filed every week, 3) we wouldn't have to worry about language detection because the user can decide whether it's appropriate to enable it. A more advanced plugin could allow the user to specify additional words such as "Die", "Les" etc.
It *might* be possible to implement such a plugin without major core changes, but it would have to do some fairly nasty things. Things that need to be done to use artist sortname tags: - field definition and conversion to/from GST_TAG_MUSICBRAINZ_SORTNAME to rb-metadata (trivial) - ability to modify the sort key version of an RBRefstring (not hard) and determine whether the sort key was explicitly set rather than generated from the normal string (not sure how to do this without wasting lots of memory) - some way to distinguish artist name refstrings from everything else, so the album 'the beta band' doesn't get the same sort key as the artist 'the beta band' (not clear how this would work) - make rhythmdb-tree save the artist sort key if it has been explicitly set (not hard) - make rhythmdb-tree read the artist sort key and set it appropriately (not hard; just add a case to rhythmdb_entry_set_internal for RHYTHMDB_PROP_ARTIST_SORT_KEY?) - if the metadata returned by musicbrainz includes an artist sort name, set that on audio CD track entries (not hard) - expose the sort name in the song properties dialog somehow (not sure how best to do this) - there appears to be some gstreamer work required to support sortname tags, at least in id3 format (probably not very hard)
It seems to me a plugin would be too much for a small problem. I agree with Christophe, that if you start ignoring certain words to sort out, you should do that regardless of the local language, because that's how you sort in whatever language the band's name is in. Making a plugin for this seems like a much more awkward solution that doing something like patching the sorting code, and making it check for every do-not-use-in-sorting word, in every language. So, basically I think we should compile a list of words to ignore, and then perhaps code them in, or we could include an ignore-words file that gets parsed at runtime with things like "the, les, el, die, das, der, la...."
The real solution to this is to use the tags defined for ID3v2.4 for this: TSOP for Performer (Artist), TSOA for Album and TSOT for title, but I think most people want Performer more than the others. For ID3v2.3 tags, MusicBrainz uses XSOP, XSOA and XSOT. Here's a table of the tags MusicBrainz' Pircard will create (for Ogg and other formats): <http://musicbrainz.org/doc/PicardQt/TagMapping?highlight=%28%28CategoryPicard%29%29> This is a real pain, and not just for "The Beatles" and "Los Lobos" but "Bob Marley" should be filed under "Marley, Bob" too. But "Pink Floyd" goes under "Pink Floyd". Even for French users. There's no programmatic way to determine the "right way" for a generic string, but there are tags to define the right way for each particular file (that are used for iTunes). Please implement this. There is no Linux-based player that does.
This is possible related to the GStreamer bug 414539.
Created attachment 103247 [details] [review] use artist-sortname for sorting in property models Turns out this is actually pretty easy. This approach will also work for album-sortname, title-sortname, etc. if we start storing those.
This patch isn't complete. It doesn't update the sort string when a sort property changes (not as easy as it looks). I'm also thinking about having it make extra metadata requests ('rb:artist-sortname' for example) when looking for sort strings, so there could be a plugin that just did s/^the //i for people who can't be bothered tagging their music properly.
As a reference, I was at a friends house last night and he was running XBMC <http://www.xboxmediacenter.com/> and it sorts artist names ignoring 'The'. Might be something to look at.
*** Bug 529641 has been marked as a duplicate of this bug. ***
Created attachment 112522 [details] [review] Check artist_sortname when calculating RHYTHMDB_PROP_ARTIST_SORT_KEY It appears that since the last patch, the RHYTHMDB_PROP_ARTIST_SORTNAME property was added to RhythmDB entries. This allows a very simple patch to be written -- when asked for the sort key of an entry, the database will check if a special sortname is already defined and use it.
That patch will affect sorting in query models. My patch was addressing sorting in property models. I'm not sure that's a good way of doing it, though. I think it might be better to do this in rhythmdb_query_model_artist_sort_func.
The suggestion number 1 in the post #19 from Christophe Dehais seems like a perfectly good solution until something better can be done. There could be a default list of omissions, which then could be edited. As an addition making an exception in some artists case could be done with a right-click context menu option. Of course this wouldn't help out sorting first names and last names properly and rest of other sorting 'problems', but this the-problem really makes the otherwise perfectly decent app seem quite unfinnished. Rhythmbox being a default player in at least Ubuntu makes this even more important. For some more ranting on the topic: http://wiki.musicbrainz.org/SortNameStyleDiscussion
*** Bug 574448 has been marked as a duplicate of this bug. ***
Does anyone know how this support is implemented in the swathes of non-linux/gnu applications out there that filter out "the" from sorting? On the subject of sorting names, such as Bob Marley, Eric Clapton, David Bowie etc this should definitely be a user preference, as personally I prefer Eric Clapton, for example, to be sorted under E.
In fact, having had a quick think about it I remembered how iTunes deals with this issue. Every media file, in addition to it's own tags, has a second set of tags stored in iTunes media library. This second set of tags are its sorting tags. If a sorting tag exists for an item, that is what is used for sorting, and if not the media files' own tag is used (Note, the files own tags are what is used for display). For example: 1st example: Media file's own tag: Artist: The Beatles Sort tag: Artist: Beatles 2nd example: Media file's own tag: Artist: Eric Clapton Sort tag: Artist: Clapton, Eric 3rd example: Media file's own tag: Artist: Hot Chip Sort tag: Artist: null (i.e. no tag is stored) This also means that I can sub sort my albums by release date, rather than album name. For example: Media file's own tag: Album: The White Album Sort tag: Album: 1968 05 30 This gives a level of control that should keep everyone happy. Add in some optional (ideally even user script-able) automatic sort tag generation to be executed on import (e.g. removing the "The" from an artist's name) and I think you have a possible solution. I would argue for this over adding in a sortname meta tag, for example, because I'm not entirely convinced sorting information should be stored in a media file; it seems more apt to me to store the sorting data for a media library inside the media library.
There's another tag for release dates which can be used for sorting by that. Abusing the "Sort Name/Artist/Album Artist/..." tags for that is just silly. And, of course, "Sort Name/Artist/..." information should be stored in the file. You don't want to lose that information when moving/copying a file. Anyway, there are already tags for all this (and tools like musicbrainz' picard which help setting them). gstreamer/rhythmbox just need to support them.
*** Bug 600174 has been marked as a duplicate of this bug. ***
*** Bug 603981 has been marked as a duplicate of this bug. ***
Created attachment 150477 [details] [review] Fixes the sorting in query models to use the artist sortname if one exists I added the properties RHYTHMDB_PROP_ARTIST_SORTNAME_SORT_KEY and RHYTHMDB_PROP_ARTIST_SORTNAME_FOLDED to RBEntry. These are to RHYTHMDB_PROP_ARTIST_SORTNAME, as RHYTHMDB_PROP_ARTIST_SORT_KEY and RHYTHMDB_PROP_ARTIST_FOLDED are to RHYTHMDB_PROP_ARTIST. Changed rhythmdb_query_model_artist_sort_func to check if the entries have a sort name (RHYTHMDB_PROP_ARTIST_SORTNAME is not an empty string). If it has one then use RHYTHMDB_PROP_ARTIST_SORTNAME_SORT_KEY to sort with. otherwise use RHYTHMDB_PROP_ARTIST_SORT_KEY to sort, as was done previously anyway.
Created attachment 150552 [details] [review] Fixes sorting in property models Based on the patch from comment 27, but DOES update the sort string when a sort property is changed. Assuming my code is decent enough and that I haven't broken the entire program, this, along with the patch in the previous comment, should fix this bug. Hopefully. :D
Review of attachment 150477 [details] [review]: This generally looks OK, and I think I prefer this approach to that taken by the earlier patch. ::: rhythmdb/rhythmdb-query-model.c @@ +2697,3 @@ + a_val = rhythmdb_entry_get_string (a, RHYTHMDB_PROP_ARTIST_SORTNAME_SORT_KEY); + else + a_val = rhythmdb_entry_get_string (a, RHYTHMDB_PROP_ARTIST_SORT_KEY); I think it'd be better to check for a non-empty sort key here, rather than first getting the artist sortname and checking that. a_val = rhythmdb_entry_get_string (a, RHYTHMDB_PROP_ARTIST_SORTNAME_SORT_KEY); if (a_val[0] == '\0') { a_val = rhythmdb_entry_get_string (a, RHYTHMDB_PROP_ARTIST_SORT_KEY); } also, should probably do this for the album sortname (in rhythmdb_query_model_album_sort_func) too. ::: rhythmdb/rhythmdb.h @@ +213,3 @@ + + RHYTHMDB_PROP_ARTIST_SORTNAME_SORT_KEY, + RHYTHMDB_PROP_ARTIST_SORTNAME_FOLDED, should add equivalents for RHYTHMDB_PROP_ALBUM_SORTNAME while you're here
Review of attachment 150552 [details] [review]: ::: rhythmdb/rhythmdb-property-model.c @@ +52,3 @@ RBRefString *string; RBRefString *sort_string; + guint sort_string_from; Any reason you didn't keep the bit allocation from my patch? There are typically many thousands of these structures in memory, and we're only going to use a couple of bits of sort_string_from at the very most. It's not a huge saving, but it helps. @@ +78,3 @@ RhythmDBEntry *entry, RhythmDBPropertyModel *propmodel); +static gboolean update_sort_string (RhythmDBPropertyModel *model, extra space at the end of the line @@ +83,2 @@ static RhythmDBPropertyModelEntry* rhythmdb_property_model_insert (RhythmDBPropertyModel *model, + RhythmDBEntry *entry); unnecessary whitespace here @@ +422,3 @@ break; case RHYTHMDB_PROP_ALBUM: + append_sort_property (model, RHYTHMDB_PROP_ALBUM); should add album sortname here too @@ +482,3 @@ model->priv->all = g_new0 (RhythmDBPropertyModelEntry, 1); model->priv->all->string = rb_refstring_new (_("All")); + extra whitespace on this line too @@ +545,3 @@ + g_array_free (model->priv->sort_propids, TRUE); + and here @@ +607,3 @@ } else { RhythmDBPropertyModelEntry *prop; + and here @@ +619,3 @@ + } + } + and here @@ +622,3 @@ if (propid != propmodel->priv->propid) return; + and here @@ +751,3 @@ g_atomic_int_inc (&prop->refcount); rb_debug ("adding \"%s\": refcount %d", propstr, prop->refcount); + more extra whitespace
I'd really like there to be some unit tests (see tests/test-rhythmdb-property-model.c) for this if we start adding complex behaviour in there. It'd also be good to have a way to set the sortname properties.. maybe in a new 'sorting' tab of the song properties window? I don't think they really belong in the 'basic' tab, and they don't fit in the 'details' tab either.
> Any reason you didn't keep the bit allocation from my patch? There are > typically many thousands of these structures in memory, and we're only going to > use a couple of bits of sort_string_from at the very most. It's not a huge > saving, but it helps. Since your patch was made refcount has started to use the g_atomic functions and the compiler doesn't seem to like using bitfields with them: "error: cannot take address of bit-field ‘refcount'" I can easily fix the whitespace problems and add album sortname support. would you like one big patch against the current HEAD with everything so far, or another patch on top of the two i submitted just to fix these problems? And I'll look into adding a 'sorting' tab to the properties window. I don't know much about unit tests though, so any pointers would be appreciated.
(In reply to comment #45) > Since your patch was made refcount has started to use the g_atomic functions > and the compiler doesn't seem to like using bitfields with them: "error: cannot > take address of bit-field ‘refcount'" OK, that makes sense. > I can easily fix the whitespace problems and add album sortname support. would > you like one big patch against the current HEAD with everything so far, or > another patch on top of the two i submitted just to fix these problems? Patches on top of patches are kind of hard to deal with, so I'd prefer patches against HEAD. > And I'll look into adding a 'sorting' tab to the properties window. I don't > know much about unit tests though, so any pointers would be appreciated. The unit test framework we use is 'check': http://check.sourceforge.net/ - there's some introductory documentation there. To run the existing unit tests, do 'make check' in the tests/ subdirectory. The idea is that we'd add test cases that create and update database entries in various ways - adding/removing/changing sortnames - and then check that the property models are updated correctly.
Please excuse this gigantic essay: I've been able to fix up the patch for query model sorting, and add a "sorting" tab to the song info dialogs no problem. Property models, however, are causing me some problems. It's not too hard to make it update sort order when an entry is edited. the patch i submitted earlier didn't actually do that, i got confused. silly me. i have it "working" now. By "working" i mean passes this test: /* add entries a, the_b, and c. give them appropriate RHYTHMDB_PROP_ARTIST values */ /* test a comes immediately before c */ rhythmdb_property_model_iter_from_string (propmodel, "a", &iter1); rhythmdb_property_model_iter_from_string (propmodel, "c", &iter2); fail_unless (iter1.user_data == g_sequence_iter_prev (iter2.user_data)); /* test c comes immediately before the_b */ rhythmdb_property_model_iter_from_string (propmodel, "c", &iter1); rhythmdb_property_model_iter_from_string (propmodel, "the b", &iter2); fail_unless (iter1.user_data == g_sequence_iter_prev (iter2.user_data)); end_step (); /* change "the b" to sort under "b, the" */ set_waiting_signal (G_OBJECT (db), "entry-changed"); set_entry_string (db, the_b, RHYTHMDB_PROP_ARTIST_SORTNAME, "b, the"); rhythmdb_commit (db); wait_for_signal (); /* test a comes immediately before the_b */ rhythmdb_property_model_iter_from_string (propmodel, "a", &iter1); rhythmdb_property_model_iter_from_string (propmodel, "the b", &iter2); fail_unless (iter1.user_data == g_sequence_iter_prev (iter2.user_data)); /* test the_b comes immediately before c */ rhythmdb_property_model_iter_from_string (propmodel, "the b", &iter1); rhythmdb_property_model_iter_from_string (propmodel, "c", &iter2); fail_unless (iter1.user_data == g_sequence_iter_prev (iter2.user_data)); but that's far from complete. The main issues i can think of are: a) removing a sort string property - if this is the last entry with that sort string set then we should probably change the sort string, but how would we know? also see point c). b) different entries have the sort propert set, but to different things. which entry do we listen to? c) some entries have a sort property set but others don't - what if some don't have it set for a reason? only listening to the most-recently-updated entry is not an option since when the model is entirely re-populated (ie artist model when a genre is selected) that might change it back again. I see two solutions to this. The first one (which is what i believe banshee does) is force all entries with the same property to have the same equivelant sort property. This seems awfully controlling of a music player, so i don't think this is a good idea. It's also a fix outwith the scope of the property model, so the property model could still potentially face this issue. So I'm starting to think that the way to do it is to have a seperate PropertyModelEntry for each different sort string. For example: The whole of Rubber Soul has no artist sortname set, but the whole of Abbey Road has a sortstring "Beatles, The". In this situation "The Beatles" should appear twice in the artist browser, once sorted under B and once sorted under T. When selecting either of these, however, entries from both albums should be shown. Thoughts etc? before i go ahead and try to massively rewrite a lot of property model code.
(In reply to comment #47) > a) removing a sort string property - if this is the last entry with that sort > string set then we should probably change the sort string, but how would we > know? also see point c). I don't think it's worth worrying about this. > b) different entries have the sort propert set, but to different things. which > entry do we listen to? Whichever one we see first. > c) some entries have a sort property set but others don't - what if some don't > have it set for a reason? What possible reason is there? Sort names aren't opinions, they're facts. If the artist name is the same, the sort name is also the same, so there's no reason to split them up. > only listening to the most-recently-updated entry is not an option since when > the model is entirely re-populated (ie artist model when a genre is selected) > that might change it back again. If there are multiple sort names for the same artist, the tags are broken. If the tags are broken, we can't display them properly anyway. > I see two solutions to this. The first one (which is what i believe banshee > does) is force all entries with the same property to have the same equivelant > sort property. This seems awfully controlling of a music player, so i don't > think this is a good idea. It's also a fix outwith the scope of the property > model, so the property model could still potentially face this issue. It's the best we can do. > So I'm starting to think that the way to do it is to have a seperate > PropertyModelEntry for each different sort string. For example: The whole of > Rubber Soul has no artist sortname set, but the whole of Abbey Road has a > sortstring "Beatles, The". In this situation "The Beatles" should appear twice > in the artist browser, once sorted under B and once sorted under T. When > selecting either of these, however, entries from both albums should be shown. That sounds like a bad idea to me. It's way too complicated and only provides a questionable benefit. > Thoughts etc? before i go ahead and try to massively rewrite a lot of property > model code. It's fine as it is. We can't do any better than this without completely rewriting the database, and we're not going to do that.
Or, more succinctly, I'm much more interested in making things work for what I consider to be the common case, where sortname tags are sparsely populated, than for odd cases where a lack of sortname tag is supposed to be significant. I think we should only worry about the latter case if and when someone presents an actual argument for it.
Created attachment 151337 [details] [review] Fixes sorting in prop models and query models Fixes query models as before but with the minor corrections and now uses album sortname too. Property models now update properly. A property model entry's sort string will be changed if a new entry has a higher preference sort property, or if an existing entry has a higher or equal preference sort property that changes. Adds a new tab to song info dialogs (multiple and single) to edit sortnames. I'm not sure what i18n consequences this has. I suppose new strings are "Sorting" for the tab label, and "Artist Sortname" and "Album Sortname" for labels next to entries to edit the values. Added a new test to test-rhythmdb-property-model.c to test sort order in property models. It adds a few entries, tests they are sorted correctly. then changes one of the sortnames and tests that the sort order is updated correctly. I'm not sure if this is complete enough or not?
Review of attachment 151337 [details] [review]: This looks pretty close to being ready. If you could split it into three patches - one for the song info window parts, one for the rhythmdb parts, and one for the query model and property model changes, I'd be happy to commit the first two straight away. I think I want to write some more test cases for the property model parts. ::: data/ui/song-info-multiple.ui @@ +250,3 @@ + <property name="visible">True</property> + <property name="xalign">0</property> + <property name="label" translatable="yes">_Album Sortname:</property> I think it'd be better to use 'sort name' rather than 'sortname'. @@ +265,3 @@ + <property name="visible">True</property> + <property name="xalign">0</property> + <property name="label" translatable="yes">_Artist Sortname:</property> The shortcut key should be different for album and artist. They need to be fixed on the main tab too, but that's not really your problem. ::: rhythmdb/rhythmdb-property-model.c @@ +620,3 @@ RhythmDBPropertyModelEntry *prop; + if (propid == propmodel->priv->propid && g_hash_table_lookup (propmodel->priv->entries, entry) == NULL) { I don't think this is right. If the entry is in the map, which means it's hidden, the property model should ignore all updates to it. @@ +627,3 @@ + } + else + { should be "} else {" all on one line @@ +647,3 @@ + prop->sort_string = rb_refstring_new (g_value_get_string (new)); + property_sort_changed (propmodel, ptr, &iter); + } should break out of the loop here
(In reply to comment #51) > Review of attachment 151337 [details] [review]: > > I think it'd be better to use 'sort name' rather than 'sortname'. > The shortcut key should be different for album and artist. They need to be > fixed on the main tab too, but that's not really your problem. Both banshee and exaile use "m" as the shortcut for album so it seems reasonable for us to do so too. Which would make the strings "_Artist Sort Name" and "Albu_m Sort Name". Sound okay? And I'll sort that for song-info.ui and song-info-multiple.ui > ::: rhythmdb/rhythmdb-property-model.c > @@ +620,3 @@ > RhythmDBPropertyModelEntry *prop; > > + if (propid == propmodel->priv->propid && g_hash_table_lookup > (propmodel->priv->entries, entry) == NULL) { > > I don't think this is right. If the entry is in the map, which means it's > hidden, the property model should ignore all updates to it. Ah, I think I understand it now! My code here ignores hidden entries (entries in the map) when checking if the property which changed is *the* prop. But we also need to ignore hidden entries if a sortprop is changed. Or even just any prop if extra behaviour is added in the future. So the check if the entry is in the map needs to be made at a higher level, eg: if (propid == RHYTHMDB_PROP_HIDDEN) { ... } else if (g_hash_table_lookup (propmodel->priv->entries, entry) == NULL) { RhythmDBPropertyModelEntry *prop; if (propid == propmodel->priv->propid) { ... } else { ... } } > I think I want to write some more test cases for the > property model parts. What sort of things are you wanting to test? Anything I can help with?
Created attachment 152439 [details] [review] Song info dialog changes Went with the strings "_Artist sort name" and "Albu_m sort name"
Created attachment 152440 [details] [review] rhythmdb changes
Created attachment 152442 [details] [review] Fixes sorting in property models and query models Includes the simple test case from the older patch
Comment on attachment 152440 [details] [review] rhythmdb changes pushed as commit f53b0f0
Comment on attachment 152439 [details] [review] Song info dialog changes committed as 3e5e42b. I changed 'sort name' to 'sort order' since that's what I used in the audio CD source.
Well, I just found a reason to handle the removal of sort name properties.. trying out this patch, I found a bunch of tracks with the artist sort name set to "Various Artists", and another set with "Unknown". The audio CD plugin definitely shouldn't set the artist sortname to "Unknown", and it should probably avoid "Various Artists" too.. I'm handling it by checking if the current sort property is being cleared at the top of update_sort_string, and if so, clearing prop->sort_string. I'm also adding a few more unit tests - changing the artist name when a sort name is set, changing the sort name (with and without affecting the sort order).
Created attachment 152631 [details] [review] updated property model sorting patch I committed the query model sorting changes.
And of course now I realise that you can't just remove the sortname tags - the tag writing code is not able to remove tags from files, and if you remove them externally, the tag reader doesn't remove the existing value, because there's no new value to replace it. Ugh.
(In reply to comment #60) > and if you remove them > externally, the tag reader doesn't remove the existing value, because there's > no new value to replace it. I fixed this in commit e49117b8.
In any case, this seems to work pretty well. Pushed as commit 6ee9f21. We can probably get rid of the sort_string_from member of RhythmDBPropertyModelEntry, but we can worry about that later. Thanks for working on this.
I see the status of this issue is "Fixed." Is there a patch or plugin I can download to implement this request? Or is it considered an abandoned request?
It's marked as fixed because the work that went on in this bug has been integrated upstream in rhythmbox development repository. The patches were attached to this bug as far as I can tell, and the development repository can be found at http://git.gnome.org/browse/rhythmbox/
(In reply to comment #64) > It's marked as fixed because the work that went on in this bug has been > integrated upstream in rhythmbox development repository. The patches were > attached to this bug as far as I can tell, and the development repository can > be found at http://git.gnome.org/browse/rhythmbox/ I'm a bit new to this bug-report-develop-implement process; is there a PPA for rhythmbox that includes these patches? I don't understand how to update my version of rhythmbox based on the link you posted. Any info would be helpful. Thanks!
We provide source packages, not prebuilt binaries, it would be better to directly ask to the ubuntu community if you are looking for prebuilt rhythmbox snapshots
*** Bug 626365 has been marked as a duplicate of this bug. ***
This issue was annoying me recently so I decided to write a plugin to allow users to specify which prefixes in artist names they wish to ignore. The plugin then attempts to automatically add artist names (minus the prefix) to the artist sort order to give the desired sorting. Since the plugin is user configurable it should work OK for any language (I hope). If the user doesn't choose one then the plugin provides a default ignore list which I've borrowed from Logitech's SqueezeCenter interface. Plugin at http://code.google.com/p/artistprefix/