Review Request: battery: Support for any amount of brightness devices

[Rafa? Mi?ecki]

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


> I understand it in principle, but would like to know which specific case this one fixes. Can you give an example of what works with the patch that doesn't work without it?

Please see bug #199520 - we display slider even when there is not brightness device to control. I reproduced this locally with my notebook by removing "video" or going back to 2.6.32 where there wasn't support for brightness in case of my machine.

Actually I just wanted to enable/disable slider when there is (not) brightness device. Then I noticed there can be multiple devices in theory, so decided to make it cover all cases instead of later fixing.

It's simply _confusing_ to display brightness slider (which can be even used!) when we don't really have brightness device.


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

    brightnessChanged is slot for Slider and without checking whole m_brightnessSliders I can not say what device is associated with slider that sent signal.
    Maybe you just did't note I use db->device as parameter of setBrightness call.



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

    Good catch, that's right. Could we actually drop whole Battery::setFullBrightness? It was introduced by sebas in 866073 and doesn't seem to be used anymore.



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

    Naming devices would be great, however names seems to be like: "/org/freedesktop/Hal/devices/computer_backlight".
    Should we just take untranslated "computer_backlight" part? Or should we enumerate them?
    
    As about adding every device, I don't think there will be (any/many) cases with more than 1 device. It's unlikely any user will be overhelmed with amount of devices.



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

    I have no idea, I've just copied current behavior. What should I do about this part, then?


- Rafa?


On 2010-07-30 08:23:16, Rafa? Mi?ecki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4796/
> -----------------------------------------------------------
> 
> (Updated 2010-07-30 08:23:16)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Currently we always display one brightness slider, even if there is not br device at all (see bug #199520) or if there are more devices that should be controlled.
> 
> With this patch we fetch all devices and generate one label&slider pair for each.
> 
> The most confusing thing for me in implementation was storing list of sliders assigned to devices names. For that purpose I introduced struct BrightnessSet. I don't know if it is clean and acceptable solution, however keeping something like
> QMap<QPair<Plasma::Label *, Plasma::Slider *> > *m_brightnessControlls
> didn't sound too great to me.
> 
> 
> This addresses bug 199520.
>     https://bugs.kde.org/show_bug.cgi?id=199520
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.h 1156862 
>   /trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp 1156862 
> 
> Diff: http://reviewboard.kde.org/r/4796/diff
> 
> 
> Testing
> -------
> 
> I don't have two brightness devices, so I didn't test that part.
> 
> After removing "video" kernel module I didn't get any slider which is good.
> 
> I also put some:
> qDebug() << "setting" << bs->device
> to make sure my list of names and sliders works fine.
> 
> 
> Thanks,
> 
> Rafa?
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20100730/98fe4785/attachment.htm