[PATCH] RunnerManager

[Jordi Polo]

New version
http://www.bahasara.org/raw-attachment/wiki/Papers/krunner_patch3

This time I've tried to only change code related to this email or today's
IRC conversation.

Changes are:

kbookmark changes out of the patch.
New RunnerManager::execQuery method that will call match directly (sync) and
use it in the switchUser interface method.
RunnerManager::exec -> RunnerManager::run
RunnerManager::updateMatches and related code went away.
SearchContext::Private::emitsChangedMatches replaced by emit
d->q->matchesChanged()
AbstractRunner::acceptType ->ignoredType
AbstractRunner::setUnacceptableTypes ->setIgnoredTypes
Q_DECLARE_FLAGS(Types, Type)  and
Q_DECLARE_OPERATORS_FOR_FLAGS(Plasma::SearchContext::Types)
in SearchContext
Interface doesn't needs SearchContext anymore (it may be even become and
internal class someday)

Issues:
Apidox for AbstractRunner::ignoredType is wrong, just noticed
There are method commented in scripting/scriptengine.cpp, it has nothing to
with with this patch, but I couldn't make it compile. I will of course not
commit that.
ultra-verbose kDebug()
Still a bulk of 2k lines.


2008/4/29 Aaron J. Seigo <aseigo at kde.org>:

> 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
>
> _______________________________________________
> 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/20080430/a0bd717d/attachment-0001.html