Review Request: Add grouping support to extenders and use this feature in the systemtray to group jobs.

[Aaron Seigo]

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


direction looks good. some small things to improve i think :)


/trunk/KDE/kdelibs/plasma/extender.cpp
<http://reviewboard.kde.org/r/344/#comment306>

    how about:
    
    foreach (ExtenderItem *item, attachedItems()) {
         if (item->isGroup()) {
               ..
        }
    }



/trunk/KDE/kdelibs/plasma/extendergroup.cpp
<http://reviewboard.kde.org/r/344/#comment303>

    seems a bit of a shame to have to go through all the items in an extender like this. i suppose, however, there won't ever be that many items so the overhead will be low and the cost of bookkeeping with an internal QList in ExtenderGroup would not pay off at all while increasing memory usage and complexity. bleh.



/trunk/KDE/kdelibs/plasma/extenderitem.h
<http://reviewboard.kde.org/r/344/#comment304>

    need apidox :)
    
    but do we even need this? one should be able to qobject_cast as needed?



/trunk/KDE/kdelibs/plasma/extenderitem.cpp
<http://reviewboard.kde.org/r/344/#comment305>

    would make sense to have an ExtenderPrivate::findGroup(const QString&) for this to prevent cycling through all items, creating a list, then cycling through that list when its returned. doesn't need to be in the public api imho, but we can make the internals more efficient here.


- Aaron


On 2009-03-16 12:38:12, Rob Scheepmaker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/344/
> -----------------------------------------------------------
> 
> (Updated 2009-03-16 12:38:12)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> I've been working on adding support for grouping to extenders and using this to group jobs in the plasma systemtray: a useful feature we already discussed at Tokamak. I've already discussed the extender api bit at Tokamak with Aaron, but changed a couple of things about it while implementing because they could have been more elegant. There are still some relatively minor problems I know of, and it hasn't been that thoroughly tested yet, but I wanted to see if you think I'm going in the right direction here.
> 
> The basic idea is that to group extender items, you'll instantiate an ExtenderGroup, which is a subclass of ExtenderItem. The thing this class adds, are expand and collapse buttons, to show/hide the items that belong to this group, and the ability to let the group automatically hide itself whenever it becomes empty. To add items to a group you'll have to call setGroup on each ExtenderItem you wish to add to that group. This association with a group will get stored in the item's config(). For the rest it behaves like any other extender item. 
> 
> I will still have to add behavior that automatically moves all items belonging to this group with it, whenever you detach the ExtenderGroup. Also, the usage of groups in systemtray could still use improvement.
> 
> Screenshot showing the job grouping in action: http://quartz.student.utwente.nl/~rob/extendergrouping.png Clicking the arrow-down here would collapse this group (keeping only the total progress item)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/CMakeLists.txt 939911 
>   /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/core/job.h 939911 
>   /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/core/manager.h 939911 
>   /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/core/manager.cpp 939911 
>   /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/applet.cpp 939911 
>   /trunk/KDE/kdebase/workspace/plasma/applets/systemtray/ui/jobwidget.cpp 939911 
>   /trunk/KDE/kdelibs/plasma/CMakeLists.txt 939911 
>   /trunk/KDE/kdelibs/plasma/applet.h 939911 
>   /trunk/KDE/kdelibs/plasma/extender.h 939911 
>   /trunk/KDE/kdelibs/plasma/extender.cpp 939911 
>   /trunk/KDE/kdelibs/plasma/extendergroup.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/extendergroup.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/extenderitem.h 939911 
>   /trunk/KDE/kdelibs/plasma/extenderitem.cpp 939911 
>   /trunk/KDE/kdelibs/plasma/private/extenderitem_p.h 939911 
> 
> Diff: http://reviewboard.kde.org/r/344/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Rob
> 
>