> 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. > > Simeon Bird wrote: > 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. > > Thomas Lübking wrote: > > 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?) > > Simeon Bird wrote: > Ah, no, not as simple as that. You're thinking (I think) of a standard > spellchecker widget, which has a button marked "language: deutsch", that you > click to change the language. > > However, here we are a krunner plugin, and krunner runs the gui thread. > We do not have access to it, or to the dispatcher mechanism, we just have to > take the string we are handed, parse it for a language name, and do what we > can. So although in 'reality' we only check one lang at a time, krunner > pretends to us that we are checking more, and we have to deal with that. > > Zack Rusin wrote: > That sounds perfect then, it already handles the signal for you. That > just means that each spell checking thread gets it own speller and each calls > setLanguage whenever it's ready.
Ok - I didn't do that first because I thought constructing all those spellers would be way too slow (presumably it involves reading the available dictionaries from disc?). But I never tested, and actually, it might well be fast enough, since we're not on the critical path. I'll give it a go. Thanks for the help. - 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
