<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 11th, 2012, 7:39 p.m., <b>Zack Rusin</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;">That's pretty bonkers. This class was never meant to be used like that. Holding an object in two threads is not going to work unless you make the entire communication synchronous effectively reducing all the multi-threading aspects to a "very complicated single thread". It just complicates this class.
The proper fix is to, instead of trying to synchronize one object owned by multiple threads, make the speller property of just the thread that does the spelling (or even just moveToThread it if you have to) and instead of calling setLanguage from another thread create a signal/slot combination between the parent thread and the speller thread and send a language change request by emitting a signal from the parent thread. </pre>
</blockquote>
<p>On September 18th, 2012, 6:02 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;">I understand that this was not in the original design for the class; if it were, the patch would not be necessary. However, the architecture of krunner renders the scheme you describe extremely difficult and messy to implement.
This is because krunner posts each set of input to a different thread, and the input contains both the word to be spelt and the language.
So, yes, we do actually need to be spelling multiple things in different threads. Which means we have to be careful to not change the language from under them. What you are suggesting won't work because all threads need to do the spelling. I can create an extra thread that does it, and then have it run an event loop fed from the helper threads, but that completely obviates the reason krunner uses multiple threads in the first place, and is in addition wildly over-complicated.
If you really don't like a mutex in this class, I can instead stick it into the krunner function directly. Thus the thing you don't like would be restricted to one obscure corner of plasma, rather than in kdelibs.</pre>
</blockquote>
<p>On September 19th, 2012, 5:18 p.m., <b>Zack Rusin</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;">No, what I'm saying is that none of the objects in this framework was designed to be used by multiple threads. It's not a matter of missing functionality, you're simply using it wrong. Sonnet shouldn't have to be solving problems of thread syncing in krunner. I don't really know what "not to change the language from under them" means, neither do I understand why connecting a change language signal from some master thread to multiple spell checking threads wouldn't work, but the solution to the problem is not try to shoehorn a paradigm for which a framework was never intended, but to fix the application to use the framework properly. In the worst case you can always create your MTSpeller class in your plugin, use sonner::speller inside it and add whatever locking your heart desires to it.</pre>
</blockquote>
<p>On September 19th, 2012, 5:57 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;">The trouble with connecting a change language signal from a master thread is that there isn't really a master thread.
There are several threads, each of which may in general be spell-checking different languages.
The most correct and elegant thing, really, is to construct a new speller object in each thread
and use that, but that would be a bit slow, I think, since a new thread is spawned on every key-press.
In any case, thanks for your review - I will try the new-object-creation thing, and if slow I will do as you suggest and move the locking to the krunner class. </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;">> there isn't really a master thread
how is the language changed? gui? -> master thread
block there until all running threads exited, update the lang, continue, ..., profit?
(you only have one lineedit, so i assume you effectively only check one lang at a time?)</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>