----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108570/#review26127 -----------------------------------------------------------
I've added a bunch of comments inline, those would need addressing. More important though: Could you attach a screenshot of the new UI so that this can be reviewed as well? plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19881> stray whitespace plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19880> if (...) { Brackets of functions (not methods!) on the same line plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19882> Where do the magic numbers come from? Try to avoid them, otherwise add a comment. plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19883> brackets around ifs, even when it's only one line plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19884> brackets plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19885> we usually prefix members with m_, not sure it's done in this class, if it is, please add m_ plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19886> stray whitespace plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19887> Again, magic numbers? Especially with geometry properties, this blows up as soon as font size changes. plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19892> i18nc comment won't hurt here plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19888> stray ws plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19889> brackets plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19890> hardcoded height, try using fontmetrics or KIconLoader sizes here, whichever fits plasma/generic/applets/systemtray/ui/applet.cpp <http://git.reviewboard.kde.org/r/108570/#comment19891> brackets plasma/generic/applets/systemtray/ui/autohide.ui <http://git.reviewboard.kde.org/r/108570/#comment19893> necessary to add width and height here? - Sebastian Kügler On Jan. 24, 2013, 5:34 a.m., Sandro Andrade wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/108570/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2013, 5:34 a.m.) > > > Review request for kde-workspace. > > > Description > ------- > > In page 'entries', user can select/deselect a specific or all entries and > then change visibility and/or reset shortcut for all of them with a single > combo selection/clear button click. Individually selecting all entries causes > header's checkbox to automatically be checked. After selecting all entries > (individually or by clicking in header's checkbox), a single entry > deselection causes header's checkbox to automatically be unchecked. General > suggestions and, in particular about UI usability, are appreciated. > > > Diffs > ----- > > plasma/generic/applets/systemtray/ui/applet.h 0b4a869 > plasma/generic/applets/systemtray/ui/applet.cpp 09482d7 > plasma/generic/applets/systemtray/ui/autohide.ui 3b6efff > > Diff: http://git.reviewboard.kde.org/r/108570/diff/ > > > Testing > ------- > > > Thanks, > > Sandro Andrade > >