[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: Fix KRunner's 'spell in foreign language' feature
From: "Simeon Bird" <bladud () gmail ! com>
Date: 2012-10-25 20:04:34
Message-ID: 20121025200434.13655.52467 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Oct. 25, 2012, 7:20 p.m., Aaron J. Seigo wrote:
> > runners/spellchecker/spellcheck.cpp, lines 48-53
> > <http://git.reviewboard.kde.org/r/106244/diff/3/?file=92352#file92352line48>
> >
> > that would actually be a bug. prepare() should _always_ be called...
> >
> > i see why this is happening: a bug in RunnerManagerPrivate::loadInstalledRunner. \
> > this is now fixed in kdelibs rev 28a310b which should make this code unnecessary.
Nice, thanks! Is it allowed to backport the kdelibs fix to the 4.9 branch?
If so, I'll do a final run of testing with the patched kdelibs and removed workaround \
this evening, just in case... and if it looks good I'll commit it.
- Simeon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/#review20913
-----------------------------------------------------------
On Oct. 25, 2012, 5:24 p.m., Simeon Bird wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106244/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 5:24 p.m.)
>
>
> Review request for Plasma, Sebastian Kügler and Matthias Fuchs.
>
>
> Description
> -------
>
> Krunner's spellcheck plugin has been broken since \
> bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it called \
> Sonnet::Speller::setLanguage every time the spellchecker was invoked, which was not \
> thread-safe. This diff fixes the segfaults, and the feature, which I understand to \
> be, basically, the ability to type 'spell french bonjour' and have it check the \
> spelling.
> The current code simply calls setLanguage on the second term in the search query, \
> and then checks whether the resulting dictionary object is valid. The spell-checker \
> expects languages like 'fr_FR' or 'French (France)' which the user was unlikely to \
> type in correctly (at least, I never figured it out until I read the source).
> Instead, this patch create a new spell-check object (the creation is guarded by a \
> mutex) when a new language is used (thus never needing to call setLanguage). Future \
> queries use the already created speller for the new language, and spellers are \
> deleted on the teardown() signal.
> We make a map between the speller codes and simple natural language language names \
> in init(); this is a little bit tricky, because languages have sub-variants. My \
> approach was to try and find the main language of the group: so 'french' gets you \
> fr_FR. For english I defaulted to US english.
> I have not tested this spelling an asian language as I don't speak one.
>
> I have not implemented 'spell canadian french' or similar. If you want a specific \
> sublanguage you have to type in the language code directly.
>
> This addresses bugs 264779 and 303831.
> http://bugs.kde.org/show_bug.cgi?id=264779
> http://bugs.kde.org/show_bug.cgi?id=303831
>
>
> Diffs
> -----
>
> runners/spellchecker/spellcheck.h 492c370
> runners/spellchecker/spellcheck.cpp 672732d
>
> Diff: http://git.reviewboard.kde.org/r/106244/diff/
>
>
> Testing
> -------
>
> Compiled, installed, ran for a week and spell-checked a bunch of things in European \
> languages.
>
> Thanks,
>
> Simeon Bird
>
>
[Attachment #5 (text/html)]
<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/106244/">http://git.reviewboard.kde.org/r/106244/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On October 25th, 2012, 7:20 p.m., <b>Aaron J. \
Seigo</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/106244/diff/3/?file=92352#file92352line48" \
style="color: black; font-weight: bold; text-decoration: \
underline;">runners/spellchecker/spellcheck.cpp</a> <span style="font-weight: \
normal;">
(Diff revision 3)
</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; \
">SpellCheckRunner::~SpellCheckRunner()</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="c1">//Also, it seems that under some circumstances init can be called without \
calling</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">49</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="c1">//prepare afterwards, eg, if disabling a runner and then adding it \
again.</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">50</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="c1">//So, just in case, construct a default speller here.</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">51</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="o">!</span><span \
class="n">m_spellers</span><span class="p">.</span><span \
class="n">contains</span><span class="p">(</span><span \
class="s">""</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">52</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">m_spellers</span><span class="p">[</span><span \
class="s">""</span><span class="p">]</span> <span class="o">=</span> <span \
class="n">QSharedPointer</span><span class="o"><</span><span \
class="n">Sonnet</span><span class="o">::</span><span class="n">Speller</span><span \
class="o">></span> <span class="p">(</span><span class="k">new</span> <span \
class="n">Sonnet</span><span class="o">::</span><span class="n">Speller</span><span \
class="p">(</span><span class="s">""</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">53</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>
<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 would actually be a \
bug. prepare() should _always_ be called...
i see why this is happening: a bug in RunnerManagerPrivate::loadInstalledRunner. this \
is now fixed in kdelibs rev 28a310b which should make this code unnecessary.</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;">Nice, \
thanks! Is it allowed to backport the kdelibs fix to the 4.9 branch?
If so, I'll do a final run of testing with the patched kdelibs and removed \
workaround this evening, just in case... and if it looks good I'll commit it.
</pre>
<br />
<p>- Simeon</p>
<br />
<p>On October 25th, 2012, 5:24 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 Plasma, Sebastian Kügler and Matthias Fuchs.</div>
<div>By Simeon Bird.</div>
<p style="color: grey;"><i>Updated Oct. 25, 2012, 5:24 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 broken since \
bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it called \
Sonnet::Speller::setLanguage every time the spellchecker was invoked, which was not \
thread-safe. This diff fixes the segfaults, and the feature, which I understand to \
be, basically, the ability to type 'spell french bonjour' and have it check \
the spelling.
The current code simply calls setLanguage on the second term in the search query, and \
then checks whether the resulting dictionary object is valid. The spell-checker \
expects languages like 'fr_FR' or 'French (France)' which the user \
was unlikely to type in correctly (at least, I never figured it out until I read the \
source).
Instead, this patch create a new spell-check object (the creation is guarded by a \
mutex) when a new language is used (thus never needing to call setLanguage). Future \
queries use the already created speller for the new language, and spellers are \
deleted on the teardown() signal.
We make a map between the speller codes and simple natural language language names in \
init(); this is a little bit tricky, because languages have sub-variants. My approach \
was to try and find the main language of the group: so 'french' gets you \
fr_FR. For english I defaulted to US english.
I have not tested this spelling an asian language as I don't speak one.
I have not implemented 'spell canadian french' or similar. If you want a \
specific sublanguage you have to type in the language code directly.</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, ran for a week and spell-checked a bunch of things \
in European languages.</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>runners/spellchecker/spellcheck.h <span style="color: \
grey">(492c370)</span></li>
<li>runners/spellchecker/spellcheck.cpp <span style="color: \
grey">(672732d)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106244/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic