diff options
| -rw-r--r-- | crates/core/src/manager/mod.rs | 17 | ||||
| -rw-r--r-- | crates/core/src/manager/monitors.rs | 18 | ||||
| -rw-r--r-- | crates/platform/src/ipc/dispatch.rs | 24 | ||||
| -rw-r--r-- | crates/platform/src/ipc/tests.rs | 40 | ||||
| -rw-r--r-- | crates/wayland/src/udev/platform.rs | 13 | ||||
| -rw-r--r-- | docs/TODO.md | 8 |
6 files changed, 94 insertions, 26 deletions
diff --git a/crates/core/src/manager/mod.rs b/crates/core/src/manager/mod.rs index bb22533..c9c77b2 100644 --- a/crates/core/src/manager/mod.rs +++ b/crates/core/src/manager/mod.rs @@ -103,6 +103,22 @@ pub struct WindowManager { /// [`MonitorSplit`]'s own doc comment for what this deliberately does /// and does not give a client (no new `wl_output`). monitor_splits: HashMap<String, MonitorSplit>, + /// Same cross-boundary-request pattern as `output_position_requests` + /// above - an IPC `set_monitor_split` dispatch (the live CLI/IPC path + /// for `srd.monitor.split`) mutating `monitor_splits` directly is not + /// enough on its own: `monitors` above is a passive cache, only + /// refreshed when a backend re-queries and calls `set_monitors` again + /// (a real hotplug, or another queued request's own drain site pushing + /// the same "just go recompute" `MonitorAdded` event - see `output_ + /// position_requests`' own drain site for the exact precedent). A + /// direct mutation with nothing to trigger that requery left `srd + /// monitors` reporting the pre-split layout indefinitely, live- + /// reproduced the first time this was tried: `{"ok":true}` came back, + /// but the very next `srd monitors` still showed one whole, unsplit + /// output. Queued here instead so the backend's own drain site can + /// apply the split *and* push that same recompute signal, exactly like + /// `output_position_requests` already does. + monitor_split_requests: Vec<(String, u32, bool)>, /// `srd.monitor.scale(name, factor)` requests, by connector name -- /// read once by a backend when it brings a head up (startup, hotplug, /// or re-enable), so a physically large, low-DPI monitor can run @@ -454,6 +470,7 @@ impl WindowManager { output_enable_requests: Vec::new(), disabled_monitors: HashMap::new(), monitor_splits: HashMap::new(), + monitor_split_requests: Vec::new(), monitor_scales: HashMap::new(), lock_requested: false, capture_requests: Vec::new(), diff --git a/crates/core/src/manager/monitors.rs b/crates/core/src/manager/monitors.rs index bff3bc9..c79f76f 100644 --- a/crates/core/src/manager/monitors.rs +++ b/crates/core/src/manager/monitors.rs @@ -233,6 +233,24 @@ impl WindowManager { self.monitor_splits.get(name).copied() } + /// Queues a live `srd dispatch set output split` request - see + /// `monitor_split_requests`' own doc comment for why this can't just + /// call `set_monitor_split` directly from the IPC dispatch handler. + /// Same "replace, don't accumulate" per-name semantics as `request_ + /// output_position`. + pub fn request_monitor_split(&mut self, name: String, parts: u32, rows: bool) { + self.monitor_split_requests.retain(|(existing, _, _)| *existing != name); + self.monitor_split_requests.push((name, parts, rows)); + } + + /// [`Self::drain_output_position_requests`]'s counterpart for split + /// requests - the backend applies each via `set_monitor_split` and + /// pushes its own "just go recompute" event afterward, same as that + /// function's own drain site. + pub fn drain_monitor_split_requests(&mut self) -> Vec<(String, u32, bool)> { + std::mem::take(&mut self.monitor_split_requests) + } + /// `srd.monitor.scale(name, factor)` - a backend applies this the /// next time it brings connector `name`'s head up (startup, hotplug, /// or re-enable). `factor <= 0.0` clears any existing override rather diff --git a/crates/platform/src/ipc/dispatch.rs b/crates/platform/src/ipc/dispatch.rs index 665cb59..918b7e0 100644 --- a/crates/platform/src/ipc/dispatch.rs +++ b/crates/platform/src/ipc/dispatch.rs @@ -322,17 +322,17 @@ pub(crate) fn handle_request(line: &[u8], wm: &std::rc::Rc<std::cell::RefCell<Wi // "rows":<bool, optional, default false>}` - the live CLI/IPC path // for `srd.monitor.split(name, parts, direction)` (`crates/config/ // src/engine/general.rs`'s own `fn_monitor_split`), which until now - // only ever ran once at config load. `WindowManager:: - // set_monitor_split` just mutates `monitor_splits`, and every - // backend's own `monitors()` already reads that map fresh on every - // single call (see the udev platform's own `monitors()`) - so, - // unlike `set_output_position`/`set_output_enabled` above, this - // needs no queue-and-drain at all: the very next `monitors()` query - // already reflects it. `parts` <= 1 clears an existing split, same - // as the Lua function. Same "resolve id to a name first" fallback - // `set_output_enabled` above already uses, since a caller working - // from a numeric id shouldn't have to look the name up itself - // first just to turn around and split it. + // only ever ran once at config load. Queued via `request_monitor_ + // split`, same cross-boundary "core has no way to trigger its own + // requery" reasoning as `set_output_position` above - see + // `WindowManager::monitor_split_requests`'s own doc comment for the + // real, live-reproduced staleness bug that came from calling + // `set_monitor_split` directly here on a first attempt. `parts` <= + // 1 clears an existing split, same as the Lua function. Same + // "resolve id to a name first" fallback `set_output_enabled` above + // already uses, since a caller working from a numeric id shouldn't + // have to look the name up itself first just to turn around and + // split it. "set_monitor_split" => { let name = match req.get("name").and_then(|v| v.as_str()) { Some(name) => Some(name.to_string()), @@ -343,7 +343,7 @@ pub(crate) fn handle_request(line: &[u8], wm: &std::rc::Rc<std::cell::RefCell<Wi return (err("missing parts"), false); }; let rows = req.get("rows").and_then(|v| v.as_bool()).unwrap_or(false); - wm.borrow_mut().set_monitor_split(name, parts as u32, rows); + wm.borrow_mut().request_monitor_split(name, parts as u32, rows); (ok(), true) } // `{"cmd":"capture_workspace","id":<workspace id>,"path":<string>, diff --git a/crates/platform/src/ipc/tests.rs b/crates/platform/src/ipc/tests.rs index 591be56..109a9ec 100644 --- a/crates/platform/src/ipc/tests.rs +++ b/crates/platform/src/ipc/tests.rs @@ -426,10 +426,15 @@ fn set_output_enabled_with_neither_name_nor_a_resolvable_id_errors() { } #[test] -fn set_monitor_split_accepts_a_name_directly_and_applies_immediately() { - // Unlike `set_output_position`/`set_output_enabled`, this one is a - // plain `WindowManager` mutation with nothing to drain - the very - // next `monitor_split` read already reflects it. +fn set_monitor_split_accepts_a_name_directly_and_queues_a_request() { + // Queued, not applied directly - see `WindowManager::monitor_split_ + // requests`'s own doc comment for why a direct mutation here left + // `srd monitors` reporting the stale, unsplit layout indefinitely + // (nothing re-triggers `monitors`' own passive cache). Only the + // backend that owns real output hardware can call `set_monitor_split` + // and push the matching recompute event, so this test - run from the + // platform crate alone, with no such backend - can only observe the + // queued request, not the applied state. let dir = tempfile::tempdir().unwrap(); let mut server = IpcServer::bind_in(dir.path(), "test").unwrap(); let wm = Rc::new(RefCell::new(WindowManager::new())); @@ -441,9 +446,7 @@ fn set_monitor_split_accepts_a_name_directly_and_applies_immediately() { server.poll(&wm); let _ = read_line(&mut reader); - let split = wm.borrow().monitor_split("eDP-1").unwrap(); - assert_eq!(split.parts, 2); - assert!(!split.rows); + assert_eq!(wm.borrow_mut().drain_monitor_split_requests(), vec![("eDP-1".to_string(), 2, false)]); } #[test] @@ -459,26 +462,35 @@ fn set_monitor_split_resolves_an_id_to_its_name() { server.poll(&wm); let _ = read_line(&mut reader); - let split = wm.borrow().monitor_split("HDMI-A-1").unwrap(); - assert_eq!(split.parts, 3); - assert!(split.rows); + assert_eq!(wm.borrow_mut().drain_monitor_split_requests(), vec![("HDMI-A-1".to_string(), 3, true)]); } #[test] -fn set_monitor_split_with_one_part_clears_an_existing_split() { +fn set_monitor_split_replaces_a_still_pending_request_for_the_same_name() { + // Same "last write wins per name" semantics as `request_output_ + // position` - only the latest requested split for a given output + // matters if several arrive before the backend's next drain. let dir = tempfile::tempdir().unwrap(); let mut server = IpcServer::bind_in(dir.path(), "test").unwrap(); let wm = Rc::new(RefCell::new(WindowManager::new())); wm.borrow_mut().set_monitors(vec![srdwm_core::Monitor::new(0, "eDP-1", srdwm_core::Rect::new(0, 0, 1920, 1080))]); - wm.borrow_mut().set_monitor_split("eDP-1".to_string(), 2, false); let mut client = UnixStream::connect(&server.path).unwrap(); let mut reader = std::io::BufReader::new(client.try_clone().unwrap()); - client.write_all(b"{\"cmd\":\"set_monitor_split\",\"name\":\"eDP-1\",\"parts\":1}\n").unwrap(); + client.write_all(b"{\"cmd\":\"set_monitor_split\",\"name\":\"eDP-1\",\"parts\":2,\"rows\":false}\n").unwrap(); server.poll(&wm); let _ = read_line(&mut reader); - assert!(wm.borrow().monitor_split("eDP-1").is_none()); + // A fresh connection - each `srd dispatch` is its own one-shot socket + // connection in practice, not a second write on an already-answered + // one (which the server closes after replying). + let mut client2 = UnixStream::connect(&server.path).unwrap(); + let mut reader2 = std::io::BufReader::new(client2.try_clone().unwrap()); + client2.write_all(b"{\"cmd\":\"set_monitor_split\",\"name\":\"eDP-1\",\"parts\":1}\n").unwrap(); + server.poll(&wm); + let _ = read_line(&mut reader2); + + assert_eq!(wm.borrow_mut().drain_monitor_split_requests(), vec![("eDP-1".to_string(), 1, false)]); } #[test] diff --git a/crates/wayland/src/udev/platform.rs b/crates/wayland/src/udev/platform.rs index 4012e3c..37abf5d 100644 --- a/crates/wayland/src/udev/platform.rs +++ b/crates/wayland/src/udev/platform.rs @@ -603,6 +603,19 @@ impl Platform for UdevPlatform { self.pending.borrow_mut().push(CoreEvent::MonitorAdded(srdwm_core::Monitor::new(0, "", srdwm_core::Rect::new(0, 0, 0, 0)))); } } + // Applies any `srd dispatch set output split` IPC requests queued + // since the last poll - see `WindowManager::monitor_split_ + // requests`'s own doc comment for why this needs the same "apply, + // then push a recompute event" shape `set_output_position`'s own + // drain just above uses, rather than `set_monitor_split` being + // called straight from the IPC dispatch handler. + let split_requests = self.state.wm.borrow_mut().drain_monitor_split_requests(); + if !split_requests.is_empty() { + for (name, parts, rows) in split_requests { + self.state.wm.borrow_mut().set_monitor_split(name, parts, rows); + } + self.pending.borrow_mut().push(CoreEvent::MonitorAdded(srdwm_core::Monitor::new(0, "", srdwm_core::Rect::new(0, 0, 0, 0)))); + } // Applies any `srd set_output_enabled` IPC requests queued since // the last poll - `disable_connector_by_name`/`enable_connector_ // by_name` already push their own `MonitorRemoved`/`MonitorAdded` diff --git a/docs/TODO.md b/docs/TODO.md index f58f1df..67ff09a 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -13,6 +13,14 @@ that has the full story. Keep this list current as items close or open; update the source doc's own entry too, don't let this drift into a second stale copy the way `PANEL_SUPPORT_TODO.md` did. +## Real bug in the same-day monitor-split live-exposure: `srd monitors` never reflected it (2026-08-27) + +Tried it live for the user right after shipping it: `srd dispatch set output split eDP-1 2 columns` returned `{"ok":true}`, but the very next `srd monitors` still showed one whole, unsplit output. Root cause: `WindowManager::monitors` is a passive cache, only refreshed when a backend re-queries and calls `set_monitors` again (a real hotplug, or another request's own drain site pushing a `MonitorAdded` "just go recompute" event - see `output_position_requests`' own drain site, which already does exactly this after applying a position). The IPC handler called `set_monitor_split` directly, mutating the split map correctly but never triggering that requery - the same class of bug `output_position_requests` was already built to avoid, just missed when this feature was added. + +Fixed by making it a proper queued cross-boundary request like every other backend-owned effect on this socket: new `WindowManager::request_monitor_split`/`drain_monitor_split_requests` (`monitor_split_requests`, replace-not-accumulate per name, same as `request_output_position`), IPC dispatch now queues instead of mutating directly, and the udev backend's own poll drains it, applies via `set_monitor_split`, and pushes the same recompute event `set_output_position`'s drain site does. `srd.monitor.split`'s Lua config-time path is untouched - it runs before the very first startup `monitors()` query, so it never had this staleness problem to begin with. + +Full workspace build/test/clippy clean (34 platform tests). Live-verified this time before calling it done, not just build-clean - exactly the mistake this entry itself is about. + ## Fake-monitor incident, root-caused and fixed jointly with the AGS peer session (2026-08-27) Follow-up to the incident entry directly below. `dotfiles-1a` (AGS) confirmed from AGS's own session log (`~/.local/state/wm-session-*.log` - not `~/.cache/ags/ags.log`, which was stale) that the X-position jump was AGS's own remembered "extend-left" layout restore firing twice, once per fake monitor appearing (`arrange()` places the primary at 0,0 and walks the rest `left -= width`, then normalizes - correct behavior for a *real* second monitor; the only fault was a fake one being in the arrangeable set at all). Fixed on AGS's side: `readArrangeable()` now filters `!m.split && !m.virtual`. Their match was keyed on connector *name*, not index, ruling out the index-shift half of my own original theory. |