----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1942/#review5536 -----------------------------------------------------------
Ship it! Looks good - just one minor annoyance in the API I'd like to get fixed before committing. Anyway, considered that in 4.6 S::C will disappear, it's a very minor thing. So commit straight away right after. trunk/KDE/kdebase/workspace/libs/solid/control/powermanager.h <http://reviewboard.kde.org/r/1942/#comment5210> This is not very explainatory in my opinion: brightnessKeyPressed(false); looks more like something has not been pressed. Maybe an enum with Increase/Decrease as values would be better here. - Dario On 2010-05-05 20:58:51, Felix Geyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/1942/ > ----------------------------------------------------------- > > (Updated 2010-05-05 20:58:51) > > > Review request for Plasma and Dario Freddi. > > > Summary > ------- > > This patch addresses the following issues: > - KDE doesn't provide an OSD notification when the user changes the > display brightness. > - The brightness Fn keys don't have an effect if they aren't handled by > the kernel/hardware. > > Implementation overview: > PowerDevil uses a KActionCollection to register the > Qt::Key_MonBrightnessUp/Down keys > and forwards them to Solid. > Solid then decides if the brightness should be increased/decreased by > comparing the > current brightness value with the cached one. The brightness is only changed > if both are equal and the brightness_in_hardware HAL property is false. > This prevents a double increase/decrease of the brightness and is consistent > with > gnome-power-manager using the HAL backend. > In any case PowerDevil emits the brightnessChanged() dbus signal. > The battery applet listens to that signal and displays an OSD. > It uses an adapted copy of the OSDWidget class from KMix. This is suboptimal > but KDE > doesn't provide a generic way to show this kind of OSD yet. > > > This addresses bugs 182400 and 187960. > https://bugs.kde.org/show_bug.cgi?id=182400 > https://bugs.kde.org/show_bug.cgi?id=187960 > > > Diffs > ----- > > trunk/KDE/kdebase/workspace/libs/solid/control/ifaces/powermanager.h > 1122901 > trunk/KDE/kdebase/workspace/libs/solid/control/powermanager.h 1122901 > trunk/KDE/kdebase/workspace/libs/solid/control/powermanager.cpp 1122901 > trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/CMakeLists.txt > 1122901 > trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.h > 1122901 > trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp > 1122901 > > trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/brightnessosdwidget.h > PRE-CREATION > > trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/brightnessosdwidget.cpp > PRE-CREATION > trunk/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.h 1122901 > trunk/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.cpp 1122901 > trunk/KDE/kdebase/workspace/powerdevil/daemon/org.kde.PowerDevil.xml > 1122901 > trunk/KDE/kdebase/workspace/solid/hal/halpower.h 1122901 > trunk/KDE/kdebase/workspace/solid/hal/halpower.cpp 1122901 > > Diff: http://reviewboard.kde.org/r/1942/diff > > > Testing > ------- > > This patch in a slightly different implementation (some code moved from > PowerDevil to Solid) > is being shipped with Kubuntu Karmic and Lucid and I think Fedora. I haven't > seen any bug reports > concering the OSD or incorrect brightness changes. > > Apart from that I tested the changes on my laptop and have verified that the > code correctly > respects the HAL property. > > > Thanks, > > Felix > > _______________________________________________ Plasma-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/plasma-devel
