Review Request: Multiple action support for KRunner

[Ryan Bitanga]

> 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. It also provides a repository for shared actions (standard actions) common to all runners if they need any. 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. :)

QueryActionPool is thread-safe because it used to be accessed in the match methods. With my latest patch, it is _supposed_ to only be accessed in the GUI thread. 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. 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.

Another possibility for QueryActionPool is something like an actionsForMimetype method. I was also planning to work on adding actions to the xesam runner which would have benefited from this... But I'm also considering just using a dummy QMenu, populating it via libkonq and getting the actions added to that QMenu.

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.


> On 2008-09-17 08:34:10, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/krunner/resultitem.cpp, lines 680-681
> > <http://reviewboard.vidsolbach.de/r/178/diff/3/?file=1034#file1034line680>
> >
> >     setting a title on a QMenu doesn't actually do anything unless the menu is put into a submenu; KMenu lets you put a visible title in a menu, if that's what you're looking for.
> >     
> >     if the menu title is visible in the menu itself (as opposed to a sub-menu entry that shows the menu), it should contain the name of the action. i18n("Actions For '%1'", name()) or something like that ...

This isn't the intended front end for actions. I only added this for the sake of seeing them in the default KRunner. The way actions are treated in QuickSand are my preferred way of dealing with them. I didn't want to think of some new way of handling them in the current interface so I just put a context menu. :)


> On 2008-09-17 08:34:10, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/querymatch.h, line 177
> > <http://reviewboard.vidsolbach.de/r/178/diff/3/?file=1044#file1044line177>
> >
> >     "current" sounds like an iterator; perhaps "selected" or "active"? setSelectedAction sounds good to me ..

Agreed. It was a remnant of the old patch and I couldn't think of an appropriate method at the time.


> On 2008-09-17 08:34:10, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/plasma/queryactionpool.h, line 43
> > <http://reviewboard.vidsolbach.de/r/178/diff/3/?file=1042#file1042line43>
> >
> >     for applications that use runners internally, versus an application like krunner which is only about runners, the constructor will be used (think of multiple kickoff applets, for instance?); perhaps the apidox should be altered to make self() sound less required =)

:)


- Ryan


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


On 2008-09-17 02:11:27, Ryan Bitanga wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.vidsolbach.de/r/178/
> -----------------------------------------------------------
> 
> (Updated 2008-09-17 02:11:27)
> 
> 
> 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.
> 
> As of the latest patch, registration and unregistration from the global action registry is handled by the QueryAction itself. Actions are added by the actionsForMatch() method in abstract runner allowing on-demand creation of actions if so desired.
> 
> 
> 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/abstractrunner.h
>   /trunk/KDE/kdebase/workspace/libs/plasma/abstractrunner.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/queryactionpool.h
>   /trunk/KDE/kdebase/workspace/libs/plasma/queryactionpool.cpp
>   /trunk/KDE/kdebase/workspace/libs/plasma/querymatch.h
>   /trunk/KDE/kdebase/workspace/libs/plasma/querymatch.cpp
>   /trunk/KDE/kdebase/workspace/libs/plasma/runnermanager.h
>   /trunk/KDE/kdebase/workspace/libs/plasma/runnermanager.cpp
> 
> Diff: http://reviewboard.vidsolbach.de/r/178/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ryan
> 
>