Review Request: Multiple action support for KRunner

[Ryan P. Bitanga]

2008/9/20 Aaron J. Seigo <aseigo at kde.org>:
> On Thursday 18 September 2008, you wrote:
>> > On 2008-09-17 08:34:10, Aaron Seigo wrote:
>> > > i mostly like this patch ...
>> > >
>> > > however, QueryActionPool doesn't seem to actually be used; where it is
>> > > used, it seems to be an implementation detail and therefore shouldn't
>> > > need to be exported .. but even more evident is that it probably isn't
>> > > needed at all?
>> > >
>> > > what plans do you have for QueryActionPool, if any?
>> > >
>> > > (without QueryActionPool, QueryAction seems unecessary as well?)
>>
>> Well, QueryActionPool provides an easy means to determine whether or not an
>> action was created and loaded already without having to have each runner
>> have its own way of tracking.
>
> .. the downside is that now we have to coordinate the actions at a central
> point. as they are delete'able pointers and not references, that makes me
> nervous.

Deleting the action automatically removes it from the pool, so instead
of storing the pointer we can store a QString which would be used to
retrieve the action from the action pool. That way we could get rid of
possible dangling pointers in QueryMatch but I don't think anyone
would be foolish enough to delete the pointers anyway. I still think a
centralized location makes it easier to write runners by reducing
unnecessary duplicated code per runner (and it also allows standard
actions).

>> It also provides a repository for shared
>> actions (standard actions) common to all runners if they need any.
>
> provided by the host application? if so, couldn't this go directly into
> RunnerManager instead?

Yep it could. That's why the parent of QueryAction doesn't have a
default value of 0 nor is the parent an AbstractRunner*. Since action
providers need not be runners, other classes can be written to provide
necessary actions and handle their execution.

>> The
>> patch originally contained a "set as default" action that could be used to
>> boost the relevance of a particular match to set it as the default. It had
>> a QHash<QString, QString> that contained the query and the id of the match
>> but the matching had to be done in RunnerContext. That would provide us our
>> first limited learning implementation in KRunner. I decided to leave it out
>> to have a more focused discussion first. :)
>
> i think this is a good idea, but one that belongs in RunnerManager rather than
> another class.

It was supposed to be in RunnerManager; however, appending items is
done in RunnerContext and it's more efficient to automatically boost
relevance through RunnerContext as they are added instead of iterating
over all the matches in a separate method in RunnerManager. It is
possible however, for the QHash to be provided and managed by the
RunnerManager.

>> QueryActionPool is thread-safe because it used to be accessed in the match
>> methods.
>
> that much i understood =)
>
>> With my latest patch, it is _supposed_ to only be accessed in the
>> GUI thread.
>
> this gets back to my nervousness ...
>
Keeping it thread-safe shouldn't make you nervous :) I only mentioned
this to point out that it can now be made non-thread-safe.

>> I considered changing it to inherit KActionCollection, but I
>> think KActionCollection is a bit too bloated and I don't like the idea of
>> KActionCollection owning the actions.
>
> yes, i think that was a good call
>
>> Anyone can delete the action by
>> accident using KActionCollection leaving dangling pointers behind. I think
>> the actions should be owned by the runners or the class in which it is
>> created.
>
> sure..
>
>> Another possibility for QueryActionPool is something like an
>> actionsForMimetype method.
>
> wouldn't this be runner specific? if not, it also belongs in RunnerManager
True.
>
>> Having our own QueryAction class will allow us to put a mimetype method, a
>> pointer or reference to the context, and other pertinent methods if needed.
>
> until we have concrete use cases we don't need QueryAction, though. QAction
> would suffice until then. and we may find we never want or need anything more.
>
Well I think it's pretty obvious I want a three pane interface ala
Quicksilver. :) And with plain QActions, it's difficult to determine
whether an action has an implied target or not. I know you said that
whether or not an action requires a target is something that could be
resolved after the action is triggered, but the thing is there is no
guarantee that the action _will_ be triggered. See my window manager
runner as an example. Triggering the action is left up to the runner
author. For actions that aren't owned by the runner (which can be
determined by examining the parent), the runner can trigger the
action. For actions owned by the runner, the runner author has
complete control over what to do.

> as for a mimetype method, i have a feeling that that would eventually end up
> being used to populate a ranked and/or hashed set for preferential and quick
> access .. making putting it in QueryAction itself not so useful.
>
> --
> 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
>
>
> _______________________________________________
> Plasma-devel mailing list
> Plasma-devel at kde.org
> https://mail.kde.org/mailman/listinfo/plasma-devel
>
>