GNOME Bugzilla – Bug 688227
idle-monitor: Reset the alarm so it can be triggered again
Last modified: 2013-02-15 08:13:37 UTC
.
Created attachment 228862 [details] [review] idle-monitor: Reset the alarm so it can be triggered again http://www.x.org/archive/X11R7.6/doc/xextproto/sync.txt says: [... If] the delta is 0 and test-type is PositiveComparison or NegativeComparison, no change is made to value and the alarm state is changed to Inactive before the event is generated. But we want to receive to receive events about the idleness/business of the counter, so reset the alarm when the alarm is triggered.
Attachment 228862 [details] pushed as 7f9fba8 - idle-monitor: Reset the alarm so it can be triggered again
Uhm... I didn't test this, so it probably solves the bug, but one comment: we use Negative/PositiveTransition, not Negative/PositiveComparison, so the above spec reference does not apply here. Also, last time I tried to debug a similar issue, I remember not seeing any AlarmNotify indicating that the alarm was inactive.
It fixed problems for me idle timeouts not getting triggered again unless I destroyed the monitor and created another one. Even with those fixes, the API is more than peculiar and too complicated for my taste. Feel free to double-check my fixes with the idle test I added to gnome-desktop some time before that.
From debugging a similar issue. For context, on whot's machine, the test case worked perfectly with the negative transition always triggering, and on my machine, it only triggered once. <whot> Jasper: still here? <Jasper> whot, sure <whot> I think I found the issue <whot> if you have a negative transition trigger, you get the event when idle falls below the value. but when you get the event, the server computes the value for the next transition. when it does that, the idle time in the server is 0, so the checks never register and the alarm isn't re-scheduled <whot> because current_idle < alarm value. so nothing happens. I think this works sometimes because if anything else registers on the idle timer and triggers an alarm, it's recalculated again and then it may be rescheduled <whot> alternatively, you can force that by changing the alarm, this will reschedule it too <whot> makes sense? <Jasper> so the reason it was working for you was because some other random client had an alarm scheduled <Jasper> ? <whot> that, or my computer was too slow :) <Jasper> heh <Jasper> so, would you consider this a server bug? <whot> i think so, because the alarm is still considered active but it can never trigger <Jasper> whot, wouldn't fixing this mean that clients that set a negative transition alarm on IDLETIME would get... a lot... of events? <whot> not if we fix it properly, since the transition is defined as _transition_, not state. but I don't think we keep the state around properly <Jasper> whot, well, IDLETIME is milliseconds <Jasper> so if the user is moving his mouse, but every millisecond doesn't result in motion, there's a lot of traffic going on there <Jasper> Unless there's already a base threshold in the server. <whot> there isn't, at least I didn't see one <whot> so if you set your threshold to 1, then yes, you'd see a lot of events <Jasper> whot, any other threshold for a negative transition for IDLETIME makes no sense <whot> for your use-case you can take the current value and use that <whot> since idletime only goes backwards when not idle <whot> you only need one trigger, right? <Jasper> That's not the way our API works right now, but maybe it makes sense to do that. Updating the alarm causes it to be rescheduled. Given that future X servers may actually send all of the negative transitions, I think it makes sense to restructure the GnomeIdleMonitor API to set a one-time alarm for became-active, rather than having it be always persistent. In gnome-shell's case, this would be set when we show a notification. In GPM's case, this would be set when we enter an idle state. In both cases, we actually don't care about every single time we become active, we just want to say "please tell me the next time you become active".
Created attachment 233719 [details] [review] idle-monitor: Rename xalarm_reset to became_active_alarm This name is a lot clearer about its purpose.
Created attachment 233720 [details] [review] idle-monitor: Clarify code and comments about alarm rescheduling workaround Segment the code out in its own function, point out that this is an Xorg bug, and don't actually set any attributes -- just call XSyncChangeAlarm.
Created attachment 233721 [details] [review] idle-monitor: Change the ::became-active API As all current uses of the :;became-active signal are one-shot, it makes sense for the test user to explicitly specify when they want that state transition to happen, so we don't get a flood of events on all pointer motion or key events that we won't do anything with.
This is an experimental set of changes to the API. I have patches for the three major components (gnome-shell, gnome-settings-daemon, gnome-session) to adapt to the new API, but I'm not going to bother filing them until we OK the new API. First, the name: gnome_idle_monitor_set_became_active_alarm is a bit too unwieldy in my opinion. Also, I thought about dropping the signal in favor of a callback-based approach, given that the set_*_alarm would be tied to one function invocation.
(In reply to comment #5) > In both cases, we actually don't care about every single time we become active, > we just want to say "please tell me the next time you become active". I don't want to have to request GnomeIdleMonitor to tell me it became active every single time. For gnome-settings-daemon's power plugin, the use case is different, I already have positive transitions, and I'm interested in receiving became-active when I have negative transitions for _those timeouts_.
(In reply to comment #10) > I don't want to have to request GnomeIdleMonitor to tell me it became active > every single time. > For gnome-settings-daemon's power plugin, the use case is different, I already > have positive transitions, and I'm interested in receiving became-active when I > have negative transitions for _those timeouts_. The API would still need to be changed anyway to respect that, otherwise we would get too many events if the user became-active when the state hadn't timed out yet. The issue is "which timeout"? The minimum one is the only one that makes sense, in the g-s-d case. This means that we would need an API: 1) If using watches, uses the minimum watch timeout as the became-active source. 2) If not using watches, needs a special function to prime a one-fire no-threshold alarm. Which sounds complicated. Suggestions?
This whole xsync api is a trainwreck. Nobody can understand it, and even smart people get it wrong all the time. That being said, I think it would make sense to have an api like: guint gnome_idle_monitor_add_nonidle_watch (GnomeIdleMonitor *monitor, guint interval_msec, GnomeIdleMonitorWatchFunc callback, gpointer user_data, GDestroyNotify notify); to install a watch that gets triggered whenever idletime jumps from a value >= interval_msec back to zero. Might make sense to rename add_watch to add_idle_watch to match.
That makes sense to me. Bastien, what do you feel about that? Note that the original bug is going to get fixed upstream, so this means that we'll get a bunch of events if we don't get smart soon: https://bugs.freedesktop.org/show_bug.cgi?id=59644
(In reply to comment #13) > That makes sense to me. Bastien, what do you feel about that? Fine by me, as long as we remove the became-active signal too and the counterpart signal to "triggered-idle". It provides a nice symmetry to the API, infinitely cleaner than what we have now.
Created attachment 234244 [details] [review] idle-monitor: Clarify code and comments about alarm rescheduling workaround Segment the code out in its own function, point out that this is an Xorg bug, and don't actually set any attributes -- just call XSyncChangeAlarm.
Created attachment 234245 [details] [review] idle-monitor: Fix documentation
Created attachment 234246 [details] [review] idle-monitor: Make sure we publicly make watch intervals 64-bits wide
Created attachment 234247 [details] [review] idle-monitor: Change the API to be function-based As the current API design has been unknowingly relying on an Xorg server bug that may be fixed in the future, we need to modify the API so that became-active is explicitly asked for by the user so that we won't get flooded with a large number of events for almost every input event.
Created attachment 234257 [details] [review] gsm-presence: Update to new GnomeIdleMonitor API
Created attachment 234258 [details] [review] power: Adapt to new GnomeIdleMonitor API
Created attachment 234259 [details] [review] Update to new GnomeIdleMonitor API
Review of attachment 234246 [details] [review]: You should flag the commit somehow, so that we don't forget to bump the soname at the next release.
Review of attachment 234244 [details] [review]: ::: libgnome-desktop/gnome-idle-monitor.c @@ +127,3 @@ + XSyncAlarmAttributes attr; + + /* Some versions of Xorg have an issue where alarms aren't citation needed (or bugzilla link, upstream ML thread).
Review of attachment 234245 [details] [review]: ::: libgnome-desktop/gnome-idle-monitor.c @@ +447,3 @@ * @interval_msec: The idletime interval, in milliseconds * @callback: (allow-none): The callback to call when the user has + * accumulated @interval_msec seconds of idle time. s/seconds/milliseconds/ I would think.
Review of attachment 234246 [details] [review]: Looks good.
Review of attachment 234247 [details] [review]: ::: libgnome-desktop/gnome-idle-monitor.c @@ -72,3 @@ -{ - BECAME_ACTIVE, - TRIGGERED_IDLE, What was wrong with sending signals? The callbacks are nice, but sending a signal is nicer to bindings, especially when, in some programs, they might all be handled by the same function. @@ +482,3 @@ */ guint +gnome_idle_monitor_add_user_active_watch (GnomeIdleMonitor *monitor, If I were to add such a function, I would make the prototype match that of add_idle_watch(), and remove the automatically created "active" watch. With the code below, you'd receive xalarms even if you didn't call gnome_idle_monitor_add_user_active_watch().
I'll leave the other 3 patches to be reviewed when the API changes are settled.
Review of attachment 234247 [details] [review]: I'd be happy with the signals removal if I had assurance that this code is usable from gobject-introspection based bindings without additional C code. ::: libgnome-desktop/gnome-idle-monitor.c @@ +482,3 @@ */ guint +gnome_idle_monitor_add_user_active_watch (GnomeIdleMonitor *monitor, That last bit about receiving xalarms for the user became active aren't correct.
(In reply to comment #28) > Review of attachment 234247 [details] [review]: > > I'd be happy with the signals removal if I had assurance that this code is > usable from gobject-introspection based bindings without additional C code. As the gnome-shell patch shows, it should be usable from bindings. Even if I prefer signals, Jasper doesn't, so fine to commit.
Review of attachment 234257 [details] [review]: Looks fine.
Review of attachment 234258 [details] [review]: Please split off the cursor patch from the power patch, and bump the gnome-desktop dep. ::: plugins/power/gsd-power-manager.c @@ +2425,3 @@ + /* if we're moving to an idle mode, make sure + * we add a watch to take us back to normal */ You can't do that here, there's nothing saying that GSD_POWER_IDLE_MODE_NORMAL has to happen when the idle isn't idle (that's not the case when plugging in the AC for example). This should be done in idle_triggered_idle_cb(). You can double-check that this works properly by running the test suite ("make check" in plugins/power/, make sure that the latest python-dbusmock is installed).
Created attachment 235910 [details] [review] idle-monitor: Change the API to be function-based As the current API design has been unknowingly relying on an Xorg server bug that may be fixed in the future, we need to modify the API so that became-active is explicitly asked for by the user so that we won't get flooded with a large number of events for almost every input event.
Review of attachment 235910 [details] [review]: The one thing I'm not sure about is "add_user_active_watch" and "add_idle_watch". "add_active_watch" sounded wrong, and "add_user_idle_watch" sounds a bit unnecessary. Opinions?
Created attachment 235913 [details] [review] idle-monitor: Change the API to be function-based As the current API design has been unknowingly relying on an Xorg server bug that may be fixed in the future, we need to modify the API so that became-active is explicitly asked for by the user so that we won't get flooded with a large number of events for almost every input event.
Created attachment 235914 [details] [review] idle-monitor: Fix documentation
Created attachment 235915 [details] [review] gsm-presence: Update to new GnomeIdleMonitor API
(In reply to comment #33) > Review of attachment 235910 [details] [review]: > > The one thing I'm not sure about is "add_user_active_watch" and > "add_idle_watch". "add_active_watch" sounded wrong, and "add_user_idle_watch" > sounds a bit unnecessary. Opinions? Really not fussed.
could solve the naming inconsistency by adding a enum GnomeIdleMonitorWatchMode { GNOME_IDLE_MONITOR_WATCH_USER_ACTIVITY, GNOME_IDLE_MONITOR_WATCH_USER_INACTIVITY }; and then just having one function. but doesn't really matter. api cleanliness is already a lost cause for gnome-desktop
since these are one-shot functions I think it would probably be better if they followed the _async (..., GCancellable, GAsyncReadyCallback, ...) idiom but doesn't really matter.
(In reply to comment #38) > could solve the naming inconsistency by adding a > > enum GnomeIdleMonitorWatchMode > { > GNOME_IDLE_MONITOR_WATCH_USER_ACTIVITY, > GNOME_IDLE_MONITOR_WATCH_USER_INACTIVITY > }; > > and then just having one function. but doesn't really matter. api cleanliness > is already a lost cause for gnome-desktop "inactivity" isn't 1-1 with "became idle" though.
(In reply to comment #39) > since these are one-shot functions I think it would probably be better if they > followed the _async (..., GCancellable, GAsyncReadyCallback, ...) idiom but > doesn't really matter. I wouldn't use GIO style async functions for something which might, or might not be called, depending on user interaction. I would have preferred a signal, but the API is already mushy enough. I'll commit this as-is.
Hi, > I'll commit this as-is. sounds good.
Attachment 234244 [details] pushed as 626de2a - idle-monitor: Clarify code and comments about alarm rescheduling workaround Attachment 234246 [details] pushed as 36c472b - idle-monitor: Make sure we publicly make watch intervals 64-bits wide Attachment 235913 [details] pushed as 1fc10e6 - idle-monitor: Change the API to be function-based Attachment 235914 [details] pushed as 21348a1 - idle-monitor: Fix documentation
Comment on attachment 235915 [details] [review] gsm-presence: Update to new GnomeIdleMonitor API Attachment 235915 [details] pushed as 377e024 - gsm-presence: Update to new GnomeIdleMonitor API
Comment on attachment 234259 [details] [review] Update to new GnomeIdleMonitor API This patch doesn't apply anymore.
Created attachment 236130 [details] [review] idle: add compat #define This commit is just temporary until g-s-d is updated.
Comment on attachment 236130 [details] [review] idle: add compat #define pushing since the build is apparently broken atm.
(In reply to comment #43) > Attachment 235913 [details] pushed as 1fc10e6 - idle-monitor: Change the API to be > function-based Is someone already working on unbreaking gnome-shell or should I do it?
Created attachment 236180 [details] [review] Update to new GnomeIdleMonitor API
*** Bug 693845 has been marked as a duplicate of this bug. ***
Created attachment 236182 [details] [review] power: Adapt to new GnomeIdleMonitor API
Created attachment 236183 [details] [review] cursor: Adapt to new GnomeIdleMonitor API OK, here are my patches for gnome-settings-daemon. Note that these are not even close to tested, because I can't run the power test suite for some reason, even with an unpatched everything.
Review of attachment 236182 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +2722,3 @@ + manager->priv->idle_logout_warning_id = gnome_idle_monitor_add_idle_watch (manager->priv->idle_monitor, + timeout_logout_warning * 1000, + idle_triggered_idle_cb, manager, NULL); These should be sleep_warning, I think
Comment on attachment 236182 [details] [review] power: Adapt to new GnomeIdleMonitor API Attachment 236182 [details] pushed as e8bc8e8 - power: Adapt to new GnomeIdleMonitor API
Comment on attachment 236183 [details] [review] cursor: Adapt to new GnomeIdleMonitor API Got committed as part of the power plugin port...
Attachment 236180 [details] pushed as 261fbef - Update to new GnomeIdleMonitor API And we'll handle bugs separately, if any.