Skip to content

Improve the new PolicyAgent D-Bus API for inhibitor blocking

Jakob Petsovits requested to merge work/jpetso/policyagent-dbus-fix into master

CC @nclarius @broulik

Follow-up to !363 (comment 949734) which we merged under the condition that we'd tweak the API (mostly meant as internal, but exposed anyway) before 6.2 Beta.

These recently introduced interface additions had some shortcomings:

  • The distinction between "permanent" and "temporary" blocking of inhibitors introduced excessive complexity in the interface.
  • The new getter methods and change signals adopted the style of ListInhibitors/InhibitorsChanged, which is both wrong (due to the use of key/value arguments where a list is required), incomplete (due to missing policy info and lack of ordering). On top of this, only one of added or removed was ever provided in any given change signal. Neither of these were actually used as value by the applet, instead it called ListInhibitors again.
  • The term "block" is used by both by the UI (i.e. "Block sleep") and by the login1 D-Bus interface ("block" mode for inhibitors, with the same meaning). Using "block inhibitors" in the D-Bus interface with a different meaning is confusing.

This commit restructures the PolicyAgent D-Bus interface and adapts the relevant code both on the daemon and batterymonitor applet side. A lot of code is touched, but it boils down to a few essential changes:

  • The terms "block inhibition" and "unblock inhibition" are replaced by "suppress inhibition" and "unsuppress inhibition".
  • ListInhibitions and InhibitionsChanged are deprecated, the recently introduced getter methods and change signals are removed.
  • Instead, the D-Bus interface introduces properties named ActiveInhibitions and SuppressedInhibitions complete with standard D-Bus PropertiesChanged signals being emitted.
  • Both inhibition lists follow the same format, which includes the standard login1 "what", "who", "why" and "mode", as well as a flags argument with "configured to suppress" as its only currently supported option. (Temporary suppression is implicit by which list it's listed in.)

Earlier description, for posterity

First shot, in case you want to follow along. This is fairly close in terms what I want for API structure, but I'll want a little more time to think about naming.

I realized that there really isn't any "key" as in key/value for a given inhibition, so for any type of InhibitionAdded or InhibitionRemoved kind of signal we'd need to pass all of its properties. But then one has to compare all properties of a removed inhibition, and if the only change is perhaps a change in permanent blocking or a newly added block type, state tracking will get complicated.

So instead I'm doing something entirely different: a property each for active and blocked inhibitions, paired with the standard PropertiesChanged signal that the D-Bus spec itself defines. This also preserves order and avoids weird naming ambiguities (e.g.: does BlockedInhibitionAdded get issued when BlockInhibition persists a block, or when an otherwise active inhibition appears and gets blocked?).

Furthermore, in order to allow a fix for BUG: 418433 later, this API refactoring also includes policies in each inhibition tuple. Plus the "permanently blocked" boolean, which I preliminarily renamed to "block later" so it can also be used in active inhibitions without confusion.

Both active and blocked inhibitions are therefore a list of tuples, each consisting of {what, who, why, block_later}. The three "w"s reflecting systemd naming, meaning the new API uses "sleep:idle" as what instead of a set of flags.

The reason why I'm still not 100% happy with this is that systemd also uses "block" in its terminology, and in fact exposes a BlockInhibited list of "what"s in addition to its less important sibling DelayInhibited. So a "block inhibitor" is an inhibitor lock that blocks, rather than delays something. But a "blocked inhibition" is a block inhibitor that gets blocked?

It would be really neat if we could avoid this overloading of terms, and perhaps instead expose something like BlockInhibitors next to SuppressedBlockInhibitors or such. Although at that point it also gets weird for the casual reader. Maybe instead each inhibitor should get the extra mode (block or delay) from systemd added to its properties, and we stick with ActiveInhibitors vs. perhaps SuppressedInhibitors.

Anyway. It builds and seems to work again like it currently does. API improvement as it stands. Have a look if you can, and I'll tweak this some more. The rest should be more straightforward.

Edited by Jakob Petsovits

Merge request reports