srdusr
aboutsummaryrefslogtreecommitdiffstats
path: root/crates
diff options
context:
space:
mode:
authorsrdusr <[email protected]>2025-10-29 22:24:00 +0200
committersrdusr <[email protected]>2025-10-29 22:24:00 +0200
commita4710d792a1b16698fc30b9e97e6c08c82129d6d (patch)
tree9d56a3c2106db783e1fad0cf817ecc1d7510bd8f /crates
parent9592fd7acb5b68f3fae139dfac22b728c35b6198 (diff)
downloadsrdwm-a4710d792a1b16698fc30b9e97e6c08c82129d6d.tar.gz
srdwm-a4710d792a1b16698fc30b9e97e6c08c82129d6d.zip
Fix set_monitor_split never actually reaching srd monitors
Live-tested right after shipping it and caught immediately: srd dispatch set output split returned ok, but srd monitors kept reporting the whole, unsplit output. WindowManager::monitors is a passive cache, only refreshed when a backend re-queries and calls set_monitors again - the IPC handler mutated the split map directly but never triggered that requery, unlike set_output_position's own drain site, which already pushes a "just go recompute" event after applying. Makes it a proper queued cross-boundary request instead, the same shape as every other backend-owned effect on this socket: WindowManager:: request_monitor_split/drain_monitor_split_requests, dispatch queues instead of mutating, the udev backend's poll drains it, applies via set_monitor_split, and pushes the same recompute event. srd.monitor. split's Lua config-time path is untouched - it runs before the very first startup query, so it never had this problem.
Diffstat (limited to 'crates')
-rw-r--r--crates/core/src/manager/mod.rs17
-rw-r--r--crates/core/src/manager/monitors.rs18
-rw-r--r--crates/platform/src/ipc/dispatch.rs24
-rw-r--r--crates/platform/src/ipc/tests.rs40
-rw-r--r--crates/wayland/src/udev/platform.rs13
5 files changed, 86 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`