On Wed, Oct 12, 2005 at 07:11:28AM +1000, Jonathan Matthew wrote:
> On Tue, Oct 04, 2005 at 10:43:45PM -0500, Paul Kuliniewicz wrote:
>
> > [dbus patch]
>
> 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.
That's pretty much my thinking.
> > 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.
I'd be inclined to do away with the duration_changed signal instead.
Shouldn't formatting the elapsed time into whatever format is
appropriate be the job of the UI code, not this? I vaguely remember
only one piece of code in Rhythmbox listens for duration_changed, so the
string formatting could just be moved to there.
But if you want to keep the string, then I guess there's no compelling
reason to not merge the two signals. I'd suggest using elapsed_changed
instead of duration_changed for the name, though, since the latter is
pretty misleading.
> > @@ -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.
Makes sense. I'll split that out.
> > @@ -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.
Hmm, not sure what I was thinking there. I don't know why anyone would
pass NULL in for playing, but if it does happen, then the code should
definitely just do nothing. The other D-Bus-ified methods should check
that the second argument isn't NULL too, which I don't think many do.
Noting that the GError is for the D-Bus calling convention is a good
idea too. I can definitely see someone coming along and thinking the
GError could be taken out since it's unused.
> Otherwise, it looks good to me.
Once I can get myself reasonably unburied from my current load of
schoolwork, I'll send out a revised patch.
Attachment:
signature.asc
Description: Digital signature