Merge lp:~albaguirre/indicator-power/use-new-brightness-dbus-interface into lp:indicator-power/14.10

[Alberto Aguirre]

Changes to address setBrightness interface moving from powerd to unity-system-compositor

Changes to address setBrightness interface moving from powerd to unity-system-compositor

Depends on:
https://code.launchpad.net/~albaguirre/unity-system-compositor/screen-power-state-handling/+merge/213957
https://code.launchpad.net/~albaguirre/powerd/remove-input-inactivity-handling/+merge/219449

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:247
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/105/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-amd64-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-armhf-ci/6/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-power-ci/105/rebuild

review: Needs Fixing (continuous-integration)

[Iain Lane (laney)]

Cheers. Since you asked me to review I've taken a look, but Charles I think usually takes care of i-power. One inline comment too.

The main problem is that you need to rename the 'powerd' references everywhere since that name is now inaccurate except for getBrightnessParams.

What provides this service? Please either add a dependency or (maybe this is a good idea anyway) keep the fallback to powerd if nothing has the new name.

review: Needs Fixing

[Charles Kerr (charlesk)]

alberto, laney, IMO we should remove the brightness code from indicator-power altogether. IMO it doesn't make sense for i-power to maintain a feature that it's not using.

[Charles Kerr (charlesk)]

In the 50 minutes since my last comment, it looks like Design has decided to restore the slider to indicator-power after all -- this MR brought the discussion to a head. :)

After this MR goes through as an interim step, I'll revert the previous code that disabled the slider.

[lp:~albaguirre/indicator-power/use-new-brightness-dbus-interface]

248. By Alberto Aguirre on 2014-06-19

Remove references to powerd since the brightness interface is now provided by Unity.Screen

249. By Alberto Aguirre on 2014-06-19

Remove powerd reference in message

250. By Alberto Aguirre on 2014-06-19

Add powerd and unity-system-compositor to Depends

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:248
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/106/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-amd64-ci/7/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-armhf-ci/7/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-power-ci/106/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:250
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/107/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-armhf-ci/8/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-power-ci/107/rebuild

review: Needs Fixing (continuous-integration)

[lp:~albaguirre/indicator-power/use-new-brightness-dbus-interface]

251. By Alberto Aguirre on 2014-06-19

Add 1.11 as allowed version for lcov

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:251
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/108/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-amd64-ci/9
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-armhf-ci/9
        deb: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-armhf-ci/9/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-power-ci/108/rebuild

review: Approve (continuous-integration)

[Iain Lane (laney)]

