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

[Rob Scheepmaker]

> On 2009-03-17 10:02:31, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/extenderitem.h, line 187
> > <http://reviewboard.kde.org/r/344/diff/1/?file=3132#file3132line187>
> >
> >     need apidox :)
> >     
> >     but do we even need this? one should be able to qobject_cast as needed?

Yeah, that is certainly a possibility. I like this way because it's analog to for example isWidget(). Shorter and more readable. If you think this is needless api bloat I am willing to remove this function though.


> On 2009-03-17 10:02:31, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/extender.cpp, line 213
> > <http://reviewboard.kde.org/r/344/diff/1/?file=3129#file3129line213>
> >
> >     how about:
> >     
> >     foreach (ExtenderItem *item, attachedItems()) {
> >          if (item->isGroup()) {
> >                ..
> >         }
> >     }

Oops, you're right, I missed that one.


> On 2009-03-17 10:02:31, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/extendergroup.cpp, line 118
> > <http://reviewboard.kde.org/r/344/diff/1/?file=3131#file3131line118>
> >
> >     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.

Yeah, that was exactly what I was thinking when I decided to do this the way I did. If it turns out applets start using such large amount of items that this becomes a problem, we can always change to using a QList instead.


> On 2009-03-17 10:02:31, Aaron Seigo wrote:
> > /trunk/KDE/kdelibs/plasma/extenderitem.cpp, line 109
> > <http://reviewboard.kde.org/r/344/diff/1/?file=3133#file3133line109>
> >
> >     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.

Agreed. I'll update the patch soon.


- Rob


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


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
> 
>