After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 706005 - Add support for idle tracking
Add support for idle tracking
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
wayland
Depends on:
Blocks: 706006
 
 
Reported: 2013-08-14 16:42 UTC by Giovanni Campagna
Modified: 2013-08-23 14:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a new helper for tracking user idle activity (27.01 KB, patch)
2013-08-14 16:42 UTC, Giovanni Campagna
none Details | Review
MetaIdleMonitor: add a DBus interface for the idle monitor (11.86 KB, patch)
2013-08-14 16:42 UTC, Giovanni Campagna
needs-work Details | Review
Replace GnomeIdleMonitor with MetaIdleMonitor (11.49 KB, patch)
2013-08-14 16:43 UTC, Giovanni Campagna
needs-work Details | Review
Add a new helper for tracking user idle activity (22.89 KB, patch)
2013-08-20 11:53 UTC, Giovanni Campagna
reviewed Details | Review
MetaIdleMonitor: add a DBus interface for the idle monitor (14.52 KB, patch)
2013-08-20 11:53 UTC, Giovanni Campagna
reviewed Details | Review
Replace GnomeIdleMonitor with MetaIdleMonitor (3.19 KB, patch)
2013-08-20 12:05 UTC, Giovanni Campagna
committed Details | Review
Add a new helper for tracking user idle activity (22.98 KB, patch)
2013-08-23 13:30 UTC, Giovanni Campagna
committed Details | Review
MetaIdleMonitor: add a DBus interface for the idle monitor (13.44 KB, patch)
2013-08-23 13:35 UTC, Giovanni Campagna
committed Details | Review
MetaIdleMonitor: add wayland support (7.70 KB, patch)
2013-08-23 14:09 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2013-08-14 16:42:06 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.
Comment 1 Giovanni Campagna 2013-08-14 16:42:08 UTC
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.
Comment 2 Giovanni Campagna 2013-08-14 16:42:15 UTC
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.
Comment 3 Giovanni Campagna 2013-08-14 16:43:04 UTC
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.
Comment 4 Matthias Clasen 2013-08-18 19:49:24 UTC
Review of attachment 251628 [details] [review]:

idle-monitor.xml is missing
Comment 5 Matthias Clasen 2013-08-18 19:59:51 UTC
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.
Comment 6 Matthias Clasen 2013-08-18 20:04:37 UTC
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.
Comment 7 Giovanni Campagna 2013-08-20 11:53:46 UTC
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.
Comment 8 Giovanni Campagna 2013-08-20 11:53:54 UTC
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.
Comment 9 Giovanni Campagna 2013-08-20 12:05:13 UTC
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.
Comment 10 Matthias Clasen 2013-08-23 12:44:21 UTC
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...
Comment 11 Matthias Clasen 2013-08-23 12:50:49 UTC
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
Comment 12 Matthias Clasen 2013-08-23 12:52:31 UTC
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.
Comment 13 Matthias Clasen 2013-08-23 12:53:53 UTC
Review of attachment 252394 [details] [review]:

looks fine
Comment 14 Giovanni Campagna 2013-08-23 13:26:23 UTC
(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)
Comment 15 Giovanni Campagna 2013-08-23 13:30:11 UTC
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.
Comment 16 Giovanni Campagna 2013-08-23 13:35:43 UTC
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.
Comment 17 Giovanni Campagna 2013-08-23 14:09:31 UTC
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.
Comment 18 Matthias Clasen 2013-08-23 14:10:38 UTC
Review of attachment 252853 [details] [review]:

ok then
Comment 19 Matthias Clasen 2013-08-23 14:14:45 UTC
Review of attachment 252854 [details] [review]:

ok
Comment 20 Matthias Clasen 2013-08-23 14:16:21 UTC
Review of attachment 252865 [details] [review]:

looks ok to me (haven't tested it)
Comment 21 Matthias Clasen 2013-08-23 14:18:57 UTC
(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.
Comment 22 Giovanni Campagna 2013-08-23 14:21:41 UTC
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 23 Giovanni Campagna 2013-08-23 14:23:05 UTC
Comment on attachment 252394 [details] [review]
Replace GnomeIdleMonitor with MetaIdleMonitor

Attachment 252394 [details] pushed as e0574d2 - Replace GnomeIdleMonitor with MetaIdleMonitor
Comment 24 Giovanni Campagna 2013-08-23 14:35:08 UTC
Let's get this tested then! :)

Attachment 252865 [details] pushed as 9affbf1 - MetaIdleMonitor: add wayland support