To be honest I don't see any benefits we'd get from using libdmapsharing instead of the existing code.  All I see happening is the process of building from source getting more complicated, and potentially API changes to deal with.

Assuming we can stabilize the API once I am satisfied with the library, I think there are a lot of benefits. Namely, the same library can be used by several projects. The existing code is fine, but it can't be used by gthumb or the DMAP server I am working on without cutting and pasting. Once this is done, collaboration on the code is essentially over. The DMAP suite of protocols could support desirable features in many applications and encapsulating them in a library seems to be good software engineering.

I was only really thinking about this from rhythmbox's point of view.  As things stand, the daap code more or less works and doesn't require any significant amount of maintenance, which is good because it doesn't seem like anyone is willing to expend much effort to maintain it.

Created attachment 139344 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

The bad: this patch does not work yet. It applies, builds and runs, but my integration with RhythmDB does not yet work. Playlists are broken. The audio format is hard-coded to MP3. I hope to fix this in the next few weeks.

The good: roughly 6,700 lines of DAAP-related code reduced to 1,200 with the difference being moved to a library that may be shared with other projects. Real time transcoding (e.g., from Ogg/Vorbis to an iTunes supported format) is supported by libdmapsharing. I am actively supporting libdmapsharing.

Looks like this is coming along pretty well.  What library are you using for transcoding, and is there any kind of format negotiation involved, or does it just pick based on user agent strings or something like that?  I haven't looked at how the database adapter classes work in detail, but is there much of an overhead in terms of memory usage?

Once there are no major regressions compared to the current code, I'll look at doing a full review.

I am glad you are interested in this. I will continue to work on this patch. I have made a lot of progress since comment #4, but I am not yet ready to provide another patch.

I am using GStreamer / decodebin to transcode. On the other hand, I still have to implement some format negotiation. My play was to do it based on user agent strings.

The overhead due to my database adapter class should be minimal. My implementation does not maintain any records. Instead, it simply acts as an intermediary between the DMAPDb interface and RhythmDB interface. My foreach function adapter is able to create one DAAPRecord at a time (from a RhythmDB record), execute a function and then free the record.

The only place I can think of where there is more than one record duplicated is where there is a query for a subset of records (that is, when the HTTP GET request contains "query" in its URL parameters). If I remember right, I added support for the query parameter because rhythmbox would previously ignore it and always provide a list of every item in its database.

Created attachment 140853 [details] [review]
 Patch to replace DAAP-related code with libdmapsharing

Created attachment 141119 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

The patch doesn't apply cleanly to git master, which makes it hard for me to review and test it.  Can you please rebase the patch to git master?

Created attachment 141307 [details] [review]
 Patch to replace DAAP-related code with libdmapsharing

This patch is my first attempt after I rebased my rhythmbox tree to Git HEAD (20 AUG 09).

Thanks for updating the patch.  Here's my first round of review comments - I haven't actually compiled it yet, let alone tested it at all.  Hopefully the formatting isn't too screwed up to make sense of it all..

rb-daap-source:

+       /* FIXME: Arg 4 was priv->password_protected, but this is not
+        * what daap_connection_new expects. */
+       daap_source->priv->connection = daap_connection_new (name,
                                                     daap_source->priv->host,
                                                     daap_source->priv->port,
-                                         daap_source->priv->password_protected,
+                                                               NULL,
                                                                db,
-                                                               type);
+                                                               factory);

What does daap_connection_new want?  the actual password?
This looks like an API mistake in libdmapsharing - dmap_connection_new takes
a password-protected flag..

rb-daap-record-factory:

+        /* FIXME: dumb cast: */
+        record = DAAP_RECORD (rb_daap_record_new ((RhythmDBEntry *) path));

this makes no sense at all.  'path' is a string..

rb-daap-record:

Looks like you need separate 'create from entry' and 'create blank' record
factory methods.

+                /* FIXME: Support transcoding: */

so transcoding doesn't work yet?  what needs to be done to get it working?

+                /* FIXME: we should use RHYTHMDB_PROP_MIMETYPE instead */

I agree.

+                /* FIXME: */
+                record->priv->rating = 0;

+                /* FIXME: */
+                record->priv->year = 0;

these shouldn't be hard to implement.  rating is a double ranging from 0 to 5,
the rhythmdb date property is a GDate.

rb-daap-sharing:

+       /* FIXME: NULL for sharing entry type for browsing */
+       db = DMAP_DB (rb_dmap_db_adapter_new (rdb, NULL));

what needs to be done here?

rb-dmap-container-db:

