<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/111912/">http://git.reviewboard.kde.org/r/111912/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 9th, 2013, 1:49 p.m. CEST, <b>Kevin Ottens</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="http://git.reviewboard.kde.org/r/111912/diff/4/?file=177521#file177521line237" style="color: black; font-weight: bold; text-decoration: underline;">tier1/sonnet/src/ui/highlighter.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">void Highlighter::slotAutoDetection()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">237</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">I still don't see what the changes in this file have to do with the rest of the patch. Could you explain it to me?
Beside it looks odd that it turns a singleShot timer into potentially a recurring one (it's not created single shot and the calls to setSingleShot are scattered everywhere, there's a non-null probability of things going sour here).</pre>
</blockquote>
<p>On August 12th, 2013, 6:14 p.m. CEST, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This change is here to unbreak the language change: without it, changing languages using the dictionary combo box in the test program does nothing.
Apologies for the fat review, it is actually made of 11 commits. I just pushed them here: http://agateau.com/tmp/sonnet
rehighlightRequest is set to be a single shot timer in line 122. Would probably be cleaner to remove all other calls to setSingleShot().</pre>
</blockquote>
<p>On August 13th, 2013, 8:49 a.m. CEST, <b>Kevin Ottens</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It's fat *and* doesn't match the review description. :-)
Now seeing how you structured your patches behind the scene, I think some of them could have been squashed together. In any case, please make this kind of things clearer in the future, it was highly confusing to me (of course can happen easily since reviews don't map 1:1 with the commits).
I think it can go in *except* what touches sonnet/src/ui/highlighter.cpp, please open a separate review for that part since it's obviously a different thing (fixing a bug) and it still needs work.</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;">OK, I am going to split it.</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On August 9th, 2013, 12:03 a.m. CEST, Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated Aug. 9, 2013, 12:03 a.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 introduces a new class: Sonnet::TextEditInstaller. It makes it easy to add spellcheck support to a QTextEdit.
Spellcheck support means two things:
1. Install Sonnet::Highlighter to highlight spelling error.
2. Intercept context menu to replace it with a list of suggestions when user right-clicks on a misspelled word.
Minimal usage is simple: create a new TextEditInstaller, passing it the QTextEdit as argument. The patch adds a test_textedit executable which demonstrates the class.
I am posting it early to get feedback on the API and the class name, I am not completely happy with either.
PS: This patch includes my plugin fixes [1], since it is useless without them.
[1]: https://git.reviewboard.kde.org/r/111895/</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;">Tested with test_textedit.</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>tier1/sonnet/src/ui/CMakeLists.txt <span style="color: grey">(723d8f3)</span></li>
<li>tier1/sonnet/src/ui/highlighter.h <span style="color: grey">(c303db1)</span></li>
<li>tier1/sonnet/src/ui/highlighter.cpp <span style="color: grey">(5c6a590)</span></li>
<li>tier1/sonnet/src/ui/spellcheckdecorator.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/sonnet/src/ui/spellcheckdecorator.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/sonnet/tests/CMakeLists.txt <span style="color: grey">(6e0e450)</span></li>
<li>tier1/sonnet/tests/test_textedit.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111912/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>