fix(scap-targets): keep partially visible X11 windows selectable

.github/workflows/ci.yml

@@ -282,6 +282,7 @@ jobs:
           cargo test --locked -p cap --bin cap record::tests
           cargo test --locked -p cap-recording --lib -- --test-threads=1
           cargo test --locked -p cap-recording --test x11_shm_capture -- --test-threads=1
+          cargo test --locked -p scap-targets --lib --test linux_targets
           cargo test --locked -p cap-enc-ffmpeg
 
   build-gpui:

crates/scap-targets/src/platform/linux.rs

@@ -294,17 +294,7 @@ impl WindowImpl {
     }
 
     pub fn display(&self) -> Option<DisplayImpl> {
-        let bounds = self.physical_bounds()?;
-        let center = PhysicalPosition::new(
-            bounds.position().x() + bounds.size().width() / 2.0,
-            bounds.position().y() + bounds.size().height() / 2.0,
-        );
-
-        DisplayImpl::list().into_iter().find(|display| {
-            display
-                .physical_bounds()
-                .is_some_and(|display_bounds| display_bounds.contains_point(center))
-        })
+        display_for_bounds(self.physical_bounds()?, &DisplayImpl::list())
     }
 
     pub fn name(&self) -> Option<String> {
@@ -322,6 +312,24 @@ impl WindowImpl {
     }
 }
 
+fn display_for_bounds(bounds: PhysicalBounds, displays: &[DisplayImpl]) -> Option<DisplayImpl> {
+    displays
+        .iter()
+        .filter_map(|display| {
+            let display_bounds = display.physical_bounds()?;
+            let left = bounds.position().x().max(display_bounds.position().x());
+            let top = bounds.position().y().max(display_bounds.position().y());
+            let right = (bounds.position().x() + bounds.size().width())
+                .min(display_bounds.position().x() + display_bounds.size().width());
+            let bottom = (bounds.position().y() + bounds.size().height())
+                .min(display_bounds.position().y() + display_bounds.size().height());
+            let area = (right - left).max(0.0) * (bottom - top).max(0.0);
+            (area > 0.0).then_some((*display, area))
+        })
+        .max_by(|(_, a), (_, b)| a.total_cmp(b))
+        .map(|(display, _)| display)
+}
+
 fn filter_window_candidates(windows: Vec<Window>) -> Vec<WindowImpl> {
     windows
         .into_iter()
@@ -491,3 +499,72 @@ fn get_cursor_position() -> Option<PhysicalPosition> {
         reply.root_y.into(),
     ))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn display(id: u32, x: i32, y: i32, width: u32, height: u32) -> DisplayImpl {
+        DisplayImpl {
+            id,
+            x,
+            y,
+            width,
+            height,
+            refresh_rate: 60.0,
+            logical_bounds: None,
+            wayland_uuid: None,
+        }
+    }
+
+    fn bounds(x: f64, y: f64, width: f64, height: f64) -> PhysicalBounds {
+        PhysicalBounds::new(
+            PhysicalPosition::new(x, y),
+            PhysicalSize::new(width, height),
+        )
+    }
+
+    #[test]
+    fn window_center_in_monitor_gap_still_resolves_to_visible_display() {
+        let displays = [display(0, 0, 0, 1000, 800), display(1, 1200, 0, 1000, 800)];
+        let window = bounds(900.0, 100.0, 350.0, 200.0);
+        assert_eq!(display_for_bounds(window, &displays).unwrap().id, 0);
+    }
+
+    #[test]
+    fn window_uses_largest_overlap_on_offset_monitors() {
+        let displays = [
+            display(0, -1920, 0, 1920, 1080),
+            display(1, 0, -900, 1440, 900),
+        ];
+        let window = bounds(-150.0, -100.0, 400.0, 300.0);
+        assert_eq!(display_for_bounds(window, &displays).unwrap().id, 0);
+        assert_eq!(
+            display_for_bounds(window, &[displays[1], displays[0]])
+                .unwrap()
+                .id,
+            0
+        );
+        assert_eq!(
+            display_for_bounds(bounds(-150.0, -200.0, 400.0, 300.0), &displays)
+                .unwrap()
+                .id,
+            1
+        );
+    }
+
+    #[test]
+    fn nonoverlapping_and_edge_touching_windows_have_no_display() {
+        let displays = [display(0, 0, 0, 1920, 1080)];
+        for window in [
+            bounds(-320.0, 100.0, 320.0, 240.0),
+            bounds(1920.0, 100.0, 320.0, 240.0),
+            bounds(100.0, -240.0, 320.0, 240.0),
+            bounds(100.0, 1080.0, 320.0, 240.0),
+            bounds(-400.0, -400.0, 320.0, 240.0),
+        ] {
+            assert!(display_for_bounds(window, &displays).is_none());
+        }
+        assert!(display_for_bounds(bounds(0.0, 0.0, 320.0, 240.0), &[]).is_none());
+    }
+}

crates/scap-targets/tests/linux_targets.rs

