ayatana-commits team mailing list archive

[Ted Gould <ted@xxxxxxxx>]

Review: Approve
On Wed, 2010-08-04 at 11:38 +0000, Conor Curran wrote:
> +}*/
> +

In general, I'd avoid commenting out code.  It doesn't get tested by the
complier, and can be found easily enough using version control.  Axe
early and axe often :)
 
> +static gboolean
> +new_volume_slider_widget(DbusmenuMenuitem * newitem, DbusmenuMenuitem * parent, DbusmenuClient * client)
> +{
> +  g_debug("indicator-sound: new_volume_slider_widget");
> +
> +  GtkWidget* volume_widget = NULL;
> +  IndicatorObject *io = NULL;
> +
> +  g_return_val_if_fail(DBUSMENU_IS_MENUITEM(newitem), FALSE);
> +  g_return_val_if_fail(DBUSMENU_IS_GTKCLIENT(client), FALSE);
> +
> +  volume_widget = volume_widget_new (newitem);	
> +	io = g_object_get_data (G_OBJECT (client), "indicator");

Uhg, this is why the "new item" function really needs user_data :(  I
realized too late, when making the API.

>  static void
> -fetch_sink_availability_from_dbus()
> +fetch_sink_availability_from_dbus(IndicatorSound* self)
>  {
>    GError * error = NULL;
>    gboolean * available_input;
> @@ -600,38 +567,25 @@
>      g_free(available_input);
>      return;
>    }
> +	
>    device_available = *available_input;
>    if (device_available == FALSE) {
>      update_state(STATE_SINKS_NONE);
>      g_debug("NO DEVICE AVAILABLE");
>    }
> +	if(IS_INDICATOR_SOUND(self) == FALSE){
> +		g_warning("okay pointer is arseways");
> +	}

This should probably be a g_return_if_fail(IS_INDICATOR_SOUND(self)) as
you don't want the rest of the code running if it's FUBAR.
 
>  static void
>  fetch_mute_value_from_dbus()

It's an internal function, but I'd probably pass the object here anyway
just to make modifying it later easier.

> @@ -669,22 +623,6 @@
>  
> 
>  static void
> -catch_signal_sink_volume_update(DBusGProxy *proxy, gdouble volume_percent, gpointer userdata)
> -{
> -  if (slider_in_direct_use == FALSE) {
> -    GtkWidget *slider = ido_scale_menu_item_get_scale((IdoScaleMenuItem*)volume_slider);
> -    GtkRange *range = (GtkRange*)slider;
> -
> -    // DEBUG
> -    gdouble current_value = gtk_range_get_value(range);
> -    g_debug("SIGNAL- update sink volume - current_value : %f and new value : %f", current_value, volume_percent);
> -    exterior_vol_update = volume_percent;
> -    gtk_range_set_value(range, volume_percent);
> -    determine_state_from_volume(volume_percent);
> -  }
> -}
> -
> -static void
>  catch_signal_sink_mute_update(DBusGProxy *proxy, gboolean mute_value, gpointer userdata)
>  {
>    //We can be sure the service won't send a mute signal unless it has changed !

One of the reason I put small comments at the top of each function is so
that the diffs have a bunch of unique text to not merge the functions :)

> +//GObject instance struct
> +struct _IndicatorSound {
> +  IndicatorObject parent;
> +  IndicatorServiceManager *service;
> +};

It's in vogue to put an anonymous pointer in here for the private
structure instead of calling GET_PRIVATE all the time.  Basically for
faster lookup.  Probably not worth changing now, but for future objects.

> === added file 'src/volume-widget.c'

I don't see this file getting added to the list of translatable files.

> +struct _VolumeWidgetPrivate
> +{
> +	DbusmenuMenuitem* twin_item;	
> +	GtkWidget* ido_volume_slider;
> +	gboolean grabbed;
> +};
<snip>
> +static void
> +volume_widget_dispose (GObject *object)
> +{
> +	G_OBJECT_CLASS (volume_widget_parent_class)->dispose (object);
> +}
> +
> +static void
> +volume_widget_finalize (GObject *object)
> +{
> +	G_OBJECT_CLASS (volume_widget_parent_class)->finalize (object);
> +}

Are none of those items in the private structure referenced?  Should
they be?

> +	// We just want this callback to catch mouse icon press events
> +	// which set the slider to 0 or 100
> +	if(current_value == 0 || current_value == 100){
> +		volume_widget_update(mitem, current_value);
> +	}

Should this be <= 0 and >= 100?  Seems like an edge case, but could
catch something.

> === added file 'src/volume-widget.h'
> --- src/volume-widget.h	1970-01-01 00:00:00 +0000
> +++ src/volume-widget.h	2010-08-04 11:38:45 +0000
> +#include <libdbusmenu-gtk/menu.h>

Should this be menuitem.h ?

Overall, minor comments.  Looks good, I like the refactoring to a
separate object!

  review approve

-- 
https://code.launchpad.net/~cjcurran/indicator-sound/volume-slider-refactor/+merge/31739
Your team ayatana-commits is subscribed to branch lp:indicator-sound.