[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: Display brightness OSD notification and change
From:       "Dario Freddi" <drf54321 () gmail ! com>
Date:       2010-05-09 12:00:42
Message-ID: 20100509120042.17240.93845 () localhost
[Download RAW message or body]


-----------------------------------------------------------
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
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel


[prev in list] [next in list] [prev in thread] [next in thread]