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

[Sebastian Kügler]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1942/#review5450
-----------------------------------------------------------


Your changes to the battery applet look OK, I've only done a shallow review of the rest though, would be good if Dario looked over it as well.

In general, this is a very welcome addition, thanks for working on it!


trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp
<http://reviewboard.kde.org/r/1942/#comment5154>

    Any good reason to not use Qt's parenting mechanism to do the dirty memory management work?



trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/brightnessosdwidget.h
<http://reviewboard.kde.org/r/1942/#comment5155>

    Your copyright is missing :)



trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/brightnessosdwidget.cpp
<http://reviewboard.kde.org/r/1942/#comment5156>

    trailing whitespace



trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/brightnessosdwidget.cpp
<http://reviewboard.kde.org/r/1942/#comment5157>

    would be good to properly parent those items, to make clear that memory management is taken care of, it takes a bit of digging right now.
    
    I personally prefer 0-ing the pointers there, and then initialize them inside the function, as that makes the code more readable



trunk/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.cpp
<http://reviewboard.kde.org/r/1942/#comment5160>

    context for translators wouldn't hurt (i18nc())


- Sebastian


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