When the session becomes inactive, clear all idle timeouts, so we don't get them delivered while we're inactive or just when we're aout to become active again.
BUG: 354250
FIXED-IN: 5.7.0
Details
Details
- Reviewers
oliverhenshaw sebas - Group Reviewers
Plasma - Commits
- R122:ece8adacb35b: Clear idle timeouts when session becomes inactive
Separate changes include:
- m_pendingResumeFromIdleActions becomes a QSet so save the if (!contains) { append } dance
- iterate over pendingResumeFromIdleActions and then clear the whole thing afterwards rather than erasing-while-iterating, might help Bug 345618
- Using auto for iterator names
Testing done:
- Let my session idle, screen dimmed, moved mouse, screen brightened again
- Switched to other session, waited for the timeout on the other session, switched back, screen did not dim (previously would do that and eventually suspend)
- Switched to other session, waited for timeout, switched back, screen did not dim, did not touch the mouse during that, screen eventually dimmed as it should (but not immediately after switching)
Diff Detail
Diff Detail
- Repository
- R122 Powerdevil
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Hmm, not sure. There might be places where one wouldn't care about session switching and just expect the event to be always delivered at some point.
Comment Actions
Cancelling all timeouts set by powerdevil when the session becomes inactive is probably the right thing to do here.
Also, by my reading, QList::erase(iterator pos) is a safe way to delete iterator elements. But I could be wrong.
| daemon/powerdevilcore.cpp | ||
|---|---|---|
| 151 | Like you said, other kidletime users might not care whether the session is active or not. This will remove all timeouts set by other kded services, right? Just off the top of my head, Is ActionPool::unloadAllActiveActions() better here, or is that not appropriate? It's too late in the day here for me to look into that today. | |
Comment Actions
Good observation. Switching to a session is user interaction, so the idle timers should be reset here.
Comment Actions
I may not have much pull, but I'd rather these changes were revised again before being landed. Sorry!
Note that although this is the right thing to do, the bug reporter found this change did not fix their problem - https://bugs.kde.org/show_bug.cgi?id=354250#c25 . I haven't tested it myself but it makes sense that the call to erase idle timeouts will eventually make xlib calls, which will block if your xserver is blocked (which it seems to for some users when switched to another VT)
| daemon/powerdevilcore.cpp | ||
|---|---|---|
| 141–143 | This comment should reflect that this is actively a good choice and not just a workaround for a corner case. IIRC the policyagent will forbid actions when the session is not active so not scheduling idle timeouts is just good efficiency. | |
| 151 | Again, removeAllIdleTimeouts is too big a hammer, when arbitrary other kded services use the same kidletime instance. | |
| 777–780 | It would be better for this set of changes to have its own review and own commit message. Also how can this solve a crash bug? I'm speculating wildly here, but: As far as I can see, using the iterator returned by erase() should be safe. Unless there's not really an implicit null pointer check in the while loop and the compiler optimises it away? Or perhaps onWakeFromIdle() for some action ends up invalidating the iterator somehow, possibly if it re-enters another instance of onResumingFromIdle()? If so, clearing m_pendingResumeFromIdleActions is definitely wrong because there could well have been a nested call to onKIdleTimeoutReached() Either way, the real bug is somewhere else. | |
Comment Actions
Don't worry, Oliver, I actually wasn't going to push this before I had further feedback from you. :)
I'll address the clear all idle timeouts call (oh, how I wish I had already done the powerdevil-as-separate-binary thing) and then should be good to go, minus the erase iterator part.
Comment Actions
So, what should we do with this? Since we now have powerdevil as a separate binary we could savely do
KIdleTime::instance()->removeAllIdleTimeouts();
without jeopardizing other kded modules.
Comment Actions
So, should we go with this patch then? Powerdevil is now a separate binary, so
IdleTime::instance()->removeAllIdleTimeouts();
shouldn't cause side-effects on other modules and also should fix it for non-weird X setups like mine. ;)
Comment Actions
Yep, the removeAllIdleTimeouts now looks good.
The bad news is that I've been running with an evolution of https://phabricator.kde.org/D2393 with no problem for a while and still got the suspend-after-user-switch happen today. Looking through logs it seems like this time kded (I'm still using a patched stable release) was getting blocked even before onActiveSessionChanged (so before the login1 dbus signal) so I would still have had the problem with this patch.
I never got the the bottom of the blocking in X issue, best guess was either that the xserver was drawing by itself and blocking, or that kded was blocked since it's a DRI client. The powerdevil binary does drawing for the fadeout effect, right?
How about checking (KIdleTime::instance()->idleTime() >= msec) in Core::onKIdleTimeoutReached() before preceding with the idle action?
Comment Actions
The powerdevil binary does drawing for the fadeout effect, right?
It just sets an X property on the root window and KWin does the fading.