Skip to content

Improve per-display D-Bus API & refactor brightness actions

Jakob Petsovits requested to merge work/jpetso/brightness-actions-reorg into master

Following MR !361 (merged), I wanted to make some progress to stop using ScreenBrightnessController's legacy API methods so I was going to move the "action" part from the (now deprecated) BrightnessControl action into the new ScreenBrightnessControl action. But then realized that conflating an always-available D-Bus API and a conditionally-enabled brightness change action really shouldn't be co-located in a single action class.

So this MR introduces a stand-alone class for exposing the D-Bus API, called ScreenBrightnessAgent (after PolicyAgent) with org.kde.ScreenBrightness as a better, action-independent bus name. The recently introduced ScreenBrightnessControl action is still present and handles the aforementioned profile-based brightness change action as well as keyboard shortcuts. BrightnessControl is left with just the deprecated D-Bus API.

Taking a page from my other D-Bus improvement MR !423, I also changed the new D-Bus interface structurally. It now uses a tree of objects: the main org.kde.ScreenBrightness root, and display children with properties instead of root object getters. This is closer to the original root/display split I had intended before fitting everything into the mandatory single interface necessitated by the action. And also objectively better, see commit message below.

The MR is segmented into four commits, one for each moved piece of functionality:

daemon: Move brightness key handling to ScreenBrightnessControl action

The BrightnessControl action is deprecated and we want to move away from it.

daemon: Move the new ScreenBrightness D-Bus API out of actions

Being an action ties the interface to a particular path and interface name. It also ties the lifetime of the D-Bus API to the isSupported() method, which in turn complicates moving the actual profile-based brightness change action away from the deprecated BrightnessControl class.

Instead, let's elevate the new interface to the same level as PolicyAgent, initializing it unconditionally in PowerDevilApp and giving it a nice D-Bus bus name / path / interface at "org.kde.ScreenBrightness".

daemon: Replace ScreenBrightness getter methods with properties

The recently introduced org.kde.ScreenBrightness interface had a few conceptual shortcomings:

  • DisplayAdded/DisplayRemoved are useful in certain contexts, but they don't allow for preserving the order of displays within the full list. Calling ListDisplayIds() would be able to resolve this, but could contain additional changes that are only signaled by the next DisplayAdded/DisplayRemoved. If multiple changes happen at once, this would also be excessive.

  • Multiple getter methods for individual display properties require multiple roundtrips to query all values for a single display.

This commit remodels the org.kde.ScreenBrightness interface in a way that's structurally similar to org.kde.KWin.InputDevice. The updated interface allows resolving both problems more elegantly (albeit with slightly more code) and conforms to D-Bus best practices.

This is what the remodeled getters looked like before:

  • /org/kde/ScreenBrightness
    • GetDisplayIds()
    • GetLabel(id)
    • GetIsInternal(id)
    • GetBrightness(id)
    • GetMaxBrightness(id)
    • SetBrightness(id, brightness, flags)
    • SetBrightnessWithContext(id, brightness, flags, context)

Here are the equivalents after this commit, mostly as properties:

  • /org/kde/ScreenBrightness
    • DisplaysDBusNames
  • /org/kde/ScreenBrightness/<dbus_name>
    • DBusName
    • Label
    • IsInternal
    • Brightness
    • MaxBrightness
    • SetBrightness(brightness, flags)
    • SetBrightnessWithContext(brightness, flags, context)

All property changes are signaled with a PropertiesChanged signal through the standard org.freedesktop.DBus.Properties interface.

Existing change signals in the parent interface are preserved as is, avoiding the need to subscribe to multiple objects. The SetBrightness pair of setter methods is also kept there, co-locating it with the signals that it will trigger.

daemon: Port profile-based brightness changes away from legacy API

The relevant code is moved from the deprecated BrightnessControl action to the new ScreenBrightnessControl action, where its isSupported() result is now independent of whether the (deprecated) BrightnessControl D-Bus interface should be exposed or not.

The new per-display implementation of profile-based brightness changes will preserve legacy API behavior and limit changes to internal displays if any are available. See code comment for a more detailed explanation.

BrightnessControl is now only responsible for its D-Bus API, and can be removed whenever we decide it's not worth keeping.

Test plan

  • Test that per-display brightness changes (applet slider & scrolling, brightness keys) still work as intended.
  • Check that org.kde.ScreenBrightness now exists as a separate session bus service in e.g. qdbus.
  • Open "Power Management" in System Settings, enable "Change screen brightness" also for "On Battery" and apply. Then get on battery from AC power (screen should reduce, but not increase brightness).
    • This action is still annoying as heck when there isn't another profile switching back to another pre-defined brightness percentage. We may be able to fix this later by reinterpreting the setting as dimming with brightness value limit, or such. (Another thing to consider for !360.)

Bugs fixed

No reported ones, but this protects 6.2 against expanding the scope of BUG: 484663 to external monitors as well.

Edited by Jakob Petsovits

Merge request reports