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

[Sebastian Kügler]

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


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?

I've also added some comments inline.


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

    Just sender should be fine here, this part looks a little like you were trying to make sure you don't receive bogus signals. Any special case that I'm missing?



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

    This won't work if you check for the sender(), no?



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

    There's no user-visible distinction between the brightness sliders. How should the user tell which slider she's manipulating? Just adding every brightness device we find is a bit rough I think.



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

    remove this, and we have kDebug() :-)



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

    Is this really necessary? If at all, it should use Plasma's font, not KGlobalSetting's font.
    
    Anyway, what's the reason to do this?


- Sebastian


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/568e13fd/attachment.htm