Merge lp:~ycheng-twn/indicator-power/indicator-power_set-brightness-via-powerd into lp:indicator-power/14.04

[Yuan-Chen Cheng]

Set brightness via powerd if it exist (using dbus)

use powerd dbus interface to control panel brightness if available.
fall back to existing way if powerd does not running.

[Yuan-Chen Cheng (ycheng-twn)]

1. powerd also need to be updated (to accept dbus command from non-root user)
2. as tested on n4, it seems not introduce noticeable delay (it seems to have bit latency before change)

[Charles Kerr (charlesk)]

Thanks for the patch, Yuan-Chen! I like the idea of using powerd-via-dbus as the primary method and I appreciate the patch.

Some issues:

 * If we're able to get the powerd_proxy but then getBrightnessParams() fails, we don't fall back to the old control mechanism.

 * In powerd_get_proxy(), error->message is accessed after error's been freed

 * In _brightness_init(), why is ib_brightness_powerd_control_set_value(max brightness) being called? Do we want to unconditionally force max brightness whenever the power indicator loads?

 * In _brightness_init(), the call to ib_brightness_powerd_control_set_value(max brightness) is a no-op because it's being called before 'inited' is set to TRUE

 * In _control_set_value, wouldn't it be better to CLAMP(value,self->min,self->max) rather than returning silently if the value is out-of-range?

 * In -powerd-control.c, please put "return;" statements on their own line for readability.

 * In -powerd-control.c, need to use a cancellable when making dbus calls.

 * In service.c's get_brightness_range(), we need to test brightness_powerd_control for NULL before using it

 * In service.c's dispose(), no need for the NULL check before calling g_clear_pointer(). g_clear_pointer() contains its own NULL test.

 * In service.c's init(), no need to NULL out brightness_control and brightness_powerd_control; priv is already zeroed out

 * Not a blocker, but I'd be happier if this didn't introduce extra conditional logic into service.c. It would be nicer if there was a simpler wrapper that owned the two controls privately and gave service.c a unified API to call.

 * Not a blocker, but I'd be happier if avoided _sync calls. If you choose make them async, this will require a little extra plumbing that could be put into the wrapper class suggested above.

review: Needs Fixing

[Yuan-Chen Cheng (ycheng-twn)]

Charles, Thanks a lot for review, I learned a lot.

I modify the code and change as requested except two non-blocker and the cancellable one.

As using cancellable with sync dbus call, I have another branch for that, not sure whether that's the right way to handle it.

lp:~ycheng-twn/indicator-power/indicator-power_set-brightness-via-powerd_add-gcancellable

BTW, what's the recommended way to use gcancellable with async dbus call in our case ? One new cancellable object per request, and cancel previous request if not finished before issue another new one ?

One more thing, there seems no procedure for indicator-power to shutdown gracefully except it lost the bus name. It seems to be an non-issue.

review: Needs Resubmitting

[Charles Kerr (charlesk)]

Oh this is a nice revision, thank you for all the fixes!

I like the way that brightness_params_t eliminates the whole 'initialized' flag in ib-brightness-powerd-control; it makes the getters dead simple :)

There's one more thing I'd like to see fixed. Right now running this on the desktop generates an error that we don't really need to log:

  > (process:17523): Indicator-Power-WARNING **: getBrightnessParams failed:
  > GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name com.canonical.powerd
  > was not provided by any .service files

which we could silence by changing the getBrightnessParams dbus call's error checking to:

  > if (!ret) {
  > if (error != NULL) {
  > if (!g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN))
  > g_warning("getBrightnessParams failed: %s", error->message);
  > g_error_free(error);
  > }
  > return FALSE;
  > }

There are a handful of things that you can fix if you like, but are minor and I won't block on them:

 * In _service_init(), typo "brigtness"

 * In powerd-control.c, we don't need to #include gudev.h, errno.h, stdlib.h, fcntl.h, or string.h.

 * In _powerd_control_new(), I still think the call to _control_set_value() needs a comment explaining why it's needed

 * In _powerd_get_proxy(), the (params == NULL) check should be "g_return_val_if_fail (params != NULL, NULL);"

 * In _powerd_control_set_value(), the range check should be "CLAMP (value, self->min, self->max)"

 * In _service_init(), dead store "= NULL;" when powerd_proxy is defined

