Skip to content

Hide and ignore "Change screen brightness" setting for systems without battery / internal display

So, this looks more daunting than it really is. One verbatim move of unrelated (brightness key) functionality that we don't want to break, one new helper class, and two complementary commits to limit the ScreenBrightnessControl action to built-in laptop/tablet/phone screens only, preventing confusion and potential race conditions. I'm hoping that it's not too much to go into 6.3 still, but let me know what you think.

Commit messages

actions/screenbrightnesscontrol: Only enable for systems with battery

Users with only the AC Power profile aren't getting anything useful from this setting in the Power Management KCM, it's rather more confusing than it helps. This change complements the previous commit that limited profile-based screen brightness changes to internal displays only.

Recommended alternatives for setting screen brightness are the Brightness and Color applet and the Display Configuration KCM, both of which allow setting brightness separately for each display.

actions/screenbrightnesscontrol: More robust against display changes

To avoid weird racy behavior in the case where an external monitor may have been added prior to the internal display, we now always limit the set of displays affected by profile state configuration to internal displays.

This will make the "Change screen brightness" option disappear from the Power Management KCM for systems without internal display. No big loss for users of desktop PCs with only external monitors and no batteries, their KCM is now tidier and Wayland users still have brightness sliders available in the Display Configuration KCM in addition to the Brightness and Color applet.

Also simplify conditions: the check for higher-than-min isn't needed, so we don't have to duplicate it in the displayAdded handler. ScreenBrightnessController::setBrightness() will already clamp brightness to a value between min and max brightness.

Lastly, profile brightness is now also applied on displayAdded. This will avoid future bugs in case we ever have more than one internal display, one that might turn off and back on. (The case of the first internal display appearing and disappearing is already handled by PowerDevil::Core deleting the action if unsupported.)

daemon/controllers: Introduce DisplayFilter helper class

Unlike the previously added DisplayMatch class, this can be used to match more than one display, every matching criterion is optional. A default-constructed filter includes all displays.

For now, it merely replaces similar checks in ScreenBrightnessController::tryMatchKScreenOutput(). The plan is to use it for dimming limit constraints as well.

daemon: Move brightness key handling to ScreenBrightnessAgent

Being located in the ScreenBrightnessControl action meant that we can't change the "supported" condition of the action while leaving brightness key shortcuts enabled in the right conditions.

Instead of making yet another "action" that isn't actually a profile action, we'll now move brightness key handling to ScreenBrightnessAgent (otherwise known to expose D-Bus interfaces) where it can exist regardless of Action::isSupported() results.

Bugs fixed

Requested reviewer(s)

Ideally @zamundaaa or @nicolasfella, but I'm happy about any reviews :)

Testing

On a system with no battery, or systems without internal display, or both, ensure that the "Change screen brightness" option is now hidden from Power Management settings. "Apply" should not change screen brightness, regardless of whether powerdevilrc still contains the checked setting for it.

On a laptop with internal display, ensure that "Change screen brightness" still exists for each power state tab and works (e.g. when plugging in or out). Even if the internal display is disabled, it should never change brightness of the external monitor.

Evil schemes

Just fyi - I'm hoping to use the DisplayFilter class for more stuff later on:

  • To help with implementing the remaining use cases from #38 (closed) (narrow down dimming or a temporary brightness override to certain displays),
  • Perhaps also to help with the brightness applet adjusting the currently active screen rather than all screens on applet icon scrolling,
    • (I'm thinking of exposing one filter as "auto" D-Bus object similar to logind "auto" user session objects, and letting the user control its constraints)
  • Ideally also to make D-Bus scripting more convenient for external apps/services in light of ever-changing display D-Bus IDs.

None of that is part of this MR, and none of that is set in stone (or even coded up) yet.

Merge request reports

Loading