Review Request: taskmanager library: Manual Sorting Fix

[Aaron Seigo]

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



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

    won't compile with pedantic due to the extra ';'; should really be in the implementation and not the header anyways.



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

    the groups in this collection are new'd, but never deleted. looks like a memory leak.



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

    wrong indent



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

    at this point the signals connected to/from the previous currentRootGroup() should be disconnected and the new currenRootGroup() should be connected up.
    
    otherwise, things like "group only when full" won't work properly.



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

    reload, or reconnect() like the above calls?



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

    more readable and consistent with totalSize below as if (groupable->isGroupItem())



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

    won't compile with pedantic


- Aaron


On 2009-10-19 21:10:07, Christian Mollekopf wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1526/
> -----------------------------------------------------------
> 
> (Updated 2009-10-19 21:10:07)
> 
> 
> Review request for Plasma, Aaron Seigo and Marco Martin.
> 
> 
> Summary
> -------
> 
> this fixes the manual sorting strategy, which is broken atm if the desktop is changed.
> 
> Since the manual sorting strategy relies on AbstractGroupableItem pointer not to change, we cannot remove it from the bookkeeping in case it returns (after a desktop change for instance).
> I don't know if this is acceptable because this results in the item never being removed from the itemList until the groupmanager instance is deleted (lifetime of the applet which created the instance).
> 
> Another option would be to identify tasks and groups by WId, which is a bit more complicated, but if you think it would be better/cleaner, i could supply a patch.
> 
> 
> This addresses bug 200255.
>     https://bugs.kde.org/show_bug.cgi?id=200255
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/tasks.h 1034424 
>   /trunk/KDE/kdebase/workspace/plasma/desktop/applets/tasks/tasks.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/manualsortingstrategy.h 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/manualsortingstrategy.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/manualgroupingstrategy.h 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/manualgroupingstrategy.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.h 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.cpp 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h 1034424 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.h 1034424 
> 
> Diff: http://reviewboard.kde.org/r/1526/diff
> 
> 
> Testing
> -------
> 
> Tried it, works.
> 
> 
> Thanks,
> 
> Christian
> 
>