[Battery Monitor] avoid invalid brightness setting

Review Request #128098 - Created June 4, 2016 and updated

Information
Peter Wu
plasma-workspace
356132
Reviewers
broulik
When starting plasma with the Battery Monitor widget in an expanded
state, the brightness is somehow set to 1. On a laptop with intel_video
backlight having a maximum of 416, this is effectively unreadable black.

This probably happened:
1. batterymonitor is being initialized, screenBrightness is still invalid.
2. brightnessSlider is being constructed, initializing from these invalid
   values. onValueChanged is invoked, but since the value is invalid, the
   minimum valid value (1) is used.
3. Now batterymonitor.screenBrightness is set, which triggers the
   onScreenBrightnessChangd event (which triggers
   modifies brightnessSlider.valueChanged, but since 1 = 1,
   onScreenBrightnessChangd is not called again). Also as a side-effect, the
   powerdevil daemon is asked to change the brightness to 1.
4. The screen finally becomes black.

BUG: 356132

Applied patch, re-login. Screen brightness is no longer set to 1. Also applied various debugging prints that shows that the screenBrightness is not set to insane values. Can also be tested with plasmoidviewer (change 10 to 1 for Plasmoid.switchWidth, likewise for switchHeight), this also changes the brightness level (but does not have the below slider display bug).

NOTE: the brightness slider is still invisible (but scrolling works), ~~a future patch should probably modify the maximum value when the brightness becomes valid~~. I have no idea why it is hidden, setting visible or maximumValue to 0 does not have the same effect. Anyway, scrolling works, so this is a cosmetic issue that can be fixed later if desired.

Issues

  • 0
  • 1
  • 0
  • 1
Description From Last Updated
Kai Uwe Broulik
Peter Wu
Peter Wu
Review request changed

Testing Done:

~  

Applied patch, re-login. Screen brightness is no longer set to 1. Also applied various debugging prints that shows that the screenBrightness is not set to insane values. Can also be tested with plasmoidviewer, this also changes the brightness level (but does not have the below slider display bug).

  ~

Applied patch, re-login. Screen brightness is no longer set to 1. Also applied various debugging prints that shows that the screenBrightness is not set to insane values. Can also be tested with plasmoidviewer (change 10 to 1 for Plasmoid.switchWidth, likewise for switchHeight), this also changes the brightness level (but does not have the below slider display bug).

   
   

NOTE: the brightness slider is still invisible (but scrolling works), ~~a future patch should probably modify the maximum value when the brightness becomes valid~~. I have no idea why it is hidden, setting visible or maximumValue to 0 does not have the same effect. Anyway, scrolling works, so this is a cosmetic issue that can be fixed later if desired.

Peter Wu

Any other issues with the patch that needs to be addressed?

  1. Maybe you can bind function on property changed
    onIsBrightnessAvailableChanged:
    if (!isBrightnessAvailable) {
    return
    }
    brightnessSlider.valueChanged.connect(function() {
    batterymonitor.screenBrightness = brightnessSlider.value
    })
    I have experience but look the same or some invalid value is triggered
    1. Suspend laptop
    2. Connect power supply
    3. Resume laptop -> screen brightness is 100%

Loading...