I hope this doesn't look like I'm piling on -- again, I like this proposal and appreciate the fixes that you made :)

[Charles Kerr (charlesk)]

Regarding GCancellable, I agree it's more an issue of code correctness at this point, especially since the _sync calls are staying in there -- there's no way for the _control_free() function to get called while the thread is blocked...

[Charles Kerr (charlesk)]

Looks good. Thank you!

review: Approve

[Ricardo Salveti (rsalveti)]

2 --- debian/changelog 2014-02-07 16:32:36 +0000
3 +++ debian/changelog 2014-03-06 03:27:42 +0000

Just please remove the changes in debian/changelog as that's now handled by the CI train.

review: Needs Fixing

[lp:~ycheng-twn/indicator-power/indicator-power_set-brightness-via-powerd]

234. By Yuan-Chen Cheng on 2014-03-26

remove modificaiton in changelog for CI

[Yuan-Chen Cheng (ycheng-twn)]

remove modificaiton in changelog for CI

review: Needs Resubmitting

[Ricardo Salveti (rsalveti)]

Code looks fine, also tested and works as expected.

Thanks!

review: Approve

=== modified file 'src/Makefile.am'
--- src/Makefile.am	2013-10-03 20:42:50 +0000
+++ src/Makefile.am	2014-03-26 04:59:30 +0000
@@ -46,6 +46,8 @@
 libindicatorpower_service_a_SOURCES = \
   ib-brightness-control.c \
   ib-brightness-control.h \
+  ib-brightness-powerd-control.c \
+  ib-brightness-powerd-control.h \
   device-provider.c \
   device-provider.h \
   device.c \