Cheers. It looks okay to me now (haven't tested though).

Just one thing. Could you version the u-s-c dependency to (>= the-one-which-introduced-this-interface)?

review: Approve

[lp:~albaguirre/indicator-power/use-new-brightness-dbus-interface]

252. By Alberto Aguirre on 2014-06-20

Specify required unity-system-compositor version

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:252
http://jenkins.qa.ubuntu.com/job/indicator-power-ci/109/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/indicator-power-utopic-armhf-ci/10/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-power-ci/109/rebuild

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

I would have preferred to rename the functions in a backend-agnostic way, but I won't block on that.

Thanks for the patch!

review: Approve

=== modified file 'debian/control'
--- debian/control	2014-04-15 13:16:44 +0000
+++ debian/control	2014-06-20 19:57:33 +0000
@@ -24,6 +24,8 @@
 Depends: ${shlibs:Depends},
          ${misc:Depends},
          upower,
+         powerd,
+         unity-system-compositor (>= 0.0.4),
 Recommends: unity-control-center | gnome-control-center (>= 3.1) | ubuntu-system-settings | xfce4-power-manager,
             indicator-applet (>= 0.2) | indicator-renderer,
 Description: Indicator showing power state.
 
=== modified file 'm4/gcov.m4'
--- m4/gcov.m4	2013-12-03 14:00:14 +0000
+++ m4/gcov.m4	2014-06-20 19:57:33 +0000
@@ -30,7 +30,7 @@
     AC_MSG_ERROR([ccache must be disabled when --enable-gcov option is used. You can disable ccache by setting environment variable CCACHE_DISABLE=1.])
   fi
 
-  lcov_version_list="1.6 1.7 1.8 1.9 1.10"
+  lcov_version_list="1.6 1.7 1.8 1.9 1.10 1.11"
   AC_CHECK_PROG(LCOV, lcov, lcov)
   AC_CHECK_PROG(GENHTML, genhtml, genhtml)
 
 
=== modified file 'src/Makefile.am'
--- src/Makefile.am	2014-03-03 09:18:05 +0000
+++ src/Makefile.am	2014-06-20 19:57:33 +0000
@@ -46,8 +46,8 @@
 libindicatorpower_service_a_SOURCES = \
   ib-brightness-control.c \
   ib-brightness-control.h \
-  ib-brightness-powerd-control.c \
-  ib-brightness-powerd-control.h \
+  ib-brightness-uscreen-control.c \
+  ib-brightness-uscreen-control.h \
   device-provider.c \
   device-provider.h \
   device.c \
 
=== renamed file 'src/ib-brightness-powerd-control.c' => 'src/ib-brightness-uscreen-control.c'
--- src/ib-brightness-powerd-control.c	2014-04-28 23:15:18 +0000
+++ src/ib-brightness-uscreen-control.c	2014-06-20 19:57:33 +0000
@@ -17,19 +17,20 @@
  *     Yuan-Chen Cheng <[e-mail]>
  */
 
-#include "ib-brightness-powerd-control.h"
+#include "ib-brightness-uscreen-control.h"
 
 static gboolean getBrightnessParams(GDBusProxy* powerd_proxy, int *dim, int *min,
     int *max, int *dflt, gboolean *ab_supported);
 
 GDBusProxy*
-powerd_get_proxy(brightness_params_t *params)
+uscreen_get_proxy(brightness_params_t *params)
 {
     GError *error = NULL;
     gboolean ret;
 
     g_return_val_if_fail (params != NULL, NULL);
 
+    /* For now we still need to obtain the brigthness params from powerd */
     GDBusProxy* powerd_proxy = g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SYSTEM,
                 G_DBUS_PROXY_FLAGS_NONE,
                 NULL,
@@ -55,8 +56,26 @@
         g_object_unref (powerd_proxy);
         return NULL;
     }
-    
-    return powerd_proxy;
+
+    g_clear_object (&powerd_proxy);
+
+    GDBusProxy* uscreen_proxy = g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SYSTEM,
+                G_DBUS_PROXY_FLAGS_NONE,
+                NULL,
+                "com.canonical.Unity.Screen",
+                "/com/canonical/Unity/Screen",
+                "com.canonical.Unity.Screen",
+                NULL,
+                &error);
+
+    if (error != NULL)
+    {
+        g_debug ("could not connect to unity screen: %s", error->message);
+        g_error_free (error);
+        return NULL;
+    }
+
+    return uscreen_proxy;
 }
 
 
@@ -89,18 +108,18 @@
     return TRUE;
 }
 
