diff options
| -rw-r--r-- | crates/core/src/manager/tests.rs | 29 | ||||
| -rw-r--r-- | crates/core/src/manager/windows.rs | 25 | ||||
| -rw-r--r-- | crates/wayland/src/state/desktop_icons.rs | 105 | ||||
| -rw-r--r-- | crates/wayland/src/state/lifecycle.rs | 5 | ||||
| -rw-r--r-- | crates/wayland/src/udev/platform.rs | 11 | ||||
| -rw-r--r-- | crates/wayland/src/xwayland.rs | 4 | ||||
| -rw-r--r-- | docs/TODO.md | 12 |
7 files changed, 177 insertions, 14 deletions
diff --git a/crates/core/src/manager/tests.rs b/crates/core/src/manager/tests.rs index c82d3dd..3d628a8 100644 --- a/crates/core/src/manager/tests.rs +++ b/crates/core/src/manager/tests.rs @@ -363,6 +363,35 @@ } #[test] + fn closing_a_window_remembers_its_geometry_even_if_it_was_never_dragged_or_resized() { + // Real report: "windows don't remember their placement/size" -- + // true for any window the user never manually touched, since only + // `end_drag`/`end_resize` used to write `remembered_geometry` at + // all. A window that was simply placed by SmartPlacement, looked + // at, and closed had nothing recorded, so reopening it always fell + // back to a fresh placement - indistinguishable from the memory + // feature not existing at all for that (extremely common) case. + let mut wm = wm_with_monitor(); + wm.set_layout(wm.current_workspace(), "tiling"); + let a = wm.alloc_window_id(); + let mut w = Window::new(a, "a"); + w.app_id = "alacritty".into(); + w.geometry = Rect::new(321, 111, 444, 222); + wm.add_window(w); + // Never dragged, never resized - closed exactly as SmartPlacement + // left it. + wm.remove_window(a); + + let b = wm.alloc_window_id(); + let mut w2 = Window::new(b, "b"); + w2.app_id = "alacritty".into(); + w2.geometry = Rect::new(0, 0, 800, 600); + wm.add_window(w2); + let placed = wm.window(b).unwrap().geometry; + assert_eq!((placed.x, placed.y, placed.width, placed.height), (321, 111, 444, 222), "the next alacritty window must open where/how large the first one was when it closed"); + } + + #[test] fn a_remembered_position_on_a_monitor_that_no_longer_exists_falls_back_to_placement() { let mut wm = wm_with_monitor(); wm.set_layout(wm.current_workspace(), "dynamic"); diff --git a/crates/core/src/manager/windows.rs b/crates/core/src/manager/windows.rs index 5468796..40b4c17 100644 --- a/crates/core/src/manager/windows.rs +++ b/crates/core/src/manager/windows.rs @@ -261,7 +261,30 @@ impl WindowManager { if self.focused == Some(id) { self.focused = self.order.last().copied(); } - self.windows.remove(&id) + let window = self.windows.remove(&id); + // Remembers wherever this app's window actually ended up, not just + // wherever a manual drag/resize left it (`dragresize.rs`'s own + // `end_drag`/`end_resize` sites) - without this, an app the user + // never dragged or resized had nothing recorded at all, so closing + // and reopening it always fell back to a fresh cascade placement + // regardless of where it had actually been sitting. Reported live + // as "windows don't remember their placement", indistinguishable + // from a broken feature even though the underlying store and its + // read side (`WindowManager::add_window`'s own `remembered_ + // geometry` lookup) were already both correct - this was the one + // write path that never fired for an app the user just opens, + // looks at, and closes. Same `app_id`-non-empty gate as the + // drag/resize sites, and the same reasoning for not also gating on + // `floating`: a tiled window's geometry is layout-computed and + // simply never consulted again on the read side once `layout_name + // == "tiling"`, so remembering it anyway is harmless, not wasted + // work worth a special case. + if let Some(w) = &window { + if !w.app_id.is_empty() { + self.remembered_geometry.insert(w.app_id.clone(), (w.geometry.x, w.geometry.y, w.geometry.width, w.geometry.height)); + } + } + window } pub fn window(&self, id: WindowId) -> Option<&Window> { diff --git a/crates/wayland/src/state/desktop_icons.rs b/crates/wayland/src/state/desktop_icons.rs index a1b1968..8ed22a5 100644 --- a/crates/wayland/src/state/desktop_icons.rs +++ b/crates/wayland/src/state/desktop_icons.rs @@ -72,22 +72,16 @@ impl CompState { self.desktop_icon_buffers.clear(); } - /// One grid origin per monitor icons should mirror onto: every enabled - /// monitor when `general.desktop_icons_all_monitors` is on (the - /// default - see that field's own doc comment), otherwise just the - /// primary monitor's, matching the original single-monitor behaviour. - /// Sorted by monitor id so the list (and therefore which origin - /// `icon_at` matches first) is stable call to call, not at the mercy of - /// `WindowManager::monitors()`'s own iteration order. + /// One grid origin per monitor icons should mirror onto - see + /// `icon_origins_for`'s own doc comment for the actual selection + /// logic. Sorted by monitor id first so the list (and therefore which + /// origin `icon_at` matches first) is stable call to call, not at the + /// mercy of `WindowManager::monitors()`'s own iteration order. fn desktop_icon_origins(&self) -> Vec<(i32, i32)> { let wm = self.wm.borrow(); let mut monitors = wm.monitors().to_vec(); monitors.sort_by_key(|m| m.id); - if wm.desktop_icons_all_monitors { - monitors.iter().map(|m| (m.geometry.x + GRID_MARGIN, m.geometry.y + GRID_MARGIN)).collect() - } else { - monitors.iter().find(|m| m.primary).map(|m| vec![(m.geometry.x + GRID_MARGIN, m.geometry.y + GRID_MARGIN)]).unwrap_or_default() - } + icon_origins_for(&monitors, wm.desktop_icons_all_monitors) } /// Re-derives the icon list from the real filesystem (a new/removed @@ -706,6 +700,47 @@ fn on_path(bin: &str) -> bool { std::env::var_os("PATH").map(|paths| std::env::split_paths(&paths).any(|dir| dir.join(bin).is_file())).unwrap_or(false) } +/// The actual logic behind [`CompState::desktop_icon_origins`] - pulled +/// out so it's testable without a real `WindowManager`/`Output`, the same +/// reasoning `udev/outputs.rs::next_logical_x` already applies. `monitors` +/// must already be sorted by id. +/// +/// One origin per real, physically distinct screen when `all_monitors` is +/// set - not one per `Monitor` entry, which `srd.monitor.split` can +/// multiply several of out of the *same* real output purely for +/// placement/tiling purposes (see `Monitor::split`'s own doc comment: "not +/// a second wl_output, not a second physical connector"). Mirroring the +/// full icon set onto every split part put two, visually side by side, on +/// what is still one continuous physical desktop - reported live as +/// "doesn't look like 2 more monitors, just showing double desktop icons" +/// the moment a two-way split was tried. A split part's own name is +/// always `"{connector}-{part}"` (`platform.rs`'s `sub_name` +/// construction) - recovering the real connector from it and keeping +/// only the first (lowest-id) part per connector collapses every split +/// group back to the one real screen it actually is, while a genuinely +/// separate additional monitor (real or fake, `split == false`) still +/// gets its own origin exactly as before. When `all_monitors` is unset, +/// just the primary monitor's origin, matching the original single- +/// monitor behaviour. +fn icon_origins_for(monitors: &[srdwm_core::Monitor], all_monitors: bool) -> Vec<(i32, i32)> { + if all_monitors { + let mut seen_split_connectors = std::collections::HashSet::new(); + monitors + .iter() + .filter(|m| { + if !m.split { + return true; + } + let connector = m.name.rsplit_once('-').map(|(base, _)| base).unwrap_or(&m.name); + seen_split_connectors.insert(connector.to_string()) + }) + .map(|m| (m.geometry.x + GRID_MARGIN, m.geometry.y + GRID_MARGIN)) + .collect() + } else { + monitors.iter().find(|m| m.primary).map(|m| vec![(m.geometry.x + GRID_MARGIN, m.geometry.y + GRID_MARGIN)]).unwrap_or_default() + } +} + #[cfg(test)] mod tests { use super::*; @@ -745,4 +780,50 @@ mod tests { let (c, r) = nearest_free_cell((0, 0), &occupied); assert!(c >= 0 && r >= 0); } + + fn split_part(id: srdwm_core::MonitorId, name: &str, x: i32) -> srdwm_core::Monitor { + let mut m = srdwm_core::Monitor::new(id, name, srdwm_core::Rect::new(x, 0, 960, 1080)); + m.split = true; + m + } + + #[test] + fn a_two_way_split_screen_gets_only_one_icon_origin_not_two() { + // The live-reported bug: "doesn't look like 2 more monitors, just + // showing double desktop icons" - a split screen is still one + // physical desktop, so mirroring the icon column onto both halves + // read as a visual duplication bug, not a second monitor. + let monitors = vec![split_part(0, "eDP-1-1", 0), split_part(1, "eDP-1-2", 960)]; + let origins = icon_origins_for(&monitors, true); + assert_eq!(origins.len(), 1, "a split screen must contribute exactly one icon origin, not one per split part"); + assert_eq!(origins[0], (GRID_MARGIN, GRID_MARGIN), "the one origin kept must be the first (lowest-id) split part's"); + } + + #[test] + fn a_split_screen_plus_a_genuinely_separate_monitor_gets_two_origins() { + let mut real_second = srdwm_core::Monitor::new(2, "HDMI-A-1", srdwm_core::Rect::new(1920, 0, 1920, 1080)); + real_second.split = false; + let monitors = vec![split_part(0, "eDP-1-1", 0), split_part(1, "eDP-1-2", 960), real_second]; + let origins = icon_origins_for(&monitors, true); + assert_eq!(origins.len(), 2, "a genuinely separate monitor must still get its own origin alongside the split screen's one"); + } + + #[test] + fn two_different_split_connectors_each_get_their_own_origin() { + // Guards the grouping logic against merging two *different* real + // outputs that both happen to be split, not just deduplicating one + // connector's own parts. + let monitors = vec![split_part(0, "eDP-1-1", 0), split_part(1, "eDP-1-2", 960), split_part(2, "HDMI-A-1-1", 1920), split_part(3, "HDMI-A-1-2", 2880)]; + let origins = icon_origins_for(&monitors, true); + assert_eq!(origins.len(), 2, "each split connector is its own physical screen and must get its own origin"); + } + + #[test] + fn without_all_monitors_only_the_primary_gets_an_origin_even_when_split() { + let mut a = split_part(0, "eDP-1-1", 0); + a.primary = true; + let b = split_part(1, "eDP-1-2", 960); + let origins = icon_origins_for(&[a, b], false); + assert_eq!(origins, vec![(GRID_MARGIN, GRID_MARGIN)]); + } } diff --git a/crates/wayland/src/state/lifecycle.rs b/crates/wayland/src/state/lifecycle.rs index 53a5e86..1749a3c 100644 --- a/crates/wayland/src/state/lifecycle.rs +++ b/crates/wayland/src/state/lifecycle.rs @@ -301,6 +301,11 @@ impl CompState { self.close_snap_flyout(); } self.wm.borrow_mut().remove_window(id); + // Persists whatever `remove_window` just snapshotted into + // `remembered_geometry` - see that function's own doc comment for + // why a window closing, not just a manual drag/resize release, + // needs to reach disk too. + crate::window_memory::save_all(self.wm.borrow().all_remembered_geometry()); self.pending.borrow_mut().push(CoreEvent::WindowDestroyed(id)); foreign_toplevel::window_closed(self, id); // `remove_window` may have picked a new focused window on its own diff --git a/crates/wayland/src/udev/platform.rs b/crates/wayland/src/udev/platform.rs index 37abf5d..d7132ee 100644 --- a/crates/wayland/src/udev/platform.rs +++ b/crates/wayland/src/udev/platform.rs @@ -856,7 +856,16 @@ impl Platform for UdevPlatform { // erasing the split it was placed to respect. m.full_geometry = srdwm_core::monitor::split_rect(full, part, parts, rows); m.maximize_geometry = srdwm_core::monitor::split_rect(maximize, part, parts, rows); - m.primary = primary_name.as_deref() == Some(name.as_str()); + // Only the first part of a split connector, not every one + // of them - `primary_name` names the *connector*, which + // doesn't change across `0..parts`, so this used to mark + // every split part primary at once. Two (or more) `Monitor` + // entries all claiming `primary: true` broke the "exactly + // one primary" assumption every caller of this field + // reasonably makes (`desktop_icon_origins`'s own single- + // monitor branch, concretely, which just took whichever + // `.find(|m| m.primary)` happened to match first). + m.primary = part == 0 && primary_name.as_deref() == Some(name.as_str()); m.split = parts > 1; m.scale = scale; out.push(m); diff --git a/crates/wayland/src/xwayland.rs b/crates/wayland/src/xwayland.rs index 5b537d2..9fba4e5 100644 --- a/crates/wayland/src/xwayland.rs +++ b/crates/wayland/src/xwayland.rs @@ -759,6 +759,10 @@ impl CompState { self.close_snap_flyout(); } self.wm.borrow_mut().remove_window(id); + // Same reason as `state/lifecycle.rs`'s native `remove_window`: + // persist whatever `remove_window` just snapshotted into + // `remembered_geometry`, not just a manual drag/resize release. + crate::window_memory::save_all(self.wm.borrow().all_remembered_geometry()); self.pending.borrow_mut().push(CoreEvent::WindowDestroyed(id)); crate::foreign_toplevel::window_closed(self, id); // Same reason as the equivalent call in `state/lifecycle.rs`'s native diff --git a/docs/TODO.md b/docs/TODO.md index 67ff09a..fbe5ec9 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -1,5 +1,17 @@ # TODO / planned features - master checklist +## Three real bugs found from one screenshot: window memory never saved on close, split screens duplicated desktop icons, split parts all claimed primary (2026-08-27) + +Asked directly why windows always spawn top-left and don't remember placement/size, and to screenshot the just-split display since it "doesn't look like 2 more monitors, just showing double desktop icons." Took a real `grim` screenshot rather than guessing from code, and it showed both reported symptoms at once plus revealed why the first one happens at all. + +**Window memory only ever saved on a manual drag/resize release.** `WindowManager::remembered_geometry` (per-`app_id` last floating position+size, read at window-map time) was real and correctly wired on the *read* side, but the only two places that ever wrote to it were `end_drag`/`end_resize` in `dragresize.rs` - a real user action, mouse button released after actually dragging or resizing. An app the user opens, looks at, and closes without ever touching its edges or titlebar had nothing recorded at all, so reopening it always fell back to a fresh cascade placement - indistinguishable from the feature not existing, for what is probably the *majority* of ordinary window lifecycles. Fixed by also snapshotting geometry into `remembered_geometry` from `WindowManager::remove_window` itself (gated the same `app_id`-non-empty way the drag/resize sites already are), and persisting it to disk at both of `remove_window`'s two wayland-side call sites (`state/lifecycle.rs`'s native unmap path, `xwayland.rs`'s X11 equivalent) the same way `input/pointer.rs`'s drag/resize-release site already does. + +**A split screen mirrored the full desktop-icon set onto every split part.** `desktop_icon_origins`'s "mirror icons onto every monitor" mode (`general.desktop_icons_all_monitors`, the default) iterated every `Monitor` entry `srd monitors` reports - which, after a `srd.monitor.split`, includes one entry *per split part*, not one per real physical screen (see `Monitor::split`'s own doc comment: "not a second wl_output, not a second physical connector"). Two icon columns side by side on what is still one continuous physical desktop read as a visual bug, not "2 more monitors" - which is exactly the screenshot. Fixed in a new, separately-testable `icon_origins_for` (pulled out of `desktop_icon_origins` the same way `udev/outputs.rs::next_logical_x` was pulled out of `relayout_outputs`): a split part's name is always `"{connector}-{part}"`, so recovering the connector and keeping only the lowest-id part per connector collapses every split group back to the one real screen it is, while a genuinely separate monitor (real or fake) still gets its own origin. + +**Every split part reported `primary: true`, not just one.** Found while fixing the above: `platform.rs`'s per-part loop computed `m.primary` from the *connector's* name, which doesn't change across `0..parts`, so a primary connector's split produced two-or-more `Monitor` entries all claiming primary - silently broke the "exactly one primary monitor" assumption several callers reasonably make (including the icon-origin fix's own single-monitor branch, which just took whichever `.find(|m| m.primary)` matched first). Fixed by gating on `part == 0` too. + +Full workspace build/test/clippy clean (224 core / 146 wayland tests, both up from before). Not yet live-verified against a real restart - needs one, same as everything else this session. + The single consolidated list of pending work. Before this file, "what's left" was scattered across four places that each grew their own list independently (`MISSING.md`, `PANEL_SUPPORT_TODO.md`, |