+rb_dmap_container_db_foreach (DMAPContainerDb *db,
+                                   void (*fn) (DMAPContainerRecord *record,
+                                               gpointer data),
+                                   gpointer data)

use typedefs for function pointer arguments.

+        /* Don't use g_hash_table_foreach because we
+         * need values, not key / value pairs. */
+        GList *value = g_hash_table_get_values (RB_DMAP_CONTAINER_DB (db)->priv->db);
+        g_list_foreach (value, (GFunc) fn, data);

this is inefficient and it leaks the list too.  you should use g_hash_table_foreach instead.

rb-dmap-db-adapter:

remove the g_warnings.  these probably shouldn't even be rb_debug calls.

+        /* NOTE: do not g_object_ref() because this is just a transient copy
+         * of the real data that is in the RhythmDB.
+         */

this doesn't make much sense.  there's no reason to ref an object you're
returning, unless you actually keep a reference to it.

+static void rb_dmap_db_adapter_foreach  (const DMAPDb *db,
+                                         void (*fn) (gpointer id,
+                                                     DMAPRecord *record,
+                                                     gpointer data),
+                                         gpointer data)

again, use a typedef for the function pointer argument.

+        /* NOTE: do not g_object_ref() the record because its data will be
+         * copied into the RhythmDB.
+         */

this comment doesn't really make any sense.  you'd ref the record if you kept a reference to it, but you don't.

+static void rb_dmap_db_adapter_init (RBDMAPDbAdapter *db)
+{
+        db->priv = RB_DMAP_DB_ADAPTER_GET_PRIVATE (db);
+        db->priv->db = NULL;
+}

object private data structures are always initialized with 0, so there's
no need to explicitly set pointers to NULL.

plugins/daap/Makefile.am:

no need to add GLIB_LIBS and GLIB_CFLAGS, they're implicitly included in
RHYTHMBOX_LIBS and RHYTHMBOX_CFLAGS, amongst others.

configure.ac:

The test to enable building the daap plugin is a bit simplistic.  generally we have a --enable-daap type argument that can be used to explicitly disable the plugin (--disable-daap), make configure fail if it can't be built (--enable-daap) or built it if possible (unspecified or --enable-daap=auto).

Created attachment 141976 [details] [review]
Patch to replace DAAP-related code with libdmapsharing   

This patch fixes most of the issues raised in comment #11.

The comments regarding ref/unref make sense to me in the context of the overall libdmapsharing API. They serve as a reminder that this adapter technique provides "transient" records that should be free'd after use (users of the API will unref -- in this application, the reference count should go to 0). This is because the real data lies in the RhythmDB.

Transcoding works as far as libdmapsharing is concerned. All that is required is a field added to the plugin GUI that would allow the user to select the target format. Once this is in place, we would just need to pass the target mimetype to rb-daap-sharing.c:daap_share_new().

(In reply to comment #12)
> The comments regarding ref/unref make sense to me in the context of the overall
> libdmapsharing API. They serve as a reminder that this adapter technique
> provides "transient" records that should be free'd after use (users of the API
> will unref -- in this application, the reference count should go to 0). This is
> because the real data lies in the RhythmDB.

In that case, those comments belong in the libdmapsharing API documentation.  You shouldn't need comments in the RB code explaining how the API works.

> Transcoding works as far as libdmapsharing is concerned. All that is required
> is a field added to the plugin GUI that would allow the user to select the
> target format. Once this is in place, we would just need to pass the target
> mimetype to rb-daap-sharing.c:daap_share_new().

Why would the user have to select the target format manually?

