<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102465/">http://git.reviewboard.kde.org/r/102465/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Looks very good from my point of view. The new files kitemlistkeyboardsearchmanager* seem to be missing in the diff though?
I think we should unit-test at least the new code in KFileItemModel, but I can take care of that later on (or we can do it together). The KItemListController code is worth unit-testing as well, but that class doesn't have a real testing infrastructure yet.</pre>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/102465/diff/5/?file=33117#file33117line136" style="color: black; font-weight: bold; text-decoration: underline;">dolphin/src/kitemviews/kitemlistcontroller.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">private slots:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">136</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="n">slotKeyboardActivationRequested</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span><span class="o">&</span> <span class="n">text</span><span class="p">,</span> <span class="k">const</span> <span class="n">bool</span> <span class="n">searchFromNextItem</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In general, one should avoid bool arguments unless the name of the function clearly tells what the argument is about (http://doc.qt.nokia.com/qq/qq13-apis.html#thebooleanparametertrap), but I think it's fine here because that slot is not going to be used outside this class, and making it an enum would require some more code.</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/102465/diff/5/?file=33118#file33118line243" style="color: black; font-weight: bold; text-decoration: underline;">dolphin/src/kitemviews/kitemlistcontroller.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void KItemListController::slotKeyboardActivationRequested(const QString& text, const bool searchFromNextItem)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">243</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">index</span> <span class="o">=</span> <span class="n">m_model</span><span class="o">-></span><span class="n">indexForKeyboardSearch</span><span class="p">(</span><span class="n">text</span><span class="p">,</span> <span class="n">currentIndex</span> <span class="o">+</span> <span class="mi">1</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">When I first looked at the patch, I wondered if it will work in the case that the last item is active and searchFromNextItem == true:
"currentIndex + 1" will be an invalid index then. Looking at the code in KFileItemModel showed me that it will work nonetheless, but I would change "currentIndex + 1" to "(currentIndex + 1) % m_model->count()" anyway.
That's just a matter of taste though, other people may have other opinions on this matter.</pre>
</div>
<br />
<p>- Frank</p>
<br />
<p>On August 27th, 2011, 11:22 p.m., Tirtha Chatterjee wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Base Apps and Peter Penz.</div>
<div>By Tirtha Chatterjee.</div>
<p style="color: grey;"><i>Updated Aug. 27, 2011, 11:22 p.m.</i></p>
<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;">This patch allows finding items by typing on the keyboard while the KItemListController is in focus. Timer and key queuing is handled by KItemListKeyboardManager, and search itself is done by reimplementing indexForKeyboardSearch(QString) from KItemModelBase. This implementation does not, at the moment, take care of the repetitive typing as suggested by Frank. I think we can implement that once this is in.
p.s. Not sure if line 213 in kitemlistcontroller.cpp is the best way to do it. Returning true does not work.
p.s. I feel the name KItemListKeyboardManager can be changed to KItemListKeyboardSearchManager, although a little too big.</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;">yes. tested and works.</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>dolphin/src/CMakeLists.txt <span style="color: grey">(31d3f89)</span></li>
<li>dolphin/src/kitemviews/kfileitemmodel.h <span style="color: grey">(654c7db)</span></li>
<li>dolphin/src/kitemviews/kfileitemmodel.cpp <span style="color: grey">(f36ab83)</span></li>
<li>dolphin/src/kitemviews/kitemlistcontroller.h <span style="color: grey">(134e116)</span></li>
<li>dolphin/src/kitemviews/kitemlistcontroller.cpp <span style="color: grey">(2e32880)</span></li>
<li>dolphin/src/kitemviews/kitemmodelbase.h <span style="color: grey">(4670469)</span></li>
<li>dolphin/src/kitemviews/kitemmodelbase.cpp <span style="color: grey">(fc604e7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102465/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>