----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/637/#review1000 -----------------------------------------------------------
other than moving the config method to Corona (nice symmetry with containment->config() and applet->config()) it looks good. /trunk/KDE/kdelibs/plasma/applet.h <http://reviewboard.kde.org/r/637/#comment682> i think this should just be KConfigGroup Corona::config() const ... would make the implementation of it a bit cleaner; it means having to do KConfigGroup(corona()->config(), "Shortcuts") as well in applet.cpp, but it seems the config getter method would make more sense in Corona. - Aaron On 2009-04-28 12:19:48, Chani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/637/ > ----------------------------------------------------------- > > (Updated 2009-04-28 12:19:48) > > > Review request for Plasma. > > > Summary > ------- > > this is just the code to read shortcuts from the rc file. it works, but I'm > not 100% happy with it - that function added to Applet feels awkward and its > name sucks, and I can't help wondering if maybe I should put it somewhere > else, or duplicate it in containment so that it can be private... any > thoughts? > > > Diffs > ----- > > /trunk/KDE/kdelibs/plasma/applet.h 960161 > /trunk/KDE/kdelibs/plasma/applet.cpp 960161 > /trunk/KDE/kdelibs/plasma/containment.cpp 960161 > /trunk/KDE/kdelibs/plasma/corona.cpp 960161 > > Diff: http://reviewboard.kde.org/r/637/diff > > > Testing > ------- > > > Thanks, > > Chani > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