=== added file 'src/ib-brightness-powerd-control.c'
--- src/ib-brightness-powerd-control.c	1970-01-01 00:00:00 +0000
+++ src/ib-brightness-powerd-control.c	2014-03-26 04:59:30 +0000
@@ -0,0 +1,181 @@
+/*
+ * Copyright 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Yuan-Chen Cheng <[e-mail]>
+ */
+
+#include "ib-brightness-powerd-control.h"
+
+static gboolean getBrightnessParams(GDBusProxy* powerd_proxy, int *min, int *max,
+    int *dflt, gboolean *ab_supported);
+
+GDBusProxy*
+powerd_get_proxy(brightness_params_t *params)
+{
+    GError *error = NULL;
+    gboolean ret;
+
+    g_return_val_if_fail (params != NULL, NULL);
+
+    GDBusProxy* powerd_proxy = g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SYSTEM,
+                G_DBUS_PROXY_FLAGS_NONE,
+                NULL,
+                "com.canonical.powerd",
+                "/com/canonical/powerd",
+                "com.canonical.powerd",
+                NULL,
+                &error);
+
+    if (error != NULL)
+    {
+        g_debug ("could not connect to powerd: %s", error->message);
+        g_error_free (error);
+        return NULL;
+    }
+
+    ret = getBrightnessParams(powerd_proxy, &(params->min), &(params->max),
+        &(params->dflt), &(params->ab_supported));
+
+    if (! ret)
+    {
+        g_debug ("can't get brightness parameters from powerd");
+        g_object_unref (powerd_proxy);
+        return NULL;
+    }
+    
+    return powerd_proxy;
+}
+
+
+static gboolean
+getBrightnessParams(GDBusProxy* powerd_proxy, int *min, int *max, int *dflt, gboolean *ab_supported)
+{
+    GVariant *ret = NULL;
+    GError *error = NULL;
+
+    ret = g_dbus_proxy_call_sync(powerd_proxy,
+            "getBrightnessParams",
+            NULL,
+            G_DBUS_CALL_FLAGS_NONE,
+            400, NULL, &error); // timeout: 400 ms
+    if (!ret)
+    {
+        if (error != NULL)
+        {
+            if (!g_error_matches(error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN))
+            {
+                g_warning("getBrightnessParams from powerd failed: %s", error->message);
+            }
+            g_error_free(error);
+        }
+        return FALSE;
+    }
+
+    g_variant_get(ret, "((iiib))", min, max, dflt, ab_supported);
+    g_variant_unref(ret);
+    return TRUE;
+}
+
+static gboolean setUserBrightness(GDBusProxy* powerd_proxy, GCancellable *gcancel, int brightness)
+{
+    GVariant *ret = NULL;
+    GError *error = NULL;
+
+    ret = g_dbus_proxy_call_sync(powerd_proxy,
+            "setUserBrightness",
+            g_variant_new("(i)", brightness),
+            G_DBUS_CALL_FLAGS_NONE,
+            -1, gcancel, &error);
+    if (!ret) {
+        g_warning("setUserBrightness via powerd failed: %s", error->message);
+        g_error_free(error);
+        return FALSE;
+    } else {
+        g_variant_unref(ret);
+        return TRUE;
+    }
+}
+
+struct _IbBrightnessPowerdControl
+{
+    GDBusProxy *powerd_proxy;
+    GCancellable *gcancel;
+
+    int min;
+    int max;
+    int dflt; // defalut value
+    gboolean ab_supported;
+
+    int current;
+};
+
+IbBrightnessPowerdControl*
+ib_brightness_powerd_control_new (GDBusProxy* powerd_proxy, brightness_params_t params)
+{
+    IbBrightnessPowerdControl *control;
+
+    control = g_new0 (IbBrightnessPowerdControl, 1);
+    control->powerd_proxy = powerd_proxy;
+    control->gcancel = g_cancellable_new();
+
+    control->min = params.min;
+    control->max = params.max;
+    control->dflt = params.dflt;
+    control->ab_supported = params.ab_supported;
+
+    // XXX: set the brightness value is the only way to sync the brightness value with
+    // powerd, and we should set the user prefered / last set brightness value upon startup.
+    // Before we have code to store last set brightness value or other mechanism, we set
+    // it to default brightness that powerd proposed.
+    ib_brightness_powerd_control_set_value(control, control->dflt);
+
+    return control;
+}
+
+void
+ib_brightness_powerd_control_set_value (IbBrightnessPowerdControl* self, gint value)
+{
+    gboolean ret;
+
+    value = CLAMP(value, self->min, self->max);
+    ret = setUserBrightness(self->powerd_proxy, self->gcancel, value);
+    if (ret)
+    {
+        self->current = value;
+    }
+}
+
+gint
+ib_brightness_powerd_control_get_value (IbBrightnessPowerdControl* self)
+{
+    return self->current;
+}
+
+gint
+ib_brightness_powerd_control_get_max_value (IbBrightnessPowerdControl* self)
+{
+    return self->max;
+}
+
+void
+ib_brightness_powerd_control_free (IbBrightnessPowerdControl *self)
+{
+    g_cancellable_cancel (self->gcancel);
+    g_object_unref (self->gcancel);
+    g_object_unref (self->powerd_proxy);
+    g_free (self);
+}
+

=== added file 'src/ib-brightness-powerd-control.h'
--- src/ib-brightness-powerd-control.h	1970-01-01 00:00:00 +0000
+++ src/ib-brightness-powerd-control.h	2014-03-26 04:59:30 +0000
@@ -0,0 +1,42 @@
+/*
+ * Copyright 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Y.C Cheng <[e-mail]>
+ */
+
+#ifndef __IB_BRIGHTNESS_POWERD_CONTROL_H__
+#define __IB_BRIGHTNESS_POWERD_CONTROL_H__
+
+#include <gio/gio.h>
+
+typedef struct {
+    int max;
+    int min;
+    int dflt;
+    gboolean ab_supported;
+} brightness_params_t;
+
+GDBusProxy* powerd_get_proxy(brightness_params_t *);
+
+typedef struct _IbBrightnessPowerdControl IbBrightnessPowerdControl;
+
+IbBrightnessPowerdControl* ib_brightness_powerd_control_new (GDBusProxy* powerd_proxy, brightness_params_t params);
+void                       ib_brightness_powerd_control_set_value (IbBrightnessPowerdControl* self, gint value);
+gint                       ib_brightness_powerd_control_get_value (IbBrightnessPowerdControl* self);
+gint                       ib_brightness_powerd_control_get_max_value (IbBrightnessPowerdControl* self);
+void                       ib_brightness_powerd_control_free (IbBrightnessPowerdControl *self);
+
+#endif