(In reply to comment #13)
>> The comments regarding ref/unref make sense to me in the context of the
>> overall libdmapsharing API. They serve as a reminder that this adapter
>> technique provides "transient" records that should be free'd after use 
>> (users of the API will unref -- in this application, the reference count 
>> should go to 0). This is because the real data lies in the RhythmDB.
 
> In that case, those comments belong in the libdmapsharing API documentation. 
> You shouldn't need comments in the RB code explaining how the API works.

I agree. I have pulled these comments out of my Rythmbox patch and will add comments like them to libdmapsharing's documentation instead.
 
>> Transcoding works as far as libdmapsharing is concerned. All that is required
>> is a field added to the plugin GUI that would allow the user to select the
>> target format. Once this is in place, we would just need to pass the target
>> mimetype to rb-daap-sharing.c:daap_share_new().

> Why would the user have to select the target format manually?

I have been going back and forth on this because of the different client capabilities out there and the lack of a format interrogation feature in DAAP. iTunes has a pretty straight-forward list of supported formats. Many other players may or may not support a CODEC, based on the plugins available. So, I'm not sure I'd want to rely on user agent strings. Of the most common DAAP formats, MP3 is sometimes not available (possibly due to patent issues) and WAV is high bandwidth.

Created attachment 142370 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

Review of attachment 142370 [details] [review]:

I'm not sure how you generated this patch, but it includes a lot of unrelated changes that have been committed to git.

::: configure.ac
@@ -793,0 +772,9 @@
+
+dnl ================================================================
+dnl Use libdmapsharing for DAAP?
... 6 more ...

There should be an --enable-daap style flag here, where 'yes' will cause configuration to fail if libdmapsharing is not available, 'no' will ignore libdmapsharing entirely, and 'auto' (or no setting) would have the current behaviour.

::: plugins/daap/rb-daap-record.c
@@ +312,3 @@
+	record = RB_DAAP_RECORD (g_object_new (TYPE_RB_DAAP_RECORD, NULL));
+
+	/* FIXME: when browsing, entry will be NULL because we will pull

Looks like there should be separate 'new' and 'new empty' methods for these cases

::: plugins/daap/rb-dmap-db-adapter.c
@@ +157,3 @@
+	/* year */
+	if (year != 0) {
+		GDate *date;

It'd be a better idea to use a GDate on the stack instead, to save the heap allocation per entry

Created attachment 145403 [details] [review]
Patch to replace DAAP-related code with libdmapsharing 

This new patch addresses Jonathan's first and third comment about the previous patch.

I'm having a bit of trouble patching with Git at the moment (my connectivity is over a TDMA satellite link with a lot of services filtered out -- I can't pull Git changes, for example). As a result, my patches may be a bit off. But, I do want to continue to provide an indication of progress. I will clean up my patches as soon as I can.

The patch depends on changes to libdmapsharing that aren't yet in git master (the change to the DMAPRecordFactory create function, for a start), so I can't really do anything with it until you can push your changes there.

Created attachment 148714 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

Libdmapsharing-1.9.0.14 is now available at:

http://www.flyn.org/projects/libdmapsharing/download.html

or as Git master (note: the Git tree's LIBDMAPSHARING_1_9_0_14 is mistagged -- I won't make that mistake again).

Great, we should be able to get this finished soon. One small problem: I don't see any of the new files in this version of the patch.

Created attachment 148873 [details] [review]
Successive patch to "git add" new files

Review of attachment 148873 [details] [review]:

The licensing of the new files is inconsistent - some are LGPL, some are GPL. Could you change them all to GPL+exception for consistency with everything else?

::: plugins/daap/rb-daap-record-factory.c
@@ +49,3 @@
+	DMAPRecordFactoryInterface *factory = iface;
+
+	g_assert (G_TYPE_FROM_INTERFACE (factory) == TYPE_DMAP_RECORD_FACTORY);

what does this assertion do?

::: plugins/daap/rb-daap-record-factory.h
@@ +26,3 @@
+G_BEGIN_DECLS
+
+#define TYPE_RB_DAAP_RECORD_FACTORY         (rb_daap_record_factory_get_type ())

Everywhere else, this macro would be named RB_TYPE_DAAP_RECORD_FACTORY

@@ +31,3 @@
+#define RB_DAAP_RECORD_FACTORY_CLASS(k)     (G_TYPE_CHECK_CLASS_CAST((k), \
+				               TYPE_RB_DAAP_RECORD_FACTORY, RBDAAPRecordFactoryClass))
+#define IS_RB_DAAP_RECORD_FACTORY(o)        (G_TYPE_CHECK_INSTANCE_TYPE ((o), \

likewise, this would be RB_IS_DAAP_RECORD_FACTORY

@@ +33,3 @@
+#define IS_RB_DAAP_RECORD_FACTORY(o)        (G_TYPE_CHECK_INSTANCE_TYPE ((o), \
+				               TYPE_RB_DAAP_RECORD_FACTORY))
+#define IS_RB_DAAP_RECORD_FACTORY_CLASS(k)  (G_TYPE_CHECK_CLASS_TYPE ((k), \

and RB_IS_DAAP_RECORD_FACTORY_CLASS

::: plugins/daap/rb-daap-record.c
@@ +38,3 @@
+	gboolean has_video;
+	gint rating;
+	gint32 duration;

Why are duration and the following properties gint32s?  The property type is G_TYPE_INT, which maps to int, not gint32.

@@ +207,3 @@
+}
+
+gboolean rb_daap_record_itunes_compat (DAAPRecord *record)

code style nit: should be a line break after 'gboolean' here (and everywhere else)

@@ +222,3 @@
+	GInputStream *fnval = NULL;
+
+	file = g_file_new_for_uri (RB_DAAP_RECORD (record)->priv->location);

