GNOME Bugzilla – Bug 682224
libgnome-desktop: Add GnomeIdleMonitor
Last modified: 2012-11-12 18:20:17 UTC
This code has been floating around for a while. I stole it from gnome-shell, cleaned it up so it was less icky and fit in with gnome-desktop style guides and things. gnome-screensaver and gnome-session have the same code, and I'm quite sure there's other code that cares about small amounts of user idle time (smaller than gnome-session presence provides) and either has its own IDLETIME hacks or uses timeouts (uggh) to try it.
Created attachment 221782 [details] [review] libgnome-desktop: Add GnomeIdleMonitor This copy/paste code has long been part of gnome-shell, gnome-session, gnome-screensaver, and probably other places as well. It deserves a standard place in gnome-desktop for code deduplication.
Oh, I forgot to add: I have patches for gnome-shell and gnome-session. They're not too difficult (s/gs/gnome/ or s/Shell./GnomeDesktop./), so I'm not sure they're worthy of attaching here.
Created attachment 221789 [details] [review] libgnome-desktop: Add GnomeIdleMonitor This copy/paste code has long been part of gnome-shell, gnome-session, gnome-screensaver, and probably other places as well. It deserves a standard place in gnome-desktop for code deduplication. Make sure to add a ref in the constructor.
Review of attachment 221782 [details] [review]: We could also move to the model where just gnome-settings-daemon is monitoring, and everything else watches over DBus. But this is fine with me. I'm assuming you didn't modify the code other than renaming; if so, looks correct to me.
Review of attachment 221789 [details] [review]: ACN based on previous review.
I did modify the code a lot.
I'm also not super happy with the (existing) API. Maybe look at a chance to improve it?
(In reply to comment #6) > I did modify the code a lot. Eeee....you know this code has been a continual source of evil, hard to reproduce bugs right? (Both in it and the X server). Hmm...do you plan to replace plugins/power/gpm-idletime.c too in gnome-settings-daemon? Let's see if we can get hughsie in on this bug.
(In reply to comment #8) > (In reply to comment #6) > > I did modify the code a lot. > > Eeee....you know this code has been a continual source of evil, hard to > reproduce bugs right? (Both in it and the X server). Yes, I do. That's why I modified (read: simplified) the code a lot. :) > Hmm...do you plan to replace plugins/power/gpm-idletime.c too in > gnome-settings-daemon? I swore I grepped through gnome-settings-daemon and didn't find anything. But yes, looks like I can easily replace it. > Let's see if we can get hughsie in on this bug.
OK, I ported plugins/power, and at the same time decided to clean the API of GnomeIdleMonitor up. New patches incoming, along with gnome-session, gnome-shell and gnome-settings-daemon patches.
Created attachment 224279 [details] [review] libgnome-desktop: Add GnomeIdleMonitor This copy/paste code has long been part of gnome-shell, gnome-session, gnome-screensaver, and probably other places as well. It deserves a standard place in gnome-desktop for code deduplication.
Created attachment 224280 [details] [review] Port to GnomeIdleMonitor
Created attachment 224281 [details] [review] power: Power to GnomeIdleMonitor
Created attachment 224282 [details] [review] Port to GnomeDesktopIdleMonitor
Review of attachment 224281 [details] [review]: Looks fine with me; that XIDLETIME code is super-hard to get right so it probably deserves to be abstracted out. Thanks.
Created attachment 224333 [details] [review] Port to GnomeDesktopIdleMonitor
Created attachment 224334 [details] [review] Port to GnomeDesktopIdleMonitor Update shell patch, and rebase to master.
I'd like to get this in for 3.8 (or 3.6.1 if we have time)
Review of attachment 224279 [details] [review]: Code looks reasonable, having been reviewed by Richard as well is good.
Review of attachment 224334 [details] [review]: One minor question, otherwise looks right. ::: js/ui/messageTray.js @@ +2128,3 @@ + let userIdle = this.idleMonitor.get_idletime() > IDLE_TIME; + if (userIdle) { + this._userActiveWhileNotificationShown = false; This is not a new bug, but can't we end up leaking watches here if showNotification() gets called multiple times?
Comment on attachment 224279 [details] [review] libgnome-desktop: Add GnomeIdleMonitor Attachment 224279 [details] pushed as df95944 - libgnome-desktop: Add GnomeIdleMonitor
Comment on attachment 224334 [details] [review] Port to GnomeDesktopIdleMonitor Attachment 224280 [details] pushed as d106191 - Port to GnomeIdleMonitor Indeed we will! Fixed and pushed.
Can I get feedback on the g-s-d and g-s patches here?
(In reply to comment #23) > Can I get feedback on the g-s-d and g-s patches here? Seeing as you've already committed the code here, why don't you create new bugs for the ports?
*** Bug 672039 has been marked as a duplicate of this bug. ***
FWIW, gnome_idle_monitor_add_watch() is broken if init_xsync() fails. It's also missing per-device idle support as requested in bug 672039. Would also be great in the future that you ask g-s-d maintainers if you want to move code from g-s-d somewhere else, just to make sure all the requirements are fulfilled.
Review of attachment 224281 [details] [review]: ::: configure.ac @@ +48,3 @@ GCONF_REQUIRED_VERSION=2.6.1 GIO_REQUIRED_VERSION=2.26.0 +GNOME_DESKTOP_REQUIRED_VERSION=3.5.91 Version will need changing. ::: plugins/power/gsd-power-manager.c @@ +3853,3 @@ } + g_object_unref (manager->priv->idle_monitor); Use g_clear_object() instead.
(In reply to comment #26) > FWIW, gnome_idle_monitor_add_watch() is broken if init_xsync() fails. That's just a fatal error I'm not sure what we should do. > It's also missing per-device idle support as requested in bug 672039. Sorry, I didn't see that bug, and I didn't see anything using per-device idle timers yet, so I assumed they weren't wanted yet. They're not hard to add. > Would also be great in the future that you ask g-s-d maintainers if you want to > move code from g-s-d somewhere else, just to make sure all the requirements are > fulfilled. That's why I tried to get Richard's input.
(In reply to comment #28) > (In reply to comment #26) > > FWIW, gnome_idle_monitor_add_watch() is broken if init_xsync() fails. > > That's just a fatal error I'm not sure what we should do. Either a GInitable, or a binding-friendly _get_error() would be my guess. > > It's also missing per-device idle support as requested in bug 672039. > > Sorry, I didn't see that bug, and I didn't see anything using per-device idle > timers yet, so I assumed they weren't wanted yet. They're not hard to add. Excellent, would be very useful. > > Would also be great in the future that you ask g-s-d maintainers if you want to > > move code from g-s-d somewhere else, just to make sure all the requirements are > > fulfilled. > > That's why I tried to get Richard's input. He probably was a wee bit busy at the time ;) No big deal.
Created attachment 227658 [details] [review] idle-monitor: Fix a memory leak when finding the proper IDLETIME counter
Created attachment 227659 [details] [review] idle-monitor: Remove the global constructor To support device-specific counters, we're going to need the ability to create multiple instances of the idle-monitor.
Created attachment 227660 [details] [review] idle-monitor: Allow counting device-specific idle-times For more complicated cases, it's possible that users may want to track per-device idle times. Allow them to specify a "device" property so that they can do this properly. Note that this is untested -- I haven't actually sat down and tested that this works properly, but it doesn't break the old IDLETIME case, and it compiles.
Created attachment 227734 [details] [review] idle-monitor: Remove the global constructor To support device-specific counters, we're going to need the ability to create multiple instances of the idle-monitor. Fix a potential crash where the filter would stay alive after the IdleMonitor was disposed.
Created attachment 227735 [details] [review] idle-monitor: Allow counting device-specific idle-times For more complicated cases, it's possible that users may want to track per-device idle times. Allow them to specify a "device" property so that they can do this properly. No changes, reattaching for order.
Comment on attachment 224280 [details] [review] Port to GnomeIdleMonitor Removing gnome-session patch from gnome-desktop bug.
Review of attachment 227658 [details] [review]: ++
Review of attachment 227734 [details] [review]: ++
Review of attachment 227735 [details] [review]: What happens when the server doesn't support the device-specific idletime counter? What happens when the GdkDevice is invalidated? (eg. somebody unplugged the keyboard this was tracking) ::: libgnome-desktop/gnome-idle-monitor.c @@ +191,3 @@ +{ + if (device) { + gint device_id = gdk_x11_device_get_id (device); Split this in 2 lines. @@ +206,3 @@ XSyncSystemCounter *counters; XSyncCounter counter = None; + char *counter_name = counter_name_for_device (monitor->priv->device); Please, no assignment in declarations.
Attachment 227658 [details] pushed as d56495c - idle-monitor: Fix a memory leak when finding the proper IDLETIME counter Attachment 227734 [details] pushed as ec61247 - idle-monitor: Remove the global constructor
Comment on attachment 224281 [details] [review] power: Power to GnomeIdleMonitor Attachment 224281 [details] pushed as 09b764b - power: Power to GnomeIdleMonitor
Created attachment 228800 [details] [review] idle-monitor: Allow counting device-specific idle-times For more complicated cases, it's possible that users may want to track per-device idle times. Allow them to specify a "device" property so that they can do this properly.
Attachment 228800 [details] pushed as 35ee57f - idle-monitor: Allow counting device-specific idle-times