Draft: Introduce setBrightnessMultiplier() as better dimming API
Builds on top of !361 (merged) and prior commits in the chain. Part of the ongoing effort of allowing per-display brightness controls ( #19 (closed)). CC @zamundaaa, @nclarius
Commit message
Prior to this commit, DimDisplay
used to track brightness values
of the only display it was told about by ScreenBrightnessController
.
When dimming stopped, the original brightness value was restored.
This doesn't work very well when multiple displays are present,
each presumably with a different brightness value. DimDisplay
will
have an even harder time with edge cases such as restoring
brightness values for a display that was temporarily unplugged.
ScreenBrightnessController
is a better place for this complexity.
It can exempt displays from dimming, reset the newly introduced
brightness multiplier to 1.0 when a new display or external
brightness change is detected, and pretend that brightness is
still the same to avoid confusing other clients.
I'm a little uncomfortable with all the various flows within
onDetectorDisplaysChanged()
, and ideally we should add autotests
for some of the expected scenarios, but at least it's all localized
in one function. The rest is pretty straightforward.
CCBUG: 476550 (partial fix)
Bugs introduced
Great section to have in an MR, right?
This will behave poorly when self-induced brightness changes are detected as external brightness changes by a DisplayBrightness
implementation. Reason for that being that an external brightness change will (1) adopt the externally changed brightness as the new 100% unmultiplied brightness, (2) reset dimming to 100% for all dimmable displays, and (3) dim it again later, but now down from the lower baseline that we incorrectly detected while dimming down.
In particular, this is bad for BacklightBrightness
which has unreliable change event suppression. We can fix this by either completely owning the brightness value, in which case we don't care about external changes and just overwrite them again next time, or by making change event suppression robust at least for dimming animations. Let's start the latter by getting !342 (merged) reviewed and merged, and then I'll submit the follow-up.
We can remove draft status once that's fixed and I've done some more testing with more recent revisions of this MR.
Conceptually though, I attempted to keep the behavior largely similar to the current one in order to avoid regressions in general. In particular, the multiplier only affects the "legacy API" display set a.k.a. anything other than the display(s) you can already control with the brightness slider. The idea is that we keep our existing features and bugs for legacy display behavior, everything else only reacts to direct slider drags and don't participate in dimming whatsoever. In case anything goes wrong, one can fix the brightness without per-display controls available in the applet yet. We can expand the scope at a later point.
Test plan
Various combinations of the following:
- Set brightness manually via applet or keyboard shortcut
- Idle until the dimming timeout and check if it dims correctly
- Use input devices to come back from dimming, check that the original brightness is back
- Plug/unplug displays to confuse the code, ideally while dimmed, even better: during the dimming brightness animation
- Change brightness via sysfs brightness file to confuse the code (e.g. SSH or
sleep
to write it while dimmed)
Not fixed, but related & making progress
-
CCBUG: 452492 (external screen brightness is not restored after wakeup)
- Plan for later: Introduce an additional
ScreenBrightnessController::setSuspended(bool)
, called from DPMS action, and restore correct (now available) dimming state/brightness uponsetSuspended(true)
.
- Plan for later: Introduce an additional
-
CCBUG: 448623 (Only laptop screen is dimmed, external monitors are not affected)
- Plan for later: Set
willUseMultiplier = true;
also for external monitors once we have proper wake-up & brightness storage figured out (currently only set for the legacy API display set).
- Plan for later: Set