-static gboolean setUserBrightness(GDBusProxy* powerd_proxy, GCancellable *gcancel, int brightness)
+static gboolean setUserBrightness(GDBusProxy* uscreen_proxy, GCancellable *gcancel, int brightness)
 {
     GVariant *ret = NULL;
     GError *error = NULL;
 
-    ret = g_dbus_proxy_call_sync(powerd_proxy,
+    ret = g_dbus_proxy_call_sync(uscreen_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_warning("setUserBrightness via unity.screen failed: %s", error->message);
         g_error_free(error);
         return FALSE;
     } else {
@@ -109,9 +128,9 @@
     }
 }
 
-struct _IbBrightnessPowerdControl
+struct _IbBrightnessUScreenControl
 {
-    GDBusProxy *powerd_proxy;
+    GDBusProxy *uscreen_proxy;
     GCancellable *gcancel;
 
     int dim;
@@ -123,13 +142,13 @@
     int current;
 };
 
-IbBrightnessPowerdControl*
-ib_brightness_powerd_control_new (GDBusProxy* powerd_proxy, brightness_params_t params)
+IbBrightnessUscreenControl*
+ib_brightness_uscreen_control_new (GDBusProxy* uscreen_proxy, brightness_params_t params)
 {
-    IbBrightnessPowerdControl *control;
+    IbBrightnessUscreenControl *control;
 
-    control = g_new0 (IbBrightnessPowerdControl, 1);
-    control->powerd_proxy = powerd_proxy;
+    control = g_new0 (IbBrightnessUscreenControl, 1);
+    control->uscreen_proxy = uscreen_proxy;
     control->gcancel = g_cancellable_new();
 
     control->dim = params.dim;
@@ -139,21 +158,21 @@
     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.
+    // unity.screen, 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);
+    ib_brightness_uscreen_control_set_value(control, control->dflt);
 
     return control;
 }
 
 void
-ib_brightness_powerd_control_set_value (IbBrightnessPowerdControl* self, gint value)
+ib_brightness_uscreen_control_set_value (IbBrightnessUscreenControl* self, gint value)
 {
     gboolean ret;
 
     value = CLAMP(value, self->min, self->max);
-    ret = setUserBrightness(self->powerd_proxy, self->gcancel, value);
+    ret = setUserBrightness(self->uscreen_proxy, self->gcancel, value);
     if (ret)
     {
         self->current = value;
@@ -161,23 +180,23 @@
 }
 
 gint
-ib_brightness_powerd_control_get_value (IbBrightnessPowerdControl* self)
+ib_brightness_uscreen_control_get_value (IbBrightnessUscreenControl* self)
 {
     return self->current;
 }
 
 gint
-ib_brightness_powerd_control_get_max_value (IbBrightnessPowerdControl* self)
+ib_brightness_uscreen_control_get_max_value (IbBrightnessUscreenControl* self)
 {
     return self->max;
 }
 
 void
-ib_brightness_powerd_control_free (IbBrightnessPowerdControl *self)
+ib_brightness_uscreen_control_free (IbBrightnessUscreenControl *self)
 {
     g_cancellable_cancel (self->gcancel);
     g_object_unref (self->gcancel);
-    g_object_unref (self->powerd_proxy);
+    g_object_unref (self->uscreen_proxy);
     g_free (self);
 }
 
 
=== renamed file 'src/ib-brightness-powerd-control.h' => 'src/ib-brightness-uscreen-control.h'
--- src/ib-brightness-powerd-control.h	2014-04-28 23:15:18 +0000
+++ src/ib-brightness-uscreen-control.h	2014-06-20 19:57:33 +0000
@@ -17,8 +17,8 @@
  *     Y.C Cheng <[e-mail]>
  */
 
-#ifndef __IB_BRIGHTNESS_POWERD_CONTROL_H__
-#define __IB_BRIGHTNESS_POWERD_CONTROL_H__
+#ifndef __IB_BRIGHTNESS_USCREEN_CONTROL_H__
+#define __IB_BRIGHTNESS_USCREEN_CONTROL_H__
 
 #include <gio/gio.h>
 
@@ -30,14 +30,14 @@
     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);
+GDBusProxy* uscreen_get_proxy(brightness_params_t *);
+
+typedef struct _IbBrightnessUScreenControl IbBrightnessUscreenControl;
+
+IbBrightnessUscreenControl* ib_brightness_uscreen_control_new (GDBusProxy* uscreen_proxy, brightness_params_t params);
+void                       ib_brightness_uscreen_control_set_value (IbBrightnessUscreenControl* self, gint value);
+gint                       ib_brightness_uscreen_control_get_value (IbBrightnessUscreenControl* self);
+gint                       ib_brightness_uscreen_control_get_max_value (IbBrightnessUscreenControl* self);
+void                       ib_brightness_uscreen_control_free (IbBrightnessUscreenControl *self);
 
 #endif
 
=== modified file 'src/service.c'
--- src/service.c	2014-04-15 13:08:08 +0000
+++ src/service.c	2014-06-20 19:57:33 +0000
@@ -27,7 +27,7 @@
 #include "device.h"
 #include "device-provider.h"
 #include "ib-brightness-control.h"
-#include "ib-brightness-powerd-control.h"
+#include "ib-brightness-uscreen-control.h"
 #include "service.h"
 
 #define BUS_NAME "com.canonical.indicator.power"
@@ -105,7 +105,7 @@
   GSettings * settings;
 
   IbBrightnessControl * brightness_control;
-  IbBrightnessPowerdControl * brightness_powerd_control;
+  IbBrightnessUscreenControl * brightness_uscreen_control;
 
   guint own_id;
   guint actions_export_id;
@@ -467,9 +467,9 @@
     {
       max = ib_brightness_control_get_max_value (self->priv->brightness_control);
     }
-  else if (p->brightness_powerd_control)
+  else if (p->brightness_uscreen_control)
     {
-      max = ib_brightness_powerd_control_get_max_value (self->priv->brightness_powerd_control);
+      max = ib_brightness_uscreen_control_get_max_value (self->priv->brightness_uscreen_control);
     }
   *low  = max * 0.05; /* 5% minimum -- don't let the screen go completely dark */
   *high = max;
@@ -500,9 +500,9 @@
     {
       brightness = ib_brightness_control_get_value (p->brightness_control);
     }
-  else if (p->brightness_powerd_control)
+  else if (p->brightness_uscreen_control)
     {
-      brightness = ib_brightness_powerd_control_get_value (p->brightness_powerd_control);
+      brightness = ib_brightness_uscreen_control_get_value (p->brightness_uscreen_control);
     }
   return g_variant_new_double (brightness_to_percentage (self, brightness));
 }
