kdreview: dictionary runner; changes needed to Plasma::RunnerManager?

[Aaron J. Seigo]

On Saturday, August 28, 2010, Jason A. Donenfeld wrote:
> On Tue, Aug 24, 2010 at 16:59, Aaron J. Seigo <aseigo at kde.org> wrote:
> > * when the teardown() signal is emitted, the runner should disconnect
> > from the DataEngine
> 
> Fixed with r1169251.

:)

> > * a thread can wait indefinitely on m_wait.wait(&m_mutex);
> 
> But isn't dataUpdated guaranteed to be run at /some point/ ? Or should
> we not rely on this.

can't rely on it, no. it probably happens in this case, but there is no actual 
guarantee in the DataEngine system that it does.
 
> > * if the user types more than $THREAD_POOL_COUNT letters before the dict
> > engine returns, the thread pool will be filled with dictionary runner
> > threads and therefore be exhausted
> > 
> > looking at it further, it's evident that this runner is really working
> > around the fact that it is multithreaded;
> 
> Exactly -- the runner is a gigantic hack.

well, we can fix that. if nothing else, this was a great exercise in 
discovering what needs to be improved exactly, and how :)
 
> > it would be far easier in this case to
> > simply make match() re-entrant and have just one thread of it around.
> 
> It'd be nice to be able to access match()'s RunnerContext argument in
> a slot, so that match could parse the input, call a method of object
> A, and then return, and then a signal coming from A would call a slot
> that populates the RunnerContext with matches.

match is given the RunnerContext on each invocation. that RunnerContext may 
become invalid at any time, though, so there is no "current" or "the" 
RunnerContext in this sense.

a runner could create a separate QObject subclass for each call that holds 
onto the RunnerContext passed it as a member variable, and that object could 
do exactly as you describe (and then delete itself when finished).
 
> But in such a design what about keeping track of multiple match calls
> and multiple RunnerContexts? Always populate the most recent one? a
> currentContext() accessor method?

no, that's handled internal to RunnerContext: if it isn't valid (isn't the 
current one in the main thread), any matches passed are just silently 
discarded.

> > the nice thing about the current design is that match() methods do not
> > need to be made re-entrant, and this makes it simpler for many of the
> > runners. but it assumes that the runners exit quickly (freeing up the
> > thread pool) and don't rely on any resources that have a single-thread
> > restriction on them (as is the case with the dictionary plasmoid, among
> > one or two others).
> 
> Could we have both designs avaiable controlled by the aforementioned
> setReentrant(true) flag?

yes, that's exactly what i'm thinking :)

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Qt Development Frameworks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/plasma-devel/attachments/20100828/e4766d0a/attachment.sig