KRunner config dialog

[Ryan P. Bitanga]

2008/5/7 Aaron J. Seigo <aseigo at kde.org>:
> On Wednesday 07 May 2008, Ryan P. Bitanga wrote:
>  > My patch will be a little bit delayed yet again due to conflicting
>  > changes in libplasma.
>
>  which classes are you working on, so we can avoid hitting the same ones? can
>  you post a draft patch so we can sync up with ideas? i'm concerned that
>  waiting on your patch will hold up mainline development, but not holding up
>  mainline development will hold up waiting on your patch (both due to
>  integration of moving targets).
>
>  so .. i'd suggest getting your patch up sooner rather than later.
>
Gimme a few hours. I'm almost done even if I'm working on this on my
spare time and my day job (internship) is about to end in two weeks so
I'll have more free time soon.

I've touched: RunnerContext, AbstractRunner, RunnerManager and
Interface. I might revert some of my changes to RunnerContext though.

>
>  > Similarly I also used id() for getting KPluginInfo->pluginName because
>  > I needed it for reloading configs of a specific runner.
>
>  right =)
>
>
>  > * Added a reparseConfiguration method to AbstractRunner to reload the
>  > config of a specific runner.
>
>  loadConfiguration()?

It was originally loadConfiguration but I chose reparseConfiguration
to be consistent with KConfig but it's trivial to change the method
name again :)

>  > * Got rid of the blacklist and revamped the load function. (Whitelists
>  > make more sense since the default behavior should be "plugin disabled
>  > by default" ie X-KDE-PluginInfo-EnabledByDefault=false)
>
>  i'm really on the fence on this one. white lists mean newly added runners
>  aren't automatically picked up; that actually isn't what we want here. we
>  should listen to EnabledByDefault, but the default should be enabled.
>  krunner's usefulness is really based on this concept.

In that case I'm adding a "load all plugin option" to the config
dialog that will bypass normal behavior.

>  are you working just locally, or do you have an svn branch or git repo
>  somewhere?
Locally. But I've been reading up on git the past three days for an
eventual switch.

>  in practice we still locked everything we added matches: when agregating to
>  the global context. that locking was also compounded by comparing terms (more
>  locking) and then add to one QList and destroying another.
>
>  we really do need to look into the locking details in QueryContext, but
>  currently things feel a lot smoother in the UI and memory usage should also
>  be down.
>
>  the downside here is that if you repeatedly call addMatch() versus
>  addMatches() then we'll get more locks than before. however, all the runners
>  currently use addMatches(). i'm of the opinion that calling addMatch when you
>  have multiple matches is an abuse of the API. i've toyed with removing
>  addMatch() altogether, but while that would encourage runner writers to do
>  the Right Thing(tm) always (only calling addMatch* once), it would also
>  result in a lot of unecessary QList<QueryMatch*>s being made. i think we need
>  to trust the developer a bit in this case.
>
>  also, i've noticed that addMatches *probably* doesn't require checking the
>  query string anymore. i need to test that theory, however. if it is true,
>  that would get rid of another (read) lock.
>
>  note that performMatch is also used for rate limiting, of course, so it
>  retains its usefulness.

Yes I noticed that part but it might have been possible to move that
logic elsewhere. I also noticed that rateLimiting has been causing
starvation on my end. Xesam runner gets promoted to fast runner, then
starves everything else when it becomes slow again... I'll look into
it soon.

>
>  > and DataPolicy is useless.
>
>  yes, it isn't used at the moment.

So I took it out in my patch (and this is the part I'm thinking of
reverting) which I'll post within the day I hope.

Cheers,
Ryan