Re: [Rhythmbox-devel] D-Bus interface enhancements patch

[Jonathan Matthew <jonathan kaolin hn org>]

On Tue, Oct 04, 2005 at 10:43:45PM -0500, Paul Kuliniewicz wrote:

> [dbus patch]

> In [1] I had mentioned wanting to fix the ABI changes in the Bonobo
> interface.  After giving that some thought, I suspect it'd be easier to
> revert the interface back to how it was in 0.8.8 and add whatever new
> RPC features are desired only to the D-Bus interface.  I think it's
> *possible* to both preserve compatibility and allow for Bonobo
> enhancements if the interface is versioned, but it strikes me as adding
> far more complexity than it's worth.

I think this is a reasonable approach, assuming no one has written a new
bonobo client using the features added in 0.9.0, and I'm pretty sure no
one has.

> So, if this patch looks good, feel free to add it into CVS.  If there
> are problems, let me know and I'll try to take care of them.  Thanks.

A few minor comments:

> @@ -251,6 +249,7 @@
>  {
>  	WINDOW_TITLE_CHANGED,
>  	DURATION_CHANGED,
> +	ELAPSED_CHANGED,
>  	PLAYING_SOURCE_CHANGED,
>  	PLAYING_CHANGED,
>  	PLAYING_SONG_CHANGED,
> @@ -436,6 +435,17 @@
>  			      1,
>  			      G_TYPE_STRING);
>  
> +	rb_shell_player_signals[ELAPSED_CHANGED] =
> +		g_signal_new ("elapsed_changed",
> +			      G_OBJECT_CLASS_TYPE (object_class),
> +			      G_SIGNAL_RUN_LAST,
> +			      G_STRUCT_OFFSET (RBShellPlayerClass, elapsed_changed),
> +			      NULL, NULL,
> +			      g_cclosure_marshal_VOID__UINT,
> +			      G_TYPE_NONE,
> +			      1,
> +			      G_TYPE_UINT);
> +
>  	rb_shell_player_signals[PLAYING_SOURCE_CHANGED] =
>  		g_signal_new ("playing-source-changed",
>  			      G_OBJECT_CLASS_TYPE (object_class),

Is there any reason the elapsed time couldn't just be added as another
argument on the duration_changed signal?  It seems a bit weird to have
two signals emitted in exactly the same places carrying the same
information in different forms.


> @@ -1761,13 +1774,8 @@
>  		artist = rb_refstring_get (entry->artist);
>  	}
>  
> -	if (player->priv->have_url)
> -		rb_header_set_urldata (player->priv->header_widget,
> -				       entry_title,
> -				       player->priv->url);
> -	else
> -		rb_header_set_urldata (player->priv->header_widget,
> -				       NULL, NULL);
> +	rb_header_set_urldata (player->priv->header_widget,
> +			       NULL, NULL);
>  
>  	if (player->priv->song && entry_title)
>  		title = g_strdup_printf ("%s (%s)", player->priv->song,

Might as well remove all the stuff in rb-header.c that goes along with
rb_header_set_urldata.  I think this should be split out into a separate
patch.


> @@ -1971,11 +1979,17 @@
>  }
>  
>  gboolean
> -rb_shell_player_get_playing (RBShellPlayer *player)
> +rb_shell_player_get_playing (RBShellPlayer *player,
> +			     gboolean *playing,
> +			     GError **error)
>  {
> -	g_return_val_if_fail (RB_IS_SHELL_PLAYER (player), -1);
> +	if (playing != NULL) {
> +		*playing = rb_player_playing (player->priv->mmplayer);
> +	} else {
> +		*playing = FALSE;
> +	}
>  
> -	return rb_player_playing (player->priv->mmplayer);
> +	return TRUE;
>  }
>  
>  RBPlayer *

*playing = FALSE dereferences a null pointer here.. kind of bad.
It might be worth adding a comment saying that the unused error
parameter is there so this can be a dbus interface method.
Same for the other methods you've added.

Otherwise, it looks good to me.