Merge lp:~pitti/indicator-session/libupower-glib into lp:indicator-session/0.1

Proposed by Martin Pitt on 2010-02-16
Status: Rejected
Rejected by: Ted Gould on 2010-02-18
Proposed branch: lp:~pitti/indicator-session/libupower-glib
Merge into: lp:indicator-session/0.1
Diff against target: 261 lines (+39/-138)
2 files modified
configure.ac (+3/-1)
src/session-service.c (+36/-137)
To merge this branch: bzr merge lp:~pitti/indicator-session/libupower-glib
Reviewer Review Type Date Requested Status
Martin Pitt Resubmit on 2010-02-16
Ted Gould (community) Needs Information on 2010-02-16
David Barth 2010-02-16 Needs Information on 2010-02-16
Review via email: mp+19392@code.launchpad.net
To post a comment you must log in.
Martin Pitt (pitti) wrote :

DeviceKit-power, and now upower provide a very convenient client-side library, which also respects per-user PolicyKit privileges (see bug 432598).

This branch provides the port from DeviceKit-power D-Bus to libupower-glib.

I'd like to upload this into lucid really soon, to get it into alpha-3. Can you please merge this and do a new upstream release?

Thanks!

David Barth (dbarth) wrote :

The simplification of the code + relying on a specialized library seems better. Any disadvantages or compatibility issues with using /only/ libupower?

review: Needs Information
Martin Pitt (pitti) wrote :

Well, the client-side library (libdevkit-power-gobject, and now libupower-glib) is _the_ official API for upower, which is also being used by gnome-power-manager, gnome-session, etc. So this is more stable than the upower D-Bus API (even now you can still use libdevkit-power-gobject and it will transparently use upower).

The obvious disadvantage that this patch brings is that it now introduces a dependency on a new library. I don't personally see a problem with it, but if you want to support upstream releases which build without dk-power/upower, I'm happy to add some configure autodetection and just disable the suspend/hibernate bits if libupower-glib is not available.

David Barth (dbarth) wrote :

Martin Pitt wrote:
> The obvious disadvantage that this patch brings is that it now introduces a dependency on a new library. I don't personally see a problem with it, but if you want to support upstream releases which build without dk-power/upower, I'm happy to add some configure autodetection and just disable the suspend/hibernate bits if libupower-glib is not available.
>
I think that's fine. Adding that compatibility option can be restored
later, whereas getting user testing should be as soon as possible.

I'll let Ted comment, but would support the patch as is.

Thanks

David

Ted Gould (ted) wrote :

There are a few issues with the patch.

It uses the _sync calls, but we have a requirement to make all DBus
operations async in the various indicator code. I'm unclear whether
up_client_get_can_suspend is sync or async.

I generally prefer to check for all the libraries at once in
configure.ac. I find that this frustrates users less as they get all
the errors at once instead of getting one error, the next error, as they
go through the configure script.

It should really use g_strcmp0 instead of strcmp as it protects from
NULL strings which will otherwise be crashers.

It seems that the screensaver_unthrottle() was removed from after the
sleep is issued, and I don't see where it was re-added.

  review needs-info

review: Needs Information
74. By Martin Pitt on 2010-02-16

do upower pkg-config check in the same block as the other libraries

75. By Martin Pitt on 2010-02-16

use g_strcmp0() for robustness

Martin Pitt (pitti) wrote :

> It uses the _sync calls, but we have a requirement to make all DBus
> operations async in the various indicator code. I'm unclear whether
> up_client_get_can_suspend is sync or async.

