srdusr
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--crates/core/src/manager/tests.rs40
-rw-r--r--crates/core/src/manager/windows.rs53
-rw-r--r--crates/core/src/placement.rs152
-rw-r--r--crates/wayland/src/state/geometry.rs11
4 files changed, 242 insertions, 14 deletions
diff --git a/crates/core/src/manager/tests.rs b/crates/core/src/manager/tests.rs
index e20bb74..b5a54c0 100644
--- a/crates/core/src/manager/tests.rs
+++ b/crates/core/src/manager/tests.rs
@@ -116,6 +116,46 @@
assert!(!win.size_is_provisional, "a deliberately remembered size must never be second-guessed by the client's own default");
}
+ /// A client that accepts the size placement assumed must not be moved
+ /// again: re-placing would consume another cascade step for nothing,
+ /// and the cascade wraps - measured, two windows landing on exactly
+ /// the same spot because the extra steps wrapped one back to the origin.
+ #[test]
+ fn a_window_that_keeps_the_assumed_size_is_not_placed_twice() {
+ let mut wm = wm_with_monitor();
+ let id = wm.alloc_window_id();
+ wm.add_window(Window::new(id, "w"));
+ let placed = wm.window(id).unwrap().geometry;
+ assert!(!wm.replace_with_real_size(id, (placed.width, placed.height)));
+ assert_eq!(wm.window(id).unwrap().geometry, placed);
+ }
+
+ /// And a client that chooses a different size gets placed for the size
+ /// it really is, not for the placeholder a backend guessed.
+ #[test]
+ fn a_window_that_chooses_its_own_size_is_placed_again_for_it() {
+ let mut wm = wm_with_monitor();
+ let first = wm.alloc_window_id();
+ wm.add_window(Window::new(first, "first"));
+ let second = wm.alloc_window_id();
+ wm.add_window(Window::new(second, "second"));
+ let assumed = wm.window(second).unwrap().geometry;
+ // The client turns out to be much smaller than the placeholder.
+ {
+ let w = wm.window_mut(second).unwrap();
+ w.geometry.width = 300;
+ w.geometry.height = 200;
+ }
+ // The return value is an implementation detail - it says whether
+ // anything moved, and the right position may be the one it already
+ // had. What matters is where it ends up.
+ wm.replace_with_real_size(second, (assumed.width, assumed.height));
+ let placed = wm.window(second).unwrap().geometry;
+ assert_eq!((placed.width, placed.height), (300, 200), "the real size must be kept");
+ let other = wm.window(first).unwrap().geometry;
+ assert!(!other.overlaps(&placed), "a window small enough to fit clear should not be stacked: {other:?} vs {placed:?}");
+ }
+
/// The real shape of the bug: a Wayland toplevel exists before its
/// client sends `set_app_id`, so `add_window` searched the store for the
/// empty string and every window fell through to a fresh cascade. That
diff --git a/crates/core/src/manager/windows.rs b/crates/core/src/manager/windows.rs
index 1e8a6cb..69d5e9e 100644
--- a/crates/core/src/manager/windows.rs
+++ b/crates/core/src/manager/windows.rs
@@ -411,6 +411,59 @@ impl WindowManager {
self.order.iter().filter_map(|id| self.windows.get(id))
}
+ /// Re-runs smart placement for a window that has just learned its real
+ /// size.
+ ///
+ /// `add_window` has to place a window before its client has committed
+ /// anything, so it places the placeholder size a backend guessed --
+ /// `800x600` - and not the size the window actually turns out to be.
+ /// Every placement decision was therefore made about the wrong
+ /// rectangle: measured on a 1280x800 screen, four small terminals were
+ /// all told they were 800x600, no two of those fit side by side, so
+ /// every one of them fell through to the cascade and landed on top of
+ /// the last. The windows themselves were a quarter of that size and
+ /// would have fitted four abreast.
+ ///
+ /// Called from the backend at the moment the client's real size is
+ /// adopted, which is still before its first buffer, so nothing has been
+ /// drawn at the placeholder-sized position.
+ ///
+ /// Only for a window still sitting where placement put it: a remembered
+ /// geometry, a rule, a dialog and a maximize have all already cleared
+ /// `size_is_provisional` by this point, and each is a more deliberate
+ /// decision than smart placement.
+ pub fn replace_with_real_size(&mut self, id: WindowId, placed_size: (u32, u32)) -> bool {
+ let Some(w) = self.windows.get(&id) else { return false };
+ if w.is_dialog || w.maximized || w.fullscreen {
+ return false;
+ }
+ // The client accepted the size placement already assumed, so the
+ // decision it made was about the right rectangle after all. Leaving
+ // it alone matters: re-running placement here would consume another
+ // cascade step for no reason, and the cascade wraps - measured, two
+ // windows landing on exactly the same spot because the extra steps
+ // wrapped one of them back to the origin.
+ if placed_size == (w.geometry.width, w.geometry.height) {
+ return false;
+ }
+ let (workspace, monitor_id, size) = (w.workspace, w.monitor, (w.geometry.width, w.geometry.height));
+ let layout_name = self.workspace(workspace).map(|ws| ws.layout.clone()).unwrap_or_default();
+ if layout_name == "tiling" {
+ return false;
+ }
+ let Some(monitor) = self.monitors.iter().find(|m| m.id == monitor_id).cloned() else { return false };
+ let existing: Vec<Rect> = self.windows_on_workspace(workspace).filter(|other| other.id != id).map(|other| other.geometry).collect();
+ let step = self.next_cascade_step.get();
+ self.next_cascade_step.set(step.wrapping_add(1));
+ let placed = SmartPlacement::place(&monitor, &existing, size, &self.placement, step);
+ let Some(w) = self.windows.get_mut(&id) else { return false };
+ if w.geometry == placed {
+ return false;
+ }
+ w.geometry = placed;
+ true
+ }
+
pub(super) fn windows_on_workspace(&self, workspace: WorkspaceId) -> impl Iterator<Item = &Window> {
self.windows.values().filter(move |w| w.workspace == workspace)
}
diff --git a/crates/core/src/placement.rs b/crates/core/src/placement.rs
index cd3c29c..482d175 100644
--- a/crates/core/src/placement.rs
+++ b/crates/core/src/placement.rs
@@ -158,6 +158,16 @@ pub fn clamp_into(rect: Rect, area: Rect) -> Rect {
pub struct SmartPlacement;
+/// How many of the most central free positions `SmartPlacement::free_spot`
+/// rotates between.
+///
+/// One would be the best placement every time and put every consecutively
+/// opened window in the same place; the whole list would scatter windows
+/// into corners for no reason. Four is enough that opening the same
+/// application repeatedly does not stack it in one spot, while every choice
+/// is still among the most central positions available.
+const CENTRAL_CHOICES: usize = 4;
+
impl SmartPlacement {
/// Place a new window of `size` given the geometries of windows already
/// occupying `monitor`. Tries a grid cell first, falling back to cascade.
@@ -180,7 +190,95 @@ impl SmartPlacement {
if existing.is_empty() {
return Self::cascade(monitor, size, cfg, cascade_step);
}
- Self::grid(monitor, existing, size, cfg, cascade_step).unwrap_or_else(|| Self::cascade(monitor, size, cfg, cascade_step))
+ // A spot where the window covers nothing is always the right answer
+ // when one exists, and is what "smart placement" means in every
+ // window manager that has it. Only when the screen genuinely cannot
+ // fit the window clear of everything else does this fall through to
+ // the diagonal cascade, which is what Windows does once its own
+ // screen fills up.
+ Self::free_spot(monitor, existing, size, cascade_step)
+ .or_else(|| Self::grid(monitor, existing, size, cfg, cascade_step))
+ .unwrap_or_else(|| Self::cascade(monitor, size, cfg, cascade_step))
+ }
+
+ /// A position where a `size` window overlaps nothing already on screen,
+ /// or `None` when the monitor cannot fit one.
+ ///
+ /// The candidate positions are the edges of what is already there --
+ /// every existing window's left and right edge, plus the monitor's own,
+ /// and the same vertically - taken both as "put my left edge here" and
+ /// "put my right edge here". That is Openbox's `place_overlap` reduced
+ /// to the case this needs, and the reasoning behind it is that a
+ /// rectangle packed against other rectangles is always flush with one of
+ /// their edges: nothing is gained by testing the space between two
+ /// edges, so a handful of candidates covers every distinct arrangement.
+ ///
+ /// Why this replaced a fixed grid: the grid asked whether a *cell* was
+ /// free and then placed the window at the cell's corner at its own,
+ /// larger size. An ordinary 800x600 window on a 1280x800 screen overlaps
+ /// every cell of a 2x2 grid, so no cell was ever free, the grid returned
+ /// nothing, and every window fell through to the cascade - measured,
+ /// five windows opening at 30,30 then 60,60 then 90,90 and so on, which
+ /// is exactly "they spawn on top of each other, predominantly one side".
+ ///
+ /// Ties go to the position nearest the middle of the monitor. Several
+ /// free spots usually exist and the first in scan order is always the
+ /// top-left one, which is the other half of the same complaint.
+ fn free_spot(monitor: &Monitor, existing: &[Rect], size: (u32, u32), cascade_step: u32) -> Option<Rect> {
+ let area = monitor.geometry;
+ let (w, h) = (size.0 as i32, size.1 as i32);
+ if w > area.width as i32 || h > area.height as i32 {
+ return None;
+ }
+ let mut xs: Vec<i32> = vec![area.x, area.right() - w];
+ let mut ys: Vec<i32> = vec![area.y, area.bottom() - h];
+ for r in existing {
+ xs.push(r.x);
+ xs.push(r.right());
+ xs.push(r.x - w);
+ xs.push(r.right() - w);
+ ys.push(r.y);
+ ys.push(r.bottom());
+ ys.push(r.y - h);
+ ys.push(r.bottom() - h);
+ }
+ xs.retain(|&x| x >= area.x && x + w <= area.right());
+ ys.retain(|&y| y >= area.y && y + h <= area.bottom());
+ xs.sort_unstable();
+ xs.dedup();
+ ys.sort_unstable();
+ ys.dedup();
+
+ let centre = (area.x + area.width as i32 / 2, area.y + area.height as i32 / 2);
+ let mut free: Vec<(i64, Rect)> = Vec::new();
+ for &x in &xs {
+ for &y in &ys {
+ let candidate = Rect::new(x, y, size.0, size.1);
+ if existing.iter().any(|r| r.overlaps(&candidate)) {
+ continue;
+ }
+ let cx = x + w / 2 - centre.0;
+ let cy = y + h / 2 - centre.1;
+ free.push(((cx as i64) * (cx as i64) + (cy as i64) * (cy as i64), candidate));
+ }
+ }
+ if free.is_empty() {
+ return None;
+ }
+ // Nearest the middle first: several free spots usually exist and the
+ // first in scan order is always the top-left one, which is half of
+ // "windows spawn predominantly one side".
+ free.sort_by_key(|(distance, rect)| (*distance, rect.x, rect.y));
+ // Then rotate through the best few rather than always taking the
+ // single best. Least-overlap placement is deterministic, so opening
+ // one window at a time - open, use, close, open the next - puts
+ // every one of them in exactly the same place, which was reported
+ // here before as "every window spawns in the exact same spot, not at
+ // all like Windows". Rotating keeps the no-overlap guarantee (every
+ // candidate in this list is free) while giving consecutive windows
+ // somewhere different to land.
+ let choices = free.len().min(CENTRAL_CHOICES);
+ Some(free[cascade_step as usize % choices].1)
}
fn grid(monitor: &Monitor, existing: &[Rect], size: (u32, u32), cfg: &PlacementConfig, cascade_step: u32) -> Option<Rect> {
@@ -336,20 +434,46 @@ mod tests {
assert!(!first.overlaps(&second), "second window must not overlap the first: {first:?} vs {second:?}");
}
+ /// Cascade is the last resort now, not the second one: it runs when the
+ /// screen genuinely cannot fit the window clear of what is already
+ /// there. This test used to assert the opposite - that a full *grid*
+ /// forced a cascade - which stopped being true once a free position
+ /// was looked for first, and rightly so: a free spot existed in that
+ /// case and cascading on top of things instead was the bug.
#[test]
- fn cascade_kicks_in_once_grid_is_full() {
- let cfg = PlacementConfig { max_grid: 1, ..Default::default() };
- // max_grid=1 means the grid is always a single cell, so a second
- // window can never find a free grid cell and must cascade.
- let first = SmartPlacement::place(&monitor(), &[], (400, 300), &cfg, 0);
- let second = SmartPlacement::place(&monitor(), &[first], (400, 300), &cfg, 1);
- assert_ne!(first, second);
- // First window is grid-placed (offset by grid_margin); the second no
- // longer fits any grid cell and falls back to cascade, which steps
- // from the monitor origin by `cascade_offset` per window opened so
- // far this session (the caller's own counter, passed in as `1` here).
- assert_eq!(second.x, cfg.cascade_offset * 2);
- assert_eq!(second.y, cfg.cascade_offset * 2);
+ fn cascade_is_the_last_resort_when_nothing_fits_clear() {
+ let cfg = PlacementConfig::default();
+ let area = monitor().geometry;
+ // One window covering the whole usable area: no free position for
+ // anything, at any size.
+ let covered = [area];
+ let placed = SmartPlacement::place(&monitor(), &covered, (400, 300), &cfg, 1);
+ assert_eq!(placed.x, cfg.cascade_offset * 2);
+ assert_eq!(placed.y, cfg.cascade_offset * 2);
+ }
+
+ /// The point of looking for a free position at all.
+ #[test]
+ fn a_second_window_does_not_land_on_top_of_the_first() {
+ let cfg = PlacementConfig::default();
+ // The measured case: 800x600 windows on a 1280x800 screen, where
+ // every cell of a 2x2 grid overlaps the first window, so the grid
+ // could never find one and everything cascaded into a pile.
+ let first = Rect::new(30, 30, 800, 600);
+ let second = SmartPlacement::place(&monitor(), &[first], (800, 600), &cfg, 1);
+ assert!(!first.overlaps(&second), "second window landed on the first: {first:?} vs {second:?}");
+ }
+
+ /// Every rotated choice must still be a free one - rotating is for
+ /// variety, never at the cost of the guarantee.
+ #[test]
+ fn every_rotation_choice_is_still_free_of_the_windows_already_open() {
+ let cfg = PlacementConfig::default();
+ let existing = [Rect::new(0, 0, 300, 200)];
+ for step in 0..8 {
+ let placed = SmartPlacement::place(&monitor(), &existing, (300, 200), &cfg, step);
+ assert!(!existing[0].overlaps(&placed), "step {step} overlapped: {placed:?}");
+ }
}
#[test]
diff --git a/crates/wayland/src/state/geometry.rs b/crates/wayland/src/state/geometry.rs
index 9780ae3..43e0f87 100644
--- a/crates/wayland/src/state/geometry.rs
+++ b/crates/wayland/src/state/geometry.rs
@@ -668,6 +668,10 @@ impl CompState {
let width = ((content.size.w as f64 * scale).round() as u32).max(srdwm_core::placement::MIN_WINDOW_WIDTH);
let height = ((content.size.h as f64 * scale).round() as u32).max(srdwm_core::placement::MIN_WINDOW_HEIGHT) + band;
let Some(w) = wm.window_mut(id) else { return };
+ // What placement was given to work with, kept so
+ // `replace_with_real_size` can tell whether the client actually
+ // chose something different.
+ let placed_size = (w.geometry.width, w.geometry.height);
// The client has now made a real choice, so this size is no longer
// a guess. Clearing the core flag is what lets `remove_window`
// remember it: that path deliberately refuses to remember a size
@@ -680,5 +684,12 @@ impl CompState {
w.geometry.x = w.geometry.x.min(monitor.right() - width as i32).max(monitor.x);
w.geometry.y = w.geometry.y.min(monitor.bottom() - height as i32).max(monitor.y);
}
+ // Now that the real size is known, place the window again for it.
+ // `add_window` had to decide where this window went before its
+ // client had committed anything, so it placed the `800x600`
+ // placeholder rather than the window - see
+ // `replace_with_real_size`. Still before the first buffer, so
+ // nothing has been drawn at the placeholder-sized position.
+ wm.replace_with_real_size(id, placed_size);
}
}
generated by cgit v1.2.3 (git 2.39.1) at 2026-10-06 16:39:25 +0000