[PATCH] RunnerManager

[Aaron J. Seigo]

On Monday 28 April 2008, Jordi Polo wrote:
> New patch
> http://www.bahasara.org/raw-attachment/wiki/Papers/krunner_patch2
> Around 2000 lines. Getting too big to understand I guess, should I break it
> ? 

absolutely ... having changes to the bookmark runner mixed in with changes to 
search types mixed in with changes to SearchContext ... that's not very 
sensible and makes it very hard to review.

you also keep wildly changing the contents of the patch from revision to 
revision making it necessary to re-read the entire thing every time. please, 
i really can't be effective in helping you with your changes when it's so all 
over the map.

i'm going to try and review this one now, but in future i'll just send this 
kind of patch back to you until it is in a more manageable form. i'm not 
trying to be difficult here, it's just the reality of having such a jumble of 
a patch that's some 2k lines long.



this:
	SearchContext(const SearchContext& other, QObject *parent = 0);

needs to be marked explicit.

can't all occurances of d->emitsMatchesChanged just be replaced with "emit 
d->parent->matchesChanged();"? a little more straight forward?

SearchContext::Private::parent should be SearchContext::Private::q. it's just 
a convention used throughout the code (like 'd') and makes these things 
easier to read when hopping from class to class: when you see 'q' you know 
exactly what it is (and what it isn't, e.g. a parent QObject)

RunnerManager::launchQuery() *still* is both async and sync depending on the 
value of the runnerName parameter. for the love of all that is good in this 
world FIX THAT and then we can start putting this into svn.

that is the one major blocker left, aside from your constant additions and 
other changes. so let's get just what is done in this email DONE in your 
patch, then commit the damn thing and let's move *incrementally* from there 
forward, ok?

> - SearchContext now have a shared list of matches. When a local
> SeachContext copies the global one, it is sharing everything.  If you think
> calling reset should call detach():

hm.. SearchContext::reset doesn't call detach, however. it just clears the 
Private class, which means it's going to clear in all runners sharing this 
context, right? where is the detach happening?

> - The local can start with a new SearchContext in that case

right; assumign a detach actually happens somewhere here.

> - Reviewing the changes right now I think that maybe the copy constructor
> should create d with other->d.parent.

in the initialization list, you mean. yes. the new Private in the 
initialization list when copying the SearchContext is indeed wrong.

> - SearchContext now manages completions as a new type of match
> CompletionMatch. All the completions related methods are gone. The
> Kcompletion object is created in interface instead.

+1; i'm a little uncomfortable that CompletionMatch is already in the 
enumeration even though it's not used by either the interface nor any runner. 
but getting rid of the completer is a good idea.

> - added acceptsType and setUnacceptableTypes (yes, maybe the worst names
> ever) to AbstractRunner. Runners can now specify what types they are not
> interested in so not even a job is created for them. To achieve this the
> SearchContext::Type must be OR'able. The concept is tested with the
> calculator runner.

SearchMatch::Type would probably be better as QueryType ... that's a detail 
and we can change that after this set of patches is in.

the apidox for acceptsType is wrong (copy and paste error from 
setUnnacceptableTypes).

setAcceptableTypes should not take an int. rather, in SearchContext there 
should be:

Q_DECLARE_FLAGS(Types, Type)

and then outside the class definition and Plasma namespace:

Q_DECLARE_OPERATORS_FOR_FLAGS(Plasma::SearchContext::Types)

then setAcceptableTypes would take a SearchContext::QueryTypes.

in fact, instead of setUnnacceptableTypes .... 
setIgnoredTypes(SearchContext::Types)? with a SearchContext::Types 
ignoredTypes() const; accessor which would replace acceptsType (better 
symmetry between setter and getter)

> - Added support for krdc and konsole in bookmark runner

why does the runner now have a SearchContext as a member? it copies the one 
it's handed, which is rather dangerous since it doesn't own it in the first 
place and from looking at the code .. m_search is actually uneeded.

i also think that putting the creation of the config files in match is really 
dubious. the overhead of that is going to be non-trivial.

what we probably really want is to create that data once (on construction?) 
and re-use that data in each match. this is probably a separate problem to 
fix, and another one may affect the architecture of things somewhat.

can we revisit this issue once the other items are complete? in fact, hold off 
on committing this change at all until the rest is done.

and just to sound like the broken record / parent that i am: try to keep your 
focus and finish one thing before moving on to another. we'll get through 
this a lot quicker if you can. =)

-- 
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 Trolltech
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20080428/c35c8bc2/attachment.pgp