It gets the property synchronously on the first call (i. e. during initialization of indicator-session, and then just returns the cached value. I. e. in the "changed" signal handler the property values are already in the UpClient object's cache and no D-Bus communication needs to happen at all.

> I generally prefer to check for all the libraries at once in
> configure.ac. I find that this frustrates users less as they get all
.> the errors at once instead of getting one error, the next error, as they
> go through the configure script.

Fixed, r74.

> It should really use g_strcmp0 instead of strcmp as it protects from
> NULL strings which will otherwise be crashers.

Fixed, r75.

> It seems that the screensaver_unthrottle() was removed from after the
> sleep is issued, and I don't see where it was re-added.

Right after the up_client_{suspend,hibernate}_sync() call in machine_sleep(), since the callbacks disappeared.

review: Resubmit
Ted Gould (ted) wrote :

On Tue, 2010-02-16 at 15:28 +0000, Martin Pitt wrote:
> Review: Resubmit
> > It uses the _sync calls, but we have a requirement to make all DBus
> > operations async in the various indicator code. I'm unclear whether
> > up_client_get_can_suspend is sync or async.
>
> It gets the property synchronously on the first call (i. e. during
> initialization of indicator-session, and then just returns the cached
> value. I. e. in the "changed" signal handler the property values are
> already in the UpClient object's cache and no D-Bus communication
> needs to happen at all.

So I went and grabbed the upower code and basically none of this is
Async. At all. I'd love to use the libupower library so that we're in
a better place with the interface, but man:

ted@shi:~/Packaging/upower/upower-0.9.0+git20100216.b9bb78$ rgrep _async
*
src/up-daemon.c: ret = g_spawn_command_line_async (command, &error);
ted@shi:~/Packaging/upower/upower-0.9.0+git20100216.b9bb78$

I'm not sure what to do here. David, do you have opinion? I'd hate to
rewrite the PolicyKit code, but we've made it a core architectural point
to only use async DBus. I doubt we could patch libupower at this point
as it'd be a pretty big patch.

Martin Pitt (pitti) wrote :

> On Tue, 2010-02-16 at 15:28 +0000, Martin Pitt wrote:

> So I went and grabbed the upower code and basically none of this is
> Async. At all.

All the properties are cached on the client side, though, so reading properties is usually not using D-Bus at all (except for the first time, as I said).

The suspend/hibernate calls are also sync. libupower-glib deliberately does it that way, since it makes the program structure so much easier, since the machine will just die/stop underneath you anyway, and it saves you from dealing with all the async handlers.

> I'm not sure what to do here. David, do you have opinion? I'd hate to
> rewrite the PolicyKit code, but we've made it a core architectural point
> to only use async DBus.

OK, deferring to David for the general decision.

You don't actually need to ask PolicyKit yourself, though. I added upower D-Bus methods {Suspend,Hibernate}Allowed() methods which return a boolean, which do the polkit checks.

> I doubt we could patch libupower at this point as it'd be a pretty big patch.

Might be worth discussing with Richard upstream, but I don't think he'd be happy about this. With my upstream hat on, I certainly wouldn't accept it, since it's not what libupower-glib is designed to be. The current API is meant to be an easy and robust integration into a glib/gobject context, and avoiding all unnecessary D-Bus calls. Adding a series of async D-Bus calls to it would make it (1) unnecessarily more complex, and (2) not provide any significant benefit over D-Bus directly.

So in summary, if i-s wants to use async calls only, we should keep the direct D-Bus interface, and need to add a call to {Suspend,Hibernate}Allowed().

Thanks!

David Barth (dbarth) wrote :

Ted Gould wrote:
> On Tue, 2010-02-16 at 15:28 +0000, Martin Pitt wrote:
>
>> Review: Resubmit
>>
>>> It uses the _sync calls, but we have a requirement to make all DBus
>>> operations async in the various indicator code. I'm unclear whether
>>> up_client_get_can_suspend is sync or async.
>>>
>> It gets the property synchronously on the first call (i. e. during
>> initialization of indicator-session, and then just returns the cached
>> value. I. e. in the "changed" signal handler the property values are
>> already in the UpClient object's cache and no D-Bus communication
>> needs to happen at all.
>>
>
> So I went and grabbed the upower code and basically none of this is
> Async. At all. I'd love to use the libupower library so that we're in
> a better place with the interface, but man:
>
> ted@shi:~/Packaging/upower/upower-0.9.0+git20100216.b9bb78$ rgrep _async
> *
> src/up-daemon.c: ret = g_spawn_command_line_async (command, &error);
> ted@shi:~/Packaging/upower/upower-0.9.0+git20100216.b9bb78$
>
Ouch.
> I'm not sure what to do here. David, do you have opinion? I'd hate to
> rewrite the PolicyKit code, but we've made it a core architectural point
> to only use async DBus. I doubt we could patch libupower at this point
> as it'd be a pretty big patch.
>
Well, I guess then it means either keeping your async code, and...
ugh... adding the PK per-user policy bits (no idea how complex that can
be), or...

...adding a thread where calls to upower can block. The lib is not
advertised as being thread safe, but I haven't spotted big static
variables in the library source, so that may reasonably work.

David

Martin Pitt (pitti) wrote :

Guys,

I'll work on an alternative branch which ports to the upower D-Bus interface directly. Then you can pick what you like better.

Martin Pitt (pitti) wrote :

https://code.launchpad.net/~pitti/indicator-session/upower/+merge/19472 is a straightforward port to upower, but it does not yet fix bug 432598 yet. If you are serious about the full async thing, this would require yet another async call from the callback, and reading the response in another callback.

Ted Gould (ted) wrote :

Accepted merge 19472 so this one is not valid.

Unmerged revisions

75. By Martin Pitt on 2010-02-16

use g_strcmp0() for robustness

74. By Martin Pitt on 2010-02-16

do upower pkg-config check in the same block as the other libraries

73. By Martin Pitt on 2010-02-16

port to libupower-glib

Stop talking to DeviceKit-power directly. Use the glib client library instead,
which is much easier to use, and also respects the per-user PolicyKit settings
(LP #432598).

Also go straight to upower.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'configure.ac'
2--- configure.ac 2010-02-11 16:47:17 +0000
3+++ configure.ac 2010-02-16 15:30:33 +0000
4@@ -29,6 +29,7 @@
5 INDICATOR_REQUIRED_VERSION=0.3.0
6 DBUSMENUGTK_REQUIRED_VERSION=0.2.2
7 POLKIT_REQUIRED_VERSION=0.92
8+UPOWER_REQUIRED_VERSION=0.9
9
10 PKG_CHECK_MODULES(APPLET, gtk+-2.0 >= $GTK_REQUIRED_VERSION
11 indicator >= $INDICATOR_REQUIRED_VERSION
12@@ -39,7 +40,8 @@
13 DBUSMENUGLIB_REQUIRED_VERSION=0.1.1
14
15 PKG_CHECK_MODULES(SESSIONSERVICE, dbusmenu-glib >= $DBUSMENUGLIB_REQUIRED_VERSION
16- indicator >= $INDICATOR_REQUIRED_VERSION)
17+ indicator >= $INDICATOR_REQUIRED_VERSION
18+ upower-glib >= $UPOWER_REQUIRED_VERSION)
19
20 AC_SUBST(SESSIONERVICE_CFLAGS)
21 AC_SUBST(SESSIONERVICE_LIBS)
22
23=== modified file 'src/session-service.c'
24--- src/session-service.c 2010-02-08 22:46:35 +0000
25+++ src/session-service.c 2010-02-16 15:30:33 +0000
26@@ -31,6 +31,8 @@
27 #include <dbus/dbus-glib.h>
28 #include <dbus/dbus-glib-bindings.h>
29
30+#include <upower.h>
31+
32 #include <libdbusmenu-glib/server.h>
33 #include <libdbusmenu-glib/menuitem.h>
34 #include <libdbusmenu-glib/client.h>
35@@ -44,10 +46,6 @@
36 #include "users-service-dbus.h"
37 #include "lock-helper.h"
38
39-#define DKP_ADDRESS "org.freedesktop.DeviceKit.Power"
40-#define DKP_OBJECT "/org/freedesktop/DeviceKit/Power"
41-#define DKP_INTERFACE "org.freedesktop.DeviceKit.Power"
42-
43 #define GUEST_SESSION_LAUNCHER "/usr/share/gdm/guest-session/guest-session-launch"
44
45 #define LOCKDOWN_DIR "/desktop/gnome/lockdown"
46@@ -72,11 +70,7 @@
47
48 static DbusmenuMenuitem * root_menuitem = NULL;
49 static GMainLoop * mainloop = NULL;
50-static DBusGProxy * dkp_main_proxy = NULL;
51-static DBusGProxy * dkp_prop_proxy = NULL;
52-
53-static DBusGProxyCall * suspend_call = NULL;
54-static DBusGProxyCall * hibernate_call = NULL;
55+static UpClient * up_client = NULL;
56
57 static DbusmenuMenuitem * hibernate_mi = NULL;
58 static DbusmenuMenuitem * suspend_mi = NULL;
59@@ -131,15 +125,6 @@
60 }
61 }
62
63-/* A return from the command to sleep the system. Make sure
64- that we unthrottle the screensaver. */
65-static void
66-sleep_response (DBusGProxy * proxy, DBusGProxyCall * call, gpointer data)
67-{
68- screensaver_unthrottle();
69- return;
70-}
71-
72 /* Let's put this machine to sleep, with some info on how
73 it should sleep. */
74 static void
75@@ -147,105 +132,41 @@
76 {
77 gchar * type = (gchar *)userdata;
78
79- if (dkp_main_proxy == NULL) {
80- g_warning("Can not %s as no DeviceKit Power Proxy", type);
81+ if (up_client == NULL) {
82+ g_warning("Can not %s as we have no upower client", type);
83 }
84
85 screensaver_throttle(type);
86 lock_screen(NULL, 0, NULL);
87
88- dbus_g_proxy_begin_call(dkp_main_proxy,
89- type,
90- sleep_response,
91- NULL,
92- NULL,
93- G_TYPE_INVALID);
94-
95- return;
96-}
97-
98-/* A response to getting the suspend property */
99-static void
100-suspend_prop_cb (DBusGProxy * proxy, DBusGProxyCall * call, gpointer userdata)
101-{
102- suspend_call = NULL;
103-
104- GValue candoit = {0};
105- GError * error = NULL;
106- dbus_g_proxy_end_call(proxy, call, &error, G_TYPE_VALUE, &candoit, G_TYPE_INVALID);
107- if (error != NULL) {
108- g_warning("Unable to check suspend: %s", error->message);
109- g_error_free(error);
110- return;
111- }
112- g_debug("Got Suspend: %s", g_value_get_boolean(&candoit) ? "true" : "false");
113-
114- if (suspend_mi != NULL) {
115- dbusmenu_menuitem_property_set_value(suspend_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
116- }
117-
118- return;
119-}
120-
121-/* Response to getting the hibernate property */
122-static void
123-hibernate_prop_cb (DBusGProxy * proxy, DBusGProxyCall * call, gpointer userdata)
124-{
125- hibernate_call = NULL;
126-
127- GValue candoit = {0};
128- GError * error = NULL;
129- dbus_g_proxy_end_call(proxy, call, &error, G_TYPE_VALUE, &candoit, G_TYPE_INVALID);
130- if (error != NULL) {
131- g_warning("Unable to check hibernate: %s", error->message);
132- g_error_free(error);
133- return;
134- }
135- g_debug("Got Hibernate: %s", g_value_get_boolean(&candoit) ? "true" : "false");
136-
137- if (suspend_mi != NULL) {
138- dbusmenu_menuitem_property_set_value(hibernate_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
139- }
140-
141- return;
142-}
143+ if (g_strcmp0(type, "Suspend") == 0) {
144+ up_client_suspend_sync(up_client, NULL, NULL);
145+ } else {
146+ up_client_hibernate_sync(up_client, NULL, NULL);
147+ }
148+ screensaver_unthrottle();
149+
150+ return;
151+}
152+
153
154 /* A signal that we need to recheck to ensure we can still
155 hibernate and/or suspend */
156 static void
157-dpk_changed_cb (DBusGProxy * proxy, gpointer user_data)
158+up_changed_cb (UpClient * client, gpointer user_data)
159 {
160- /* Start Async call to see if we can hibernate */
161- if (suspend_call == NULL) {
162- suspend_call = dbus_g_proxy_begin_call(dkp_prop_proxy,
163- "Get",
164- suspend_prop_cb,
165- NULL,
166- NULL,
167- G_TYPE_STRING,
168- DKP_INTERFACE,
169- G_TYPE_STRING,
170- "can-suspend",
171- G_TYPE_INVALID,
172- G_TYPE_VALUE,
173- G_TYPE_INVALID);
174- }
175-
176- /* Start Async call to see if we can suspend */
177- if (hibernate_call == NULL) {
178- hibernate_call = dbus_g_proxy_begin_call(dkp_prop_proxy,
179- "Get",
180- hibernate_prop_cb,
181- NULL,
182- NULL,
183- G_TYPE_STRING,
184- DKP_INTERFACE,
185- G_TYPE_STRING,
186- "can-hibernate",
187- G_TYPE_INVALID,
188- G_TYPE_VALUE,
189- G_TYPE_INVALID);
190- }
191+ GValue candoit = {0};
192+ g_value_init (&candoit, G_TYPE_BOOLEAN);
193+
194+ /* can we suspend? */
195+ g_value_set_boolean(&candoit, up_client_get_can_suspend(up_client));
196+ dbusmenu_menuitem_property_set_value(suspend_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
197+ g_debug("upower changed: can suspend: %s", g_value_get_boolean(&candoit) ? "true" : "false");
198+
199+ /* can we hibernate? */
200+ g_value_set_boolean(&candoit, up_client_get_can_hibernate(up_client));
201+ dbusmenu_menuitem_property_set_value(hibernate_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
202+ g_debug("upower changed: can hibernate: %s", g_value_get_boolean(&candoit) ? "true" : "false");
203
204 return;
205 }
206@@ -254,39 +175,17 @@
207 DKp checking. We're even setting up the calls for the props
208 we need */
209 static void
210-setup_dkp (void) {
211- DBusGConnection * bus = dbus_g_bus_get(DBUS_BUS_SYSTEM, NULL);
212- g_return_if_fail(bus != NULL);
213-
214- if (dkp_main_proxy == NULL) {
215- dkp_main_proxy = dbus_g_proxy_new_for_name(bus,
216- DKP_ADDRESS,
217- DKP_OBJECT,
218- DKP_INTERFACE);
219- }
220- g_return_if_fail(dkp_main_proxy != NULL);
221-
222- if (dkp_prop_proxy == NULL) {
223- dkp_prop_proxy = dbus_g_proxy_new_for_name(bus,
224- DKP_ADDRESS,
225- DKP_OBJECT,
226- DBUS_INTERFACE_PROPERTIES);
227- }
228- g_return_if_fail(dkp_prop_proxy != NULL);
229+setup_up (void) {
230+ up_client = up_client_new();
231+ g_return_if_fail(up_client != NULL);
232
233 /* Connect to changed signal */
234- dbus_g_proxy_add_signal(dkp_main_proxy,
235- "Changed",
236- G_TYPE_INVALID);
237-
238- dbus_g_proxy_connect_signal(dkp_main_proxy,
239- "Changed",
240- G_CALLBACK(dpk_changed_cb),
241- NULL,
242- NULL);
243+ g_signal_connect (up_client, "changed",
244+ G_CALLBACK(up_changed_cb),
245+ NULL);
246
247 /* Force an original "changed" event */
248- dpk_changed_cb(dkp_main_proxy, NULL);
249+ up_changed_cb(up_client, NULL);
250
251 return;
252 }
253@@ -639,7 +538,7 @@
254 G_CALLBACK (user_removed),
255 root_menuitem);
256
257- setup_dkp();
258+ setup_up();
259
260 DbusmenuServer * server = dbusmenu_server_new(INDICATOR_SESSION_DBUS_OBJECT);
261 dbusmenu_server_set_root(server, root_menuitem);

Subscribers

People subscribed via source and target branches