Review Request: Display brightness OSD notification and change brightness when Fn keys are unhandled

[Dario Freddi]

-----------------------------------------------------------
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
> 
>