Code review comment for lp:~pitti/indicator-session/libupower-glib

Revision history for this message
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!

« Back to merge proposal