<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://reviewboard.kde.org/r/4796/">http://reviewboard.kde.org/r/4796/</a>
</td>
</tr>
</table>
<br />
<pre>> 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.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/4796/diff/1/?file=31992#file31992line412" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">void Battery::hibernate()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">411</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">Solid</span><span class="o">::</span><span class="n">Control</span><span class="o">::</span><span class="n">PowerManager</span><span class="o">::</span><span class="n">setBrightness</span><span class="p">(</span><span class="n">brightness</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">411</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="k">for</span> <span class="p">(</span><span class="kt">int</span> <span class="n">i</span> <span class="o">=</span> <span class="mi">0</span><span class="p">;</span> <span class="n">i</span> <span class="o"><</span> <span class="n">m_brightnessSliders</span><span class="o">-></span><span class="n">size</span><span class="p">();</span> <span class="o">++</span><span class="n">i</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em">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.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/4796/diff/1/?file=31992#file31992line436" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">void Battery::setFullBrightness()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">426</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">brightnessChanged</span><span class="p">(</span><span class="mi">100</span><span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">435</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">brightnessChanged</span><span class="p">(</span><span class="mi">100</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em">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.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/4796/diff/1/?file=31992#file31992line528" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">void Battery::initExtenderItem(Plasma::ExtenderItem *item)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">527</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">foreach</span> <span class="p">(</span><span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">device</span><span class="p">,</span> <span class="n">controls</span><span class="p">.</span><span class="n">keys</span><span class="p">())</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em">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.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/4796/diff/1/?file=31992#file31992line643" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">void Battery::popupEvent(bool show)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">640</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">m_brightnessSliders</span><span class="o">-></span><span class="n">at</span><span class="p">(</span><span class="n">i</span><span class="p">).</span><span class="n">label</span><span class="o">-></span><span class="n">setFont</span><span class="p">(</span><span class="n">infoFont</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em">I have no idea, I've just copied current behavior. What should I do about this part, then?</pre>
</div>
<br />
<p>- Rafa?</p>
<br />
<p>On July 30th, 2010, 8:23 a.m., Rafa? Mi?ecki wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma.</div>
<div>By Rafa? Mi?ecki.</div>
<p style="color: grey;"><i>Updated 2010-07-30 08:23:16</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0;">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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=199520">199520</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.h <span style="color: grey">(1156862)</span></li>
<li>/trunk/KDE/kdebase/workspace/plasma/generic/applets/battery/battery.cpp <span style="color: grey">(1156862)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4796/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>