Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-26 Thread Simeon Bird 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/
---

(Updated Oct. 26, 2012, 11:26 p.m.)


Review request for Plasma, Sebastian Kügler and Matthias Fuchs.


Changes
---

Remove workaround for now-fixed bug and load language data in prepare()


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 (updated)
-

  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

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-26 Thread Commit Hook 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/#review20981
---


This review has been submitted with commit 
124e35885b8cd1b593b7b83a070bd0bdb5758661 by Simeon Bird to branch KDE/4.9.

- Commit Hook


On Oct. 26, 2012, 11:26 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106244/
> ---
> 
> (Updated Oct. 26, 2012, 11:26 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-26 Thread Simeon Bird 


> On Oct. 25, 2012, 7:20 p.m., Aaron J. Seigo wrote:
> > runners/spellchecker/spellcheck.cpp, lines 48-53
> > 
> >
> > 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-26 Thread Simeon Bird 


> On Oct. 25, 2012, 9:25 a.m., Aaron J. Seigo wrote:
> > just some whitespace and code style issues to address; the patch logic 
> > itself looks good.

Thanks for the review Aaron! All fixed up in the latest version.


> On Oct. 25, 2012, 9:25 a.m., Aaron J. Seigo wrote:
> > runners/spellchecker/spellcheck.cpp, line 225
> > 
> >
> > 1_speller is a really odd var name, and doesn't really say much about 
> > what it does. just speller should be good enough.

l was for "local" and also "a letter next to m" :) 
I changed it to just speller.


- Simeon


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/#review20838
---


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-26 Thread Simeon Bird 

---
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.


Changes
---

Fix review comments.


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 (updated)
-

  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

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-25 Thread Aaron J. Seigo 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/#review20913
---

Ship it!


i've fixed the bug being worked around as noted below, but otherwise looks 
clean now ... cheers :)


runners/spellchecker/spellcheck.cpp


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.


- Aaron J. Seigo


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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-25 Thread Aaron J. Seigo 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/#review20838
---


just some whitespace and code style issues to address; the patch logic itself 
looks good.


runners/spellchecker/spellcheck.h


m_spellers and m_spellLock



runners/spellchecker/spellcheck.h


this should be above the data members



runners/spellchecker/spellcheck.cpp


coding style: comments go inside the branch, braces with 'else if' on the 
same line.. e.g.:

} else if {
// if the family is english, default to en_US



runners/spellchecker/spellcheck.cpp


see above :)



runners/spellchecker/spellcheck.cpp


see above :)



runners/spellchecker/spellcheck.cpp


1_speller is a really odd var name, and doesn't really say much about what 
it does. just speller should be good enough.



runners/spellchecker/spellcheck.cpp


missing whitespace: if (1_speller->isValid()) {


- Aaron J. Seigo


On Sept. 21, 2012, 1:51 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106244/
> ---
> 
> (Updated Sept. 21, 2012, 1:51 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-24 Thread Simeon Bird 


> On Oct. 11, 2012, 12:01 a.m., Simeon Bird wrote:
> > Does anyone have any objections? Given that the current behaviour is 'crash 
> > krunner when invoked' I feel I can't do much damage with this...

Ok, it seems no-one has any objections (or indeed any interest). So pending any 
comments, I'll commit the changes this weekend! 
So, please, if you have anything to say about it, now is a good time.


- Simeon


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/#review20176
---


On Sept. 21, 2012, 1:51 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106244/
> ---
> 
> (Updated Sept. 21, 2012, 1:51 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-10-11 Thread Simeon Bird 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/#review20176
---


Does anyone have any objections? Given that the current behaviour is 'crash 
krunner when invoked' I feel I can't do much damage with this...

- Simeon Bird


On Sept. 21, 2012, 1:51 p.m., Simeon Bird wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106244/
> ---
> 
> (Updated Sept. 21, 2012, 1:51 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
> 
>

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Re: Review Request: Fix KRunner's 'spell in foreign language' feature

2012-09-21 Thread Simeon Bird 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/
---

(Updated Sept. 21, 2012, 1:51 p.m.)


Review request for Plasma, Sebastian Kügler and Matthias Fuchs.


Changes
---

Updated version; this fixes the segfaults on its own, by not calling 
setLanguage at all. Thus it does not need changes to kdelibs.


Description (updated)
---

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 (updated)
-

  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

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Review Request: Fix KRunner's 'spell in foreign language' feature

2012-08-28 Thread Simeon Bird 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106244/
---

Review request for Plasma.


Description
---

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. 
Review 106242 fixes the segfaults, this commit fixes 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. 
Unfortunately, the response of the spell-checker to being fed an invalid 
language was to crash on the next query (this is fixed in 106242). Furthermore, 
the spell-checker expected languages like 'fr_FR' or 'French (France)' which 
the user was vanishingly unlikely to type in correctly. 

So what this does is to only call setLanguage when necessary (just in case I 
missed something in 106242), 
and then only after we have turned the query we are fed into a valid 
spell-checker language. 
This is a little bit tricky, because in particular "spell english" is going to 
mean different things to different people. 

My approach was to try and find the main language of the group: so 'french' 
gets you fr_FR. This works ok for most things, but not really for english. For 
that I default to US english, unless UK english is installed. Probably one 
should special case other english variants as well, but I haven't because I am 
unsure what spellings are in common use in practice. 

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.


Diffs
-

  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

___
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel