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

Proposed by Yuan-Chen Cheng on 2014-03-04
Status: Merged
Approved by: Ricardo Salveti on 2014-03-26
Approved revision: 234
Merged at revision: 235
Proposed branch: lp:~ycheng-twn/indicator-power/indicator-power_set-brightness-via-powerd
Merge into: lp:indicator-power/14.04
Diff against target: 353 lines (+269/-4)
4 files modified
src/Makefile.am (+2/-0)
src/ib-brightness-powerd-control.c (+181/-0)
src/ib-brightness-powerd-control.h (+42/-0)
src/service.c (+44/-4)
To merge this branch: bzr merge lp:~ycheng-twn/indicator-power/indicator-power_set-brightness-via-powerd
Reviewer Review Type Date Requested Status
Ricardo Salveti Approve on 2014-03-26
Yuan-Chen Cheng (community) Resubmit on 2014-03-26
Charles Kerr (community) 2014-03-04 Approve on 2014-03-07
Review via email: mp+209200@code.launchpad.net

Commit message

Set brightness via powerd if it exist (using dbus)

Description of the change

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

To post a comment you must log in.
Yuan-Chen Cheng (ycheng-twn) wrote :

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) wrote :

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) wrote :

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: Resubmit
Charles Kerr (charlesk) wrote :

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) wrote :

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) wrote :

Looks good. Thank you!

review: Approve
Ricardo Salveti (rsalveti) wrote :

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
234. By Yuan-Chen Cheng on 2014-03-26

remove modificaiton in changelog for CI

Yuan-Chen Cheng (ycheng-twn) wrote :

remove modificaiton in changelog for CI

review: Resubmit
Ricardo Salveti (rsalveti) wrote :

Code looks fine, also tested and works as expected.

Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Makefile.am'
2--- src/Makefile.am 2013-10-03 20:42:50 +0000
3+++ src/Makefile.am 2014-03-26 04:59:30 +0000
4@@ -46,6 +46,8 @@
5 libindicatorpower_service_a_SOURCES = \
6 ib-brightness-control.c \
7 ib-brightness-control.h \
8+ ib-brightness-powerd-control.c \
9+ ib-brightness-powerd-control.h \
10 device-provider.c \
11 device-provider.h \
12 device.c \
13
14=== added file 'src/ib-brightness-powerd-control.c'
15--- src/ib-brightness-powerd-control.c 1970-01-01 00:00:00 +0000
16+++ src/ib-brightness-powerd-control.c 2014-03-26 04:59:30 +0000
17@@ -0,0 +1,181 @@
18+/*
19+ * Copyright 2014 Canonical Ltd.
20+ *
21+ * This program is free software: you can redistribute it and/or modify it
22+ * under the terms of the GNU General Public License version 3, as published
23+ * by the Free Software Foundation.
24+ *
25+ * This program is distributed in the hope that it will be useful, but
26+ * WITHOUT ANY WARRANTY; without even the implied warranties of
27+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
28+ * PURPOSE. See the GNU General Public License for more details.
29+ *
30+ * You should have received a copy of the GNU General Public License along
31+ * with this program. If not, see <http://www.gnu.org/licenses/>.
32+ *
33+ * Authors:
34+ * Yuan-Chen Cheng <yc.cheng@canonical.com>
35+ */
36+
37+#include "ib-brightness-powerd-control.h"
38+
39+static gboolean getBrightnessParams(GDBusProxy* powerd_proxy, int *min, int *max,
40+ int *dflt, gboolean *ab_supported);
41+
42+GDBusProxy*
43+powerd_get_proxy(brightness_params_t *params)
44+{
45+ GError *error = NULL;
46+ gboolean ret;
47+
48+ g_return_val_if_fail (params != NULL, NULL);
49+
50+ GDBusProxy* powerd_proxy = g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SYSTEM,
51+ G_DBUS_PROXY_FLAGS_NONE,
52+ NULL,
53+ "com.canonical.powerd",
54+ "/com/canonical/powerd",
55+ "com.canonical.powerd",
56+ NULL,
57+ &error);
58+
59+ if (error != NULL)
60+ {
61+ g_debug ("could not connect to powerd: %s", error->message);
62+ g_error_free (error);
63+ return NULL;
64+ }
65+
66+ ret = getBrightnessParams(powerd_proxy, &(params->min), &(params->max),
67+ &(params->dflt), &(params->ab_supported));
68+
69+ if (! ret)
70+ {
71+ g_debug ("can't get brightness parameters from powerd");
72+ g_object_unref (powerd_proxy);
73+ return NULL;
74+ }
75+
76+ return powerd_proxy;
77+}
78+
79+
80+static gboolean
81+getBrightnessParams(GDBusProxy* powerd_proxy, int *min, int *max, int *dflt, gboolean *ab_supported)
82+{
83+ GVariant *ret = NULL;
84+ GError *error = NULL;
85+
86+ ret = g_dbus_proxy_call_sync(powerd_proxy,
87+ "getBrightnessParams",
88+ NULL,
89+ G_DBUS_CALL_FLAGS_NONE,
90+ 400, NULL, &error); // timeout: 400 ms
91+ if (!ret)
92+ {
93+ if (error != NULL)
94+ {
95+ if (!g_error_matches(error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN))
96+ {
97+ g_warning("getBrightnessParams from powerd failed: %s", error->message);
98+ }
99+ g_error_free(error);
100+ }
101+ return FALSE;
102+ }
103+
104+ g_variant_get(ret, "((iiib))", min, max, dflt, ab_supported);
105+ g_variant_unref(ret);
106+ return TRUE;
107+}
108+
109+static gboolean setUserBrightness(GDBusProxy* powerd_proxy, GCancellable *gcancel, int brightness)
110+{
111+ GVariant *ret = NULL;
112+ GError *error = NULL;
113+
114+ ret = g_dbus_proxy_call_sync(powerd_proxy,
115+ "setUserBrightness",
116+ g_variant_new("(i)", brightness),
117+ G_DBUS_CALL_FLAGS_NONE,
118+ -1, gcancel, &error);
119+ if (!ret) {
120+ g_warning("setUserBrightness via powerd failed: %s", error->message);
121+ g_error_free(error);
122+ return FALSE;
123+ } else {
124+ g_variant_unref(ret);
125+ return TRUE;
126+ }
127+}
128+
129+struct _IbBrightnessPowerdControl
130+{
131+ GDBusProxy *powerd_proxy;
132+ GCancellable *gcancel;
133+
134+ int min;
135+ int max;
136+ int dflt; // defalut value
137+ gboolean ab_supported;
138+
139+ int current;
140+};
141+
142+IbBrightnessPowerdControl*
143+ib_brightness_powerd_control_new (GDBusProxy* powerd_proxy, brightness_params_t params)
144+{
145+ IbBrightnessPowerdControl *control;
146+
147+ control = g_new0 (IbBrightnessPowerdControl, 1);
148+ control->powerd_proxy = powerd_proxy;
149+ control->gcancel = g_cancellable_new();
150+
151+ control->min = params.min;
152+ control->max = params.max;
153+ control->dflt = params.dflt;
154+ control->ab_supported = params.ab_supported;
155+
156+ // XXX: set the brightness value is the only way to sync the brightness value with
157+ // powerd, and we should set the user prefered / last set brightness value upon startup.
158+ // Before we have code to store last set brightness value or other mechanism, we set
159+ // it to default brightness that powerd proposed.
160+ ib_brightness_powerd_control_set_value(control, control->dflt);
161+
162+ return control;
163+}
164+
165+void
166+ib_brightness_powerd_control_set_value (IbBrightnessPowerdControl* self, gint value)
167+{
168+ gboolean ret;
169+
170+ value = CLAMP(value, self->min, self->max);
171+ ret = setUserBrightness(self->powerd_proxy, self->gcancel, value);
172+ if (ret)
173+ {
174+ self->current = value;
175+ }
176+}
177+
178+gint
179+ib_brightness_powerd_control_get_value (IbBrightnessPowerdControl* self)
180+{
181+ return self->current;
182+}
183+
184+gint
185+ib_brightness_powerd_control_get_max_value (IbBrightnessPowerdControl* self)
186+{
187+ return self->max;
188+}
189+
190+void
191+ib_brightness_powerd_control_free (IbBrightnessPowerdControl *self)
192+{
193+ g_cancellable_cancel (self->gcancel);
194+ g_object_unref (self->gcancel);
195+ g_object_unref (self->powerd_proxy);
196+ g_free (self);
197+}
198+
199
200=== added file 'src/ib-brightness-powerd-control.h'
201--- src/ib-brightness-powerd-control.h 1970-01-01 00:00:00 +0000
202+++ src/ib-brightness-powerd-control.h 2014-03-26 04:59:30 +0000
203@@ -0,0 +1,42 @@
204+/*
205+ * Copyright 2014 Canonical Ltd.
206+ *
207+ * This program is free software: you can redistribute it and/or modify it
208+ * under the terms of the GNU General Public License version 3, as published
209+ * by the Free Software Foundation.
210+ *
211+ * This program is distributed in the hope that it will be useful, but
212+ * WITHOUT ANY WARRANTY; without even the implied warranties of
213+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
214+ * PURPOSE. See the GNU General Public License for more details.
215+ *
216+ * You should have received a copy of the GNU General Public License along
217+ * with this program. If not, see <http://www.gnu.org/licenses/>.
218+ *
219+ * Authors:
220+ * Y.C Cheng <yc.cheng@canonical.com>
221+ */
222+
223+#ifndef __IB_BRIGHTNESS_POWERD_CONTROL_H__
224+#define __IB_BRIGHTNESS_POWERD_CONTROL_H__
225+
226+#include <gio/gio.h>
227+
228+typedef struct {
229+ int max;
230+ int min;
231+ int dflt;
232+ gboolean ab_supported;
233+} brightness_params_t;
234+
235+GDBusProxy* powerd_get_proxy(brightness_params_t *);
236+
237+typedef struct _IbBrightnessPowerdControl IbBrightnessPowerdControl;
238+
239+IbBrightnessPowerdControl* ib_brightness_powerd_control_new (GDBusProxy* powerd_proxy, brightness_params_t params);
240+void ib_brightness_powerd_control_set_value (IbBrightnessPowerdControl* self, gint value);
241+gint ib_brightness_powerd_control_get_value (IbBrightnessPowerdControl* self);
242+gint ib_brightness_powerd_control_get_max_value (IbBrightnessPowerdControl* self);
243+void ib_brightness_powerd_control_free (IbBrightnessPowerdControl *self);
244+
245+#endif
246
247=== modified file 'src/service.c'
248--- src/service.c 2014-03-13 22:27:29 +0000
249+++ src/service.c 2014-03-26 04:59:30 +0000
250@@ -27,6 +27,7 @@
251 #include "device.h"
252 #include "device-provider.h"
253 #include "ib-brightness-control.h"
254+#include "ib-brightness-powerd-control.h"
255 #include "service.h"
256
257 #define BUS_NAME "com.canonical.indicator.power"
258@@ -104,6 +105,7 @@
259 GSettings * settings;
260
261 IbBrightnessControl * brightness_control;
262+ IbBrightnessPowerdControl * brightness_powerd_control;
263
264 guint own_id;
265 guint actions_export_id;
266@@ -457,7 +459,16 @@
267 static void
268 get_brightness_range (IndicatorPowerService * self, gint * low, gint * high)
269 {
270- const int max = ib_brightness_control_get_max_value (self->priv->brightness_control);
271+ priv_t * p = self->priv;
272+ int max = 0;
273+ if (p->brightness_control)
274+ {
275+ max = ib_brightness_control_get_max_value (self->priv->brightness_control);
276+ }
277+ else if (p->brightness_powerd_control)
278+ {
279+ max = ib_brightness_powerd_control_get_max_value (self->priv->brightness_powerd_control);
280+ }
281 *low = max * 0.05; /* 5% minimum -- don't let the screen go completely dark */
282 *high = max;
283 }
284@@ -500,7 +511,15 @@
285 action_state_for_brightness (IndicatorPowerService * self)
286 {
287 priv_t * p = self->priv;
288- const gint brightness = ib_brightness_control_get_value (p->brightness_control);
289+ gint brightness = 0;
290+ if (p->brightness_control)
291+ {
292+ brightness = ib_brightness_control_get_value (p->brightness_control);
293+ }
294+ else if (p->brightness_powerd_control)
295+ {
296+ brightness = ib_brightness_powerd_control_get_value (p->brightness_powerd_control);
297+ }
298 return g_variant_new_double (brightness_to_percentage (self, brightness));
299 }
300
301@@ -519,7 +538,16 @@
302 IndicatorPowerService * self = INDICATOR_POWER_SERVICE (gself);
303 const gdouble percentage = g_variant_get_double (parameter);
304 const int brightness = percentage_to_brightness (self, percentage);
305- ib_brightness_control_set_value (self->priv->brightness_control, brightness);
306+
307+ if (self->priv->brightness_control)
308+ {
309+ ib_brightness_control_set_value (self->priv->brightness_control, brightness);
310+ }
311+ else if (self->priv->brightness_powerd_control)
312+ {
313+ ib_brightness_powerd_control_set_value (self->priv->brightness_powerd_control, brightness);
314+ }
315+
316 update_brightness_action_state (self);
317 }
318
319@@ -1016,7 +1044,9 @@
320
321 g_clear_object (&p->conn);
322
323+ // g_clear_pointer has NULL check inside.
324 g_clear_pointer (&p->brightness_control, ib_brightness_control_free);
325+ g_clear_pointer (&p->brightness_powerd_control, ib_brightness_powerd_control_free);
326
327 indicator_power_service_set_device_provider (self, NULL);
328
329@@ -1030,6 +1060,8 @@
330 static void
331 indicator_power_service_init (IndicatorPowerService * self)
332 {
333+ GDBusProxy *powerd_proxy;
334+ brightness_params_t powerd_brightness_params;
335 priv_t * p = G_TYPE_INSTANCE_GET_PRIVATE (self,
336 INDICATOR_TYPE_POWER_SERVICE,
337 IndicatorPowerServicePrivate);
338@@ -1039,7 +1071,15 @@
339
340 p->settings = g_settings_new ("com.canonical.indicator.power");
341
342- p->brightness_control = ib_brightness_control_new ();
343+ powerd_proxy = powerd_get_proxy(&powerd_brightness_params);
344+ if (powerd_proxy != NULL)
345+ {
346+ p->brightness_powerd_control = ib_brightness_powerd_control_new(powerd_proxy, powerd_brightness_params);
347+ }
348+ else
349+ {
350+ p->brightness_control = ib_brightness_control_new ();
351+ }
352
353 init_gactions (self);
354

Subscribers

People subscribed via source and target branches