From df36d1c9b0f9a217a0619f01b1df383050539fd1 Mon Sep 17 00:00:00 2001 From: Samuel Onoja Date: Sat, 24 Oct 2026 19:48:16 +0100 Subject: [PATCH] Fix floating placement and restore on overlapping outputs (#60) Carry output identity through floating placement and restore, and retain the owning output when saved geometry overlaps multiple monitors equally. Fixes #59. --- compositor/src/state/floating.rs | 38 +++- .../src/state/window_lifecycle_tests.rs | 204 ++++++++++++++---- compositor/src/state/windows.rs | 66 +++--- 3 files changed, 220 insertions(+), 88 deletions(-) diff --git a/compositor/src/state/floating.rs b/compositor/src/state/floating.rs index b4388bf..eb90cfe 100644 --- a/compositor/src/state/floating.rs +++ b/compositor/src/state/floating.rs @@ -72,9 +72,10 @@ impl Ferese { let Some(window) = self.focused_window else { return; }; - let Some(bounds) = self.output_bounds() else { + let Some((output, bounds)) = self.floating_work_area() else { return; }; + let output = output.name(); let mut floating_rect = match self.workspaces.placement(window) { Some(WindowPlacement::Tiled) => self .tiled_layout(bounds) @@ -94,6 +95,7 @@ impl Ferese { floating_rect = self.place_floating( &handle, Some(window), + &output, bounds, (floating_rect.width, floating_rect.height), None, @@ -305,10 +307,14 @@ impl Ferese { } pub(super) fn output_bounds(&self) -> Option { + self.floating_work_area().map(|(_, work)| work) + } + + pub(super) fn floating_work_area(&self) -> Option<(&Output, Rect)> { let output = self .focused_output() .or_else(|| self.space.outputs().next())?; - self.output_bounds_for(output) + self.output_bounds_for(output).map(|work| (output, work)) } pub(super) fn floating_outputs(&self) -> Vec<(String, Rect)> { @@ -377,17 +383,13 @@ impl Ferese { &mut self, window: &Window, id: Option, + output: &str, work: Rect, size: (f64, f64), overrides: Option<(Option, Option)>, ) -> Rect { use crate::floating::{centered, clamp_to_work, intersection, min_overlap}; let outputs = self.floating_outputs(); - let output = outputs - .iter() - .find(|(_, area)| *area == work) - .map(|(name, _)| name.as_str()) - .unwrap_or("unknown"); let (app, parent) = Self::floating_metadata(window); let parent_rect = parent .as_ref() @@ -492,14 +494,20 @@ impl Ferese { return; } // A drag can finish on another output without changing workspace ownership. + // Equal overlap does not identify a monitor; retain the workspace owner. + let owner = self + .floating_work_area_for_window(id) + .map(|(output, _)| output.name()); let Some((output, work)) = self .floating_outputs() .into_iter() - .max_by(|(_, a), (_, b)| { + .max_by(|(a_name, a), (b_name, b)| { let area = |work| { crate::floating::intersection(rect, work).map_or(0., |r| r.width * r.height) }; - area(*a).total_cmp(&area(*b)) + area(*a).total_cmp(&area(*b)).then_with(|| { + (owner.as_ref() == Some(a_name)).cmp(&(owner.as_ref() == Some(b_name))) + }) }) .filter(|(_, work)| crate::floating::intersection(rect, *work).is_some()) else { @@ -553,14 +561,22 @@ impl Ferese { } pub(super) fn floating_bounds_for_window(&self, window: WindowId) -> Option { + self.floating_work_area_for_window(window) + .map(|(_, work)| work) + } + + pub(super) fn floating_work_area_for_window( + &self, + window: WindowId, + ) -> Option<(&Output, Rect)> { let output_id = self .workspaces .workspace_for_window(window) .and_then(|workspace| self.output_workspaces.output_for_workspace(workspace)); output_id .and_then(|id| self.outputs_by_id.get(&id)) - .and_then(|output| self.output_bounds_for(output)) - .or_else(|| self.output_bounds()) + .and_then(|output| self.output_bounds_for(output).map(|work| (output, work))) + .or_else(|| self.floating_work_area()) } pub(crate) fn output_bounds_for(&self, output: &Output) -> Option { diff --git a/compositor/src/state/window_lifecycle_tests.rs b/compositor/src/state/window_lifecycle_tests.rs index 4b688a4..cf88716 100644 --- a/compositor/src/state/window_lifecycle_tests.rs +++ b/compositor/src/state/window_lifecycle_tests.rs @@ -424,54 +424,170 @@ fn floating_launch_restores_geometry_only_on_the_focused_output() { ) { return; } - for restore_on_focused_output in [false, true] { - let (mut events, mut state, first, second) = floating_launch_fixture(); - let saved_output = if restore_on_focused_output { - &second - } else { - &first - }; - let saved_work = state.output_bounds_for(saved_output).unwrap(); - let saved = Rect::new(saved_work.x + 100., saved_work.y + 80., 240., 160.); - state.floating_memory.save( - "dev.ferese.Settings".into(), - crate::floating::Remembered::new(saved_output.name(), saved, saved_work), - ); - // Keep the pointer on the launch output to isolate saved-placement - // redirection from the separate keyboard-versus-pointer regression. - let pointer = state.seat.get_pointer().unwrap(); - pointer.motion( - &mut state, - None, - &MotionEvent { - location: (900., 100.).into(), - serial: SERIAL_COUNTER.next_serial(), - time: smithay::backend::input::InputTime::from_millis(1), - }, - ); - state.focus_output_at(pointer.current_location()); - let output_id = state.output_ids[&second]; - let destination = state.output_workspaces.active_workspace(output_id).unwrap(); - let (window, _wire) = super::capture_privacy::tests::window_with_app_id( - &mut state, - &mut events, - 0xff112233, - "dev.ferese.Settings", - ); - let id = state.windows.ids()[&window]; - assert_eq!(state.output_workspaces.focused_output(), Some(output_id)); - assert_eq!(state.workspaces.workspace_for_window(id), Some(destination)); - let rect = state.windows.geometry(&id).unwrap().logical; - let bounds = state.output_bounds_for(&second).unwrap(); - assert_eq!(crate::floating::intersection(rect, bounds), Some(rect)); - if restore_on_focused_output { - assert_eq!(rect, saved); - } else { - assert_eq!((rect.width, rect.height), (64., 48.)); + for second_x in [800, 400, 0] { + for restore_on_focused_output in [true, false] { + let (mut events, mut state, first, second) = floating_launch_fixture(); + state.space.map_output(&second, (second_x, 0)); + let saved_output = if restore_on_focused_output { + &second + } else { + &first + }; + let saved_work = state.output_bounds_for(saved_output).unwrap(); + let saved = Rect::new(saved_work.x + 100., saved_work.y + 80., 240., 160.); + state.floating_memory.save( + "dev.ferese.Settings".into(), + crate::floating::Remembered::new(saved_output.name(), saved, saved_work), + ); + let output_id = state.output_ids[&second]; + let destination = state.output_workspaces.active_workspace(output_id).unwrap(); + state.activate_output_workspace(output_id, destination); + let (window, _wire) = super::capture_privacy::tests::window_with_app_id( + &mut state, + &mut events, + 0xff112233, + "dev.ferese.Settings", + ); + let id = state.windows.ids()[&window]; + assert_eq!(state.output_workspaces.focused_output(), Some(output_id)); + assert_eq!(state.workspaces.workspace_for_window(id), Some(destination)); + let rect = state.windows.geometry(&id).unwrap().logical; + let bounds = state.output_bounds_for(&second).unwrap(); + assert_eq!(crate::floating::intersection(rect, bounds), Some(rect)); + if restore_on_focused_output { + assert_eq!(rect, saved, "second output at x={second_x}"); + } else { + assert_eq!( + (rect.width, rect.height), + (64., 48.), + "memory belongs to the other output; second output at x={second_x}" + ); + } } } } +#[test] +fn floating_rule_restores_geometry_on_the_windows_output() { + if !crate::startup_tests::private_runtime( + "state::window_lifecycle_tests::floating_rule_restores_geometry_on_the_windows_output", + ) { + return; + } + for second_x in [800, 0] { + let (mut events, mut state, first, second) = floating_launch_fixture(); + state.space.map_output(&second, (second_x, 0)); + let owner = state.output_ids[&second]; + let workspace = state.output_workspaces.active_workspace(owner).unwrap(); + state.activate_output_workspace(owner, workspace); + let (window, _wire) = window(&mut state, &mut events, 0xff112233); + let id = state.windows.ids()[&window]; + let work = state.output_bounds_for(&second).unwrap(); + let saved = Rect::new(work.x + 100., work.y + 80., 240., 160.); + state.windows.update(id, |record| { + record.floating_memory = + Some(crate::floating::Remembered::new(second.name(), saved, work)); + }); + let focused = state.output_ids[&first]; + state.activate_output_workspace( + focused, + state.output_workspaces.active_workspace(focused).unwrap(), + ); + state.apply_window_rule_result( + &window, + crate::window_rules::WindowRuleResult { + floating: Some(true), + ..Default::default() + }, + ); + assert_eq!(state.output_workspaces.focused_output(), Some(focused)); + assert_eq!(state.workspaces.workspace_for_window(id), Some(workspace)); + assert_eq!(state.windows.geometry(&id).unwrap().logical, saved); + } +} + +#[test] +fn floating_memory_round_trip_preserves_output_identity() { + if !crate::startup_tests::private_runtime( + "state::window_lifecycle_tests::floating_memory_round_trip_preserves_output_identity", + ) { + return; + } + for second_x in [0, 400] { + for on_second in [false, true] { + let (mut events, mut state, first, second) = floating_launch_fixture(); + state.space.map_output(&second, (second_x, 0)); + let owner = if on_second { &second } else { &first }; + let output_id = state.output_ids[owner]; + let workspace = state.output_workspaces.active_workspace(output_id).unwrap(); + state.activate_output_workspace(output_id, workspace); + let (window, wire) = super::capture_privacy::tests::window_with_app_id( + &mut state, + &mut events, + 0xff112233, + "dev.ferese.Settings", + ); + let id = state.windows.ids()[&window]; + // The whole rectangle fits both outputs, so area alone cannot + // decide which monitor's placement memory should own it. + let saved = Rect::new(f64::from(second_x) + 100., 80., 240., 160.); + state.workspaces.set_floating_rect(id, saved).unwrap(); + state.relayout_window(id); + state.remember_floating(&window); + assert_eq!( + state + .floating_memory + .get("dev.ferese.Settings") + .unwrap() + .output, + owner.name(), + "owner={} second_x={second_x}", + owner.name(), + ); + drop(wire); + dispatch(&mut events, &mut state); + assert!(state.windows.ids().is_empty()); + state.activate_output_workspace(output_id, workspace); + let (restored, _wire) = super::capture_privacy::tests::window_with_app_id( + &mut state, + &mut events, + 0xff112233, + "dev.ferese.Settings", + ); + let id = state.windows.ids()[&restored]; + assert_eq!(state.windows.geometry(&id).unwrap().logical, saved); + } + } +} + +#[test] +fn floating_memory_follows_a_drag_to_another_output() { + if !crate::startup_tests::private_runtime( + "state::window_lifecycle_tests::floating_memory_follows_a_drag_to_another_output", + ) { + return; + } + let (mut events, mut state, first, second) = floating_launch_fixture(); + let owner = state.output_ids[&first]; + let workspace = state.output_workspaces.active_workspace(owner).unwrap(); + state.activate_output_workspace(owner, workspace); + let (window, _wire) = super::capture_privacy::tests::window_with_app_id( + &mut state, + &mut events, + 0xff112233, + "dev.ferese.Settings", + ); + let id = state.windows.ids()[&window]; + let saved = Rect::new(900., 80., 240., 160.); + state.workspaces.set_floating_rect(id, saved).unwrap(); + state.relayout_window(id); + state.remember_floating(&window); + assert_eq!(state.workspaces.workspace_for_window(id), Some(workspace)); + let memory = state.floating_memory.get("dev.ferese.Settings").unwrap(); + assert_eq!(memory.output, second.name()); + assert_eq!(memory.restore(&state.floating_outputs()), Some(saved)); +} + #[test] fn keyboard_floating_move_and_resize_work_in_both_layouts() { if !crate::startup_tests::private_runtime( diff --git a/compositor/src/state/windows.rs b/compositor/src/state/windows.rs index 20bee34..decac32 100644 --- a/compositor/src/state/windows.rs +++ b/compositor/src/state/windows.rs @@ -58,19 +58,16 @@ impl Ferese { return; } - let bounds = self.output_bounds(); + let Some((output, work)) = self.floating_work_area() else { + return; + }; + let output = output.name(); let outputs = self.floating_outputs(); - let Some((remembered, work)) = app_id + let Some(remembered) = app_id .as_deref() .and_then(|app| self.floating_memory.get(app)) - .and_then(|entry| { - let rect = entry.restore(&outputs)?; - let work = outputs - .iter() - .find(|(name, work)| *name == entry.output && Some(*work) == bounds)? - .1; - Some((rect, work)) - }) + .filter(|entry| entry.output == output) + .and_then(|entry| entry.restore(&outputs)) else { return; }; @@ -80,6 +77,7 @@ impl Ferese { let rect = self.place_floating( window, None, + &output, work, (remembered.width, remembered.height), Some((rule.width, rule.height)), @@ -110,24 +108,19 @@ impl Ferese { return; } - let Some(bounds) = self.output_bounds() else { + let Some((output, bounds)) = self.floating_work_area() else { self.add_tiled_window(window); return; }; + let output = output.name(); // Saved geometry may restore a position on this output, but must not // redirect a new window away from the output the user focused. - let remembered_work = app_id + let remembered_rect = app_id .filter(|app| !crate::window_rules::is_native_dialog(Some(app))) .and_then(|app| self.floating_memory.get(app)) - .and_then(|entry| { - let outputs = self.floating_outputs(); - entry.restore(&outputs)?; - outputs - .into_iter() - .find(|(name, work)| *name == entry.output && *work == bounds) - .map(|(_, work)| work) - }); + .filter(|entry| entry.output == output) + .and_then(|entry| entry.restore(&self.floating_outputs())); let id = self.windows.allocate_id(); let size_rect = client_size(&window) .map(|size| natural_floating_rect(bounds, size)) @@ -135,6 +128,7 @@ impl Ferese { let rect = self.place_floating( &window, None, + &output, bounds, (size_rect.width, size_rect.height), None, @@ -161,7 +155,7 @@ impl Ferese { if rule.width.is_none() && rule.height.is_none() && client_size(&window).is_none() - && remembered_work.is_none() + && remembered_rect.is_none() { self.windows .update(id, |w| w.natural_floating_pending = true); @@ -221,10 +215,11 @@ impl Ferese { return; }; - let Some(bounds) = self.floating_bounds_for_window(parent) else { + let Some((output, bounds)) = self.floating_work_area_for_window(parent) else { self.add_tiled_window(window); return; }; + let output = output.name(); let parent_rect = self.presented_window_rect(parent).unwrap_or(bounds); let visible_parent = crate::floating::intersection(parent_rect, bounds).unwrap_or(bounds); @@ -234,6 +229,7 @@ impl Ferese { let rect = self.place_floating( &window, None, + &output, bounds, (size_rect.width, size_rect.height), None, @@ -384,9 +380,10 @@ impl Ferese { return; }; - let Some(bounds) = self.floating_bounds_for_window(id) else { + let Some((output, bounds)) = self.floating_work_area_for_window(id) else { return; }; + let output = output.name(); if rule == Default::default() { return; @@ -414,7 +411,10 @@ impl Ferese { tracing::warn!(%error, ?id, ?workspace, "failed to apply window workspace rule"); } - let bounds = self.floating_bounds_for_window(id).unwrap_or(bounds); + let (output, bounds) = self + .floating_work_area_for_window(id) + .map(|(output, work)| (output.name(), work)) + .unwrap_or((output, bounds)); let should_float = rule .floating @@ -432,11 +432,7 @@ impl Ferese { .0 .as_deref() .and_then(|app| self.floating_memory.get(app)) - .filter(|entry| { - self.floating_outputs() - .iter() - .any(|(name, work)| *name == entry.output && *work == bounds) - }) + .filter(|entry| entry.output == output) .and_then(|entry| entry.restore(&self.floating_outputs())) .is_none() { @@ -450,6 +446,7 @@ impl Ferese { rect = self.place_floating( window, Some(id), + &output, bounds, (rect.width, rect.height), Some((rule.width, rule.height)), @@ -508,10 +505,10 @@ impl Ferese { ); return None; } - self.floating_bounds_for_window(parent) - .map(|bounds| (parent, bounds)) + self.floating_work_area_for_window(parent) + .map(|(output, bounds)| (parent, output.name(), bounds)) }); - let Some((parent, bounds)) = placement else { + let Some((parent, output, bounds)) = placement else { if removed { self.relayout(); } @@ -526,6 +523,7 @@ impl Ferese { let rect = self.place_floating( window, Some(id), + &output, bounds, (size_rect.width, size_rect.height), None, @@ -771,11 +769,13 @@ impl Ferese { self.workspaces.placement(id), Some(WindowPlacement::Floating { .. }) ) - && let Some(bounds) = self.floating_bounds_for_window(id) + && let Some((output, bounds)) = self.floating_work_area_for_window(id) { + let output = output.name(); let rect = self.place_floating( window, Some(id), + &output, bounds, (f64::from(size.width), f64::from(size.height)), Some((Some(f64::from(size.width)), Some(f64::from(size.height)))),