<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/106242/">http://git.reviewboard.kde.org/r/106242/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 6th, 2012, 12:29 p.m., <b>David Faure</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;">A number of comments on the implementation, but also a more general comment: does it even make sense to use a Speller in multiple threads, and to change the language from one thread while another one is using the speller for spell-checking? It sounds like, even if a mutex/lock can prevent crashes, this is a weird thing to do anyway, since you have no idea at which point the spell-checking will switch to another language... could happen in the very middle of a sentence...
Maybe it would make more sense to "post" the language change operation to the spell-checking thread using the same mechanism as the one used to "post" spellchecking requests to it? (Disclaimer: I know nothing of the krunner architecture).</pre>
</blockquote>
<p>On September 9th, 2012, 10:05 p.m., <b>Simeon Bird</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;">Thanks for the review.
I agree that this is a slightly weird thing to do, but, as you surmise, it was the only way I could think of to make the feature work properly.
As far as I understand it (from reading the documentation), the way krunner works is to call Plasma::AbstractRunner::match in a new thread for every runner every time input is entered.
So if I enter:
"spell hello"
I will be called with 's' 'sp' 'spe'...and so on until I get the whole match, without waiting for the previous threads to return. Thus match has to be thread-safe.
The feature I'm trying to fix here is to be able to enter "spell french bonjour" and have it spell something.
The runner is responsible for parsing the input and checking whether it should do anything else.
So we don't actually post spellchecking requests, we just post some input, and detect that we should spell-check, and change the language, by parsing strings. So I couldn't see a way to do this except to call Sonnet::Speller::setLanguage() in Plasma::AbstractRunner::match,
which meant that for it to work I had to make match thread-safe.
Sorry. I'm open to suggestions if you have a better idea.
I've fixed your other comments in v2 of the diff, let me know if there is anything else.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">would it not make more sense to just kill the pending (and now worthless?) thread instead (no idea how expensive this spellchecking is to be threaded) to not waste cpu on a result that will be ignored anyway?</pre>
<br />
<p>- Thomas</p>
<br />
<p>On September 9th, 2012, 10:06 p.m., Simeon Bird 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 kdelibs and Plasma.</div>
<div>By Simeon Bird.</div>
<p style="color: grey;"><i>Updated Sept. 9, 2012, 10:06 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;">Krunner's spellcheck plugin has been pretty broken since bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it called Sonnet::Speller::setLanguage every time the spellchecker was invoked, which was (very much) not thread-safe.
This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all access to the internal dict pointer using QReadWriteLock.
A related review request is 106244, which adds more fixes to the spellcheck feature.</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;">Compiled, installed, used for a week or so, spellchecked a bunch of things.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=264779">264779</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=303831">303831</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdecore/sonnet/speller.cpp <span style="color: grey">(b19e74d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106242/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>