Review of attachment 166509 [details] [review]:

This generally looks OK.  Some minor issues, and I think I need to figure out what to do about sources that aren't really sources on an app-wide level, but that's not really your problem.

::: plugins/daap/rb-daap-plugin.c
@@ +810,3 @@
 	eel_gconf_set_boolean (CONF_DAAP_ENABLE_SHARING, b);
 
+	//gtk_widget_set_sensitive (widget, b);

why is this commented out?

@@ +921,3 @@
 	char *password;
+	
+	g_warning("Creating config dialog");

this doesn't sound like a warning..

::: plugins/daap/rb-daap-source.c
@@ +592,3 @@
 rb_daap_source_activate (RBSource *source)
 {
+	rb_debug("DAAP Activating");

seems a bit unnecessary..

::: plugins/daap/rb-dacp-sharing.c
@@ +3,3 @@
+ *  Implmentation of DAAP (iTunes Music Sharing) sharing
+ *
+ *  Copyright (C) 2005 Charles Schmidt <cschmidt2@emich.edu>

unless this is actually derived directly from rb-daap-sharing.c, you should credit yourself there

come to think of it, does this actually need to be in a separate file?  seems like it belongs in rb-dacp-source.c.

@@ +152,3 @@
+	}
+	known_guids = g_slist_insert_sorted (known_guids, guid, (GCompareFunc) g_strcmp0);
+	eel_gconf_set_string_list (CONF_KNOWN_REMOTES, known_guids);

this leaks the list. you need to free it with rb_slist_deep_free.

@@ +162,3 @@
+	
+	known_guids = eel_gconf_get_string_list (CONF_KNOWN_REMOTES);
+	return (g_slist_find_custom (known_guids, guid, (GCompareFunc) g_strcmp0) != NULL);

also leaks the list

@@ +213,3 @@
+
+	rb_shell_player_get_playback_state (player, &(data->shuffle_state), &repeat);
+	data->repeat_state = repeat ? REPEAT_ALL : REPEAT_NONE;

does the repeat+shuffle state (which means 'weighted random' in rb) make sense here?

@@ +226,3 @@
+		data->track = g_strdup (rhythmdb_entry_get_string (entry, RHYTHMDB_PROP_TITLE));
+		data->artist = g_strdup (rhythmdb_entry_get_string (entry, RHYTHMDB_PROP_ARTIST));
+		data->album = g_strdup (rhythmdb_entry_get_string (entry, RHYTHMDB_PROP_ALBUM));

rhythmdb_entry_dup_string is a shorter way to say this

@@ +317,3 @@
+	} else {
+		rb_dacp_source_remote_found (source);
+	};

extra ;

::: plugins/daap/rb-dacp-sharing.h
@@ +42,3 @@
+#define CONF_KNOWN_REMOTES 	CONF_DACP_PREFIX "/known_remotes"
+
+DACPShare *create_remote_share (RBPlugin *plugin);

please namespace all non-static functions

::: plugins/daap/rb-dacp-source.c
@@ +103,3 @@
+	int i;
+	
+	// Find out which entry the user just entered text

no c++ style comments, please

