<table><tr><td style="">oliverhenshaw requested changes to this revision.<br />oliverhenshaw added a reviewer: oliverhenshaw.<br />oliverhenshaw added a comment.<br />This revision now requires changes to proceed.</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D2033" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>I may not have much pull, but I'd rather these changes were revised again before being landed. Sorry!</p>
<p>Note that although this is the right thing to do, the bug reporter found this change did not fix their problem - <a href="https://bugs.kde.org/show_bug.cgi?id=354250#c25" class="remarkup-link" target="_blank" rel="noreferrer">https://bugs.kde.org/show_bug.cgi?id=354250#c25</a> . 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)</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2033#inline-8463" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">powerdevilcore.cpp:141-143</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #74777d">// Bug 354250: Simulate user activity when session becomes inactive,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #74777d">// this keeps us from sending the computer to sleep when switching to an idle session.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #74777d">// (this is just being lazy as it will result in us clearing everything</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2033#inline-7796" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">oliverhenshaw</span> wrote in <span style="color: #4b4d51; font-weight: bold;">powerdevilcore.cpp:151</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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?</p>
<p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Again, removeAllIdleTimeouts is too big a hammer, when arbitrary other kded services use the same kidletime instance.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2033#inline-8462" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">powerdevilcore.cpp:777-780</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"></span><span class="n"><span class="bright">QList</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright"><</span></span><span class="bright"> </span><span class="n"><span class="bright">Action</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">>::</span></span><span class="bright"></span><span class="n"><span class="bright">iterator</span></span><span class="bright"> </span><span class="n"><span class="bright">i</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">m_pendingResumeFromIdleActions</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">begin</span></span><span class="bright"></span><span class="p"><span class="bright">();</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">while</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">i</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">!=</span></span><span class="bright"> </span><span class="n"><span class="bright">m_pendingResume</span>FromIdle<span class="bright">Actions</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">end</span></span><span class="bright"></span><span class="p"><span class="bright">())</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="p">(</span><span style="color: #aa2211">*</span><span class="n">i</span><span class="p">)</span><span style="color: #aa2211">-></span><span class="n">onWakeupFromIdle</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="n">i</span> <span style="color: #aa2211">=</span> <span class="n">m_pendingResumeFromIdleActions</span><span class="p">.</span><span class="n">erase</span><span class="p">(</span><span class="n">i</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">for</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span><span class="bright"> </span><span class="n"><span class="bright">it</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">m_pendingResumeFromIdleActions</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">constBegin</span></span><span class="bright"></span><span class="p"><span class="bright">(),</span></span><span class="bright"> </span><span class="n"><span class="bright">end</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">m_pendingResumeFromIdleActions</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">constEnd</span></span><span class="bright"></span><span class="p"><span class="bright">();</span></span><span class="bright"> </span><span class="n"><span class="bright">it</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">!=</span></span><span class="bright"> </span><span class="n"><span class="bright">end</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">++</span></span><span class="bright"></span><span class="n"><span class="bright">it</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"></span><span class="n"><span class="bright">it</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">onWakeup</span>FromIdle<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">();</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">m_pendingResumeFromIdleActions</span><span class="p">.</span><span class="n">clear</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It would be better for this set of changes to have its own review and own commit message.</p>
<p style="padding: 0; margin: 8px;">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()</p>
<p style="padding: 0; margin: 8px;">Either way, the real bug is somewhere else.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>rPOWERDEVIL Powerdevil</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D2033" rel="noreferrer">https://phabricator.kde.org/D2033</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>broulik, Plasma, sebas, oliverhenshaw<br /><strong>Cc: </strong>sebas, oliverhenshaw, graesslin, plasma-devel, jensreuterberg, abetts<br /></div>