Bug 62530

The copyright doesn't look right.

+ self->priv->is_inactive = (g_variant_get_uint32 (prop) == STATUS_IDLE);

This is going to critical and blow up if prop is not a uint32. Defensive programming please.

+ g_dbus_proxy_new_for_bus (G_BUS_TYPE_SESSION,
+ G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, NULL,
+ SERVICE_NAME, SERVICE_OBJECT_PATH, SERVICE_INTERFACE,
+ NULL,
+ proxy_new_cb, g_object_ref (self));

If it doesn't emit PropertiesChanged you should use G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES.

+ def Get(self, e):
+ prop = e.args[0] + '.' + e.args[1]
+ self.q.dbus_return(e.message, self.status, signature='v')

I'd prefer this to do *something* sensible (return an error?) if prop is not the expected thing.

- for enabled in [False, True, False]:
- mce.InactivityChanged(enabled)
+ for enabled in [STATUS_AVAILABLE, STATUS_IDLE, STATUS_BUSY]:
+ service.StatusChanged(enabled)
q.expect('dbus-method-call', method='SetPowerSaving',
args=[enabled], interface=cs.CONN_IFACE_POWER_SAVING,

Er, this used to expect SetPowerSaving to be called with a boolean value, and now expects it to be called with an integer value. That can't be right?

(I would expect args=[enabled == STATUS_IDLE] or something.)

(In reply to comment #1)
> + G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START, NULL,
> 
> If it doesn't emit PropertiesChanged you should use
> G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES.

No, I was wrong about that, please ignore.

Branch updated with review comments fixed and unit test passed *once*. Now all unit tests fails again with the timeout error:

ERROR:dbus.proxies:Introspect error on org.freedesktop.Telepathy.AccountManager:/org/freedesktop/Telepathy/AccountManager: dbus.exceptions.DBusException: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.Telepathy.AccountManager was not provided by any .service files

I don't understand how to make MC tests work reliably... but that's another story.

oh, and I tested on real-live unity desktop and it works as expected. g-s implements the same dbus iface so must be fine as well :)

+ prop = g_variant_get_child_value (parameters, 0);

This is going to crash (or at least critical) if there are no parameters at all. Either check explicitly, or supply a (static) GDBusInterfaceInfo to g_dbus_proxy_new_for_bus() (it can just include that one signal if you like).

(In reply to comment #3)
> Branch updated with review comments fixed and unit test passed *once*. Now
> all unit tests fails again with the timeout error

...

Does this still happen? What if you use the upstream branch rather than your patched version? MC 5.14 and 5.15 branches both pass reliably for me.

Created attachment 78805 [details] [review]
McdSlacker: don't crash if SessionManager is absent or  StatusChanged has no parameters

---

The crash you're experiencing is because most of the tests don't have a SessionManager, and your implementation crashes if there is no cached "status" property. Fixed that for you.

Created attachment 78806 [details] [review]
McdSlacker: Use org.gnome.SessionManager DBus service

From: Xavier Claessens <xavier.claessens@collabora.co.uk>

MCE is a long dead Nokia closed source service for Maemo.
This rewrites McdSlacker using GDBus and the much more useful
GNOME SessionManager service. That interface is implemented in
both gnome-shell and unity desktops.

---

The patch on which I'm basing Attachment #78805 [details].

+1

Summary of discussion with Xavier and Guillaume: there is some concern that this should perhaps not always be active - either runtime-configurable, or only when UPower says we are on battery, or (etc.):

* While the session is idle, users might reasonably expect to still see (or hear!) notifications. In GNOME 3.8, new message notifications (with contact names but not the actual message) appear on the lock screen by default; notification sounds appear to be on by default, but muted while in the lock screen.

* Maybe we should only do this when configured with UPower support, and it says we are on battery?

* Maybe we should only do this when the screen is blanked? (How do we tell?)

For now, I'm going to merge these patches for the forthcoming 5.15.0 release to maximize testing, but leave the bug open.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-mission-control/issues/70.