Review Request 108355: various fix for battery plasmoid

Sat Jan 12 10:42:18 UTC 2013

> On Jan. 12, 2013, 12:18 a.m., Kai Uwe Broulik wrote:
> > plasma/generic/applets/batterymonitor/contents/code/logic.js, line 97
> > <http://git.reviewboard.kde.org/r/108355/diff/2/?file=106715#file106715line97>
> >
> >     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?
> 
> Xuetian Weng wrote:
>     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.

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)

- Aaron J.

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

On Jan. 11, 2013, 11:37 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108355/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2013, 11:37 p.m.)
> 
> 
> Review request for Plasma, Aaron J. Seigo, Viranch Mehta, and Viranch Mehta.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> This addresses bugs 302130 and 311491.
>     http://bugs.kde.org/show_bug.cgi?id=302130
>     http://bugs.kde.org/show_bug.cgi?id=311491
> 
> 
> Diffs
> -----
> 
>   plasma/generic/applets/batterymonitor/contents/ui/batterymonitor.qml 5bd21a1 
>   plasma/generic/applets/batterymonitor/contents/code/logic.js 7843245 
>   plasma/generic/applets/batterymonitor/contents/ui/PopupDialog.qml eebe4fe 
> 
> Diff: http://git.reviewboard.kde.org/r/108355/diff/
> 
> 
> Testing
> -------
> 
> tested, no problem.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20130112/c88891c1/attachment-0001.html>