D26035: battery: Improve the brightness responsiveness
frenzie added inline comments. INLINE COMMENTS > PopupDialog.qml:73 > KeyNavigation.backtab: batteryList > +stepSize: batterymonitor.maximumScreenBrightness/100 > + Because it's artificially limited to 100 positions, this change makes it impossible to set a value by clicking on a position or by dragging the slider with the pointer. It snaps to one of the 100 positions and the precision we want just isn't there. On my laptop the slider currently consists of ~582 pixels in width. (Just a quick screenshot selection-based estimate; the exact value is irrelevant other than to point out that it's very unlikely to be less than ~450 and extremely likely to be much more.) The organic limit is therefore 582 on my laptop. In the example given above, 7000/582 = 12. I assume there has to be reason for this change, so an organic limit would mean something along the lines of: stepSize: batterymonitor.maximumScreenBrightness / this.width But intuitively it seems that a slider would have to do that all by itself no matter what, which just leaves me confused as to what the purpose behind this line is. I hope it can simply be removed. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26035 To: apol, #plasma, broulik Cc: frenzie, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26035: battery: Improve the brightness responsiveness
This revision was automatically updated to reflect the committed changes. Closed by commit R120:62be85f4360d: battery: Improve the brightness responsiveness (authored by apol). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26035?vs=71743=72439 REVISION DETAIL https://phabricator.kde.org/D26035 AFFECTED FILES applets/batterymonitor/package/contents/ui/BrightnessItem.qml applets/batterymonitor/package/contents/ui/PopupDialog.qml applets/batterymonitor/package/contents/ui/batterymonitor.qml applets/batterymonitor/package/contents/ui/logic.js dataengines/powermanagement/powermanagementjob.cpp dataengines/powermanagement/powermanagementjob.h To: apol, #plasma, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26035: battery: Improve the brightness responsiveness
broulik accepted this revision. broulik added a comment. This revision is now accepted and ready to land. Seems to work well, thanks REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D26035 To: apol, #plasma, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26035: battery: Improve the brightness responsiveness
apol added a comment. https://phabricator.kde.org/D26035 has been landed. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26035 To: apol, #plasma, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26035: battery: Improve the brightness responsiveness
apol marked an inline comment as done. apol added inline comments. INLINE COMMENTS > broulik wrote in logic.js:53 > Wouldn't that cause subsequent calls to fail if the previous hasn't finished > yet? Wouldn't that be annoying if you drag the slider quickly? No, it will be called eventually, which is all that matters. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D26035 To: apol, #plasma, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26035: battery: Improve the brightness responsiveness
apol updated this revision to Diff 71743. apol added a comment. Use a more meaningful step size for the screen. Having steps of 1 in a 1 to 7000 slider was awkward. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26035?vs=71643=71743 BRANCH master REVISION DETAIL https://phabricator.kde.org/D26035 AFFECTED FILES applets/batterymonitor/package/contents/ui/BrightnessItem.qml applets/batterymonitor/package/contents/ui/PopupDialog.qml applets/batterymonitor/package/contents/ui/batterymonitor.qml applets/batterymonitor/package/contents/ui/logic.js dataengines/powermanagement/powermanagementjob.cpp dataengines/powermanagement/powermanagementjob.h To: apol, #plasma, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26035: battery: Improve the brightness responsiveness
broulik added a comment.
`PlasmaComponents` 3 `Slider` doesn't react to wheel events, which must be
fixed before this can go in.
INLINE COMMENTS
> PopupDialog.qml:48
> -
> -keyboardBrightnessSlider.valueChanged.connect(function() {
> -batterymonitor.keyboardBrightness =
> keyboardBrightnessSlider.value
What about this?
> batterymonitor.qml:109
> +property QtObject updateScreenBrightnessJob
> onScreenBrightnessChanged: {
> if (disableBrightnessUpdate) {
Should we not forward the notion of "moved" here, too?
> logic.js:53
> function updateBrightness(rootItem, source) {
> +if (rootItem.updateScreenBrightnessJob ||
> rootItem.updateKeyboardBrightnessJob)
> +return;
Wouldn't that cause subsequent calls to fail if the previous hasn't finished
yet? Wouldn't that be annoying if you drag the slider quickly?
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D26035
To: apol, #plasma, broulik
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2,
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
D26035: battery: Improve the brightness responsiveness
apol created this revision. apol added reviewers: Plasma, broulik. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. apol requested review of this revision. REVISION SUMMARY Use QQC2.Slider, so that we have a moved signal. This way we can only issue new brightnesses when the user actually interacts with it. Don't adapt to the system brightness until we have finished interacting with it. TEST PLAN Manual testing, flickering is very much reduced both when scrolling over the compact plasmoid as well as the slider. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D26035 AFFECTED FILES applets/batterymonitor/package/contents/ui/BrightnessItem.qml applets/batterymonitor/package/contents/ui/PopupDialog.qml applets/batterymonitor/package/contents/ui/batterymonitor.qml applets/batterymonitor/package/contents/ui/logic.js dataengines/powermanagement/powermanagementjob.cpp dataengines/powermanagement/powermanagementjob.h To: apol, #plasma, broulik Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
