Review Request: Multiple action support for KRunner

[Ryan Bitanga]

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

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

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

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

>"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, and second because creating and reusing actions seemed more efficient.

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

Well the patch is a work in progress. QueryAction is quite noticeably bare at the moment. And I did say, I'd give an example soon :)

>"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... I didn't see anything else discussed on list.

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

On the other hand, it's easier to discuss when there's something concrete to work with :)


> 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. :)


> 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. Like I said I'd give an example soon. But I'm still looking for a more elegant solution.


> On 2008-09-08 10:37:12, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/plasma/querymatch.cpp, lines 194-223
> > <http://reviewboard.vidsolbach.de/r/178/diff/2/?file=916#file916line194>
> >
> >     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 means that the config dialog has to be opened, but by then the matches should be invalidated.

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.


- Ryan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.vidsolbach.de/r/178/#review172
-----------------------------------------------------------


On 2008-09-08 04:52:02, Ryan Bitanga wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.vidsolbach.de/r/178/
> -----------------------------------------------------------
> 
> (Updated 2008-09-08 04:52:02)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This introduces multiple action (or noun-verb) support for runners. Each QueryMatch (the noun) can now have multiple actions associated with it.
> 
> Actions are QueryActions which inherit QAction. I intend to extend QueryAction to support mimetypes and more functionality. If QueryActions have associated mimetypes, the runner can intelligently select which of the available actions to add to the match.
> 
> Another possibility would be to add object support to an action. For example, a match returned by the Desktop Search runner could have the action "Copy to...". The object of the action would be a directory. I'm still looking for ways to implement this. 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.
> 
> Because QueryActions are GUI objects, they need to live in the GUI thread. That means that QueryActions must be created outside of the match method, preferably at the time of runner construction. After creation, QueryActions must be registered with QueryActionPool, the global action registry. This allows QueryActions to only be created once during the lifetime of a runner. This also allows runners to add actions that belong to different runners. However, correct execution of the action is left up to the runner in the exec method. I'll provide an example of how to do this soon.
> 
> Multiple action support is optional. Runners that do not create actions will continue to function just as they did prior to the introduction of multiple action support.
> 
> An example implementation with multiple action support is the amarok-based mediaplayer runner currently in playground. To access the actions, right click on the icon and select the desired action. Note, the temporary solution triggers the action upon selection the action in the context menu. Pressing enter or clicking on the icon will cause the action to be performed again.
> 
> I'll address that after I finish up work on my alternative front-end QuickSand. :)
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/krunner/resultitem.h
>   trunk/KDE/kdebase/workspace/krunner/resultitem.cpp
>   trunk/KDE/kdebase/workspace/libs/plasma/CMakeLists.txt
>   trunk/KDE/kdebase/workspace/libs/plasma/actionpool.h
>   trunk/KDE/kdebase/workspace/libs/plasma/actionpool.cpp
>   trunk/KDE/kdebase/workspace/libs/plasma/includes/QueryAction
>   trunk/KDE/kdebase/workspace/libs/plasma/includes/QueryActionPool
>   trunk/KDE/kdebase/workspace/libs/plasma/queryaction.h
>   trunk/KDE/kdebase/workspace/libs/plasma/queryaction.cpp
>   trunk/KDE/kdebase/workspace/libs/plasma/querymatch.h
>   trunk/KDE/kdebase/workspace/libs/plasma/querymatch.cpp
> 
> Diff: http://reviewboard.vidsolbach.de/r/178/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ryan
> 
>