ayatana-commits team mailing list archive
-
ayatana-commits team
-
Mailing list archive
-
Message #02013
Re: [Merge] lp:~cjcurran/indicator-sound/volume-slider-refactor into lp:indicator-sound
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.
References