From c44deda7c233b2b20a88faedb81c9422448e0040 Mon Sep 17 00:00:00 2001 From: Fimeg Date: Sun, 26 Jul 2026 14:00:37 -0400 Subject: [PATCH] sessiond: restore the exact brightness the dim captured, not a guessed floor --- src/sessiond/device_state.rs | 61 +++++++++++++++++ src/sessiond/server.rs | 123 ++++++++++++++++++++++++++--------- 2 files changed, 152 insertions(+), 32 deletions(-) diff --git a/src/sessiond/device_state.rs b/src/sessiond/device_state.rs index 8eb39fc..a33ab80 100644 --- a/src/sessiond/device_state.rs +++ b/src/sessiond/device_state.rs @@ -948,6 +948,21 @@ pub struct DeviceStateMachine { /// deadline after which the panel goes dark regardless — see /// `LOCK_ACK_BUDGET` for why that fail-open is the documented choice. pending_blank: Option, + /// The exact panel brightness captured immediately before dimming. + /// + /// The dim used to be `brightnessctl -s set 10` and the restore a + /// save/restore pair with a floor: anything that came back under 26/255 + /// was pushed to 40%. That floor was defending against the old hypridle + /// `-s`/`-r` wedge — a second save while already dim pinned brightness at + /// 10 and every later wake looked like dead glass — but sessiond's + /// `dimmed` field already makes a double save impossible, so the floor was + /// guarding a bug that no longer exists and corrupting a real setting to + /// do it. A phone deliberately run dark came back *brighter* than it + /// started, which is the jump on tap-to-dismiss. + /// + /// The machine remembers the number instead of inferring it. `None` means + /// the capture failed, and only then is the floor the right answer. + brightness_before_dim: Option, /// The last raw proximity reading, before debounce. `sensor_evidence` /// carries the believed value; this carries what the sensor actually said. proximity_raw: bool, @@ -1019,6 +1034,7 @@ impl DeviceStateMachine { blank_requested: false, dimmed: false, pending_blank: None, + brightness_before_dim: None, proximity_raw: false, proximity_since: None, // Loaded, not defaulted: the user's Auto-Lock choices are settings, @@ -1491,6 +1507,19 @@ impl DeviceStateMachine { self.state } + /// Record the brightness the panel was at when the dim fired. The executor + /// reads the hardware; the machine is what remembers. + pub fn note_brightness_before_dim(&mut self, value: Option) { + self.brightness_before_dim = value; + } + + /// The captured pre-dim brightness, consumed by the restore. Taken rather + /// than read: a stale value surviving into the next dim cycle is exactly + /// the class of bug this replaced. + pub fn take_brightness_before_dim(&mut self) -> Option { + self.brightness_before_dim.take() + } + /// Should this wake be refused because something is over the sensor? /// /// Tap-to-wake only. A double tap is the one wake source a pocket can @@ -2554,6 +2583,38 @@ mod tests { ); } + #[test] + fn the_pre_dim_brightness_is_remembered_and_consumed_once() { + // Casey's report: the panel dims, and a tap to dismiss brings it back + // at a different level. Cause was `blueline-undim`'s floor — anything + // restoring under 26/255 was pushed to 40%, so a phone deliberately + // run dark came back brighter than it started. The machine remembers + // the number now instead of the executor guessing at it. + let mut sm = DeviceStateMachine::new(); + assert_eq!(sm.take_brightness_before_dim(), None, "nothing captured yet"); + + sm.note_brightness_before_dim(Some(18)); + assert_eq!( + sm.take_brightness_before_dim(), + Some(18), + "a value below the old floor must survive the round trip unchanged" + ); + assert_eq!( + sm.take_brightness_before_dim(), + None, + "taken, not read — a stale capture must not survive into the next dim" + ); + } + + #[test] + fn a_failed_brightness_read_falls_through_to_the_floor() { + // The one case the floor is right for: nothing knows what the panel + // was at, so coming back dark is worse than coming back wrong. + let mut sm = DeviceStateMachine::new(); + sm.note_brightness_before_dim(None); + assert_eq!(sm.take_brightness_before_dim(), None); + } + #[test] fn only_tap_to_wake_is_vetoed_by_a_covered_sensor() { // A double tap is the one wake a pocket can produce by itself. The diff --git a/src/sessiond/server.rs b/src/sessiond/server.rs index 40e1139..be8b8ef 100644 --- a/src/sessiond/server.rs +++ b/src/sessiond/server.rs @@ -211,43 +211,50 @@ fn execute(shared: &Arc, action: Action) { return; } + // Dim and restore carry a number, so they are not table entries. The + // machine remembers the exact brightness the panel was at; the executor + // reads the hardware and sets it back. Neither `brightnessctl -s/-r` nor + // the floor in `blueline-undim` can tell a mistakenly-saved dim value from + // a phone the user deliberately runs dark — that guess is what made a + // tap-to-dismiss come back at a different level than it started. + if action == Action::Dim { + let before = read_brightness(); + if before.is_none() { + warn!("could not read brightness before dimming — restore will use the floor"); + } + shared + .lock() + .device_state + .note_brightness_before_dim(before); + } + if action == Action::Restore { + if let Some(value) = shared.lock().device_state.take_brightness_before_dim() { + run_executor( + shared, + "brightnessctl", + &["set", &value.to_string()], + "panel-restore", + action, + ); + return; + } + // No capture. This is the only case the floor is right for: the panel + // may be sitting at the dim level with nothing that knows better. + run_executor(shared, UNDIM_EXECUTOR, &[], "panel-restore", action); + return; + } + let (program, args, label): (&str, Vec<&str>, &str) = match action { - // Save-then-dim. sessiond guarantees this runs once per dim, which is - // what the old hypridle `-s/-r` listener could not: a second save - // while already dim is what pinned brightness at 10/255. - Action::Dim => ("brightnessctl", vec!["-s", "set", "10"], "panel-dim"), - Action::Restore => (UNDIM_EXECUTOR, vec![], "panel-restore"), + // sessiond guarantees this runs once per dim, which is what the old + // hypridle `-s`/`-r` listener could not: a second save while already + // dim is what pinned brightness at 10/255. + Action::Dim => ("brightnessctl", vec!["set", "10"], "panel-dim"), + Action::Restore => unreachable!("handled above; the restore carries a value"), Action::Blank => (DPMS_EXECUTOR, vec!["off"], "panel-off"), Action::Lock => unreachable!("handled above; the lock is not a shell command"), }; - match std::process::Command::new(program).args(&args).status() { - Ok(status) if status.success() => { - if action == Action::Blank { - // Record the panel we actually turned off, so the machine's - // field tracks the hardware rather than its own intent. - let follow_up = shared.lock().device_state.set_panel(false); - for a in follow_up { - execute(shared, a); - } - } - } - Ok(status) => { - let code = status.code().unwrap_or(-1).to_string(); - warn!("{program} {args:?} exited {code}"); - shared - .lock() - .device_state - .record_error("device-state", label, &code); - } - Err(e) => { - warn!("could not run {program}: {e}"); - shared - .lock() - .device_state - .record_error("device-state", label, &e.to_string()); - } - } + run_executor(shared, program, &args, label, action); } /// Act on the machine's decision that the session must lock before the panel @@ -418,6 +425,58 @@ fn start_ack_timer(shared: &Arc, gen: u64) { /// agent composing verbs needs to know *which kind* of no it got — not now, /// never, or wrong arguments are three different next moves and a sentence /// does not distinguish them. +/// The panel's current brightness, or `None` if it cannot be read. +/// +/// Read rather than remembered from the last set: the user, the shell, or an +/// agent may have changed it since, and restoring a value the machine assumed +/// is how the panel ends up somewhere nobody asked for. +fn read_brightness() -> Option { + let out = std::process::Command::new("brightnessctl") + .arg("get") + .output() + .ok()?; + if !out.status.success() { + return None; + } + String::from_utf8(out.stdout).ok()?.trim().parse().ok() +} + +/// Run one executor and record what it did. Shared by the paths that build +/// their arguments dynamically and the table below. +fn run_executor( + shared: &Arc, + program: &str, + args: &[&str], + label: &str, + action: Action, +) { + match std::process::Command::new(program).args(args).status() { + Ok(status) if status.success() => { + if action == Action::Blank { + let follow_up = shared.lock().device_state.set_panel(false); + for a in follow_up { + execute(shared, a); + } + } + } + Ok(status) => { + let code = status.code().unwrap_or(-1).to_string(); + warn!("{program} {args:?} exited {code}"); + shared + .lock() + .device_state + .record_error("device-state", label, &code); + } + Err(e) => { + warn!("could not run {program}: {e}"); + shared + .lock() + .device_state + .record_error("device-state", label, &e.to_string()); + } + } +} + fn refuse(code: RefusalCode, reason: &str) -> serde_json::Value { serde_json::json!({ "ok": false, "code": code.as_str(), "reason": reason }) }