@@ -527,9 +527,9 @@
     {
       ib_brightness_control_set_value (self->priv->brightness_control, brightness);
     }
-  else if (self->priv->brightness_powerd_control)
+  else if (self->priv->brightness_uscreen_control)
     {
-      ib_brightness_powerd_control_set_value (self->priv->brightness_powerd_control, brightness);
+      ib_brightness_uscreen_control_set_value (self->priv->brightness_uscreen_control, brightness);
     }
 
   update_brightness_action_state (self);
@@ -1024,7 +1024,7 @@
 
   // 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);
+  g_clear_pointer (&p->brightness_uscreen_control, ib_brightness_uscreen_control_free);
 
   indicator_power_service_set_device_provider (self, NULL);
 
@@ -1038,8 +1038,8 @@
 static void
 indicator_power_service_init (IndicatorPowerService * self)
 {
-  GDBusProxy *powerd_proxy;
-  brightness_params_t powerd_brightness_params;
+  GDBusProxy *uscreen_proxy;
+  brightness_params_t brightness_params;
   priv_t * p = G_TYPE_INSTANCE_GET_PRIVATE (self,
                                             INDICATOR_TYPE_POWER_SERVICE,
                                             IndicatorPowerServicePrivate);
@@ -1049,10 +1049,10 @@
 
   p->settings = g_settings_new ("com.canonical.indicator.power");
 
-  powerd_proxy = powerd_get_proxy(&powerd_brightness_params);
-  if (powerd_proxy != NULL)
+  uscreen_proxy = uscreen_get_proxy(&brightness_params);
+  if (uscreen_proxy != NULL)
     {
-      p->brightness_powerd_control = ib_brightness_powerd_control_new(powerd_proxy, powerd_brightness_params);
+      p->brightness_uscreen_control = ib_brightness_uscreen_control_new(uscreen_proxy, brightness_params);
     }
   else
     {