----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.vidsolbach.de/r/335/#review324 -----------------------------------------------------------
other than the coding style (we use the kdelibs style) this patch looks ok. and yes, having an OSD for this at some point would be nice =) my only concern is whether or not the battery applet is the "right" place for this. i think this makes more sense either as a kded module or in the desktop shell itself. even if the battery applet isn't around, those keys should probably still work, after all. transplanting this code to one of those two places would be fine by me. in fact, what we could do is put this in the desktop shell in a class that can handle these sort of keypresses and then we have one place to add an OSD later as well. turning it into a kded module later, if that is desired, would be a matter of taking and wrapping that one class, and that can be done at any later point in time. so, summary: * code: yes * style: needs fixing * location: move to desktop shell (workspace/plasma/shells/desktop) trunk/KDE/kdebase/workspace/plasma/applets/battery/battery.cpp <http://reviewboard.vidsolbach.de/r/335/#comment273> if (!installed) { - Aaron On 2009-01-17 20:19:37, Matt Rogers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.vidsolbach.de/r/335/ > ----------------------------------------------------------- > > (Updated 2009-01-17 20:19:37) > > > Review request for Plasma. > > > Summary > ------- > > This patch adds support for laptop brightness keys on X11. Qt doesn't support > these keys yet, so this has to be implemented in a platform specific way and > I have provided support for them on X11. > > There is no OSD or other indication that the keys work other than the change > in brightness. > > > Diffs > ----- > > trunk/KDE/kdebase/workspace/plasma/applets/battery/CMakeLists.txt > trunk/KDE/kdebase/workspace/plasma/applets/battery/battery.h > trunk/KDE/kdebase/workspace/plasma/applets/battery/battery.cpp > > Diff: http://reviewboard.vidsolbach.de/r/335/diff > > > Testing > ------- > > compiled, installed, and verified the keys do work. > > > Thanks, > > Matt > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
