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