[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: [PATCH] RunnerManager
From: "Jordi Polo" <mumismo () gmail ! com>
Date: 2008-04-29 17:49:08
Message-ID: a4162420804291049uf4d8adar3e66b4716ae860b () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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@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@kde.org
> https://mail.kde.org/mailman/listinfo/panel-devel
>
>
--
Jordi Polo Carres
NLP laboratory - NAIST
http://www.bahasara.org
[Attachment #5 (text/html)]
<br>New version<br><a \
href="http://www.bahasara.org/raw-attachment/wiki/Papers/krunner_patch3">http://www.bahasara.org/raw-attachment/wiki/Papers/krunner_patch3</a><br><br>This \
time I've tried to only change code related to this email or today's IRC \
conversation.<br> <br>Changes are:<br><br>kbookmark changes out of the patch.<br>New \
RunnerManager::execQuery method that will call match directly (sync) and use it in \
the switchUser interface method. <br>RunnerManager::exec -> RunnerManager::run<br> \
RunnerManager::updateMatches and related code went away. \
<br>SearchContext::Private::emitsChangedMatches replaced by emit \
d->q->matchesChanged() <br>AbstractRunner::acceptType \
->ignoredType<br>AbstractRunner::setUnacceptableTypes ->setIgnoredTypes<br> \
Q_DECLARE_FLAGS(Types, Type) and \
<br>Q_DECLARE_OPERATORS_FOR_FLAGS(Plasma::SearchContext::Types)<br>in \
SearchContext<br>Interface doesn't needs SearchContext anymore (it may be even \
become and internal class someday)<br> <br>Issues:<br>Apidox for \
AbstractRunner::ignoredType is wrong, just noticed<br>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.<br> ultra-verbose \
kDebug() <br>Still a bulk of 2k lines.<br><br><br><div class="gmail_quote">2008/4/29 \
Aaron J. Seigo <<a \
href="mailto:aseigo@kde.org">aseigo@kde.org</a>>:<br><blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;"> <div class="Ih2E3d">On Monday 28 April 2008, Jordi \
Polo wrote:<br> > New patch<br>
> <a href="http://www.bahasara.org/raw-attachment/wiki/Papers/krunner_patch2" \
target="_blank">http://www.bahasara.org/raw-attachment/wiki/Papers/krunner_patch2</a><br>
> Around 2000 lines. Getting too big to understand I guess, should I break it<br>
> ?<br>
<br>
</div>absolutely ... having changes to the bookmark runner mixed in with changes \
to<br> search types mixed in with changes to SearchContext ... that's not \
very<br> sensible and makes it very hard to review.<br>
<br>
you also keep wildly changing the contents of the patch from revision to<br>
revision making it necessary to re-read the entire thing every time. please,<br>
i really can't be effective in helping you with your changes when it's so \
all<br> over the map.<br>
<br>
i'm going to try and review this one now, but in future i'll just send \
this<br> kind of patch back to you until it is in a more manageable form. i'm \
not<br> trying to be difficult here, it's just the reality of having such a \
jumble of<br> a patch that's some 2k lines long.<br>
<br>
<br>
<br>
this:<br>
SearchContext(const SearchContext& other, QObject \
*parent = 0);<br> <br>
needs to be marked explicit.<br>
<br>
can't all occurances of d->emitsMatchesChanged just be replaced with \
"emit<br> d->parent->matchesChanged();"? a little more straight \
forward?<br> <br>
SearchContext::Private::parent should be SearchContext::Private::q. it's just<br>
a convention used throughout the code (like 'd') and makes these things<br>
easier to read when hopping from class to class: when you see 'q' you \
know<br> exactly what it is (and what it isn't, e.g. a parent QObject)<br>
<br>
RunnerManager::launchQuery() *still* is both async and sync depending on the<br>
value of the runnerName parameter. for the love of all that is good in this<br>
world FIX THAT and then we can start putting this into svn.<br>
<br>
that is the one major blocker left, aside from your constant additions and<br>
other changes. so let's get just what is done in this email DONE in your<br>
patch, then commit the damn thing and let's move *incrementally* from there<br>
forward, ok?<br>
<div class="Ih2E3d"><br>
> - SearchContext now have a shared list of matches. When a local<br>
> SeachContext copies the global one, it is sharing everything. If you \
think<br> > calling reset should call detach():<br>
<br>
</div>hm.. SearchContext::reset doesn't call detach, however. it just clears \
the<br> Private class, which means it's going to clear in all runners sharing \
this<br> context, right? where is the detach happening?<br>
<div class="Ih2E3d"><br>
> - The local can start with a new SearchContext in that case<br>
<br>
</div>right; assumign a detach actually happens somewhere here.<br>
<div class="Ih2E3d"><br>
> - Reviewing the changes right now I think that maybe the copy constructor<br>
> should create d with other->d.parent.<br>
<br>
</div>in the initialization list, you mean. yes. the new Private in the<br>
initialization list when copying the SearchContext is indeed wrong.<br>
<div class="Ih2E3d"><br>
> - SearchContext now manages completions as a new type of match<br>
> CompletionMatch. All the completions related methods are gone. The<br>
> Kcompletion object is created in interface instead.<br>
<br>
</div>+1; i'm a little uncomfortable that CompletionMatch is already in the<br>
enumeration even though it's not used by either the interface nor any runner.<br>
but getting rid of the completer is a good idea.<br>
<div class="Ih2E3d"><br>
> - added acceptsType and setUnacceptableTypes (yes, maybe the worst names<br>
> ever) to AbstractRunner. Runners can now specify what types they are not<br>
> interested in so not even a job is created for them. To achieve this the<br>
> SearchContext::Type must be OR'able. The concept is tested with the<br>
> calculator runner.<br>
<br>
</div>SearchMatch::Type would probably be better as QueryType ... that's a \
detail<br> and we can change that after this set of patches is in.<br>
<br>
the apidox for acceptsType is wrong (copy and paste error from<br>
setUnnacceptableTypes).<br>
<br>
setAcceptableTypes should not take an int. rather, in SearchContext there<br>
should be:<br>
<br>
Q_DECLARE_FLAGS(Types, Type)<br>
<br>
and then outside the class definition and Plasma namespace:<br>
<br>
Q_DECLARE_OPERATORS_FOR_FLAGS(Plasma::SearchContext::Types)<br>
<br>
then setAcceptableTypes would take a SearchContext::QueryTypes.<br>
<br>
in fact, instead of setUnnacceptableTypes ....<br>
setIgnoredTypes(SearchContext::Types)? with a SearchContext::Types<br>
ignoredTypes() const; accessor which would replace acceptsType (better<br>
symmetry between setter and getter)<br>
<div class="Ih2E3d"><br>
> - Added support for krdc and konsole in bookmark runner<br>
<br>
</div>why does the runner now have a SearchContext as a member? it copies the one<br>
it's handed, which is rather dangerous since it doesn't own it in the \
first<br> place and from looking at the code .. m_search is actually uneeded.<br>
<br>
i also think that putting the creation of the config files in match is really<br>
dubious. the overhead of that is going to be non-trivial.<br>
<br>
what we probably really want is to create that data once (on construction?)<br>
and re-use that data in each match. this is probably a separate problem to<br>
fix, and another one may affect the architecture of things somewhat.<br>
<br>
can we revisit this issue once the other items are complete? in fact, hold off<br>
on committing this change at all until the rest is done.<br>
<br>
and just to sound like the broken record / parent that i am: try to keep your<br>
focus and finish one thing before moving on to another. we'll get through<br>
this a lot quicker if you can. =)<br>
<font color="#888888"><br>
--<br>
</font><div><div></div><div class="Wj3C7c">Aaron J. Seigo<br>
humru othro a kohnu se<br>
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43<br>
<br>
KDE core developer sponsored by Trolltech<br>
</div></div><br>_______________________________________________<br>
Panel-devel mailing list<br>
<a href="mailto:Panel-devel@kde.org">Panel-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/panel-devel" \
target="_blank">https://mail.kde.org/mailman/listinfo/panel-devel</a><br> \
<br></blockquote></div><br><br clear="all"><br>-- <br>Jordi Polo Carres<br>NLP \
laboratory - NAIST<br><a \
href="http://www.bahasara.org">http://www.bahasara.org</a><br>
_______________________________________________
Panel-devel mailing list
Panel-devel@kde.org
https://mail.kde.org/mailman/listinfo/panel-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic