[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-08 18:26:05
Message-ID: 20080908182605.5039.59659 () vidsolbach ! de
[Download RAW message or body]



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

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