this leaks the file object.  it needs to be unrefed after the g_file_read call.

@@ +233,3 @@
+}
+
+static void rb_daap_record_finalize (GObject *object);

function prototypes should go near the top of the file

@@ +315,3 @@
+	 * the metadata from the DAAP query. When sharing entry will point
+	 * to an existing entry from the Rhythmbox DB. Where should this
+	 * differentiation be handled?

I think you need two constructors: rb_daap_record_new (this one) and rb_daap_record_new_empty

@@ +323,3 @@
+						(entry, RHYTHMDB_PROP_FILE_SIZE);
+
+		record->priv->location = g_strdup (rhythmdb_entry_get_string

use rhythmdb_entry_dup_string for this

::: plugins/daap/rb-daap-record.h
@@ +26,3 @@
+G_BEGIN_DECLS
+
+#define TYPE_RB_DAAP_RECORD         (rb_daap_record_get_type ())

same comments re: these macro names as in rb-daap-record-factory.h

::: plugins/daap/rb-dmap-container-db.c
@@ +40,3 @@
+			      gpointer data)
+{
+	g_hash_table_foreach (RB_DMAP_CONTAINER_DB (db)->priv->db, (GHFunc) func, data);

shouldn't need to cast a GHFunc to a GHFunc here..

@@ +62,3 @@
+{
+	db->priv = RB_DMAP_CONTAINER_DB_GET_PRIVATE (db);
+	db->priv->db = g_hash_table_new (g_int_hash, g_int_equal);

should use g_hash_table_new (g_direct_hash, g_direct_equal) here, and use GINT_TO_POINTER(id) as the key to save memory allocations

Review of attachment 148714 [details] [review]:

::: configure.ac
@@ -345,1 @@
 AC_CHECK_LIB(z, uncompress)

I don't think the check for libz is needed any more

Created attachment 148981 [details] [review]
Successive patch that addresses comment #22

I think this patch addresses everything in comment #22, except:

> what does this assertion do?

This is a common test that I have seen in example code, including GObject's testgobject.c.

> I think you need two constructors: rb_daap_record_new (this one) and
> rb_daap_record_new_empty

This is tied to the current libdmapsharing API.

(In reply to comment #24)
> Created an attachment (id=148981) [details] [review]
> Successive patch that addresses comment #22
> 
> I think this patch addresses everything in comment #22, except:
> 
> > what does this assertion do?
> 
> This is a common test that I have seen in example code, including GObject's
> testgobject.c.

Well, OK then.  I wouldn't bother with it myself, but I don't mind it being there.

> > I think you need two constructors: rb_daap_record_new (this one) and
> > rb_daap_record_new_empty
> 
> This is tied to the current libdmapsharing API.

Sort of.  It's called from rb_dmap_db_adapter_lookup_by_id and foreach_adapter (in rb-dmap-db-adapter.c) to create records for existing entries, and from rb_daap_record_factory_create (implementing the DMAPRecordFactory interface), which as far as I can tell is only called with user_data==NULL from dmap-connection.c.

It looks to me like the record factory interface will only ever be used to construct records to hold data received from a server, rather than to represent local database entries.  If this is true, then the record factory can call rb_daap_record_new_empty() and the other callers can use rb_daap_record_new().

Actually trying this out for this first time, using rb+this patch as the client, regular rb as the server:
- connecting to password protected shares doesn't seem to work (haven't investigated at all)
- lots of this: GLib-GObject-WARNING **: value "0" of type `gint' is invalid or out of range for property `disc' of type `gint'
- same for the 'track' property
- libdmapsharing produces an awful lot of debug output without even being asked to
- my playlists came up empty
- track durations seem to have been scaled down by some factor; most tracks show up as 'unknown', an 18 hour long file shows up as 1:03

With regular rb as client, rb+libdmapsharing as server, I'm not seeing any playlists on the client.

Seems to be something wrong with this debug output:
(22:27:46) [0x16fe060] [mdns_service_removed] rb-daap-plugin.c:497: DAAP source '`=*' went away

The service-removed signal parameter is actually a DMAPMdnsBrowserService, but the handler is expecting a string. That means it's not going to handle services disappearing at all.

(In reply to comment #26)

> - libdmapsharing produces an awful lot of debug output without even being asked
> to

Libdmapsharing (git master) now uses its own log domain, "libdmapsharing." The problem now seems to be that GLib2 sends g_debug to stdout by default. I modified rb-debug.c to consider the "libdmapsharing" log domain. However, GLib2 still prints my debug statements by default. I can't set a null log handler in the DAAP plugin because this would overwrite that which may have been set by rb-debug.c. I'd like the debugging statements to be silent unless explicitly requested. How should this be handled?

I don't know, actually.  The only library I can think of that has a useful amount of debug output is GStreamer, and it has its own debug system.

I ended up not using g_debug for libmediaplayerid (which isn't really a separate library) for this reason.

(In reply to comment #28)

> The service-removed signal parameter is actually a DMAPMdnsBrowserService, but
> the handler is expecting a string. That means it's not going to handle services
> disappearing at all.

I changed libdmapsharing to provide the signal argument that Rhythmbox expects (git master).

Created attachment 149187 [details] [review]
Successive patch that adds libdmapsharing to list of log domains

(In reply to comment #31)
> (In reply to comment #28)
> 
> I changed libdmapsharing to provide the signal argument that Rhythmbox expects
> (git master).

I'm not sure this was really the right fix. Also, dmap-mdns-browser.h still shows the signal argument as being a DMAPMdnsBrowserService, not a string.

This commit: http://git.gnome.org/browse/rhythmbox/commit/?id=4072afad6f912d9e90616c3a63b28a1ec13592ec

changes the RBDAAPConnection interface - rb_daap_connection_get_headers now returns a GstStructure containing the header information, and no longer adds a range header for seeking.  I think the libdmapsharing API should change similarly.

I'm not sure if you'd want to expose GstStructure in the libdmapsharing API.  Maybe a SoupMessageHeaders object would make more sense?  We'd then have to convert that into a GstStructure to feed it to souphttpsrc (which would then convert it back..).

Created attachment 154867 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This patch requires the most recent libdmapsharing code from the GNOME Git repository. Dmap_connection_get_headers now returns a SoupMessageHeaders * and Rhythmbox converts this to a GstStructure for use with souphttpsrc. I've avoided adding a hard dependency to libdmapsharing for GStreamer.

This version of the patch is missing all the added files.

Created attachment 154869 [details] [review]
Patch to add new files

The code generally looks OK now.  Any idea whether the problems I mentioned in comment 26 and comment 27 (the first one) have been fixed?

Created attachment 155823 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This patch set requires the most recent libdmapsharing from GNOME Git (as of 2010-03-11 04:16:42 (GMT)).

This patch set begins to fix client-side support for password-protected shares. Login request gets a 200, OK. But the follow on update gets a 403, forbidden.

Does the existing rhythmbox code work with password-protected shares? I am finding that 0.12.6 does not connect to them.

Created attachment 155824 [details] [review]
Patch to add new files

It doesn't build at present because libdmapsharing/dmap.h doesn't include daap-connection.h.

Connecting to password protected shares works now.  Other problems still exist.

.. or the rb patch hasn't been updated for the removal of the DAAPConnection object.

Anyway, on the subject of debug output, I'd suggest a couple of things:
1) use INCLUDES in libdmapsharing to define G_LOG_DOMAIN everywhere, rather than having to include dmap-priv.h to do it - it's currently only set in a few files rather than for the whole library.
2) remove libdmapsharing from the list of domains in rb-debug.c
3) in rb-daap-plugin.c, install a handler for the libdmapsharing domain:

static void
libdmapsharing_debug (const char *domain, GLogLevelFlags level, const char *message, gpointer data)
{
  if ((level & G_LOG_LEVEL_DEBUG) != 0) {
    rb_debug ("%s", message);
  } else {
    g_log_default_handler (domain, level, message, data);
  }
}

This way, libdmapsharing debug output goes through rb_debug, so it doesn't show up by default but you can enable it by running rhythmbox with -D libdmapsharing, and any warnings or criticals get output as normal.

Is there a particular reason the minimum values for the DAAP record track, year, and disc properties is 1?  We use 0 for 'undefined' for all of those.

(In reply to comment #26)
> - track durations seem to have been scaled down by some factor; most tracks
> show up as 'unknown', an 18 hour long file shows up as 1:03

rb_dmap_db_adapter_add() and dmap-connection.c:handle_song_listing() both divide the length by 1000.  Only one of them should.

(In reply to comment #26)
> - my playlists came up empty

_add_location_to_playlist thinks that the list entries are RBRefStrings, but they are normal strings.

_add_location_to_playlist (const char *uri, RBStaticPlaylistSource *source)
{
  rb_static_playlist_source_add_location (source, uri, -1);
}

One of my playlists now includes the entire contents of the library, oddly enough.

(In reply to comment #27)
> With regular rb as client, rb+libdmapsharing as server, I'm not seeing any
> playlists on the client.

Looks like we need an implementation of the DMAPContainerRecord interface that handles playlist sources, and the DMAPContainerDB implementation should read from the source list.

Created attachment 156232 [details] [review]
Change DAAPConnection to DMAPConnection

Created attachment 156233 [details] [review]
Remove libdmapsharing from the list of domains in rb-debug.c

Created attachment 156235 [details] [review]
Move libdmapsharing debug handler to rb-daap-plugin.c

Created attachment 156237 [details] [review]
Don't divide track length by 1000

Created attachment 156238 [details] [review]
Fix error setting up debug logger

> the rb patch hasn't been updated for the removal of the DAAPConnection object.

See recent patches.

> use INCLUDES in libdmapsharing to define G_LOG_DOMAIN everywhere, rather than having to 
> include dmap-priv.h to do it - it's currently only set in a few files rather than for the whole library.

Fixed in libdmapsharing.

> remove libdmapsharing from the list of domains in rb-debug.c

See recent patches.

> in rb-daap-plugin.c, install a handler for the libdmapsharing domain

See recent patches. This does not work correctly yet.

> Is there a particular reason the minimum values for the DAAP record track,
> year, and disc properties is 1?  We use 0 for 'undefined' for all of those.

Fixed in libdmapsharing.

> rb_dmap_db_adapter_add() and dmap-connection.c:handle_song_listing() both
> divide the length by 1000.  Only one of them should.

See recent patches.

> Looks like we need an implementation of the DMAPContainerRecord interface that
> handles playlist sources, and the DMAPContainerDB implementation should read
> from the source list.

I will work on this next.

Please don't give me patches on top of patches.  I'd much prefer a single patch containing all your changes.

Comment on attachment 156232 [details] [review]
Change DAAPConnection to DMAPConnection

this looks OK

Comment on attachment 156233 [details] [review]
Remove libdmapsharing from the list of domains in rb-debug.c

OK

Review of attachment 156235 [details] [review]:

::: plugins/daap/rb-daap-plugin.c
@@ +228,3 @@
 	plugin->priv->shell = g_object_ref (shell);
 
+	libdmapsharing_debug ("libdmapsharing",

pretty sure you want to call g_log_set_handler here, not the handler itself

Comment on attachment 156237 [details] [review]
Don't divide track length by 1000

OK

Comment on attachment 156238 [details] [review]
Fix error setting up debug logger

er.. yeah.

also, git works better when you leave an empty line between the first (summary) line and the rest of the commit message.  See the 'discussion' section of the git-commit man page.

Created attachment 156426 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This patch requires the latest libdmapsharing code from GNOME Git: 2010-03-18 01:18:00 (GMT). The DAAP plugin debugger and server-side playlist handling have been implemented but require more work. Receiving playlists as a client, receiving songs as a client, serving songs and password-protected shares have survived basic testing.

Review of attachment 156426 [details] [review]:

I think you've misunderstood how playlists work in rhythmbox.  They don't have their own RhythmDB instance - there is only one.  Instead, the entries in the playlist are defined by a RhythmDBQueryModel.

::: plugins/daap/rb-daap-container-record.c
@@ +60,3 @@
+	switch (prop_id) {
+		case PROP_NAME:
+			/* FIXME: should I free name first? */

if 'name' is a construct-only property, there's no need.  otherwise, yes.

@@ +103,3 @@
+rb_daap_container_record_get_entry_count (DMAPContainerRecord *record)
+{
+	return rhythmdb_entry_count_by_type (RB_DAAP_CONTAINER_RECORD (record)->priv->db, RHYTHMDB_ENTRY_TYPE_SONG);

This doesn't seem right.. shouldn't this be the number of entries in the playlist, not in the database?

::: plugins/daap/rb-dmap-container-db-adapter.c
@@ +56,3 @@
+	DMAPContainerRecord *fnval = NULL;
+
+	playlists = rb_playlist_manager_get_playlists (RB_DMAP_CONTAINER_DB_ADAPTER (db)->priv->playlist_manager);

need to free the list

@@ +83,3 @@
+	record = DMAP_CONTAINER_RECORD (rb_daap_container_record_new (name, rb_playlist_source_get_db (entry)));
+
+	/* Set ID as pointer to playlist RhythmDb, this is unique and not 1: */

No, it isn't.  All playlists use the same database.  I'm not really sure what you could use instead.

@@ +99,3 @@
+	GList *playlists;
+
+	playlists = rb_playlist_manager_get_playlists (RB_DMAP_CONTAINER_DB_ADAPTER (db)->priv->playlist_manager);

need to free the list

Created attachment 156643 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

Implements a RhythmDBQueryModel adapter to fix serving playlists.

This patch requires the latest libdmapsharing code from GNOME Git:
2010-03-18 01:18:00 (GMT). The DAAP plugin debugger has been implemented
but requires more work. Receiving playlists as a client, receiving songs
as a client, serving songs, serving playlists and password-protected
shares have survived basic testing.

Review of attachment 156643 [details] [review]:

I haven't actually tried this yet, but I spotted a couple of simple things that need to be fixed.

::: plugins/daap/rb-daap-container-record.c
@@ +168,3 @@
+	record->priv->name = name;
+	/* Set ID to pointer, this is unique and not 1: */
+	record->priv->id = GPOINTER_TO_UINT (model);

I'd be a bit nervous about this on 64bit systems.  We probably need to assign sequential IDs to query models, or maybe to playlists.

::: plugins/daap/rb-dmap-container-db-adapter.c
@@ +65,3 @@
+			source = RB_PLAYLIST_SOURCE (result->data);
+			g_object_get (source, "name", &name, NULL);
+			fnval = DMAP_CONTAINER_RECORD (rb_daap_container_record_new (name, rb_playlist_source_get_query_model (source)));

This should actually use the source's base-query-model property.  The base query model includes all entries in the playlist.  The query model you're using here will reflect any search terms and browse selections the user has for the playlist at the time.

::: plugins/daap/rb-rhythmdb-query-model-dmap-db-adapter.c
@@ +48,3 @@
+rb_rhythmdb_query_model_dmap_db_adapter_lookup_by_id (DMAPDb *db, guint id)
+{
+	g_error ("Not implemented");

If needed, you could implement this by doing rhythmdb_entry_lookup_by_id to find the entry, then rhythmdb_query_model_entry_to_iter to check if it's in the query model

Created attachment 156657 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This revision addresses the first two notes in the previous comment. I added an ID to DBPlaylistSource.

This patch requires the latest libdmapsharing code from GNOME Git:
2010-03-18 01:18:00 (GMT). The DAAP plugin debugger has been implemented
but requires more work. Receiving playlists as a client, receiving songs
as a client, serving songs, serving playlists and password-protected
shares have survived basic testing.

Review of attachment 156657 [details] [review]:

I don't think the playlist ID belongs in the playlist source if it's only used for DAAP sharing.  I think it'd be better to use g_object_set_data() to attach a playlist ID to the playlist source.

::: plugins/daap/rb-daap-container-record.c
@@ +108,3 @@
+		     &model,
+		      NULL);
+	return gtk_tree_model_iter_n_children (GTK_TREE_MODEL (model), NULL);

need to unref the model here

::: plugins/daap/rb-dmap-container-db-adapter.c
@@ +46,3 @@
+static gint find_by_id (gconstpointer a, gconstpointer b)
+{
+	return rb_playlist_source_get_id (RB_PLAYLIST_SOURCE (a)) != GPOINTER_TO_INT (b);

use g_object_get_data to retrieve the daap playlist ID here

@@ +157,3 @@
+					       NULL));
+
+	db->priv->playlist_manager = playlist_manager;

assign IDs to playlists here using g_object_set_data, then use the playlist manager's playlist-created signal to assign IDs to newly created playlists.

::: plugins/daap/rb-rhythmdb-query-model-dmap-db-adapter.c
@@ +150,3 @@
+					       NULL));
+
+	db->priv->model = model;

this essentially adopts the reference on the model, so it needs to be unreffed when disposing the adapter

Aside from that, I think all of the above problems have been fixed, except that one of the playlists (which should have been empty) served from rb+libdmapsharing contains the full library.

When I enabled password protection on my non-libdmapsharing server, I had to restart the rb+libdmapsharing client before it could connect.

Created attachment 157936 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This patch requires the latest libdmapsharing code from GNOME Git: 2010-03-18 01:18:00 (GMT). The DAAP plugin debugger has been implemented but requires more work. Receiving playlists as a client, receiving songs as a client, serving songs, serving playlists and password-protected shares have survived basic testing.

Jonathan, what remains before this patch may be accepted into Rhythmbox? I am mentoring Alexandre Rosenfeld's Google Summer of Code project to bring DACP support to libdmapsharing and Rhythmbox. Getting this patch into Rhythmbox is a prerequisite to his work. Eventually, Alexandre should be submitting a patch to Rhythmbox that will provide DACP functionality (using libdmapsharing). This will allow a DACP client (e.g., an iPod touch or Linux client) to control Rhythmbox over the network.

My main conditions are that 1) the code looks OK and 2) no major regressions.  Minor regressions are OK if you're planning to maintain the code, which I assume you are.  I think we're pretty much there, I just haven't got around to reviewing and testing the latest patch.

Created attachment 160557 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This patch requires the latest libdmapsharing code from GNOME Git: 2010-05-06 23:23:26 (GMT). The DAAP plugin debugger has been implemented but requires more work. Receiving playlists as a client, receiving songs as a client, serving songs, serving playlists and password-protected shares have survived basic testing. This patch integrates with the new artist and album sort order code in libdmapsharing.

Yes, I plan on maintaining libdmapsharing. I am presently working on documentation for the library. See http://www.flyn.org/projects/libdmapsharing/docs/index.html. Also, libdmapsharing is the subject of a Google Summer of Code project, Alexandre Rosenfeld's effort to add DACP support to libdmapsharing and Rhythmbox. Getting this patch into Rhythmbox will set the conditions for Alexandre's efforts. The Google Summer of Code "Community Bonding Period" ends on May 24th.

Review of attachment 160557 [details] [review]:

What is the 'DAAP plugin debugger' mentioned in your commit message?  I don't see anything like that anywhere..

::: plugins/daap/rb-daap-record.c
@@ +223,3 @@
+		case PROP_REAL_FORMAT:
+			g_value_set_string (value, record->priv->real_format);
+			break;

Need to add PROP_ARTIST_SORT_NAME and PROP_ALBUM_SORT_NAME here.

libdmapsharing currently crashes in add_entry_to_mlcl() if an entry has a NULL artist or album sortname.

Created attachment 161516 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This patch requires libdmapsharing 1.9.0.16. Receiving playlists as a client, receiving songs as a client, serving songs, serving playlists and password-protected shares have survived basic testing. This patch integrates with the new artist and album sort order code in libdmapsharing.

The "DAAP plugin debugger" note in the previous patch was a relic from when I first started integrating libdmapsharing's debug system with Rhythmbox. I have removed this comment from the patch.

FIxed plugins/daap/rb-daap-record.c as noted in comment #71.

Libdmapsharing 1.9.0.16 addresses the NULL sort name issue mentioned in comment #71. It also integrates the changes that were previously only in Git.

Created attachment 162080 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

I have rebased this patch to a newer Rhythmbox revision. I also modified it to adopt two small libdmapsharing API changes:

1) dmap_db_add now returns a guint id, not a gint.

2) Some properties within the DAAPRecord object have adopted DAAP keywords as their name (e.g., daap.songgenre). The purpose of this is to make some of the logic in libdmapsharing simpler.

Note that I do not plan on making API changes to libdmapsharing without a major new release once Rhythmbox has adopted the library. I plan to release the Rhythmbox-accepted API as libdmapsharing 2.0.

I just released libdmapsharing 1.9.0.19. This version may be used with the previous patch in lieu of Git master.

Review of attachment 162080 [details] [review]:

OK, I think this is ready once a few minor things are fixed.

::: configure.ac
@@ +786,3 @@
+              enable_daap=auto)
+if test "x$enable_daap" != "xno"; then
+	PKG_CHECK_MODULES(DMAPSHARING, libdmapsharing-1.9 >= 1.9.0.16,

Probably should depend on 1.9.0.19, since earlier versions will crash a lot due to the sortname changes.

::: plugins/daap/Makefile.am
@@ +36,3 @@
 	$(DBUS_LIBS)					\
 	$(RHYTHMBOX_LIBS)				\
 	$(MDNS_LIBS)

Should remove MDNS_LIBS and MDNS_CFLAGS since they aren't defined any more

::: plugins/daap/rb-dmap-container-db-adapter.c
@@ +48,3 @@
+static gint find_by_id (gconstpointer a, gconstpointer b)
+{
+	return GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (a), "daap_id")) != GPOINTER_TO_INT (b);

Probably should be casting both of these to the same type.

It'd be good for the ID types to be consistent.  dmap_container_db_lookup_by_id and dmap_container_record_get_id use gint, but dmap_db_lookup_by_id uses guint.

::: sources/rb-playlist-source.c
@@ -133,3 @@
 };
 
-

Please remove this

Created attachment 162279 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

This patch requires libdmapsharing 1.9.0.20. Receiving playlists as a client, receiving songs as a client, serving songs, serving playlists and password-protected shares have survived basic testing. This patch integrates with the new artist and album sort order code in libdmapsharing.

This patch revision implements the recommendations from comment #75.

There isn't yet a version 1.9.0.20 in libdmapsharing git, but I assume that'll appear shortly.

Comment on attachment 162279 [details] [review]
Patch to replace DAAP-related code with libdmapsharing

Pushed as commit ef58049.

Congratulations, you are now the owner of the DAAP plugin.