Review Request: Window runner to switch windows and desktops

[Martin Gräßlin]

> On 2009-07-26 19:19:43, Aaron Seigo wrote:
> > ok, now the implementation review part of things :)
> > 
> > one thing that is useful to observe about this is that it takes a more memory intensive approach, storing the individual icons, window info and winid of all available windows, to make matching (at least in theory?) faster.
> > 
> > the downside is that krunner is going to wake up (and therefore stay in main memory and be executing code) whenever windows change. that may not be exactly optimal ;)
> > 
> > the windowinfo isn't actually used except for when one of the keywords like desktop is used, and the icons aren't used unless a match is made. but fetching the list of windowinfo in each pass through match can't be great either.
> > 
> > what would really be useful here would be some way to tell the runner "you should be prepping yourself for use now" and then later "ok, we're done with this search session". krunner itself could delete the runners between invocations of the interface, but depending on the runner that can be rather slow-ish and if there are runners that ever require a constant and stateful connection to something external that would hurt.
> > 
> > so it would seem we need some mechanism to prod a runner to get ready for searches and then let it know again when it can not worry about it at all. that way this runner could populate the windowInfo collection and only pay attention to window changes when the krunner interface is actually activated by the user.
> >

Of course I didn't want to cache all data. The problem is that when I fetch the data during execeution, KRunner crashes somewhere in the upper Qt stack :-( I rarely saw DrKonqui and it looks like the problem was in xcb not being able to handle so many request at the same time.

So yes that preparing to run and finishing afterwards sounds like a good solution. That way nothing would need to be cached and KRunner could remain sleeping.


> On 2009-07-26 19:19:43, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/runners/windows/windowsrunner.cpp, lines 187-188
> > <http://reviewboard.kde.org/r/1114/diff/4/?file=9093#file9093line187>
> >
> >     plasma style is " } else if (..) {"
> >     
> >     if this is meant to go into kdebase (where i thin it does belong) those ws changes will need to be made at some point :)

no problem - I'll change. Btw. that rule is missing on http://techbase.kde.org/Policies/Kdelibs_Coding_Style


> On 2009-07-26 19:19:43, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/plasma/runners/windows/windowsrunner.cpp, line 206
> > <http://reviewboard.kde.org/r/1114/diff/4/?file=9093#file9093line206>
> >
> >     don't use keys(); it's a very slow mechanism (iterates over the collection, allocates a new list, and then you iterate over that again in the foreach). should use a QMapIterator here instead

thanks for pointing out. I'll change all of those.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1114/#review1793
-----------------------------------------------------------


On 2009-07-26 12:34:22, Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1114/
> -----------------------------------------------------------
> 
> (Updated 2009-07-26 12:34:22)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This runner lists the windows and virtual desktops. It allows to activate a selected window or switch to a selected desktop. The runner works in the following way:
>  * input is matched on window name, class or role; matching windows are listed
>  * input is matched on desktop name. Matching desktops are shown for switching to, all windows on matching desktops are listed.
>  * keyword "desktop": lists all desktops (except current) if additional number is inserted the list is reduced to that desktop
>  * keyword "window": lists all windows. Additional text will be used to restrict like in first case. Following sub queries to restrict search are possible:
>    * name=: restrict on name
>    * class=: restrict on window class
>    * role=: restrict on window role
>    * desktop=: restrict on desktop
>   those subqueries can be combined in any order. Each input not containing a '=' will be interpreted as a name restriction if there is no explicit name restriction. In that case it's ignored. Example query: "window desktop=2 class=kmail role=composer" will list all open KMail composer windows on desktop 2.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/plasma/runners/CMakeLists.txt 1000707 
>   trunk/KDE/kdebase/workspace/plasma/runners/windows/CMakeLists.txt PRE-CREATION 
>   trunk/KDE/kdebase/workspace/plasma/runners/windows/plasma-runner-windows.desktop PRE-CREATION 
>   trunk/KDE/kdebase/workspace/plasma/runners/windows/windowsrunner.h PRE-CREATION 
>   trunk/KDE/kdebase/workspace/plasma/runners/windows/windowsrunner.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/1114/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin
> 
>