GNOME Bugzilla – Bug 745182
[PATCH] power: Add support for dimming using an ambient light sensor
Last modified: 2015-05-22 13:25:02 UTC
Created attachment 297923 [details] [review] initial patch This adds a new line in the power panel, only shown when ambient light sensor hardware is detected. I'm also happy to send a free USB device to anyone that wants to help with this functionality. Comments welcome, thanks.
Review of attachment 297923 [details] [review]: ::: panels/power/cc-power-panel.c @@ +1228,3 @@ + g_dbus_proxy_call (self->priv->als_properties, + "Set", + g_variant_new_parsed ("('org.gnome.SettingsDaemon.Power.AmbientLightSensor'," Needs porting to iio-sensor-proxy's API. @@ +1229,3 @@ + "Set", + g_variant_new_parsed ("('org.gnome.SettingsDaemon.Power.AmbientLightSensor'," + "'IsEnabled', %v)", IsEnabled should be a gsetting instead.
Can you also upload a screenshot?
(In reply to Bastien Nocera from comment #1) > Review of attachment 297923 [details] [review] [review]: > > ::: panels/power/cc-power-panel.c > @@ +1228,3 @@ > + g_dbus_proxy_call (self->priv->als_properties, > + "Set", > + g_variant_new_parsed > ("('org.gnome.SettingsDaemon.Power.AmbientLightSensor'," > > Needs porting to iio-sensor-proxy's API. I don't think the control center needs to do anything other than enabling/disabling the behaviour in g-s-d -- I used D-Bus, but we could equally use a GSettings key and watch it from both processes if you'd prefer. Richard.
(In reply to Richard Hughes from comment #3) > We could equally use a GSettings key and watch it from both processes if you'd prefer. Actually, we'd need to have both -- we only show the UI if there is hardware on the machine, so you actually do need to do this over D-Bus. It would be a bit odd to have a GSetting key for state like "has_hardware". Would you rather just IsEnabled was a settings key?
IsEnabled should be a gsettings key in gnome-settings-daemon. The presence or absence of hardware will be advertised using iio-sensor-proxy' D-Bus API.
Created attachment 303543 [details] [review] new patch using the iio-proxy API
Created attachment 303544 [details] what this looks like
Review of attachment 303543 [details] [review]: Pending the g-s-d changes, that looks fine.
Review of attachment 303543 [details] [review]: Sorry, one small comment. ::: panels/power/cc-power-panel.c @@ +974,3 @@ + return; + + v = g_dbus_proxy_get_cached_property (priv->iio_proxy, "HasAmbientLight"); We also need to check whether there would a brightness slider, otherwise no point in showing this toggle.
Created attachment 303589 [details] [review] new patch using the iio-proxy API this is with checking for a backlight as well
Review of attachment 303589 [details] [review]: Note that I'm currently making some changes to the iio-sensor-proxy API, to be better at avoiding polling. ::: panels/power/cc-power-panel.c @@ +3,3 @@ * Copyright (C) 2010 Red Hat, Inc * Copyright (C) 2008 William Jon McCann <jmccann@redhat.com> + * Copyright (C) 2010-2015 Richard Hughes <richard@hughsie.com> Use "2010, 2015" for discrete dates. @@ +988,3 @@ + g_variant_unref (v); + } + One linefeed too much here. @@ +1572,3 @@ + /* ambient light sensor */ + priv->iio_proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, iio-sensor-proxy will disappear if there are no sensors to monitor. As this can happen when unplugging the ColorHug ALS USB sensor, it would be best to always create the UI, and use g_bus_watch_name() to look for iio-sensor-proxy appearing and disappearing. (we could also have the same problem with g-s-d crashing and coming back, but seeing as that's not really handled in the rest of the panel, let's ignore that)
Created attachment 303596 [details] [review] new patch using the iio-proxy API New patch with review comment addressed. If this is right yell and I'll redo the g-s-d one the same.
Review of attachment 303596 [details] [review]: ::: panels/power/cc-power-panel.c @@ +99,3 @@ + GDBusProxy *iio_proxy; + guint iio_proxy_id; iio_proxy_watch_id; @@ +976,3 @@ + GVariant *v; + + if (priv->iio_proxy == NULL) You won't be hiding the row if the iio proxy disappeared. @@ +1543,3 @@ +{ + CcPowerPanel *self = CC_POWER_PANEL (user_data); + g_clear_object (&self->priv->iio_proxy); You need to hide the row here, don't you?
Created attachment 303600 [details] [review] new patch using the iio-proxy API Fixing all review issues. Thanks!
Review of attachment 303600 [details] [review]: Looks good! Marking as reviewed, it will still need updating for the client tracking API changes: https://github.com/hadess/iio-sensor-proxy/issues/11
Pushed to master, thanks!