> 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. > > Aaron J. Seigo wrote: > 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) > > Kai Uwe Broulik wrote: > Looking at the code, the Dataengine calls PowerDevil's SetBrightness via > DBus which then calls the BrightnessControl action with the Explicit > parameter set which tells the action to always show the OSD, no matter what, > because the Explicit indicates that the user changed it and not some > automatic process (profile change, or similar)
Shame on me for not looking at the DataEngine code. (I was running out the door to an appointment, but that's really not a good excuse). If that is indeed the case, then the DataEngine absolutely also needs a fix. It will not be 100% perfect due to the race condition Xuetian points out, but it will work in nearly all real world cases and should have no bad effect if it doesn't get this right (the user can always hit the brightness up/down button again / change the slider :) - 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