@@ +110,3 @@
+	}
+	
+	if (new_char < '0' || new_char > '9') { 

isdigit(new_char), maybe?

@@ +203,3 @@
+	source_class->impl_delete = NULL;
+	source_class->impl_show_popup = NULL;
+	source_class->impl_get_config_widget = NULL;

no need to set method pointers to NULL.  setting impl_show_popup to NULL may cause crashes, so don't do that.  the default implementation does nothing.

@@ +408,3 @@
+	if (connecting) {
+		gtk_widget_show (source->priv->pairing_status_widget);
+		gtk_label_set_markup (GTK_LABEL (source->priv->pairing_status_widget), "Connecting...");

this string needs to be marked for translation

@@ +410,3 @@
+		gtk_label_set_markup (GTK_LABEL (source->priv->pairing_status_widget), "Connecting...");
+	} else {
+		gtk_label_set_markup (GTK_LABEL (source->priv->pairing_status_widget), "<span foreground=\"red\">Could not pair with this Remote.</span>");

.. so does the actual message part of this; the presentation part shouldn't be

@@ +439,3 @@
+{
+	/* FIXME: What is this used for? */
+	return g_strdup (CONF_STATE_SHOW_REMOTE);

if your source doesn't have a browser, it isn't used at all.  you shouldn't need a get_browser_key method implementation.

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

?

(In reply to comment #1)

Hi Jonathan, thanks for the code review.

> Review of attachment 166509 [details] [review]:
> 
> This generally looks OK.  Some minor issues, and I think I need to figure out
> what to do about sources that aren't really sources on an app-wide level, but
> that's not really your problem.

The biggest thing I noticed is that every source is playable, while many shouldn't be, like Stores (maybe some should) and now this Remote source.

> 
> ::: plugins/daap/rb-daap-plugin.c
> @@ +810,3 @@
>      eel_gconf_set_boolean (CONF_DAAP_ENABLE_SHARING, b);
> 
> +    //gtk_widget_set_sensitive (widget, b);
> 
> why is this commented out?

I'm not done with the configuration dialog yet, this was the last thing I did. I can't make the surrounding table unsensitive because the password checkbox and the password entry are not contained together anymore (the library name is separate from the password because DACP uses that name with or without DAAP sharing). I'll need to add some logic to activate the password entry only when both sharing and password is checked. Should be easy but couldn't do it yet.
    
> +    g_warning("Creating config dialog");
> this doesn't sound like a warning..
> +    rb_debug("DAAP Activating");
> seems a bit unnecessary..

I should be reviewing more of the debug statements I put there, many are useless. Removed already.

> ::: plugins/daap/rb-dacp-sharing.c
> + *
> + *  Copyright (C) 2005 Charles Schmidt <cschmidt2@emich.edu>
> 
> unless this is actually derived directly from rb-daap-sharing.c, you should
> credit yourself there
> 
> come to think of it, does this actually need to be in a separate file?  seems
> like it belongs in rb-dacp-source.c.

I moved it into rb-dacp-source.c. I was using rb-dacp-sharing.c to keep the rb-daap-sharing.c convention, but it seems better to keep it under rb-dacp-source.c.

> +    eel_gconf_set_string_list (CONF_KNOWN_REMOTES, known_guids);
> this leaks the list. you need to free it with rb_slist_deep_free.
> +    
> +    known_guids = eel_gconf_get_string_list (CONF_KNOWN_REMOTES);
> +    return (g_slist_find_custom (known_guids, guid, (GCompareFunc) g_strcmp0)
> != NULL);
> 
> also leaks the list

Done for both, thank you for noticing that.

> 
> @@ +213,3 @@
> +
> +    rb_shell_player_get_playback_state (player, &(data->shuffle_state),
> &repeat);
> +    data->repeat_state = repeat ? REPEAT_ALL : REPEAT_NONE;
> 
> does the repeat+shuffle state (which means 'weighted random' in rb) make sense
> here?

Not sure, the remote cares only about the ON/OFF state of these properties (actually for repeat it could use REPEAT_SINGLE, but I don't think Rhythmbox supports that). Maybe if they are both ON, the "weighted random" order is better? Or that is the default when they are both on? Would it be better to retrieve the order in the plugin and see if it's random or shuffle?

> ...
> @@ +226,3 @@
> +        data->track = g_strdup (rhythmdb_entry_get_string (entry,
> RHYTHMDB_PROP_TITLE));
> ...
> rhythmdb_entry_dup_string is a shorter way to say this
> ...
> +    };
> extra ;
> ...
> +DACPShare *create_remote_share (RBPlugin *plugin);
> 
> please namespace all non-static functions
> ...
> +    // Find out which entry the user just entered text
> 
> no c++ style comments, please
> ...
> +    if (new_char < '0' || new_char > '9') { 
> 
> isdigit(new_char), maybe?
> ...
> no need to set method pointers to NULL.  setting impl_show_popup to NULL may
> cause crashes, so don't do that.  the default implementation does nothing.
> ...

All done, thanks.

> "Connecting...");
> 
> this string needs to be marked for translation
>
> +        gtk_label_set_markup (GTK_LABEL (source->priv->pairing_status_widget),
> "<span foreground=\"red\">Could not pair with this Remote.</span>");
> 
> .. so does the actual message part of this; the presentation part shouldn't be

Ok, still haven't done but will do as soon as possible. Marking these strings with _("...") is enough?

> +    /* FIXME: What is this used for? */
> +    return g_strdup (CONF_STATE_SHOW_REMOTE);
> 
> if your source doesn't have a browser, it isn't used at all.  you shouldn't
> need a get_browser_key method implementation.

Got rid of that.

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

And that also ;)

Created attachment 166616 [details] [review]
DACP Support for Rhythmbox, second version

Created attachment 166617 [details] [review]
DACP Support for Rhythmbox, third version (actually remove rb-dacp-sharing)

(In reply to comment #2)
> (In reply to comment #1)
> 
> Hi Jonathan, thanks for the code review.
> 
> > Review of attachment 166509 [details] [review] [details]:
> > 
> > This generally looks OK.  Some minor issues, and I think I need to figure out
> > what to do about sources that aren't really sources on an app-wide level, but
> > that's not really your problem.
> 
> The biggest thing I noticed is that every source is playable, while many
> shouldn't be, like Stores (maybe some should) and now this Remote source.

Right, having them identify themselves is the first step.
 
> > 
> > ::: plugins/daap/rb-daap-plugin.c
> > @@ +810,3 @@
> >      eel_gconf_set_boolean (CONF_DAAP_ENABLE_SHARING, b);
> > 
> > +    //gtk_widget_set_sensitive (widget, b);
> > 
> > why is this commented out?
> 
> I'm not done with the configuration dialog yet, this was the last thing I did.
> I can't make the surrounding table unsensitive because the password checkbox
> and the password entry are not contained together anymore (the library name is
> separate from the password because DACP uses that name with or without DAAP
> sharing). I'll need to add some logic to activate the password entry only when
> both sharing and password is checked. Should be easy but couldn't do it yet.

OK, I can ignore these bits until you're done with them.
 
> > 
> > @@ +213,3 @@
> > +
> > +    rb_shell_player_get_playback_state (player, &(data->shuffle_state),
> > &repeat);
> > +    data->repeat_state = repeat ? REPEAT_ALL : REPEAT_NONE;
> > 
> > does the repeat+shuffle state (which means 'weighted random' in rb) make sense
> > here?
> 
> Not sure, the remote cares only about the ON/OFF state of these properties
> (actually for repeat it could use REPEAT_SINGLE, but I don't think Rhythmbox
> supports that). Maybe if they are both ON, the "weighted random" order is
> better? Or that is the default when they are both on? Would it be better to
> retrieve the order in the plugin and see if it's random or shuffle?

Random and shuffle both set means weighted random in RB, I'm asking whether that makes sense to DACP clients.

> > "Connecting...");
> > 
> > this string needs to be marked for translation
> >
> > +        gtk_label_set_markup (GTK_LABEL (source->priv->pairing_status_widget),
> > "<span foreground=\"red\">Could not pair with this Remote.</span>");
> > 
> > .. so does the actual message part of this; the presentation part shouldn't be
> 
> Ok, still haven't done but will do as soon as possible. Marking these strings
> with _("...") is enough?

Yes.

You'll also need to update the libdmapsharing version requirement in configure.ac, but I guess you don't know the actual version you'll require yet.

Thanks, Jonathan!

To answer Jonathan's question, the version number for libdmapsharing with
DACP support will be 2.2. The jump to 2.2 will be necessary because we
are changing some of the properties in DAAPRecord. Alexandre, you may
change configure.ac to reflect this.

Next, I have a few comments. I will defer to Jonathan on several of these,
as they may be a matter of opinion (in this case, they end in a question
mark):

> diff --git a/plugins/daap/rb-daap-plugin.c b/plugins/daap/rb-daap-plugin.c
> index c03afe0..45b9235 100644
> --- a/plugins/daap/rb-daap-plugin.c
> +++ b/plugins/daap/rb-daap-plugin.c

>  #include <libdmapsharing/dmap.h>
> +#include <libdmapsharing/dacp-share.h>

Add dacp-share.h to dmap.h in your libdmapsharing patch so that developers
do not have to include it explicitly.

> +	DACPShare *dacp_share;

Move to global in rb-dacp-source.c, like rb-daap-sharing.c?
 
> +	guint enable_remote_notify_id;

Move to global in rb-dacp-source.c, like rb-daap-sharing.c?
 
> +	/* Check if Remote (DACP) lookup is enabled */
> +	value = gconf_client_get_without_default (client,
> +						  CONF_ENABLE_REMOTE, NULL);
> +	if (value != NULL) {
> +		remote_enabled = gconf_value_get_bool (value);
> +		gconf_value_free (value);
> +	}
> +
> +	plugin->priv->dacp_share = create_remote_share (RB_PLUGIN (plugin));
> +	if (remote_enabled) {
> +		dacp_share_start_lookup (plugin->priv->dacp_share);
> +	}
> +
> +	plugin->priv->enable_remote_notify_id =
> +		eel_gconf_notification_add (CONF_ENABLE_REMOTE,
> +					    (GConfClientNotifyFunc) enable_remote_changed_cb,
> +					    plugin);

Unless there is a compelling reason for this to be in
rb-daap-plugin.c, create rb_dacp_sharing_init(), modeled after
rb-daap-plugin.c:rb_daap_sharing_init()?

> +	g_object_unref (plugin->priv->dacp_share);
> +	
> +	if (plugin->priv->enable_remote_notify_id != EEL_GCONF_UNDEFINED_CONNECTION) {
> +		eel_gconf_notification_remove (plugin->priv->enable_remote_notify_id);
> +		plugin->priv->enable_remote_notify_id = EEL_GCONF_UNDEFINED_CONNECTION;
> +	}
> +

Move to global in rb-dacp-source.c, like rb-daap-sharing.c?

> +RBShell *
> +rb_daap_plugin_get_shell (RBDaapPlugin *plugin)
> +{
> +	return plugin->priv->shell;
> +}

Make shell a property, remove this function and use g_object_get?

> +enable_remote_changed_cb (GConfClient *client,
> +			    guint cnxn_id,
> +			    GConfEntry *entry,
> +			    RBDaapPlugin *plugin)
> +{
> +	gboolean enabled = eel_gconf_get_boolean (CONF_ENABLE_REMOTE);
> +
> +	if (enabled) {
> +		dacp_share_start_lookup (plugin->priv->dacp_share);
> +	} else {
> +		dacp_share_stop_lookup (plugin->priv->dacp_share);
> +	}
> +}

Move this to rb-dacp-source.c, like rb-daap-sharing.c?

> diff --git a/plugins/daap/rb-dacp-source.c b/plugins/daap/rb-dacp-source.c
> new file mode 100644
> index 0000000..ff60042
> --- /dev/null
> +++ b/plugins/daap/rb-dacp-source.c

[...]

> +	GtkBuilder *builder;

RBShell has a GtkUIManager * member ui_manager. Can you use that and
remove the requirement for builder?

(In reply to comment #6)
> Thanks, Jonathan!
> 
> To answer Jonathan's question, the version number for libdmapsharing with
> DACP support will be 2.2. The jump to 2.2 will be necessary because we
> are changing some of the properties in DAAPRecord. Alexandre, you may
> change configure.ac to reflect this.

Depending on timing, it might make sense to make the DACP stuff conditional based on the libdmapsharing version.  Don't worry about this for now, we can work out whether we need to do it later.

> Next, I have a few comments. I will defer to Jonathan on several of these,
> as they may be a matter of opinion (in this case, they end in a question
> mark):
> 
> > +	DACPShare *dacp_share;
> 
> Move to global in rb-dacp-source.c, like rb-daap-sharing.c?
> 
> > +	guint enable_remote_notify_id;
> 
> Move to global in rb-dacp-source.c, like rb-daap-sharing.c?

I'd rather have all the stuff in rb-daap-sharing.c move into rb-daap-plugin.c, actually.  Not saying this needs to be done as part of the DACP work, though.

> 
> > +RBShell *
> > +rb_daap_plugin_get_shell (RBDaapPlugin *plugin)
> > +{
> > +	return plugin->priv->shell;
> > +}
> 
> Make shell a property, remove this function and use g_object_get?

Probably a good idea.


> > diff --git a/plugins/daap/rb-dacp-source.c b/plugins/daap/rb-dacp-source.c
> > new file mode 100644
> > index 0000000..ff60042
> > --- /dev/null
> > +++ b/plugins/daap/rb-dacp-source.c
> 
> [...]
> 
> > +	GtkBuilder *builder;
> 
> RBShell has a GtkUIManager * member ui_manager. Can you use that and
> remove the requirement for builder?

This doesn't make any sense.  GtkUIManager and GtkBuilder are completely different things, you can't use one instead of the other.

Additional drive-by comment in rb-daap-record.c:

+               /* Since we don't support album id's on Rhythmbox, use the hash instead */
+               record->priv->albumid  = (gint64) g_quark_from_string (record->priv->album);

This doesn't look like a very good idea.  Use the address of the RBRefstring for the album name instead (that is, rhythmdb_entry_get_refstring (entry, RHYTHMDB_PROP_ALBUM)).  The albumid is an int64, so it can always fit a pointer.

Created attachment 167924 [details] [review]
DACP Support for Rhythmbox, fourth version

- Finished the UI
- Bumps libdmapsharing required version to 2.2.0
- Follows API changes in libdmapsharing patch
- Implements DACPPlayer interface
- Includes remote-icon.png in diff (must be applied with git apply)
- Doesn't fix Jonathan's comment #8, testing still must be done on that

Created attachment 168351 [details] [review]
DACP Support for Rhythmbox, fifth version

- Follow changes in libdmapsharing v5 patch
- Can now play and queue selected songs in the Remote
- Added player update change notification (much more responsive in the Remote side)
- Translatable strings for UI
- Using suggestion from Jonathan in comment #8

Created attachment 168367 [details] [review]
DACP Support for Rhythmbox, version 5.1

- Small change to accommodate changes from the libdmapsharing patch, which makes gdk-pixbuf optional and pass only filenames.

This generally looks OK now.  There are a few commented out things that need to be removed, but otherwise I'm happy for this to go in once there's a libdmapsharing release containing DACP support.

I have released version 2.1.0 of the libdmapsharing library. This version integrates Alexandre's DACP work. 2.1 is a development series to allow for testing before 2.2. There is still some work to be done in order to complete DACP support. However, Alexandre's work so far warranted being accepted so that we can begin to work on focused bug fix and feature addition patches.

Alexandre will have to make at least one small change to his Rhythmbox patch: I pulled his addition of the itemid property from the DAAPRecord class. Leaving the itemid as a database key known to DMAPDb is a bit less intrusive.

Alexandre, will you be able to make these changes any time soon?  I'd like to get this code merged soon, but I'd prefer not to work on it myself as I can't test it.

I do pretend to work on this, just not sure when I'll have the time. I've moved to another country, started working and have no internet connection at my home for the time being.

The small change Michael refered to should be simple enough, remove the item id property in RBDAAPRecord and, I believe, change one or two callback function handlers. Maybe in the next few days I can get it done.

The problem with DAAP not working if DACP is enabled, it is an issue in libdmapsharing. It is a bigger problem in Rhythmbox, because a DACPShare instance always exist if the DAAP plugin is enabled, so DAAP would never work. So my patch should probably not be merged before this issue is corrected.

Alexandre, I thought that the DAAP / DACP issue was either-or. That is, one could disable DACP and DAAP would work. This is different than what you describe above, where "DAAP would never work." Is my memory correct? Either way and as we've discussed, this is not ideal and needs to be fixed.

Could we get the patch in despite this? Mutually exlusive DAAP / DACP is not really a regression. We could have the configuration GUI reflect this mutual exclusivity.

It's not a regression, but it's pretty lame.  What's the problem?

(In reply to comment #17)
> It's not a regression, but it's pretty lame.  What's the problem?

Registering a service in libdmapsharing overwrote a previously existing service in the same process. I have pushed an update to libdmapsharing's Git master that turns DmapMdnsPublisherAvahi into a singleton that maintains a running list of services. DAAP and DACP should be possible in the same process now and can be tested.

moch, if you have an Android phone, you could try http://dacp.jsharkey.org/
The remote app also runs on ipads and ipod touches.

Created attachment 171406 [details] [review]
 DACP Support for Rhythmbox, version 5.2

I have modified Alexandre's patch to fit the latest libdmapsharing 2.[1|2] API (small changes, discussed above). This is the accepted libdmapsharing API set after his GSoC work. I'd like to get this patch into Git so that I may make small revisions to the DAAP plugin moving forward. I am presently looking into Apple XCode because I think it includes an iPhone simulator I might be able to use to test the DACP code. Like Jonathan, I do not have an iPhone.

Created attachment 171547 [details] [review]
DACP Support for Rhythmbox, version 5.3

Rebased after pushing libdmapsharing 2.2 API change to Rhythmbox Git.

Borrowed a friend's iphone to take a look at this, and it's not working.  The callback for the pairing request sent to the device is not being called.  Kind of blaming libsoup..

Seems to work OK with libsoup git master.

Searching by artist crashes:

+ Trace 224195

After fixing that, enough stuff works that I'm going to commit this shortly.  I filed bug 632431 against libdmapsharing for the album search crashes.

I think bug 611481 was the libsoup change that made it work.

OK, done.