GNOME Bugzilla – Bug 616669
[patch] Send artist and album sort order to DAAP clients
Last modified: 2010-05-08 09:14:37 UTC
Created attachment 159453 [details] [review] Patch to add artist and album sort order codes. This proposed patch adds the artist and album sort order content codes to DAAP. It allows clients to efficiently sort the tracks as they receive the list at connexion. Otherwise, the client has to wait until it receives the track file to read the sort order data. With some clients, this will cause a reordering of the track list and may break linear playing. Tested with Rhythmbox 0.18.12 (git HEAD) as server, and iTunes 9.0 as client.
Review of attachment 159453 [details] [review]: It'd be great if you could add client side support for this too. This would be done in the 'handle_song_listing' function in rb-daap-connection.c. ::: plugins/daap/rb-daap-share.c @@ +903,3 @@ + SONG_YEAR, + SORT_ARTIST, + SORT_ALBUM these should be SONG_SORT_ARTIST and SONG_SORT_ALBUM
I will look for implementing client support. You are right that it's probably just a matter of mapping back the content codes to the model. The DAAP codes such as SONG_YEAR and SORT_ARTIST are based on the long code names. I think SORT_* should be left as is. It there a particular reason to follow a different pattern? For example: dmap.itemid -> ITEM_ID dmap.containeritemid -> CONTAINER_ITEM_ID daap.songyear -> SONG_YEAR daap.sortartist -> SORT_ARTIST
Created attachment 159457 [details] [review] Patch to add client support of sort order codes
I don't think there's any reason to reflect the inconsistency of the protocol's identifiers in our own.
Review of attachment 159457 [details] [review]: ::: plugins/daap/rb-daap-connection.c @@ +1092,3 @@ + /* artist sort order */ + if (sort_artist && *sort_artist != '\0') { + entry_set_string_prop (priv->db, entry, RHYTHMDB_PROP_ARTIST_SORTNAME, sort_artist); I'd rather have an additional parameter to entry_set_string_prop that determines whether it sets the property to "Unknown" or leaves it unset if it's not given a value.
Created attachment 159614 [details] [review] Patch to add artist and album sort order codes (combined) This matches merges the changes in previous patches and includes the last suggestions. However, the client portion of this code needs more testing.
Created attachment 160201 [details] [review] Patch to add artist and album sort order codes (final) Properly generated patch (after learning about git-rebase) that implements sending and interpreting the proper DAAP sort order codes.
I fixed up a few things - most notably that we weren't requesting the daap.sortartist and daap.sortalbum items from the server, so we'd never see them on the client side - and pushed this as commit 23f9b94.
We should also add these to libdmapsharing, since we're probably going to switch to that for daap support soon.
Created attachment 160434 [details] [review] untested patch
I just applied a modified patch to Git. See http://git.gnome.org/browse/libdmapsharing/commit/?id=c3f8959a29fb8a889df1583b2bf34a198c96bb96.
This seems a little excessive. @@ -597,6 +599,7 @@ dmap_structure_add (GNode *parent, va_start (list, cc); dmap_type = dmap_content_code_dmap_type (cc); + g_print ("%d %d %d %d\n\n\n", dmap_type, cc, DMAP_TYPE_SHORT, DMAP_TYPE_STRING); gtype = dmap_content_code_gtype (cc); item = g_new0(DMAPStructureItem, 1);
> + g_print ("%d %d %d %d\n\n\n", dmap_type, cc, DMAP_TYPE_SHORT, I just removed this debug statement in the latest Git push.