Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner
On Sept. 6, 2012, 12:29 p.m., David Faure wrote: 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). Simeon Bird wrote: 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. Thomas Lübking wrote: 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? David Faure wrote: Forcefully killing threads (QThread::terminate()) is a big no-no, it leaks memory, potentially worse: it can leave a mutex locked, so any further thread trying to lock it, will hang for ever. The only way to terminate a thread is to ask it nicely to terminate from within (i.e. to exit run()), which means it has to finish what it's currently doing, first, before it looks up for the next command or posted event which tells it to quit. Thomas Lübking wrote: That is generally true and i admit to not know how the plasma spell checker is implemented (or even where ;-), but would expect some sort of ro non-joinable threading, operating on shared data (as the only purpose seems to prevent blocking the main event loop - not running two checks at the same time and merge their results) ... or does QThread alone bring structures to forbid such approach (The API doc does not mention inevitable leaks for QThread itself on ::terminate())? (PS: I just start to wonder whether the speller threads are then tagged to catch 2,1 exits...) I don't entirely follow what you mean in the last comment; what do you mean 'tagged to catch 2,1 exits'? The spellchecking runner is, I think, very cheap compared to some of the other runners (eg, the nepomuk search), all of which are called at once. But, be that as it may, the spell-check has no control over the threading model of krunner, which it just inherits from Plasma::AbstractRunner, and changing that model for everything seems a bit overkill for this, even if we understood fully how it works (I certainly don't; I just took the documentation at face value, which may or may not be a good idea). A reasonable alternative, if you don't like doing the commit in principle (the mutex might well slow down the spell-check), is to just remove the problematic feature (which has never worked anyway). If krunner only spell-checked words in the default dictionary, we wouldn't need to call setLanguage(), and everything would be fine, as it was before 2010. - Simeon --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/#review18623 --- On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/
Broken build of KDE Base Apps and unauthorized increase in dependency
Hi Dawit, It has come to my attention that since commit 603a93268efb9d09f8c6255907f46928c651fdbd to kde-baseapps master, the build for it has been broken. This is due to it depending on new features, present only in the KDE/4.9 branch. This is problematic because: - kde-baseapps depends upon a minimum of 4.7.97 (ie. KDE 4.8) which obviously is no longer the case. It depends upon unreleased code. - This feature will not be in KDE 4.10 at the moment, because the feature was added to KDE/4.9 instead of KDE/4.10 as it should have been. Please see http://build.kde.org/view/FAILED/job/kde-baseapps_master/8/console for further information. Regards, Ben Cooksley
kde-baseapps dependency on kdelibs changed in 4.9 branch?
It seems that the set(KDE_MIN_VERSION 4.7.97) that kde-baseapps mentions as required kdelibs version is not true anymore[1] and you now need unreleased kdelibs KDE/4.9 to build kde-baseapps KDE/4.9 Are we OK with such a dependency change? Cheers, Albert [1] http://quickgit.kde.org/index.php?p=kde-baseapps.gita=commith=cb79ee6a881e2b4418bccc22480e3e269e5b0af9
Re: kde-baseapps dependency on kdelibs changed in 4.9 branch?
Hi, 2012/9/11 Albert Astals Cid: It seems that the set(KDE_MIN_VERSION 4.7.97) that kde-baseapps mentions as required kdelibs version is not true anymore[1] and you now need unreleased kdelibs KDE/4.9 to build kde-baseapps KDE/4.9 Are we OK with such a dependency change? Cheers, Albert [1] http://quickgit.kde.org/index.php?p=kde-baseapps.gita=commith=cb79ee6a881e2b4418bccc22480e3e269e5b0af9 dependency changes in stable branches of kde-baseapps have already been done in the past, see, e.g., [1], so I thought that Dawit's commit, which fixes a regression in Konqueror's mime type filter, is OK. However, the version check in the CMake file should be updated, of course, and the kdelibs change required to build the current kde-baseapps should probably also be merged into the KDE/4.10 and master branches ASAP (I can't do it now because I'm not at my development machine) to fix the build for people using these branches. Best regards, Frank [1] https://projects.kde.org/projects/kde/kde-baseapps/repository/revisions/4caa285c065ea362ae52d93a567680513d8beb2a
Re: kde-baseapps dependency on kdelibs changed in 4.9 branch?
El Dimarts, 11 de setembre de 2012, a les 12:49:47, Frank Reininghaus va escriure: Hi, 2012/9/11 Albert Astals Cid: It seems that the set(KDE_MIN_VERSION 4.7.97) that kde-baseapps mentions as required kdelibs version is not true anymore[1] and you now need unreleased kdelibs KDE/4.9 to build kde-baseapps KDE/4.9 Are we OK with such a dependency change? Cheers, Albert [1] http://quickgit.kde.org/index.php?p=kde-baseapps.gita=commith=cb79ee6a8 81e2b4418bccc22480e3e269e5b0af9 dependency changes in stable branches of kde-baseapps have already been done in the past, see, e.g., [1], so I thought that Dawit's commit, which fixes a regression in Konqueror's mime type filter, is OK. I was just asking, not saying it was wrong ;-) However, the version check in the CMake file should be updated Well, that can't be done (right now) as we have not marked kdelibs/4.9 as 4.9.2 yet, someone needs to remember to do it when we release 4.9.2 Cheers, Albert , of course, and the kdelibs change required to build the current kde-baseapps should probably also be merged into the KDE/4.10 and master branches ASAP (I can't do it now because I'm not at my development machine) to fix the build for people using these branches. Best regards, Frank [1] https://projects.kde.org/projects/kde/kde-baseapps/repository/revisions/4ca a285c065ea362ae52d93a567680513d8beb2a
Re: Broken build of KDE Base Apps and unauthorized increase in dependency
On Tue, Sep 11, 2012 at 6:22 AM, Ben Cooksley bcooks...@kde.org wrote: Hi Dawit, It has come to my attention that since commit 603a93268efb9d09f8c6255907f46928c651fdbd to kde-baseapps master, the build for it has been broken. This is due to it depending on new features, present only in the KDE/4.9 branch. That is only a temporary situation. All the changes in the 4.9 branch are periodically merged into the 4.10 branch by David, but I guess that does not resolve this issue. This is problematic because: - kde-baseapps depends upon a minimum of 4.7.97 (ie. KDE 4.8) which obviously is no longer the case. It depends upon unreleased code. Ahh... I was not aware of this was a requirement though I still do not understand why we have started insisting on forward compatibility. Regardless, I will fix this since it is only a matter of wrapping the code with #ifdef. - This feature will not be in KDE 4.10 at the moment, because the feature was added to KDE/4.9 instead of KDE/4.10 as it should have been. It is not a new feature. It is a bug fix. It is needed to fix the directory filter plugin for Konqueror.
Re: kde-baseapps dependency on kdelibs changed in 4.9 branch?
On Tue, Sep 11, 2012 at 6:27 AM, Albert Astals Cid aa...@kde.org wrote: It seems that the set(KDE_MIN_VERSION 4.7.97) Was that done intentionally or was it forgotten to be updated for the KDE 4.9 release ? If the former, then have we now started guaranteeing forward compatibility too ? that kde-baseapps mentions as required kdelibs version is not true anymore[1] and you now need unreleased kdelibs KDE/4.9 to build kde-baseapps KDE/4.9 Are we OK with such a dependency change? AFAIC, no such forward compatibility should have been allowed in the first place. Otherwise, it would be impossible to fix bugs and regressions that require the addition of new functionality in kdelibs. And we always have had to do such fixing from time to time.
Appmenu support for KDE 4.10
Hello, since last mail, many changes. Kded-appmenu: https://projects.kde.org/projects/kdereview/kded-appmenu - DBus interface allowing KWin to ask for applications menu to popup - New Top menubar: * Auto hide with glow effect for easy access * Multihead: menubar follow application window screen allowing usage in multihead kde-workspace patch: https://git.reviewboard.kde.org/r/104344/ Configuration has been moved to kcm_style. libkappmenu: http://kde-apps.org/content/show.php/libkappmenu?content=153883 http://kde-apps.org/content/show.php/plasma-widget-kappmenubar Library allowing applications to get a QMenu from kded-appmenu. Aurelien Gateau plasmoid patched to use this library and works along side kded-appmenu. Not ready IMO for KDE 4.10 due to problems pointed by Alejandro Fiestas Olivares in previous mails. But this is really not needed to have appmenu support in KDE available. Two solutions: - Add support for another menu model access in libdbusmenu-qt - If we fail: add this model to this libkappmenu branch based on krunner- appmenu code (proof of concept, segfaulting version): https://gitorious.org/libkappmenu/libkappmenu/commits/dbusmenu Here how it looks: http://www.youtube.com/watch?v=cID1KR_dlIs regards, -- Cédric
Re: kde-baseapps dependency on kdelibs changed in 4.9 branch?
On Tue, Sep 11, 2012 at 10:08 AM, Albert Astals Cid aa...@kde.org wrote: El Dimarts, 11 de setembre de 2012, a les 09:51:22, Dawit A va escriure: On Tue, Sep 11, 2012 at 6:27 AM, Albert Astals Cid aa...@kde.org wrote: It seems that the set(KDE_MIN_VERSION 4.7.97) Was that done intentionally or was it forgotten to be updated for the KDE 4.9 release ? If the former, then have we now started guaranteeing forward compatibility too ? It was done on purpose. Why require newer kdelibs when you don't need them. Because you really don't know that you need them until you actually do ? Like I said bug fixes and regressions sometimes require changes in kdelibs. If we purposefully shackle ourselves to forward compatibility then such bugs and regressions, no matter how bad, cannot be fixed until the next major release. If that is acceptable for the sake of being forward compatible, then that is fine. However, I really do not see the point of it. Why would anyone want to use new version of applications with the older version of the required libraries ?
Re: kde-baseapps dependency on kdelibs changed in 4.9 branch?
Hi, On Tue, Sep 11, 2012 at 4:42 PM, Dawit A ada...@kde.org wrote: On Tue, Sep 11, 2012 at 10:08 AM, Albert Astals Cid aa...@kde.org wrote: El Dimarts, 11 de setembre de 2012, a les 09:51:22, Dawit A va escriure: On Tue, Sep 11, 2012 at 6:27 AM, Albert Astals Cid aa...@kde.org wrote: It seems that the set(KDE_MIN_VERSION 4.7.97) Was that done intentionally or was it forgotten to be updated for the KDE 4.9 release ? If the former, then have we now started guaranteeing forward compatibility too ? It was done on purpose. Why require newer kdelibs when you don't need them. Because you really don't know that you need them until you actually do ? Like I said bug fixes and regressions sometimes require changes in kdelibs. If we purposefully shackle ourselves to forward compatibility then such bugs and regressions, no matter how bad, cannot be fixed until the next major release. If that is acceptable for the sake of being forward compatible, then that is fine. However, I really do not see the point of it. Why would anyone want to use new version of applications with the older version of the required libraries ? Because upgrading libs is not always easily possible. Thats not so much an issue with kde-baseapps since those are really basic things and probably only few people build those without also building kdelibs. For any other apps people might be using binary packages for the base stuff which are one or even two releases behind current stable and they'd still like to work on apps from master. And I think its important to support these people if possible. So keeping backwards compatibility. Also this is not about being forward compatible, but having apps be backwards compatible with older kdelibs. Forward compatibility of kdelibs is mostly given anyway due to the promises of keeping BC and SC of existing API and only adding (and hopefully also keeping existing behaviour). Andreas
Re: Appmenu support for KDE 4.10
On Tuesday 11 September 2012 15:24:44 Cedric Bellegarde wrote: Hello, since last mail, many changes. Kded-appmenu: https://projects.kde.org/projects/kdereview/kded-appmenu - DBus interface allowing KWin to ask for applications menu to popup - New Top menubar: * Auto hide with glow effect for easy access * Multihead: menubar follow application window screen allowing usage in multihead just to check, do you mean multi-screen (xrandr) or really multi-head (one X server per screen)? signature.asc Description: This is a digitally signed message part.
Re: kde-baseapps dependency on kdelibs changed in 4.9 branch?
On Tue, September 11, 2012 4:33 pm, Andreas Pakulat wrote: Hi, On Tue, Sep 11, 2012 at 4:42 PM, Dawit A ada...@kde.org wrote: On Tue, Sep 11, 2012 at 10:08 AM, Albert Astals Cid aa...@kde.org wrote: El Dimarts, 11 de setembre de 2012, a les 09:51:22, Dawit A va escriure: On Tue, Sep 11, 2012 at 6:27 AM, Albert Astals Cid aa...@kde.org wrote: It seems that the set(KDE_MIN_VERSION 4.7.97) Was that done intentionally or was it forgotten to be updated for the KDE 4.9 release ? If the former, then have we now started guaranteeing forward compatibility too ? It was done on purpose. Why require newer kdelibs when you don't need them. Because you really don't know that you need them until you actually do ? Like I said bug fixes and regressions sometimes require changes in kdelibs. If we purposefully shackle ourselves to forward compatibility then such bugs and regressions, no matter how bad, cannot be fixed until the next major release. If that is acceptable for the sake of being forward compatible, then that is fine. However, I really do not see the point of it. Why would anyone want to use new version of applications with the older version of the required libraries ? Because upgrading libs is not always easily possible. Thats not so much an issue with kde-baseapps since those are really basic things and probably only few people build those without also building kdelibs. For any other apps people might be using binary packages for the base stuff which are one or even two releases behind current stable and they'd still like to work on apps from master. And I think its important to support these people if possible. So keeping backwards compatibility. Also this is not about being forward compatible, but having apps be backwards compatible with older kdelibs. Forward compatibility of kdelibs is mostly given anyway due to the promises of keeping BC and SC of existing API and only adding (and hopefully also keeping existing behaviour). If someone runs with an older version of kdelibs, they should realise that that version may contain some bugs which are fixed in later versions. So if they build an up-to-date application against that kdelibs, it's at their own risk to some degree. But if it helps some developers to do their work, it is surely a good thing to provide (via #ifdef or whatever) backwards compatibility to applications. -- David Jarvie. KDE developer. KAlarm author - http://www.astrojar.org.uk/kalarm
Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner
On Sept. 6, 2012, 12:29 p.m., David Faure wrote: 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). Simeon Bird wrote: 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. Thomas Lübking wrote: 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? David Faure wrote: Forcefully killing threads (QThread::terminate()) is a big no-no, it leaks memory, potentially worse: it can leave a mutex locked, so any further thread trying to lock it, will hang for ever. The only way to terminate a thread is to ask it nicely to terminate from within (i.e. to exit run()), which means it has to finish what it's currently doing, first, before it looks up for the next command or posted event which tells it to quit. Thomas Lübking wrote: That is generally true and i admit to not know how the plasma spell checker is implemented (or even where ;-), but would expect some sort of ro non-joinable threading, operating on shared data (as the only purpose seems to prevent blocking the main event loop - not running two checks at the same time and merge their results) ... or does QThread alone bring structures to forbid such approach (The API doc does not mention inevitable leaks for QThread itself on ::terminate())? (PS: I just start to wonder whether the speller threads are then tagged to catch 2,1 exits...) Simeon Bird wrote: I don't entirely follow what you mean in the last comment; what do you mean 'tagged to catch 2,1 exits'? The spellchecking runner is, I think, very cheap compared to some of the other runners (eg, the nepomuk search), all of which are called at once. But, be that as it may, the spell-check has no control over the threading model of krunner, which it just inherits from Plasma::AbstractRunner, and changing that model for everything seems a bit overkill for this, even if we understood fully how it works (I certainly don't; I just took the documentation at face value, which may or may not be a good idea). A reasonable alternative, if you don't like doing the commit in principle (the mutex might well slow down the spell-check), is to just remove the problematic feature (which has never worked anyway). If krunner only spell-checked words in the default dictionary, we wouldn't need to call setLanguage(), and everything would be fine, as it was before 2010. Thomas Lübking wrote: Thread execution is not predictable so in general it's possible that you start a thread (1), then start another thread (2) and thread 2 finishes before thread 1 Since i assume that in this case the last exiting thread will determine the sanity of the token, this would eg. mean that if you spellcheck threa and then thread but threa finishes last, the token
Re: Appmenu support for KDE 4.10
Le mardi 11 septembre 2012 17:42:17 Martin Gräßlin a écrit : just to check, do you mean multi-screen (xrandr) or really multi-head (one X server per screen)? I mean multi screen, sorry for mistake. -- Cédric
Review Request: Allow setting a custom font list in KFontComboBox
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106425/ --- Review request for kdelibs and Chusslove Illich. Description --- KFontAction has a constructor to filter the font list by certain criteria. When it creates a KFontComboBox to allow the user to interact with it, this box always shows all fonts. Instead of adding a setFontFilterCriteria() method, I opted for adding a setFontList() method, for the following reasons: - it does not have to Qt database twice - it is more flexible (you could, for example, filter by size) @since: TBD has to be decided: - 4.9.x, because it allows to fix bug 83212 and bug 306625 - 4.10, because it adds new API - KF5, because it adds new API - keep it private, only to be called by KFontAction Note that this commit alone does NOT fix the bugs, but adds the required API. This addresses bugs 83212 and 306625. http://bugs.kde.org/show_bug.cgi?id=83212 http://bugs.kde.org/show_bug.cgi?id=306625 Diffs - kdeui/fonts/kfontcombobox.h de3cb16 kdeui/fonts/kfontcombobox.cpp 0ded6fb Diff: http://git.reviewboard.kde.org/r/106425/diff/ Testing --- Thanks, Christoph Feck
Re: Review Request: Allow setting a custom font list in KFontComboBox
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106425/ --- (Updated Sept. 11, 2012, 7:33 p.m.) Review request for kdelibs and Chusslove Illich. Changes --- typos Description (updated) --- KFontAction has a constructor to filter the font list by certain criteria. When it creates a KFontComboBox to allow the user to interact with it, this box always shows all fonts. Instead of adding a setFontFilterCriteria() method, I opted for adding a setFontList() method, for the following reasons: - it does not have to query the Qt font database twice - it is more flexible (you could, for example, filter by size) @since: TBD has to be decided: - 4.9.x, because it allows to fix bug 83212 and bug 306625 - 4.10, because it adds new API - KF5, because it adds new API - keep it private, only to be called by KFontAction Note that this commit alone does NOT fix the bugs, but adds the required API. This addresses bugs 83212 and 306625. http://bugs.kde.org/show_bug.cgi?id=83212 http://bugs.kde.org/show_bug.cgi?id=306625 Diffs - kdeui/fonts/kfontcombobox.h de3cb16 kdeui/fonts/kfontcombobox.cpp 0ded6fb Diff: http://git.reviewboard.kde.org/r/106425/diff/ Testing --- Thanks, Christoph Feck
Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/#review18865 --- 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. - Zack Rusin On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/ --- (Updated Sept. 9, 2012, 10:06 p.m.) Review request for kdelibs and 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. 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. 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 - kdecore/sonnet/speller.cpp b19e74d Diff: http://git.reviewboard.kde.org/r/106242/diff/ Testing --- Compiled, installed, used for a week or so, spellchecked a bunch of things. Thanks, Simeon Bird
Re: Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/#review18866 --- 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. - Zack Rusin On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/ --- (Updated Sept. 9, 2012, 10:06 p.m.) Review request for kdelibs and 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. 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. 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 - kdecore/sonnet/speller.cpp b19e74d Diff: http://git.reviewboard.kde.org/r/106242/diff/ Testing --- Compiled, installed, used for a week or so, spellchecked a bunch of things. Thanks, Simeon Bird
