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

Revision history for this message
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: Needs Resubmitting

« Back to merge proposal