=== modified file 'src/service.c'
--- src/service.c	2014-03-13 22:27:29 +0000
+++ src/service.c	2014-03-26 04:59:30 +0000
@@ -27,6 +27,7 @@
 #include "device.h"
 #include "device-provider.h"
 #include "ib-brightness-control.h"
+#include "ib-brightness-powerd-control.h"
 #include "service.h"
 
 #define BUS_NAME "com.canonical.indicator.power"
@@ -104,6 +105,7 @@
   GSettings * settings;
 
   IbBrightnessControl * brightness_control;
+  IbBrightnessPowerdControl * brightness_powerd_control;
 
   guint own_id;
   guint actions_export_id;
@@ -457,7 +459,16 @@
 static void
 get_brightness_range (IndicatorPowerService * self, gint * low, gint * high)
 {
-  const int max = ib_brightness_control_get_max_value (self->priv->brightness_control);
+  priv_t * p = self->priv;
+  int max = 0;
+  if (p->brightness_control)
+    {
+      max = ib_brightness_control_get_max_value (self->priv->brightness_control);
+    }
+  else if (p->brightness_powerd_control)
+    {
+      max = ib_brightness_powerd_control_get_max_value (self->priv->brightness_powerd_control);
+    }
   *low  = max * 0.05; /* 5% minimum -- don't let the screen go completely dark */
   *high = max;
 }
@@ -500,7 +511,15 @@
 action_state_for_brightness (IndicatorPowerService * self)
 {
   priv_t * p = self->priv;
-  const gint brightness = ib_brightness_control_get_value (p->brightness_control);
+  gint brightness = 0;
+  if (p->brightness_control)
+    {
+      brightness = ib_brightness_control_get_value (p->brightness_control);
+    }
+  else if (p->brightness_powerd_control)
+    {
+      brightness = ib_brightness_powerd_control_get_value (p->brightness_powerd_control);
+    }
   return g_variant_new_double (brightness_to_percentage (self, brightness));
 }
 
@@ -519,7 +538,16 @@
   IndicatorPowerService * self = INDICATOR_POWER_SERVICE (gself);
   const gdouble percentage = g_variant_get_double (parameter);
   const int brightness = percentage_to_brightness (self, percentage);
-  ib_brightness_control_set_value (self->priv->brightness_control, brightness);
+
+  if (self->priv->brightness_control)
+    {
+      ib_brightness_control_set_value (self->priv->brightness_control, brightness);
+    }
+  else if (self->priv->brightness_powerd_control)
+    {
+      ib_brightness_powerd_control_set_value (self->priv->brightness_powerd_control, brightness);
+    }
+
   update_brightness_action_state (self);
 }
 
@@ -1016,7 +1044,9 @@
 
   g_clear_object (&p->conn);
 
+  // g_clear_pointer has NULL check inside.
   g_clear_pointer (&p->brightness_control, ib_brightness_control_free);
+  g_clear_pointer (&p->brightness_powerd_control, ib_brightness_powerd_control_free);
 
   indicator_power_service_set_device_provider (self, NULL);
 
@@ -1030,6 +1060,8 @@
 static void
 indicator_power_service_init (IndicatorPowerService * self)
 {
+  GDBusProxy *powerd_proxy;
+  brightness_params_t powerd_brightness_params;
   priv_t * p = G_TYPE_INSTANCE_GET_PRIVATE (self,
                                             INDICATOR_TYPE_POWER_SERVICE,
                                             IndicatorPowerServicePrivate);
@@ -1039,7 +1071,15 @@
 
   p->settings = g_settings_new ("com.canonical.indicator.power");
 
-  p->brightness_control = ib_brightness_control_new ();
+  powerd_proxy = powerd_get_proxy(&powerd_brightness_params);
+  if (powerd_proxy != NULL)
+    {
+      p->brightness_powerd_control = ib_brightness_powerd_control_new(powerd_proxy, powerd_brightness_params);
+    }
+  else
+    {
+      p->brightness_control = ib_brightness_control_new ();
+    }
 
   init_gactions (self);