GNOME Bugzilla – Bug 706005
Add support for idle tracking
Last modified: 2013-08-23 14:35:13 UTC
The main reason for this bug, at this point, is because idle tracking already works, in a way: it uses X's IDLETIME. But once you're idle and your screenshield fell, the shell becomes modal, X no longer sees events and you're trapped on a black screen.
Created attachment 251627 [details] [review] Add a new helper for tracking user idle activity When running as a wayland compositor, we can't use the xserver's IDLETIME, because that's updated only in response to X events. But we have all the events ourselves, so we can just run the timer in process.
Created attachment 251628 [details] [review] MetaIdleMonitor: add a DBus interface for the idle monitor To allow other clients (gnome-session, gnome-settings-daemon) to monitor user activity, introduce a DBus interface for the idle monitor inside mutter.
Created attachment 251629 [details] [review] Replace GnomeIdleMonitor with MetaIdleMonitor Now that GnomeIdleMonitor is a DBus API for mutter, we need to use own in-process thing, to avoid dead locks.
Review of attachment 251628 [details] [review]: idle-monitor.xml is missing
Review of attachment 251628 [details] [review]: ::: src/core/meta-idle-monitor.c @@ +855,3 @@ + monitor = meta_idle_monitor_get_for_device (device_id); + path = g_strdup_printf ("/org/gnome/Mutter/IdleMonitor/Device%d", device_id); + } When does this monitor object ever get cleaned up ? You're not handling device-removed - maybe you should ? Also, path is leaked, as far as I can tell. @@ +882,3 @@ + GSList *devices, *iter; + + manager = g_dbus_object_manager_server_new ("/org/gnome/Mutter/IdleMonitor"); This object is never cleaned up ? Might be ok, just asking... @@ +893,3 @@ + G_CALLBACK (on_device_added), manager, 0); + + g_dbus_object_manager_server_set_connection (manager, g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL)); Looks like you're leaking a reference to the session bus here.
Review of attachment 251629 [details] [review]: ::: js/ui/components/automountManager.js @@ +26,3 @@ this._settings = new Gio.Settings({ schema: SETTINGS_SCHEMA }); this._volumeQueue = []; + this._session = new GnomeSession.SessionManager(function() { }); Unrelated change ? ::: js/ui/components/autorunManager.js @@ -164,2 @@ _init: function() { - this._session = new GnomeSession.SessionManager(); Unrelated change ? ::: js/ui/status/system.js @@ -54,3 @@ - this._session = new GnomeSession.SessionManager(); - this._haveShutdown = true; Unrelated change ? ::: src/shell-app-usage.c @@ +405,3 @@ + self->idle_monitor = meta_idle_monitor_get_core (); + self->idle_id = meta_idle_monitor_add_idle_watch (self->idle_monitor, 600 * 1000, This is configurable timeout in gnome-session, no ? If you want to do this manually here, you have to consult org.gnome.desktop.session idle-delay, I think.
Created attachment 252384 [details] [review] Add a new helper for tracking user idle activity When running as a wayland compositor, we can't use the xserver's IDLETIME, because that's updated only in response to X events. But we have all the events ourselves, so we can just run the timer in process.
Created attachment 252385 [details] [review] MetaIdleMonitor: add a DBus interface for the idle monitor To allow other clients (gnome-session, gnome-settings-daemon) to monitor user activity, introduce a DBus interface for the idle monitor inside mutter.
Created attachment 252394 [details] [review] Replace GnomeIdleMonitor with MetaIdleMonitor Now that GnomeIdleMonitor is a DBus API for mutter, we need to use own in-process thing, to avoid dead locks.
Review of attachment 252384 [details] [review]: ::: src/core/meta-idle-monitor.c @@ +98,3 @@ +} + +#define GINT64_TO_XSYNCVALUE(value, ret) XSyncIntsToValue (ret, value, ((guint64)value) >> 32) I guess this probably works, but it would be more obvious if you explicitly cut value to the low 32 bits in the second argument. Also, good practice in macros requires to add () around arguments in the replacement text - you never know what people stuff in there...
Review of attachment 252384 [details] [review]: ::: src/core/meta-idle-monitor.c @@ +406,3 @@ +static void +ensure_device_monitor (int device_id) +{ Would be good to assert device_id < 256 here
Review of attachment 252385 [details] [review]: ::: src/core/meta-idle-monitor.c @@ +805,3 @@ + g_free (path); + + g_clear_object (&device_monitors[device_id]); Where did this get added ? I don't see it in the on_device_added handler above.
Review of attachment 252394 [details] [review]: looks fine
(In reply to comment #11) > Review of attachment 252384 [details] [review]: > > ::: src/core/meta-idle-monitor.c > @@ +406,3 @@ > +static void > +ensure_device_monitor (int device_id) > +{ > > Would be good to assert device_id < 256 here It is checked in meta_idle_monitor_get_for_device (while the other path passes constant 0)
Created attachment 252853 [details] [review] Add a new helper for tracking user idle activity When running as a wayland compositor, we can't use the xserver's IDLETIME, because that's updated only in response to X events. But we have all the events ourselves, so we can just run the timer in process.
Created attachment 252854 [details] [review] MetaIdleMonitor: add a DBus interface for the idle monitor To allow other clients (gnome-session, gnome-settings-daemon) to monitor user activity, introduce a DBus interface for the idle monitor inside mutter. Note that I slightly changed the DBus API, and now we report the core monitor in addition to the device monitors for the core devices (virtual pointer and virtual keyboard). The reason for this is that in a XWayland environment, the virtual devices are the only ones with a guaranteed ID (2 and 3 respectively), so device specific monitoring would be impossible without them. It should also be pointer out that the only current user of device monitors is the cursor plugin in gnome-settings-daemon, which takes care of hiding the cursor at startup and showing it as soon as a physical mouse is used (and similarly for the OSK and a physical keyboard) I think it would be a lot simpler, in wayland or in X, if that functionality was implemented by mutter.
Created attachment 252865 [details] [review] MetaIdleMonitor: add wayland support Keep a timer source that we reset when we capture an event in MetaWayland, and fire watches accordingly.
Review of attachment 252853 [details] [review]: ok then
Review of attachment 252854 [details] [review]: ok
Review of attachment 252865 [details] [review]: looks ok to me (haven't tested it)
(In reply to comment #16) > > It should also be pointer out that the only current user of > device monitors is the cursor plugin in gnome-settings-daemon, > which takes care of hiding the cursor at startup and showing > it as soon as a physical mouse is used (and similarly for the > OSK and a physical keyboard) > I think it would be a lot simpler, in wayland or in X, if > that functionality was implemented by mutter. Makes sense to me.
Attachment 252853 [details] pushed as c63e5f7 - Add a new helper for tracking user idle activity Attachment 252854 [details] pushed as 387b539 - MetaIdleMonitor: add a DBus interface for the idle monitor
Comment on attachment 252394 [details] [review] Replace GnomeIdleMonitor with MetaIdleMonitor Attachment 252394 [details] pushed as e0574d2 - Replace GnomeIdleMonitor with MetaIdleMonitor
Let's get this tested then! :) Attachment 252865 [details] pushed as 9affbf1 - MetaIdleMonitor: add wayland support