> On Sept. 11, 2012, 7:39 p.m., Zack Rusin wrote: > > 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. > > Simeon Bird wrote: > 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. > > Zack Rusin wrote: > 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.
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. - Simeon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106242/#review18865 ----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
