oliverhenshaw requested changes to this revision. oliverhenshaw added a reviewer: oliverhenshaw. oliverhenshaw added a comment. This revision now requires changes to proceed.
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) INLINE COMMENTS > powerdevilcore.cpp:141-143 > + // Bug 354250: Simulate user activity when session becomes inactive, > + // this keeps us from sending the computer to sleep when switching to an > idle session. > + // (this is just being lazy as it will result in us clearing everything 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. > oliverhenshaw wrote in 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. Again, removeAllIdleTimeouts is too big a hammer, when arbitrary other kded services use the same kidletime instance. > powerdevilcore.cpp:777-780 > + for (auto it = m_pendingResumeFromIdleActions.constBegin(), end = > m_pendingResumeFromIdleActions.constEnd(); it != end; ++it) { > + (*it)->onWakeupFromIdle(); > } > + m_pendingResumeFromIdleActions.clear(); 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. REPOSITORY rPOWERDEVIL Powerdevil REVISION DETAIL https://phabricator.kde.org/D2033 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: broulik, #plasma, sebas, oliverhenshaw Cc: sebas, oliverhenshaw, graesslin, plasma-devel, jensreuterberg, abetts
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
