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.
_______________________________________________
rhythmbox-devel mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/rhythmbox-devel
