[PATCH] RunnerManager

[Aaron J. Seigo]

On Monday 21 April 2008, Jordi Polo wrote:
> If it is OK to submit it, I'll continue the development in SVN.

indeed; this patch is massive and invasive. maintaining such things outside of 
a revision controled repo is very hard =/

having to read through a 50k patch with no prior explanations of what it is 
doing isn't very nice either ..

so.. let's get this cleaned up and ready to go in. also, remember that you can 
always start a branches in branches/ anytime you feel it's worthwhile.

> - The configuration. I want RunnerManager to configure itself. Potentially
> other programs not only krunner can use it. That means reading its own
> configuration file. I am not sure if the right approach is create a
> runnermanagerrc. So far There is a bunch of commented code in the
> constructor. Wait for advise.

runnermanagerc won't work since that means the same configuration for all 
applications, unless each instance of RunnerManager gets its own 
KConfigGroup. which we probably want anyways, since different RunnerManagers 
may exist in the same app (concrete example: kickoff vs launcher applet)

> - So, user priority list and blacklist is not working (nor whitelist : the
> only regression I am aware about). That depends on the configuration point
> above. The code to implement it is there though.

yes, this is a detail at this point =)

> - I guess I have to write more comments, specially the interface of
> RunnerManager should improve.

yes, i'll provide a more detailed API review of this class in a separate 
email.

> - Sometimes I hit some dbus out of memory errors -> investigate. I may
> resurrect the queueMatch idea.

dbus out of memory? hm. that's.. odd. where is dbus being used at all? ah, in 
one of the runners you have installed, perhaps? in any case, when you 
say "resurrect the queueMatch idea" can you explain what you replaced it 
with, etc? i shouldn't have to reverse engineer your work when we're all here 
on the same list ;)

other general comments:

* the copyrights in the file are wrong. you can't copy other people's code 
from existing files and not carry the copyright over. so there are a few 
copyrights missing there, probably Ryan's and mine for starters.

* i see that the call to processEvents is commented out. that's good because 
you should NEVER call that unless there are very, very good reasons to. 
processEvents is evil, evil, evil and reserved only as an Escape Route From 
Some Other Level Of Hell ;)

* in the copy ctor for Context, the completer object is no longer copied over. 
at the very least the completions registered should be copied over.

* using launchQuery as both async and sync should be avoided IMHO. make two 
different methods: one that spawns threads and one that doesn't. call the 
latter one execQuery.

* returning an integer from a method as seen in launchQuery is nasty. return a 
boolean or use a proper enumeration if an error code is needed. right now i 
only see 0 and 1, so a bool would be appropriate. this is C++ after all.

* RunnerManager::getRunner is O(N). if it gets called often and N grows large 
(which we probably hope it does ;), this could be an issue. it's probably 
premature to optimize this (by using a name->Runner mapping/hash instead of a 
simple list) but it's probably worthwhile to at least put a comment there 
noting this problem. at some point we should go count how often this gets 
called to find out if isuch an optimization is worthwhile in the Real 
World(tm).

* in the RunnerManager ctor there is this:

+    d->runners += load();

load() should simply clear d->runners and set d->runners itself. this will 
open the door to changes in configuration down the road as well (just call 
load() again ..)

* please be careful with the coding style. there are a lot of instances where 
whitespace is missing, e.g.:

+    bool emptyMatches=matches.isEmpty();

which should be

+    bool emptyMatches = matches.isEmpty();

-- 
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/20080421/1ec8feda/attachment.pgp