> 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
