Review Request: Launcher support for libtaskmanager

[Aaron Seigo]

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



/trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h
<http://reviewboard.kde.org/r/4585/#comment6289>

    almost sounds like we should replace isGroupItem() with a itemType() method that returns a Type enum. would allow us to more easily add new features in the future, and keeps the virtual table smaller.



/trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h
<http://reviewboard.kde.org/r/4585/#comment6288>

    please put the (even empty :) implementation into the .cpp file.



/trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h
<http://reviewboard.kde.org/r/4585/#comment6290>

    should be "const KUrl &url"



/trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
<http://reviewboard.kde.org/r/4585/#comment6291>

    no spaces before/after parens.
    
    may be worth caching item->task()->classClass().toLower()



/trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
<http://reviewboard.kde.org/r/4585/#comment6292>

    same as comments on line 290 above.



/trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
<http://reviewboard.kde.org/r/4585/#comment6293>

    looks indented incorrectly?



/trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h
<http://reviewboard.kde.org/r/4585/#comment6294>

    should be BSD'd like the rest of the lib.



/trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h
<http://reviewboard.kde.org/r/4585/#comment6295>

    please don't line up method names :)



/trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp
<http://reviewboard.kde.org/r/4585/#comment6297>

    would be very cool if this tracking could be encapsulated in libtaskamanager itself, but that can always be a second step...



/trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp
<http://reviewboard.kde.org/r/4585/#comment6296>

    should just be:
    
    return d->windowInstances == 0;



/trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/programgroupingstrategy.cpp
<http://reviewboard.kde.org/r/4585/#comment6300>

    there's no point in doing a dynamic_cast unless you check the value. in any case, taskItem already is a TaskItem in this context. looks like an unnecessary change :)



/trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp
<http://reviewboard.kde.org/r/4585/#comment6298>

    space between foreach and the opening (



/trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp
<http://reviewboard.kde.org/r/4585/#comment6299>

    why are launchers with no window instances removed from the member list?


- Aaron


On 2010-07-10 18:21:39, Anton Kreuzkamp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4585/
> -----------------------------------------------------------
> 
> (Updated 2010-07-10 18:21:39)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Adds support for Windows 7 like launchers in libtaskmanager.
> (I'm on holliday from 12th July until 1st August so I will not be able to reply during this time.)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/programgroupingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/CMakeLists.txt 1148442 
> 
> Diff: http://reviewboard.kde.org/r/4585/diff
> 
> 
> Testing
> -------
> 
> Tested with a small test-applett and everything works.
> 
> 
> Thanks,
> 
> Anton
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20100713/0de46ebc/attachment-0001.htm