Review Request: Add 'Show only tasks from the current screen' to Task Manager

[Aaron Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://matt.rogers.name/r/134/#review126
-----------------------------------------------------------


in general looks good, just a couple of issues to work out. the signal double connection issue is the biggest one, and it would be good to get CPU usage down in checkScreenChange.


/trunk/KDE/kdebase/workspace/plasma/applets/tasks/tasks.cpp
<http://matt.rogers.name/r/134/#comment95>

    this is going to get called very often, pretty much once per pixel moved. you probably only want to check every so often, otherwise you'll spend a lot more CPU on this than you really want to.
    
    one approach is to put the changed windows in a list and start a timer that triggers a check later.
    
    try putting some debug output in this method, though, to see just how often it gets called.



/trunk/KDE/kdebase/workspace/plasma/applets/tasks/tasks.cpp
<http://matt.rogers.name/r/134/#comment94>

    this has the same double connection problem as below.



/trunk/KDE/kdebase/workspace/plasma/applets/tasks/tasks.cpp
<http://matt.rogers.name/r/134/#comment93>

    not related to your commit here so much, but it would be cool if TaskManager had a trackGeometry(bool) which counted how many times it was called with true, so that calling it an equal number of times with false would turn it off. small optimization that probably matters very little in the real world though as people are not likely to turn this on then off and run like that for a considerable period of time. just a thought though, and not a blocker for this patch.



/trunk/KDE/kdebase/workspace/plasma/applets/tasks/tasks.cpp
<http://matt.rogers.name/r/134/#comment92>

    if reconnect() gets called 2 times in a row with m_showOnlyCurrentScreen, you'll end up with double connections.
    
    call disconnect first before the if and then in the if connect again.


- Aaron


On 2008-02-14 06:43:04, Petri Damstén wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://matt.rogers.name/r/134/
> -----------------------------------------------------------
> 
> (Updated 2008-02-14 06:43:04)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Add 'Show only tasks from the current screen' to Task Manager.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/applets/tasks/tasks.h
>   /trunk/KDE/kdebase/workspace/plasma/applets/tasks/tasks.cpp
>   /trunk/KDE/kdebase/workspace/plasma/applets/tasks/tasksConfig.ui
> 
> Diff: http://matt.rogers.name/r/134/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Petri
> 
>