----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/277/#review468 -----------------------------------------------------------
Ship it! looks good from an implementation POV; i thought for a bit if there was a benefit to collapsing the three entries in each group into one, but that would just mean more text processing elsewhere and limit the future flexibility of this. /trunk/KDE/kdelibs/plasma/extender.cpp <http://reviewboard.kde.org/r/277/#comment287> personally i'd put the int comparison first in case the compiler can't figure out on its own that it's safe to rearrange this statement to do the cheaper bit first (looking at the generated asm would say for sure, i suppose) - Aaron On 2009-03-12 07:10:28, Rob Scheepmaker wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/277/ > ----------------------------------------------------------- > > (Updated 2009-03-12 07:10:28) > > > Review request for Plasma. > > > Summary > ------- > > Because ExtenderItems are persistent between sessions, applet's that use > extenders can't just blindly add ExtenderItems without checking if that item > is already there. The problem with using Extender's item(..) function for > this check, is that while plasma is starting, not all detached items might > have been instantiated already. This could lead to duplication of extender > items. For example: the way we currently solve this in libplasmaclock is by > using a single shot timer in init, so we give all extender items a chance to > load before we check if we already have a calendar. > With this patch that is no longer necessary. Besides the item(..) function we > have an hasItem(..) function that checks if the item exists at all, even if > it has not yet been instantiated. The way this works, is that every detached > item also gets an entry in a special DetachedItems section in the appletrc. > This entry contains just the name, id, and source applet, so we can scan that > information in hasItem(...) and get the desired behavior. > This might be optimized a bit in the future (it's global, so we only have to > scan that section once and cache the results), but currently the amount of > extender items that are being used is not that big. > > > Diffs > ----- > > /trunk/KDE/kdebase/workspace/libs/plasmaclock/clockapplet.cpp 938402 > /trunk/KDE/kdelibs/plasma/extender.h 938402 > /trunk/KDE/kdelibs/plasma/extender.cpp 938402 > /trunk/KDE/kdelibs/plasma/extenderitem.cpp 938402 > > Diff: http://reviewboard.kde.org/r/277/diff > > > Testing > ------- > > Replaced the single shot timer hack for the calendar in libplasmaclock with > a hasItem call. Works perfectly. > > > Thanks, > > Rob > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