@@ -0,0 +1,166 @@
+#![cfg(target_os = "linux")]
+
+use std::{
+    io::{BufRead, BufReader},
+    process::{Child, Command, Stdio},
+};
+
+use scap_targets::{Display, Window, WindowId};
+use x11rb::{
+    COPY_DEPTH_FROM_PARENT,
+    connection::Connection,
+    protocol::xproto::{
+        AtomEnum, ConfigureWindowAux, ConnectionExt as _, CreateWindowAux, PropMode, WindowClass,
+    },
+    wrapper::ConnectionExt as _,
+};
+
+struct Xvfb(Child);
+
+impl Xvfb {
+    fn start() -> Option<(Self, String)> {
+        let (reader, writer) = std::io::pipe().ok()?;
+        let child = Command::new("Xvfb")
+            .args([
+                "-displayfd",
+                "1",
+                "-screen",
+                "0",
+                "1280x720x24",
+                "-nolisten",
+                "tcp",
+            ])
+            .stdin(Stdio::null())
+            .stdout(writer)
+            .stderr(Stdio::null())
+            .spawn();
+        let child = match child {
+            Ok(child) => child,
+            Err(error) => {
+                eprintln!("skipping X11 targets test: cannot start Xvfb: {error}");
+                return None;
+            }
+        };
+        let server = Self(child);
+        let mut line = String::new();
+        BufReader::new(reader)
+            .read_line(&mut line)
+            .expect("Xvfb display number");
+        let number: u32 = line.trim().parse().expect("Xvfb display number");
+        Some((server, format!(":{number}")))
+    }
+}
+
+impl Drop for Xvfb {
+    fn drop(&mut self) {
+        let _ = self.0.kill();
+        let _ = self.0.wait();
+    }
+}
+
+#[test]
+fn partially_offscreen_window_remains_a_recording_target() {
+    let Some((_server, display)) = Xvfb::start() else {
+        return;
+    };
+    // SAFETY: this binary has one test, which sets the variables before calling scap_targets.
+    unsafe {
+        std::env::set_var("DISPLAY", &display);
+        std::env::set_var("XDG_SESSION_TYPE", "x11");
+    }
+    let (conn, screen_num) = x11rb::connect(Some(&display)).unwrap();
+    let screen = &conn.setup().roots[screen_num];
+    let id = conn.generate_id().unwrap();
+    conn.create_window(
+        COPY_DEPTH_FROM_PARENT,
+        id,
+        screen.root,
+        100,
+        100,
+        320,
+        240,
+        0,
+        WindowClass::INPUT_OUTPUT,
+        0,
+        &CreateWindowAux::new(),
+    )
+    .unwrap()
+    .check()
+    .unwrap();
+    conn.change_property8(
+        PropMode::REPLACE,
+        id,
+        AtomEnum::WM_NAME,
+        AtomEnum::STRING,
+        b"Cap offscreen target regression",
+    )
+    .unwrap()
+    .check()
+    .unwrap();
+    conn.change_property8(
+        PropMode::REPLACE,
+        id,
+        AtomEnum::WM_CLASS,
+        AtomEnum::STRING,
+        b"cap-target-fixture\0CapTargetFixture\0",
+    )
+    .unwrap()
+    .check()
+    .unwrap();
+    conn.map_window(id).unwrap().check().unwrap();
+
+    let target_id: WindowId = id.to_string().parse().unwrap();
+    let display_id = Display::primary().id();
+    let width = i32::from(screen.width_in_pixels);
+    let height = i32::from(screen.height_in_pixels);
+    for (x, y) in [
+        (100, 100),
+        (-250, 100),
+        (width - 70, 100),
+        (100, -200),
+        (100, height - 40),
+        (-250, -200),
+    ] {
+        conn.configure_window(id, &ConfigureWindowAux::new().x(x).y(y))
+            .unwrap()
+            .check()
+            .unwrap();
+        let target = Window::from_id(&target_id)
+            .unwrap_or_else(|| panic!("visible window at ({x}, {y}) must remain selectable"));
+        assert_eq!(target.owner_name().as_deref(), Some("cap-target-fixture"));
+        assert_eq!(target.display().unwrap().id(), display_id);
+        let bounds = target.display_relative_logical_bounds().unwrap();
+        assert_eq!(bounds.position().x(), f64::from(x));
+        assert_eq!(bounds.position().y(), f64::from(y));
+        assert_eq!(bounds.size().width(), 320.0);
+        assert_eq!(bounds.size().height(), 240.0);
+        conn.warp_pointer(
+            x11rb::NONE,
+            screen.root,
+            0,
+            0,
+            0,
+            0,
+            i16::try_from(x.max(0) + 10).unwrap(),
+            i16::try_from(y.max(0) + 10).unwrap(),
+        )
+        .unwrap()
+        .check()
+        .unwrap();
+        assert_eq!(Window::get_topmost_at_cursor().unwrap().id(), target_id);
+    }
+
+    conn.configure_window(id, &ConfigureWindowAux::new().x(-320).y(100))
+        .unwrap()
+        .check()
+        .unwrap();
+    assert!(Window::from_id(&target_id).is_none());
+
+    conn.configure_window(id, &ConfigureWindowAux::new().x(100).y(100))
+        .unwrap()
+        .check()
+        .unwrap();
+    assert!(Window::from_id(&target_id).is_some());
+    conn.destroy_window(id).unwrap().check().unwrap();
+    assert!(Window::from_id(&target_id).is_none());
+}