Implement the wlr-output-power-management protocol #1108
Conversation
Related to YaLTeR#108, YaLTeR#856 This protocol allows external programs (e.g. wlopm) to control and query the output power state (on/off). In the current implementation, when the client sends a `set_mode` request, the handler calls `activate_monitors()` or `deactivate_monitors()` to set that state for all monitors. This is probably simpler than adding full support for per-output power management, and is good enough for the most common use cases like idle screensavers. The `output_power.mode` event is now emitted by `activate_monitors()` and `deactivate_monitors()` when the state changes. A bit of a rewrite was needed because the tty backend tries to activate monitors on resume, but previously it wasn't calling `activate_monitors()` because it is unable to provide a reference to the `Backend` that wraps it. I worked around this by adding a second method that does not take a backend parameter, instead assuming that the backend will take care of calling itself.
agausmann
force-pushed
the
feat-wlr-output-power-management
branch
from
3cc1a5f to
0de4f47
Compare
|
I've tested this using |
|
|
||
| pub struct OutputPowerManagementManagerState { | ||
| // Active controls only. Failed ones are removed. | ||
| output_powers: HashMap<Output, ZwlrOutputPowerV1>, |
There was a problem hiding this comment.
Something felt off about this, and I think I figured it out.
I based this module off of gamma_control. After reading the docs for it, I found out it offers exclusive access to each output, so only one client can create a gamma control for it. I'm pretty sure that's not true for output power; it should allow multiple clients to connect (operating more like the output_management module.
|
While the implementation may be simpler, the behavior of all-off / all-on might be confusing/unexpected for users. Even if it is well-documented, it's not what they would expect from a protocol that seems to provide per-output power management. |
|
Sure, that makes sense to me |
Perhaps you don't need to change |
| // todo: set output power per-monitor. Currently, niri is written to power | ||
| // all monitors at the same time. |
There was a problem hiding this comment.
These callbacks on their own support it, so this comment is not needed here?
| .contains_key(&output) | ||
| { | ||
| let zwlr_output_power = data_init.init(id, OutputPowerState {}); | ||
| zwlr_output_power.mode(state.get_output_power_mode(&output).into()); |
There was a problem hiding this comment.
So yeah as I wrote in the other comment, the way this should work is OutputPowerManagerState should itself store the current power mode (to its knowledge) and send that straight to clients. Then there should be a refresh() call that tells it the current up-to-date power mode from niri. If it changes, it will send that update to clients.
|
|
||
| // Output not found, | ||
| // or output power instance already exists | ||
| data_init.init(id, OutputPowerState {}).failed(); |
There was a problem hiding this comment.
I'd put this data_init at the top of the block so that you cannot miss it
| }; | ||
| let output = output.clone(); | ||
|
|
||
| // todo: handle invalid enum values |
There was a problem hiding this comment.
Send a protocol error or something, see how other impls do it
Related to #108, #856
This protocol allows external programs (e.g. wlopm) to control and query the output power state (on/off).
In the current implementation, when the client sends a
set_moderequest, the handler callsactivate_monitors()ordeactivate_monitors()to set that state for all monitors. This is probably simpler than adding full support for per-output power management, and is good enough for the most common use cases like idle screensavers.The
output_power.modeevent is now emitted byactivate_monitors()anddeactivate_monitors()when the state changes. A bit of a rewrite was needed because the tty backend tries to activate monitors on resume, but previously it wasn't callingactivate_monitors()because it is unable to provide a reference to theBackendthat wraps itself. I workedaround this by adding a second method that does not take a backend parameter, instead assuming that the backend will take care of calling itself.