<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/123888/">https://git.reviewboard.kde.org/r/123888/</a>
</td>
</tr>
</table>
<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="https://git.reviewboard.kde.org/r/123888/diff/1/?file=370580#file370580line48" style="color: black; font-weight: bold; text-decoration: underline;">krunner/view.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">class ViewPrivate;</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">48</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QStringList</span> <span class="n">history</span><span class="p">()</span> <span class="k">const</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">const QStringList &history() const;</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Saves a copy and prevents users from manipulating this object.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You do manipulate this, but that in the QML and you put it in a "var history" which probably copies it also in a javascript object?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or i'm wrong... In that case you can ignore this comment :)</p></pre>
</div>
</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="https://git.reviewboard.kde.org/r/123888/diff/1/?file=370582#file370582line98" style="color: black; font-weight: bold; text-decoration: underline;">lookandfeel/contents/runcommand/RunCommand.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">97</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kd">var</span> <span class="nx">history</span> <span class="o">=</span> <span class="nx">runnerWindow</span><span class="p">.</span><span class="nx">history</span></pre></td>
</tr>
<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">98</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="nx">history</span><span class="p">.</span><span class="nx">indexOf</span><span class="p">(</span><span class="nx">queryString</span><span class="p">)</span> <span class="o">===</span> <span class="o">-</span><span class="mi">1</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
<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">99</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nx">history</span><span class="p">.</span><span class="nx">unshift</span><span class="p">(</span><span class="nx">queryString</span><span class="p">)</span></pre></td>
</tr>
<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">100</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nx">runnerWindow</span><span class="p">.</span><span class="nx">history</span> <span class="o">=</span> <span class="nx">history</span></pre></td>
</tr>
<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">101</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Hmm, this looks interesting.
Suppose my history looks like this:
- plasmashell
- konsole
- dolphin</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">now imagine i type "plasma". It should then show:
- plasmashell</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thus far it probably works fine.
Now imagine i clear whatever i typed (so a clean inputfield). All without closing krunner! When i clear it the history probably shows one entry:
- plasmashell</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What happened to the other two?
Well, you've rewritten history when i typed a search query.
And because you call "runnerWindow.history = history" it will call the C++ setHistory method which will overwrite your history value in the config.. Ouch!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I could be wrong, but this is how i think it behaves when reading the code.
I think you should use something with a tree. A radix tree would be the easiest, fastest and cheapest in memory, but that doesn't exist in Qt nor the C++ STL.</p></pre>
</div>
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm curious (since i don't see that code here). Where do you add items to your history?</p></pre>
<p>- Mark Gaiser</p>
<br />
<p>On mei 23rd, 2015, 10:01 p.m. UTC, Kai Uwe Broulik 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 Plasma, KDE Usability and Vishesh Handa.</div>
<div>By Kai Uwe Broulik.</div>
<p style="color: grey;"><i>Updated mei 23, 2015, 10:01 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=335731">335731</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</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;">This turns KRunner's TextField into an editable ComboBox to provide a history.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">When a result is invoked, the query string is prepended to the history, query strings are only added once. ComboBox provides letter-by-letter auto completion.</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;">Somehow I have a feeling it doesn't always save the history or nukes it at times. It also has some shortcomings due to ComboBox:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1.) You cannot use the arrow keys to cycle between entries (when the popup's not opened) because arrow keys navigate through results
2.) forceActiveFocus() on the ComboBox will not activate the embedded TextField - when you had opened the popup there's a slight chance the input field won't get focussed I'll prepare a Qt patch for this.
3.) Before Qt 5.4.2 (not sure if my patch ended up in 5.4.1) pressing space in the edit combobox will open the popup, not insert a space (nasty show stopper)
4.) Plasma's edtiable ComboBox looks a bit strange imho
5.) Plasma's editable ComboBox doesn't support clearButtonShown
6.) Plasma's ComboBox has strange bullets and margins in it, that's probably a bug in Plasma Style (need to look what Desktop style does differently from us)
7.) ComboBox doesn't have a cursorPosition, I'll prepare a Qt patch for this.</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>krunner/view.h <span style="color: grey">(1ad5075)</span></li>
<li>krunner/view.cpp <span style="color: grey">(8640e1d)</span></li>
<li>lookandfeel/contents/runcommand/RunCommand.qml <span style="color: grey">(4c6eb30)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123888/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/05/23/7ad7e5eb-4874-4f9f-9796-738fa2ac9ed5__krunnerhistory.png">History popup</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/05/23/18714844-ef28-4cdd-af00-e6685caece9b__krunnerautocompletion.png">Auto completion</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>