'Re: Review Request: Multiple action support for KRunner'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: Multiple action support for KRunner
From:       "Ryan Bitanga" <ryan.bitanga () gmail ! com>
Date:       2008-09-18 6:46:33
Message-ID: 20080918064633.7489.29264 () vidsolbach ! de
[Download RAW message or body]

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

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

[prev in list] [next in list] [prev in thread] [next in thread] 