<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127437/">https://git.reviewboard.kde.org/r/127437/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 21st, 2016, 6:18 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/127437/diff/1/?file=453655#file453655line1445" style="color: black; font-weight: bold; text-decoration: underline;">src/widgets/kurlcompletion.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1393</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">userListThread</span> <span class="o">=</span> <span class="mi">0</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1362</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">q</span><span class="o">-></span><span class="n">setItems</span><span class="p">(</span><span class="n">matches</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">this looks wrong, shouldn't it be insertItems (or ::addMatches which does the same thing)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">we'll get this method called twice, once for each thread that takes longer than the initial timeout, setItems will discard matches from the first thread.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Only <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">one</em> of the threads is running. We are <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">either</em> running user completion ("~") or directory completion.
Note how the previous code was calling stop+clear, as well.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I could have kept clear+addMatches, it would make no difference, it's the same as setItems (AFAICS).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I just don't call stop() anymore, because now stop() is "asking the thread to finish (abort)", while this slot is called once the thread has finished (or just before it really finishes, but at least it's about to, it doesn't need to be requested to stop).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe an "else" between the last two if()s in the slot would make it clearer?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks for the review, I'm open for more suggestions :-)</p></pre>
<br />
<p>- David</p>
<br />
<p>On March 20th, 2016, 2:46 p.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated March 20, 2016, 2:46 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">* Major race with other threads due to using QDir::setCurrent()
* Race conditions on m_terminationRequested and m_matches
* Race with previous completion thread when its posted event arrives after cancelling
* Cancellation code spread out in many methods and never done fully correctly
* isRunning() was missing one of the two threads, making unit test fail in valgrind
* Fix the rarely-hit code path where the thread isn't finished after 200ms
- the current search string was lost because finished() wasn't called
- the matches were not used, in case of user-completion (AFAICS)
- changing the search string while the thread was running could lead to the old search
string still being used for completion
(the misnamed finished() wasn't called, so KCompletion didn't get the new string)
=> added a variant of the unittest which doesn't wait for the thread initially</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">+ Simplify code using signal/slots rather than a custom event
+ Simplify code using enum rather than casting to/from int</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Most of these bugs are from 2004 (e.g. commit ec83c408619e3) ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">REVIEW: 127437</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hit a crash in DirectoryListThread when playing with kopenwithtest.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kurlcompletiontest extended, kurlcompletion-nowait added, both pass, also in valgrind (different timings).</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/CMakeLists.txt <span style="color: grey">(4dff0a24d31897a9641a70017bd87e33415cef14)</span></li>
<li>autotests/kurlcompletiontest.cpp <span style="color: grey">(eef39ff17940fadcb9492e5e7092070c976310d4)</span></li>
<li>src/widgets/CMakeLists.txt <span style="color: grey">(87dac50b377f6ea4c3cd39e9afa37a4680aecf31)</span></li>
<li>src/widgets/kurlcompletion.h <span style="color: grey">(14fda22a0e08bcf2da30e19fed577c8bda64a4ca)</span></li>
<li>src/widgets/kurlcompletion.cpp <span style="color: grey">(1dc8f4898fb78d6f49e687462446007a10305f98)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127437/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>