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
oliverhenshaw | |
sebas |
Plasma |
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
Separate changes include:
Testing done:
Lint Skipped |
Unit Tests Skipped |
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.
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. |
Good observation. Switching to a session is user interaction, so the idle timers should be reset here.
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. |
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.
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.
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. ;)
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?
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.