[PATCH] RunnerManager

[Jordi Polo]

Ok, I'll hopefully address these issues and send a new patch tomorrow, what
a pity we are so timezone misaligned.

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

Sorry, knowing the code well made me think other people had super
analizing-and-understanding-unknown-code powers



>
> 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.
>

I plan to do it post (beta freezing + bug hunting)


>
> > - 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 ;)
>

In fact a backtrace that showed kbookmark runner as guilty.
But my X session died with the unsaved data. I'll hit it again for sure.



> 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.
>

Sorry, very unpolite, just used to open file and ...

>
> * 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 ;)


Yes, I tried it and didn't work exactly well :P


>
>
> * 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.
>

Ok to all, will do.


>
> * 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).
>

So far only is used to get the session runner. But will do.


> * 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();
>

OK, understood, as stated check your inbox tomorrow.

BTW, in the days I started with plasma development (3 weeks ago?) someone
told me a review board was used to be around ... It is still nonfunctional?

About the configuration, if a KConfigGroup is passed it means that each app
that wants to use runners capabilites will have a group in its configuration
file for it. Create a GUI to configure it can mean creating a widget that
each app should use ...




> --
> 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
>
> _______________________________________________
> Panel-devel mailing list
> Panel-devel at kde.org
> https://mail.kde.org/mailman/listinfo/panel-devel
>
>


-- 
Jordi Polo Carres
NLP laboratory - NAIST
http://www.bahasara.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/panel-devel/attachments/20080422/98e73542/attachment-0001.html