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

@@ -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<Rect> {
+        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<WindowId>,
+        output: &str,
         work: Rect,
         size: (f64, f64),
         overrides: Option<(Option<f64>, Option<f64>)>,
     ) -> 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<Rect> {
+        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))
+            .and_then(|output| self.output_bounds_for(output).map(|work| (output, work)))
-            .or_else(|| self.output_bounds())
+            .or_else(|| self.floating_work_area())
     }
 
     pub(crate) fn output_bounds_for(&self, output: &Output) -> Option<Rect> {

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] {
+    for second_x in [800, 400, 0] {
-        let (mut events, mut state, first, second) = floating_launch_fixture();
+        for restore_on_focused_output in [true, false] {
-        let saved_output = if restore_on_focused_output {
+            let (mut events, mut state, first, second) = floating_launch_fixture();
-            &second
+            state.space.map_output(&second, (second_x, 0));
-        } else {
+            let saved_output = if restore_on_focused_output {
-            &first
+                &second
-        };
+            } else {
-        let saved_work = state.output_bounds_for(saved_output).unwrap();
+                &first
-        let saved = Rect::new(saved_work.x + 100., saved_work.y + 80., 240., 160.);
+            };
-        state.floating_memory.save(
+            let saved_work = state.output_bounds_for(saved_output).unwrap();
-            "dev.ferese.Settings".into(),
+            let saved = Rect::new(saved_work.x + 100., saved_work.y + 80., 240., 160.);
-            crate::floating::Remembered::new(saved_output.name(), saved, saved_work),
+            state.floating_memory.save(
-        );
+                "dev.ferese.Settings".into(),
-        // Keep the pointer on the launch output to isolate saved-placement
+                crate::floating::Remembered::new(saved_output.name(), saved, saved_work),
-        // redirection from the separate keyboard-versus-pointer regression.
+            );
-        let pointer = state.seat.get_pointer().unwrap();
+            let output_id = state.output_ids[&second];
-        pointer.motion(
+            let destination = state.output_workspaces.active_workspace(output_id).unwrap();
-            &mut state,
+            state.activate_output_workspace(output_id, destination);
-            None,
+            let (window, _wire) = super::capture_privacy::tests::window_with_app_id(
-            &MotionEvent {
+                &mut state,
-                location: (900., 100.).into(),
+                &mut events,
-                serial: SERIAL_COUNTER.next_serial(),
+                0xff112233,
-                time: smithay::backend::input::InputTime::from_millis(1),
+                "dev.ferese.Settings",
-            },
+            );
-        );
+            let id = state.windows.ids()[&window];
-        state.focus_output_at(pointer.current_location());
+            assert_eq!(state.output_workspaces.focused_output(), Some(output_id));
-        let output_id = state.output_ids[&second];
+            assert_eq!(state.workspaces.workspace_for_window(id), Some(destination));
-        let destination = state.output_workspaces.active_workspace(output_id).unwrap();
+            let rect = state.windows.geometry(&id).unwrap().logical;
-        let (window, _wire) = super::capture_privacy::tests::window_with_app_id(
+            let bounds = state.output_bounds_for(&second).unwrap();
-            &mut state,
+            assert_eq!(crate::floating::intersection(rect, bounds), Some(rect));
-            &mut events,
+            if restore_on_focused_output {
-            0xff112233,
+                assert_eq!(rect, saved, "second output at x={second_x}");
-            "dev.ferese.Settings",
+            } else {
-        );
+                assert_eq!(
-        let id = state.windows.ids()[&window];
+                    (rect.width, rect.height),
-        assert_eq!(state.output_workspaces.focused_output(), Some(output_id));
+                    (64., 48.),
-        assert_eq!(state.workspaces.workspace_for_window(id), Some(destination));
+                    "memory belongs to the other output; second output at x={second_x}"
-        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.));
         }
     }
 }
 
+#[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(

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| {
+            .filter(|entry| entry.output == output)
-                let rect = entry.restore(&outputs)?;
+            .and_then(|entry| entry.restore(&outputs))
-                let work = outputs
-                    .iter()
-                    .find(|(name, work)| *name == entry.output && Some(*work) == bounds)?
-                    .1;
-                Some((rect, work))
-            })
         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| {
+            .filter(|entry| entry.output == output)
-                let outputs = self.floating_outputs();
+            .and_then(|entry| entry.restore(&self.floating_outputs()));
-                entry.restore(&outputs)?;
-                outputs
-                    .into_iter()
-                    .find(|(name, work)| *name == entry.output && *work == bounds)
-                    .map(|(_, work)| work)
-            });
         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| {
+                    .filter(|entry| entry.output == output)
-                        self.floating_outputs()
-                            .iter()
-                            .any(|(name, work)| *name == entry.output && *work == bounds)
-                    })
                     .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)
+            self.floating_work_area_for_window(parent)
-                .map(|bounds| (parent, bounds))
+                .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)))),