Conversation
turns off screens after an idle period, configured by idle_timeout.timeout_duration.
max-ishere
left a comment
max-ishere
left a comment
There was a problem hiding this comment.
Hi Felipe, thanks for contributing. Full disclosure, I am just a fellow maintainer, but I noticed quite a few issues ranging from mild ones to quite important.
Please don't take the fact that I left comments on nearly every line personally. The code works great, the feature is awesome and I do want it in the project, and I haven't noticed any bugs. However, there are some things I think can be improved.
Really, I am just here to keep the code quality up, and make the project better. Some of these are my personal opinions (mostly the parts about the config structure), so feel free to disagree.
To be clear, I am not a maintainer, so my opinions aren't as important.
| [idle_timeout] | ||
| # Time in seconds before turning off screens | ||
| timeout_duration = 30 |
There was a problem hiding this comment.
A bit of bikeshedding - I think that the top-level key should just be called [idle]. But it's not that important.
The thing that bothers me more is the unspecific timeout_duration key.
I think it would be nice for the greeter to optionally suspend the system if it is idle. It doesn't need to be done in this PR, but I think such a feature will be added eventually. Thus, the key that turns the screens off should be more specific to avoid confusion in the future. For user's convenience, I don't see any issues with aliasing it in serde it to allow "screen", "monitor", and "display" timeouts.
Additionally, it would be nice to use humantime-serde because ffs if I have to convert from minutes, or hours to seconds once again, I will go insane.
[idle]
# Alias: monitor_timeout, display_timeout
screen_timeout = "20min"| } | ||
| } | ||
|
|
||
| pub fn run_idle_loop(timeout_ms: u32) -> Result<(), Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
I think a std::time::Duration makes more sense than a u32 named *_ms. The same goes for State::timeout, it should be a Duration too.
| if let Some(power_manager) = self.output_power_manager.as_ref() { | ||
| self.outputs.iter().for_each(|(_, output)| { | ||
| let power_control = power_manager.get_output_power(output, qh, ()); | ||
| power_control.set_mode(mode); | ||
| power_control.destroy(); | ||
| }); | ||
| } else { | ||
| tracing::warn!("No output power manager available to set display power mode"); | ||
| } |
There was a problem hiding this comment.
To avoid unnecessary nesting, this could be written as:
let Some(power_manager) = self.output_power_manager.as_ref() else {
tracing::warn!("No output power manager available to set display power mode");
return;
};
self.outputs.iter().for_each(|(_, output)| {
let power_control = power_manager.get_output_power(output, qh, ());
power_control.set_mode(mode);
power_control.destroy();
});But more importantly, since output_power_manager is the core of this entire thing, wouldn't it make more sense to remove the Option<> and simply fail to start the idle loop if it is not found? I am not sure if it is possible to use 2 separate State types in the trait generics.
| if let (Some(idle_notifier), Some(seat)) = (&self.idle_notifier, &self.seat) { | ||
| self.idle_notification = | ||
| Some(idle_notifier.get_idle_notification(self.timeout_ms, seat, qh, ())); | ||
| } else { | ||
| tracing::warn!("Cannot create idle notification: missing idle notifier or seat"); | ||
| } |
There was a problem hiding this comment.
Ditto regarding let else, and the fact that it may not have to be an option.
Additionally, this function should probably return a Result<(), Err> or at least a bool so that the caller can exit on line 195:
if state.create_idle_notification(&qh).is_err() {
tracing::warn!("Not starting idle monitor: Failed to create idle notification");
return;
}
loop {
event_queue.blocking_dispatch(&mut state)?;
}The Err could be an enum like { NoNotifier, NoSeat }, in which case you could write it as:
#[derive(Error)]
enum CreateNotificationError {
#[error("No idle notifier registered")]
NoNotifier,
...
}
if let Err(err) = ... {
warn!("Not starting idle monitor: {err}");
}| fn check_protocols_ready(&mut self) { | ||
| self.protocols_ready = self.has_required_protocols(); | ||
| } |
There was a problem hiding this comment.
self.protocols_ready is never used.
| state.is_idle = false; | ||
| state.set_display_power_mode(Mode::On, qh); | ||
| } | ||
| notification.destroy(); |
There was a problem hiding this comment.
Upon checking this in Cosmic, it does not seem necessary to destroy the notification every time a Resumed event is sent - the screen continues to turn off.
Is there some documentation page that mandates this?
| idle_notifier: Option<ext_idle_notifier_v1::ExtIdleNotifierV1>, | ||
| timeout_ms: u32, | ||
| idle_notification: Option<ext_idle_notification_v1::ExtIdleNotificationV1>, | ||
| is_idle: bool, |
There was a problem hiding this comment.
I am not sure I understand the point of this bool. It does not seem like it would change anything in how the monitors are turned off. Is there some reason for this? If so, a doccomment would be helpful.
| std::thread::spawn(move || { | ||
| if let Err(e) = crate::idle::run_idle_loop(timeout_ms) { | ||
| tracing::error!("Idle monitoring failed: {}", e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Please use tokio::spawn_blocking instead.
2 participants
this PR introduces a feature that turns off screens after an idle period, configured by idle_timeout.timeout_duration.