<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://git.reviewboard.kde.org/r/108355/">http://git.reviewboard.kde.org/r/108355/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 12th, 2013, 12:18 a.m. UTC, <b>Kai Uwe Broulik</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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="http://git.reviewboard.kde.org/r/108355/diff/2/?file=106715#file106715line97" style="color: black; font-weight: bold; text-decoration: underline;">plasma/generic/applets/batterymonitor/contents/code/logic.js</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<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; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">92</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// we don't want passive brightness change send setBrightness call</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">But isn't it the dataengine that prematurely triggers a brightnes change and OSD? When I restart plasma or use plasmaengineexplorer and access the PM dataengine, the screen brightness changes and the OSD appears. Shouldn't it be fixed inside the dataengine?</pre>
</blockquote>
<p>On January 12th, 2013, 12:53 a.m. UTC, <b>Xuetian Weng</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Restart Plasma shows OSD because you have battery plasmoid.
I don't see plasmaengineexplorer shows osd, unless you use setBrightness service.
The reason of this bug is, Slider have a value, when it's not equal to current brightness, it will trigger valueChanged, and then trigger brightnessChanged and send a call to dataengine service. If brightness value is not changed from user operation on slider, it should never trigger this call.
If the fix in dataengine you mentioned is:
dataengine check the current brightness and argument of setBrightness service is same or not, to decide whether to call powerdevil or not.
I don't think it's the right approach, even if it can solve the problem. Reason is, real brightness is saved on powerdevil side, which is a different process. The value in dataengine is only cached value. If setBrightness comes, there is no guarantee that this value is up-to-date (even it's just theoretically possible), so if setBrightness comes, it's always safe to send a real request to powerdevil. And the value send to powerdevil will cause powerdevil to set current brightness is explicit set, and do a lot of corresponding process, which should never be ignored just because a cached value is the current value.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">the cached value will be correct all the time, except when a new value hasn't yet made the trip from powerdevil over dbus yet. which means it will only not make the call when it is the same in the DataEngine even though it may be different in PowerDevil. however, this doesn't matter because the visualization (e.g. the battery monitor) is ALSO getting its data from the DataEngine: so for all intents and purposes if the DataEngine says its 50% bright, it's 50% bright.
the only caveat is that due to the race condition inherent here, the brightness could already be 60% in powerdevil but the DataEngine doesn't yet know and lets the user set it again to 60%. however, as this is user input, they probably DO know what they want (60%, e.g.) whether or not the DataEngine knows.
so i'd suggest that while what Xuetian describes is technically correct, it won't ever matter in real world usage. as such, it would be good to see a change in the DataEngine as well that addresses this issue so future plasmoids (or other visualizations) don't end up re-creating this same bug.
that said, i also don't understand why the OSD is shown when the actual brightness value doesn't change. might be something for powerdevil itself to catch, and then we know that the values are correct there (no possibility of race condition at all)</pre>
<br />
<p>- Aaron J.</p>
<br />
<p>On January 11th, 2013, 11:37 p.m. UTC, Xuetian Weng wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma, Aaron J. Seigo, Viranch Mehta, and Viranch Mehta.</div>
<div>By Xuetian Weng.</div>
<p style="color: grey;"><i>Updated Jan. 11, 2013, 11:37 p.m.</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; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1. fix access undefined value warning, when calling function in logic.js, pmSource.data can still be null since it might not connect at that time.
2. when brightness is passively changed from outer environment, don't trigger the dataSource service call with a bool protector called disableBrightnessUpdate, this fix bug 302130
3. move more code to DataModel.onDataChanged, under the same idea with: https://git.reviewboard.kde.org/r/108280/ , hence the plasmoid.status will always be the correct value, the changing of plasmoid.status cause bug 311491
4. remove ugly hack of resetBatteryData in logic.js.
5. fix some anchor related warning in PopupDialog.qml
QML Column: Cannot anchor to an item that isn't a parent or sibling.</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; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">tested, no problem.</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="http://bugs.kde.org/show_bug.cgi?id=302130">302130</a>,
<a href="http://bugs.kde.org/show_bug.cgi?id=311491">311491</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>plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml <span style="color: grey">(5bd21a1)</span></li>
<li>plasma/generic/applets/batterymonitor/contents/code/logic.js <span style="color: grey">(7843245)</span></li>
<li>plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml <span style="color: grey">(eebe4fe)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108355/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>