Review Request: various fix for battery plasmoid

[Xuetian Weng]

> 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?

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.


- Xuetian


-----------------------------------------------------------
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/fe92e5a2/attachment.html>