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

[Felix Geyer]

> On 2010-05-06 11:59:07, Sebastian Kügler wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp, line 131
> > <http://reviewboard.kde.org/r/1942/diff/2/?file=25713#file25713line131>
> >
> >     Any good reason to not use Qt's parenting mechanism to do the dirty memory management work?

m_brightnessOSD is a top-level window and thus can't have any parent.
Is it possible to set a parent only in the QObject/memory management sense but not as a QWidget/window parent?


> On 2010-05-06 11:59:07, Sebastian Kügler wrote:
> > trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/brightnessosdwidget.cpp, line 45
> > <http://reviewboard.kde.org/r/1942/diff/2/?file=25715#file25715line45>
> >
> >     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

You're right the parent relationsship between the objects is a bit obscure.
As said I only copied the code from KMix but I'll clean this up.


- Felix


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


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