diff options
| author | srdusr <[email protected]> | 2026-04-28 16:22:00 +0200 |
|---|---|---|
| committer | srdusr <[email protected]> | 2026-04-28 16:22:00 +0200 |
| commit | 3ea84a31ee671b88d8c343b3007de0e975eff31c (patch) | |
| tree | 6fd62a55370069dfb3e798bf816bc0da9c1fbf27 | |
| parent | c4dc99cc3211c4dc1a461f228402be1593b883c9 (diff) | |
| download | srdwm-3ea84a31ee671b88d8c343b3007de0e975eff31c.tar.gz srdwm-3ea84a31ee671b88d8c343b3007de0e975eff31c.zip | |
Stop a config reload from undoing a change the user just made by hand
Reloading rebuilds the theme and general settings from the config file.
That is right for a file edit, but it also wiped every live `srd set` - and
the titlebar right-click menu's "Customize" rows are built entirely out of
live `srd set`s. Changing a button style from that menu and then saving
init.lua for any unrelated reason silently reverted it.
Survivable while reloads only happened on Mod4+Ctrl+r. The reload-on-write
support added in the previous commit makes a reload happen on every save,
which turns a rare surprise into a reliable one. A control that silently
reverts is worse than no control, so this is a defect rather than a
documented quirk. The AGS peer session reached the same conclusion from the
other side while deciding whether to build Settings controls against these
values, and would have had to label them session-only.
Every setting changed live is recorded on the WindowManager as key -> raw
JSON text, and replayed after each reload through the very same handle_set
that applied it, so a replayed setting cannot behave differently from a real
one. Recorded only on success, so a rejected value is never replayed, and
only for real client calls, so the replay cannot rewrite what it is reading.
Last write wins per key. Raw JSON text because core has no serde dependency
and no reason to gain one for this; the platform crate parses it back.
Verified live in a nested compositor: set button_side left and button_mode
fixed, saved an unrelated config edit, both survived, and the log reported
re-applying two live settings. Three tests cover the round trip, the
rejected-value case and the one-entry-per-key case.
A live value is still a session override rather than a persisted setting.
That distinction is now written down in DEFAULTS.md instead of being a trap.
515 tests pass, clippy clean.
| -rw-r--r-- | crates/core/src/manager/lock.rs | 16 | ||||
| -rw-r--r-- | crates/core/src/manager/mod.rs | 24 | ||||
| -rw-r--r-- | crates/platform/src/ipc/dispatch.rs | 38 | ||||
| -rw-r--r-- | crates/platform/src/ipc/mod.rs | 1 | ||||
| -rw-r--r-- | crates/platform/src/ipc/tests.rs | 39 | ||||
| -rw-r--r-- | crates/platform/src/lib.rs | 2 | ||||
| -rw-r--r-- | crates/srdwm/src/main.rs | 13 | ||||
| -rw-r--r-- | docs/DEFAULTS.md | 19 | ||||
| -rw-r--r-- | docs/TODO.md | 32 |
9 files changed, 169 insertions, 15 deletions
diff --git a/crates/core/src/manager/lock.rs b/crates/core/src/manager/lock.rs index 0ffe0ca..15c0a9f 100644 --- a/crates/core/src/manager/lock.rs +++ b/crates/core/src/manager/lock.rs @@ -51,6 +51,22 @@ impl WindowManager { pub fn drain_refresh_request(&mut self) -> bool { std::mem::take(&mut self.refresh_requested) } + + /// Records that `key` was set live to `value_json`. See + /// `live_settings`' own doc comment. + /// + /// Last write wins, so setting the same key twice leaves one entry and + /// the replay applies each key exactly once. + pub fn record_live_setting(&mut self, key: &str, value_json: String) { + self.live_settings.insert(key.to_string(), value_json); + } + + /// Every live setting recorded so far, for replay after a config + /// reload. Cloned rather than borrowed: the replay mutates the same + /// `WindowManager` this came from. + pub fn live_settings(&self) -> Vec<(String, String)> { + self.live_settings.iter().map(|(k, v)| (k.clone(), v.clone())).collect() + } } #[cfg(test)] diff --git a/crates/core/src/manager/mod.rs b/crates/core/src/manager/mod.rs index 7454ce2..e778a32 100644 --- a/crates/core/src/manager/mod.rs +++ b/crates/core/src/manager/mod.rs @@ -437,6 +437,29 @@ pub struct WindowManager { /// Set by `request_refresh`, drained by the main loop. Same /// cross-boundary queued-request shape as `lock_requested`. refresh_requested: bool, + /// Every setting changed live since startup, as `srd set` key -> the + /// raw JSON text of its value, in insertion-independent key order. + /// + /// Exists so a config reload does not silently undo a change the user + /// just made by hand. `apply_general_settings` rebuilds the whole + /// `ThemeConfig` and general-settings block from the config file, which + /// is the correct precedence for a *file* edit - but it also wiped + /// every live `srd set`, and the titlebar right-click menu's own + /// "Customize" section is built entirely out of live `srd set`s. So + /// changing a button style from that menu and then saving `init.lua` + /// for any unrelated reason silently reverted it. + /// + /// That was survivable while reloads only happened on an explicit + /// `Mod4+Ctrl+r`. `general.config_reload_on_write` makes a reload + /// happen on every save, which turns a rare surprise into a reliable + /// one - a control that silently reverts is worse than no control. + /// + /// Raw JSON text rather than a typed value because this crate has no + /// serde dependency and no business gaining one for this; the platform + /// crate parses it back and replays it through the same `handle_set` + /// that recorded it, so a replayed setting cannot diverge from a real + /// one. `BTreeMap` for a deterministic replay order. + live_settings: std::collections::BTreeMap<String, String>, drag: Option<DragState>, resize: Option<ResizeState>, rules: Vec<WindowRule>, @@ -601,6 +624,7 @@ impl WindowManager { theme: ThemeConfig::default(), lock: LockConfig::default(), refresh_requested: false, + live_settings: std::collections::BTreeMap::new(), drag: None, resize: None, rules: Vec::new(), diff --git a/crates/platform/src/ipc/dispatch.rs b/crates/platform/src/ipc/dispatch.rs index d2232c8..749b2aa 100644 --- a/crates/platform/src/ipc/dispatch.rs +++ b/crates/platform/src/ipc/dispatch.rs @@ -437,6 +437,44 @@ pub(crate) fn handle_request(line: &[u8], wm: &std::rc::Rc<std::cell::RefCell<Wi /// predicate is what the two colour/width arms below walk existing /// windows with, rather than touching every window unconditionally. fn handle_set(req: &serde_json::Value, wm: &std::rc::Rc<std::cell::RefCell<WindowManager>>) -> (Vec<u8>, bool) { + let (response, changed) = apply_set(req, wm); + // Recorded only on success, so a rejected value is never replayed -- + // and only for a real client call, not for the replay itself (see + // `replay_live_settings`, which calls `apply_set` directly and would + // otherwise rewrite what it is currently reading). + if changed { + if let (Some(key), Some(value)) = (req.get("key").and_then(|v| v.as_str()), req.get("value")) { + wm.borrow_mut().record_live_setting(key, value.to_string()); + } + } + (response, changed) +} + +/// Re-applies every setting changed live since startup. +/// +/// Called after a config reload, which rebuilds the theme and general +/// settings from the config file and would otherwise silently undo them -- +/// see `WindowManager::live_settings`' own doc comment for why that matters +/// more now that a reload happens on every save. +/// +/// Replayed through `apply_set`, the same function that applied them +/// originally, so a replayed setting cannot behave differently from a real +/// one. Failures are ignored: a value that no longer applies (a monitor +/// that has gone away, say) should not stop the rest being restored. +pub fn replay_live_settings(wm: &std::rc::Rc<std::cell::RefCell<WindowManager>>) -> usize { + let recorded = wm.borrow().live_settings(); + let mut applied = 0; + for (key, raw) in recorded { + let Ok(value) = serde_json::from_str::<serde_json::Value>(&raw) else { continue }; + let req = serde_json::json!({ "cmd": "set", "key": key, "value": value }); + if apply_set(&req, wm).1 { + applied += 1; + } + } + applied +} + +fn apply_set(req: &serde_json::Value, wm: &std::rc::Rc<std::cell::RefCell<WindowManager>>) -> (Vec<u8>, bool) { let key = req.get("key").and_then(|v| v.as_str()).unwrap_or(""); let value = req.get("value"); match key { diff --git a/crates/platform/src/ipc/mod.rs b/crates/platform/src/ipc/mod.rs index 796c459..7094f6e 100644 --- a/crates/platform/src/ipc/mod.rs +++ b/crates/platform/src/ipc/mod.rs @@ -37,6 +37,7 @@ use std::path::PathBuf; use srdwm_core::WindowManager; mod dispatch; +pub use dispatch::replay_live_settings; mod types; #[cfg(test)] mod tests; diff --git a/crates/platform/src/ipc/tests.rs b/crates/platform/src/ipc/tests.rs index c2a678c..2e61761 100644 --- a/crates/platform/src/ipc/tests.rs +++ b/crates/platform/src/ipc/tests.rs @@ -26,6 +26,45 @@ fn read_line(reader: &mut std::io::BufReader<UnixStream>) -> String { } #[test] +fn a_live_setting_is_recorded_and_survives_a_replay() { + // The concrete regression: a config reload rebuilds the theme from the + // file, so a value changed by hand (the titlebar menu's own Customize + // rows are all live `srd set`s) was silently reverted. + let wm = std::rc::Rc::new(std::cell::RefCell::new(WindowManager::new())); + let req = serde_json::json!({"cmd": "set", "key": "button_side", "value": "left"}); + let (_, changed) = super::dispatch::handle_request(serde_json::to_vec(&req).unwrap().as_slice(), &wm); + assert!(changed); + assert!(wm.borrow().theme.buttons_left); + + // Stand in for what a reload does to the theme. + wm.borrow_mut().theme = srdwm_core::ThemeConfig::default(); + assert!(!wm.borrow().theme.buttons_left, "the rebuild really did drop it"); + + assert_eq!(super::replay_live_settings(&wm), 1); + assert!(wm.borrow().theme.buttons_left, "the live change must come back"); +} + +#[test] +fn a_rejected_value_is_never_recorded_for_replay() { + let wm = std::rc::Rc::new(std::cell::RefCell::new(WindowManager::new())); + let req = serde_json::json!({"cmd": "set", "key": "button_side", "value": "sideways"}); + let (_, changed) = super::dispatch::handle_request(serde_json::to_vec(&req).unwrap().as_slice(), &wm); + assert!(!changed, "an invalid value must be rejected"); + assert_eq!(super::replay_live_settings(&wm), 0, "nothing should have been recorded"); +} + +#[test] +fn setting_the_same_key_twice_replays_only_the_last_value() { + let wm = std::rc::Rc::new(std::cell::RefCell::new(WindowManager::new())); + for value in ["left", "right"] { + let req = serde_json::json!({"cmd": "set", "key": "button_side", "value": value}); + super::dispatch::handle_request(serde_json::to_vec(&req).unwrap().as_slice(), &wm); + } + assert_eq!(super::replay_live_settings(&wm), 1, "one entry per key, not one per call"); + assert!(!wm.borrow().theme.buttons_left, "the last value written is the one that survives"); +} + +#[test] fn subscribe_gets_an_immediate_snapshot() { let dir = tempfile::tempdir().unwrap(); let mut server = IpcServer::bind_in(dir.path(), "test").unwrap(); diff --git a/crates/platform/src/lib.rs b/crates/platform/src/lib.rs index 5de701f..b69aa02 100644 --- a/crates/platform/src/lib.rs +++ b/crates/platform/src/lib.rs @@ -12,7 +12,7 @@ mod appmenu_registrar; pub use appmenu_registrar::{AppmenuRegistrarState, RegistrarEvent}; mod ipc; -pub use ipc::IpcServer; +pub use ipc::{replay_live_settings, IpcServer}; #[cfg(unix)] mod pam_auth; diff --git a/crates/srdwm/src/main.rs b/crates/srdwm/src/main.rs index e21e585..94da20c 100644 --- a/crates/srdwm/src/main.rs +++ b/crates/srdwm/src/main.rs @@ -798,6 +798,15 @@ fn main() -> Result<(), Box<dyn std::error::Error>> { Err(e) => report_config_error(&format!("Config edit not applied, keeping the last working one.\n{e}")), } apply_general_settings(&engine, &wm); + // After `apply_general_settings`, which rebuilds the + // theme from the config file - see + // `WindowManager::live_settings` for why a hand-made + // change has to win over that on a *reload*, even + // though the file wins at startup. + let replayed = srdwm_platform::replay_live_settings(&wm); + if replayed > 0 { + log::info!("re-applied {replayed} live setting(s) after the reload"); + } } } } @@ -810,6 +819,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> { Ok(()) => log::info!("config reloaded (desktop refresh)"), Err(e) => report_config_error(&format!("Config reload failed, keeping the last working one.\n{e}")), } + apply_general_settings(&engine, &wm); + srdwm_platform::replay_live_settings(&wm); // After the reload, so a handler edited in the config since // startup is the one that runs. engine.dispatch_event("refresh"); @@ -825,6 +836,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> { Ok(()) => log::info!("config reloaded"), Err(e) => report_config_error(&format!("Config reload failed, keeping the last working one.\n{e}")), } + apply_general_settings(&engine, &wm); + srdwm_platform::replay_live_settings(&wm); } else if !engine.dispatch_keybinding(&combo) { log::debug!("no binding for '{combo}'"); } diff --git a/docs/DEFAULTS.md b/docs/DEFAULTS.md index a130434..3497410 100644 --- a/docs/DEFAULTS.md +++ b/docs/DEFAULTS.md @@ -867,10 +867,17 @@ checks the config directory's `.lua` modification times once a second and reloads when one changes. Set it to `false` for a config that does expensive work at load time. `Mod4+Ctrl+r` still reloads on demand in either case. -Two limits to know. A reload does not re-register key *grabs* with the +**A hand-made change is not undone by a reload.** A reload rebuilds the +theme and general settings from the config file, which is right for a file +edit but would also wipe every live `srd set` - and the titlebar +right-click menu's "Customize" rows are all live `srd set`s. Every setting +changed live is recorded and re-applied after each reload, so changing a +button style from that menu and then saving `init.lua` for an unrelated +reason keeps the change. A live value stays until it is changed again or the +session ends; it is a session override, not a persisted setting, so write it +into the config to keep it across restarts. + +One limit to know: a reload does not re-register key *grabs* with the backend, so a brand new key combination needs a restart before the -compositor sees that key at all; an existing combination picks up its new -action immediately. And a reload rebuilds the theme from the config file, so -it discards live `srd set` theme changes - including titlebar settings -changed through the right-click menu. That is the correct precedence, but -with reload-on-write it now happens every time the config is saved. +compositor sees that key at all. An existing combination picks up its new +action immediately. diff --git a/docs/TODO.md b/docs/TODO.md index 49f7312..d24f9b8 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -104,14 +104,30 @@ investigations in one day started from a screenshot that was quietly lying -- draw (border strips and the desktop icon grid), and `winit/render.rs` carries a pointer to it at the place a new tier gets added. -**One interaction worth knowing.** A config reload rebuilds `ThemeConfig` from -the config file, so it discards live `srd set` theme changes - correct -precedence, and pre-existing, but auto-reload makes it happen on every save -rather than only when the reload key is pressed. A titlebar customised live -through the right-click menu reverts the next time `init.lua` is saved. - -Full workspace build/test/clippy clean: 512 tests (262 core / 160 wayland / -43 platform / 34 config / 13 ctl), 0 failed, 0 clippy warnings. +**A follow-on defect, found and fixed rather than documented away.** A config +reload rebuilds `ThemeConfig` from the config file, so it discarded every +live `srd set` - and the titlebar right-click menu's "Customize" rows are +built entirely out of live `srd set`s. That was survivable while reloads only +happened on `Mod4+Ctrl+r`; reload-on-write turned a rare surprise into a +reliable one, and a control that silently reverts is worse than no control. +Flagged independently by the AGS peer session while deciding whether to build +Settings controls against these values, which is the same conclusion from the +other side. + +Every setting changed live is now recorded (`WindowManager::live_settings`, +key -> raw JSON text) and replayed after each reload through the very same +`handle_set` that applied it, so a replayed setting cannot behave differently +from a real one. Recorded only on success, so a rejected value is never +replayed; last write wins per key. Verified live: set `button_side left` and +`button_mode fixed`, saved an unrelated config edit, both survived and the +log reported "re-applied 2 live setting(s) after the reload". + +A live value is still a session override rather than a persisted setting -- +it lasts until changed again or the session ends. That distinction is now +documented in `DEFAULTS.md` instead of being a trap. + +Full workspace build/test/clippy clean: 515 tests (262 core / 160 wayland / +46 platform / 34 config / 13 ctl), 0 failed, 0 clippy warnings. ## Nemo's right-click menu: confirmed working, and two real bugs found doing it (2026-08-28) |