Review Request: Multiple action support for KRunner

[Aaron J. Seigo]

On Monday 08 September 2008, you wrote:
> > On 2008-09-08 10:37:12, Aaron Seigo wrote:
> > > "Another possibility would be to add object support to an action."
> > >
> > > isn't this up to whatever happens as a result of the action itself?
> > >
> > > "One way would be to have a set of core runners designated as object
> > > providers (e.g. Places, Contacts) and run a secondary query on a
> > > specific runner"
> > >
> > > .. or just run once for matches, then when a match is selected (right
> > > click, whatever) run again for actions.
> > >
> > > "Because QueryActions are GUI objects, they need to live in the GUI
> > > thread."
> > >
> > > they are QObjects. the GUI bit is the associated QIcon. since those
> > > never paint directly to screen and would only be accessed from the GUI
> > > thread, i bet we could work around that part just fine.
> > >
> > > "That means that QueryActions must be created outside of the match
> > > method"
> > >
> > > yes, that makes sense.
> > >
> > > "preferably at the time of runner construction."
> > >
> > > perhaps they could be created on-demand as well since they will need to
> > > be associated on-demand. this should work out fine since a first run
> > > for nouns would need to be done anyways, the verbs can follow after
> > > that in a second pass.
> > >
> > > "QueryActions must be registered with QueryActionPool, the global
> > > action registry"
> > >
> > > would a KActionCollection suffice? or do they even need to be
> > > registered at all? could runners simply provide them as needed?
> > >
> > > "This also allows runners to add actions that belong to different
> > > runners."
> > >
> > > this sounds rather unusual. how, exactly, will other runners know what
> > > the meaning of matches they don't produce are?
> > >
> > > "after I finish up work on my alternative front-end QuickSand."
> > >
> > > there is GUI work already planned for krunner that didn't make it into
> > > 4.1. it would be interesting to at least coordinate so we don't get in
> > > each other's way.
> > >
> > > personally, i think this patch is premature. there are a number of
> > > design concepts that need to be hammered out first which would be
> > > easily discussable on the list, sans code.
> >
> >"Another possibility would be to add object support to an action."
> >isn't this up to whatever happens as a result of the action itself?
>
> Not really, the action in itself may be incomplete. For example, given an
> action "copy to", where would the item be copied?

that would be resolved on action trigger.

> I agree that it is
> possible to handle this elsewhere but it is more convenient for the user to
> choose the location from within the interface than perhaps some dialog box
> that pops up after the user presses enter.

a dialog box is in no way different than a popup menu, an onscree listing or 
any other post-focus population. so this isn't about dialogs or not, but about 
whether or not the user needs to interact with the action further or not (as a 
general case)

in the situation that the user does not need to interact with the action 
further (the target is implied, for instance), obviously nothing needs to be 
done. this is the easy cas.e

in the situation where the user does need to select additional information, 
the options at hand can be populated at the point the user expresses interest 
in the action.

> >"One way would be to have a set of core runners designated as object
> > providers (e.g. Places, Contacts) and run a secondary query on a specific
> > runner" .. or just run once for matches, then when a match is selected
> > (right click, whatever) run again for actions.
>
> Are you suggesting we search for actions?

i'm suggesting that instead of associating actions up front for all N match 
that get displayed, only associate actions with the match the user is focussed 
on (e.g. right clicks on in the current patch, but i could see this happening 
on keyboard focus or first-step activiation even in a quicksilver style UI)

> >"Because QueryActions are GUI objects, they need to live in the GUI
> > thread." they are QObjects. the GUI bit is the associated QIcon. since
> > those never paint directly to screen and would only be accessed from the
> > GUI thread, i bet we could work around that part just fine.
>
> Well, there is the issue of thread affinity. Creating a QAction within a
> thread that will be destroyed is not a good idea. Even if threads now have
> their own event loops, the QAction might outlive the thread with our
> current setup.

yes, they should be moved to the GUI thread i suppose; i don't want to see 
this turn into restrictions on their creation, however, as that would mean 
having to pre-allocate all possible actions. (ugh)

> >"preferably at the time of runner construction."
> >perhaps they could be created on-demand as well since they will need to be
> > associated on-demand. this should work out fine since a first run for
> > nouns would need to be done anyways, the verbs can follow after that in a
> > second pass.
>
> By on demand, where would this be placed? The exec method? The set of
> actions won't vary much from match to match for a given runner, creating
> them all at once eliminates the overhead of creating them on demand.

i don't think it needs to happen in the match method. it can be done in 
another method used specifically for this. there is no point in having N 
actions hanging around that never get used but need to be updated from time to 
time (which is exactly what will happen..); there is also no point in 
associating N actions with M matches when the user only interacts with some 
fraction of M.

> >"QueryActions must be registered with QueryActionPool, the global action
> > registry" would a KActionCollection suffice? or do they even need to be
> > registered at all? could runners simply provide them as needed?
>
> Yes, a KActionCollection might do. I admit I overlooked that. Registration
> was chosen first because we can't create the actions in the match method,

yes, i don't think that's the right method for action creation either.

> and second because creating and reusing actions seemed more efficient.

that's a decision we should leave up to the runner imho.

> >"after I finish up work on my alternative front-end QuickSand."
> >there is GUI work already planned for krunner that didn't make it into
> > 4.1. it would be interesting to at least coordinate so we don't get in
> > each other's way.
>
> Well, the front-end was in development since December '07. I kinda let it
> bitrot the past nine months before resuming work last week. I thought the
> the whole spinning icons thing was the new GUI though...

it's the first bits to the new UI. the still missing parts:

* proper paging
* access to per-match actions
* results page for single matches (already mocked up; lets us use the space 
more effectively when there are only 1 or a few matches vs dozens)

> > On 2008-09-08 10:37:12, Aaron Seigo wrote:
> > > trunk/KDE/kdebase/workspace/libs/plasma/actionpool.h, lines 42-44
> > > <http://reviewboard.vidsolbach.de/r/178/diff/2/?file=909#file909line42>
> > >
> > >     makes no sense to have deprecated methods in a new class =)
>
> Well deprecated means discouraged, and the recommended usage is the
> singleton instance. :)

deprecated means "there because we can't remove it yet, but it will be removed 
in the future, do not use in new code"

> > On 2008-09-08 10:37:12, Aaron Seigo wrote:
> > > trunk/KDE/kdebase/workspace/libs/plasma/querymatch.cpp, line 189
> > > <http://reviewboard.vidsolbach.de/r/178/diff/2/?file=916#file916line189
> > >>
> > >
> > >     QList<const QueryAction*>?
>
> Non-const to permit modification and usage if the action belongs to another
> runner, or action provider.

creating an action in one runner for a match in a second runner and then 
modifying it in a third (or the second) seems a bit crazy.

> Like I said I'd give an example soon.

ok ...

> > >     QList<QueryActions*> should be enough? why the additional internal
> > > iterator? slightly dangerous if actions are removed as well, i'd
> > > imagine (either way, but more so with the internal iterator)
>
> Well actions aren't supposed to be removed until a runner is unloaded. That

the "supposed to" is the part that scares me =)

> means that the config dialog has to be opened, but by then the matches
> should be invalidated.

/me nods

> The iterator is to determine which of the available actions to execute.
> This way the exec method of existing runners can be kept intact and new
> runners can inspect the selected action in the match to determine what to
> do.

why not just include the selected action as part of the Plasma::QueryMatch 
passed in? or perhaps part of the SearchContext, though i think the Match 
makes more sense in this case.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/plasma-devel/attachments/20080908/ed0f183b/attachment-0001.sig