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

[Martin Pitt]

[Martin Pitt (pitti)]

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)]

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)]

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)]

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)]

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

[lp:~pitti/indicator-session/libupower-glib]

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)]

> 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: Needs Resubmitting

[Ted Gould (ted)]

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)]

> 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)]

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)]

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)]

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)]

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.

=== modified file 'configure.ac'
--- configure.ac	2010-02-11 16:47:17 +0000
+++ configure.ac	2010-02-16 15:30:33 +0000
@@ -29,6 +29,7 @@
 INDICATOR_REQUIRED_VERSION=0.3.0
 DBUSMENUGTK_REQUIRED_VERSION=0.2.2
 POLKIT_REQUIRED_VERSION=0.92
+UPOWER_REQUIRED_VERSION=0.9
 
 PKG_CHECK_MODULES(APPLET, gtk+-2.0 >= $GTK_REQUIRED_VERSION
                           indicator >= $INDICATOR_REQUIRED_VERSION
@@ -39,7 +40,8 @@
 DBUSMENUGLIB_REQUIRED_VERSION=0.1.1
 
 PKG_CHECK_MODULES(SESSIONSERVICE, dbusmenu-glib >= $DBUSMENUGLIB_REQUIRED_VERSION
-                                  indicator >= $INDICATOR_REQUIRED_VERSION)
+                                  indicator >= $INDICATOR_REQUIRED_VERSION
+                                  upower-glib >= $UPOWER_REQUIRED_VERSION)
 
 AC_SUBST(SESSIONERVICE_CFLAGS)
 AC_SUBST(SESSIONERVICE_LIBS)

=== modified file 'src/session-service.c'
--- src/session-service.c	2010-02-08 22:46:35 +0000
+++ src/session-service.c	2010-02-16 15:30:33 +0000
@@ -31,6 +31,8 @@
 #include <dbus/dbus-glib.h>
 #include <dbus/dbus-glib-bindings.h>
 
+#include <upower.h>
+
 #include <libdbusmenu-glib/server.h>
 #include <libdbusmenu-glib/menuitem.h>
 #include <libdbusmenu-glib/client.h>
@@ -44,10 +46,6 @@
 #include "users-service-dbus.h"
 #include "lock-helper.h"
 
-#define DKP_ADDRESS    "org.freedesktop.DeviceKit.Power"
-#define DKP_OBJECT     "/org/freedesktop/DeviceKit/Power"
-#define DKP_INTERFACE  "org.freedesktop.DeviceKit.Power"
-
 #define GUEST_SESSION_LAUNCHER  "/usr/share/gdm/guest-session/guest-session-launch"
 
 #define LOCKDOWN_DIR  "/desktop/gnome/lockdown"
@@ -72,11 +70,7 @@
 
 static DbusmenuMenuitem * root_menuitem = NULL;
 static GMainLoop * mainloop = NULL;
-static DBusGProxy * dkp_main_proxy = NULL;
-static DBusGProxy * dkp_prop_proxy = NULL;
-
-static DBusGProxyCall * suspend_call = NULL;
-static DBusGProxyCall * hibernate_call = NULL;
+static UpClient * up_client = NULL;
 
 static DbusmenuMenuitem * hibernate_mi = NULL;
 static DbusmenuMenuitem * suspend_mi = NULL;
@@ -131,15 +125,6 @@
     }
 }
 
-/* A return from the command to sleep the system.  Make sure
-   that we unthrottle the screensaver. */
-static void
-sleep_response (DBusGProxy * proxy, DBusGProxyCall * call, gpointer data)
-{
-	screensaver_unthrottle();
-	return;
-}
-
 /* Let's put this machine to sleep, with some info on how
    it should sleep.  */
 static void
@@ -147,105 +132,41 @@
 {
 	gchar * type = (gchar *)userdata;
 
-	if (dkp_main_proxy == NULL) {
-		g_warning("Can not %s as no DeviceKit Power Proxy", type);
+	if (up_client == NULL) {
+		g_warning("Can not %s as we have no upower client", type);
 	}
 
 	screensaver_throttle(type);
 	lock_screen(NULL, 0, NULL);
 
-	dbus_g_proxy_begin_call(dkp_main_proxy,
-	                        type,
-	                        sleep_response,
-	                        NULL,
-	                        NULL,
-	                        G_TYPE_INVALID);
-
-	return;
-}
-
-/* A response to getting the suspend property */
-static void
-suspend_prop_cb (DBusGProxy * proxy, DBusGProxyCall * call, gpointer userdata)
-{
-	suspend_call = NULL;
-
-	GValue candoit = {0};
-	GError * error = NULL;
-	dbus_g_proxy_end_call(proxy, call, &error, G_TYPE_VALUE, &candoit, G_TYPE_INVALID);
-	if (error != NULL) {
-		g_warning("Unable to check suspend: %s", error->message);
-		g_error_free(error);
-		return;
-	}
-	g_debug("Got Suspend: %s", g_value_get_boolean(&candoit) ? "true" : "false");
-
-	if (suspend_mi != NULL) {
-		dbusmenu_menuitem_property_set_value(suspend_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
-	}
-
-	return;
-}
-
-/* Response to getting the hibernate property */
-static void
-hibernate_prop_cb (DBusGProxy * proxy, DBusGProxyCall * call, gpointer userdata)
-{
-	hibernate_call = NULL;
-
-	GValue candoit = {0};
-	GError * error = NULL;
-	dbus_g_proxy_end_call(proxy, call, &error, G_TYPE_VALUE, &candoit, G_TYPE_INVALID);
-	if (error != NULL) {
-		g_warning("Unable to check hibernate: %s", error->message);
-		g_error_free(error);
-		return;
-	}
-	g_debug("Got Hibernate: %s", g_value_get_boolean(&candoit) ? "true" : "false");
-
-	if (suspend_mi != NULL) {
-		dbusmenu_menuitem_property_set_value(hibernate_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
-	}
-
-	return;
-}
+	if (g_strcmp0(type, "Suspend") == 0) {
+		up_client_suspend_sync(up_client, NULL, NULL);
+	} else {
+		up_client_hibernate_sync(up_client, NULL, NULL);
+	}
+	screensaver_unthrottle();
+
+	return;
+}
+
 
 /* A signal that we need to recheck to ensure we can still
    hibernate and/or suspend */
 static void
-dpk_changed_cb (DBusGProxy * proxy, gpointer user_data)
+up_changed_cb (UpClient * client, gpointer user_data)
 {
-	/* Start Async call to see if we can hibernate */
-	if (suspend_call == NULL) {
-		suspend_call = dbus_g_proxy_begin_call(dkp_prop_proxy,
-		                                       "Get",
-		                                       suspend_prop_cb,
-		                                       NULL,
-		                                       NULL,
-		                                       G_TYPE_STRING,
-		                                       DKP_INTERFACE,
-		                                       G_TYPE_STRING,
-		                                       "can-suspend",
-		                                       G_TYPE_INVALID,
-		                                       G_TYPE_VALUE,
-		                                       G_TYPE_INVALID);
-	}
-
-	/* Start Async call to see if we can suspend */
-	if (hibernate_call == NULL) {
-		hibernate_call = dbus_g_proxy_begin_call(dkp_prop_proxy,
-		                                         "Get",
-		                                         hibernate_prop_cb,
-		                                         NULL,
-		                                         NULL,
-		                                         G_TYPE_STRING,
-		                                         DKP_INTERFACE,
-		                                         G_TYPE_STRING,
-		                                         "can-hibernate",
-		                                         G_TYPE_INVALID,
-		                                         G_TYPE_VALUE,
-		                                         G_TYPE_INVALID);
-	}
+	GValue candoit = {0};
+	g_value_init (&candoit, G_TYPE_BOOLEAN);
+
+	/* can we suspend? */
+	g_value_set_boolean(&candoit, up_client_get_can_suspend(up_client));
+	dbusmenu_menuitem_property_set_value(suspend_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
+	g_debug("upower changed: can suspend: %s", g_value_get_boolean(&candoit) ? "true" : "false");
+
+	/* can we hibernate? */
+	g_value_set_boolean(&candoit, up_client_get_can_hibernate(up_client));
+	dbusmenu_menuitem_property_set_value(hibernate_mi, DBUSMENU_MENUITEM_PROP_VISIBLE, &candoit);
+	g_debug("upower changed: can hibernate: %s", g_value_get_boolean(&candoit) ? "true" : "false");
 
 	return;
 }
@@ -254,39 +175,17 @@
    DKp checking.  We're even setting up the calls for the props
    we need */
 static void
-setup_dkp (void) {
-	DBusGConnection * bus = dbus_g_bus_get(DBUS_BUS_SYSTEM, NULL);
-	g_return_if_fail(bus != NULL);
-
-	if (dkp_main_proxy == NULL) {
-		dkp_main_proxy = dbus_g_proxy_new_for_name(bus,
-		                                           DKP_ADDRESS,
-		                                           DKP_OBJECT,
-		                                           DKP_INTERFACE);
-	}
-	g_return_if_fail(dkp_main_proxy != NULL);
-
-	if (dkp_prop_proxy == NULL) {
-		dkp_prop_proxy = dbus_g_proxy_new_for_name(bus,
-		                                           DKP_ADDRESS,
-		                                           DKP_OBJECT,
-		                                           DBUS_INTERFACE_PROPERTIES);
-	}
-	g_return_if_fail(dkp_prop_proxy != NULL);
+setup_up (void) {
+	up_client = up_client_new();
+	g_return_if_fail(up_client != NULL);
 
 	/* Connect to changed signal */
-	dbus_g_proxy_add_signal(dkp_main_proxy,
-	                        "Changed",
-	                        G_TYPE_INVALID);
-
-	dbus_g_proxy_connect_signal(dkp_main_proxy,
-	                            "Changed",
-	                            G_CALLBACK(dpk_changed_cb),
-	                            NULL,
-	                            NULL);
+	g_signal_connect (up_client, "changed", 
+			  G_CALLBACK(up_changed_cb), 
+			  NULL);
 
 	/* Force an original "changed" event */
-	dpk_changed_cb(dkp_main_proxy, NULL);
+	up_changed_cb(up_client, NULL);
 
 	return;
 }
@@ -639,7 +538,7 @@
                       G_CALLBACK (user_removed),
                       root_menuitem);
 
-	setup_dkp();
+	setup_up();
 
     DbusmenuServer * server = dbusmenu_server_new(INDICATOR_SESSION_DBUS_OBJECT);
     dbusmenu_server_set_root(server, root_menuitem);