Switch to using xwayland shell protocol over WL_SURFACE_ID

Seems to fix #13. This means a hard requirement on Xwayland 23.1.

README.md

@@ -3,6 +3,7 @@ xwayland-satellite grants rootless Xwayland integration to any Wayland composito
 This is particularly useful for compositors that (understandably) do not want to go through implementing support for rootless Xwayland themselves.
 
 ## Dependencies
+- Xwayland >=23.1
 - xcb
 - xcb-util-cursor
 - clang (building only)

src/server/dispatch.rs

@@ -1,5 +1,5 @@
 use super::*;
-use log::{debug, trace, warn};
+use log::{debug, error, trace, warn};
 use std::sync::{Arc, OnceLock};
 use wayland_protocols::{
     wp::{
@@ -34,6 +34,10 @@ use wayland_protocols::{
             zxdg_output_v1::{self as s_xdgo, ZxdgOutputV1 as XdgOutputServer},
         },
     },
+    xwayland::shell::v1::server::{
+        xwayland_shell_v1::{self, XwaylandShellV1},
+        xwayland_surface_v1::{self, XwaylandSurfaceV1},
+    },
 };
 use wayland_server::{
     protocol::{
@@ -128,9 +132,15 @@ impl<C: XConnection> Dispatch<WlSurface, ObjectKey> for ServerState<C> {
             Request::<WlSurface>::Destroy => {
                 let mut object = state.objects.remove(*key).unwrap();
                 let surface: &mut SurfaceData = object.as_mut();
+                if let Some(window_data) = surface.window.and_then(|w| state.windows.get_mut(&w)) {
+                    window_data.surface_key.take();
+                }
                 surface.destroy_role();
                 surface.client.destroy();
-                debug!("deleting key: {:?}", key);
+                debug!(
+                    "deleting key: {key:?} (surface {:?})",
+                    surface.server.id().protocol_id()
+                );
             }
             Request::<WlSurface>::SetBufferScale { scale } => {
                 surface.client.set_buffer_scale(scale);
@@ -181,37 +191,25 @@ impl<C: XConnection>
             Request::<WlCompositor>::CreateSurface { id } => {
                 let mut surface_id = None;
 
-                let key = state.objects.insert_with_key(|key| {
+                state.objects.insert_with_key(|key| {
-                    debug!("new surface with key {key:?}");
                     let client = client.create_surface(&state.qh, key);
                     let server = data_init.init(id, key);
                     surface_id = Some(server.id().protocol_id());
+                    debug!("new surface with key {key:?} ({surface_id:?})");
 
                     SurfaceData {
                         client,
                         server,
                         key,
+                        serial: Default::default(),
                         attach: None,
                         frame_callback: None,
                         role: None,
+                        xwl: None,
+                        window: None,
                     }
                     .into()
                 });
-
-                let surface_id = surface_id.unwrap();
-
-                if let Some((win, window_data)) =
-                    state.windows.iter_mut().find_map(|(win, data)| {
-                        Some(*win).zip((data.surface_id == surface_id).then_some(data))
-                    })
-                {
-                    window_data.surface_key = Some(key);
-                    state.associated_windows.insert(key, win);
-                    debug!("associate surface {surface_id} with window {win:?}");
-                    if window_data.mapped {
-                        state.create_role_window(win, key);
-                    }
-                }
             }
             Request::<WlCompositor>::CreateRegion { id } => {
                 let c_region = client.create_region(&state.qh, ());
@@ -1075,3 +1073,95 @@ global_dispatch_no_events!(PointerConstraintsServer, PointerConstraintsClient);
 global_dispatch_with_events!(WlSeat, client::wl_seat::WlSeat);
 global_dispatch_with_events!(WlOutput, client::wl_output::WlOutput);
 global_dispatch_with_events!(WlDrmServer, WlDrmClient);
+
+impl<C: XConnection> GlobalDispatch<XwaylandShellV1, ()> for ServerState<C> {
+    fn bind(
+        _: &mut Self,
+        _: &DisplayHandle,
+        _: &wayland_server::Client,
+        resource: wayland_server::New<XwaylandShellV1>,
+        _: &(),
+        data_init: &mut wayland_server::DataInit<'_, Self>,
+    ) {
+        data_init.init(resource, ());
+    }
+}
+
+impl<C: XConnection> Dispatch<XwaylandShellV1, ()> for ServerState<C> {
+    fn request(
+        state: &mut Self,
+        client: &wayland_server::Client,
+        _: &XwaylandShellV1,
+        request: <XwaylandShellV1 as Resource>::Request,
+        _: &(),
+        dhandle: &DisplayHandle,
+        data_init: &mut wayland_server::DataInit<'_, Self>,
+    ) {
+        use xwayland_shell_v1::Request;
+        match request {
+            Request::GetXwaylandSurface { id, surface } => {
+                let key: ObjectKey = surface.data().copied().unwrap();
+                let data: &mut SurfaceData = state.objects[key].as_mut();
+                if data.xwl.is_some() {
+                    error!("Surface {surface:?} already has the xwayland surface role!");
+                    client.kill(
+                        dhandle,
+                        wayland_client::backend::protocol::ProtocolError {
+                            code: 0,
+                            object_id: surface.id().protocol_id(),
+                            object_interface: "wl_surface".to_string(),
+                            message: "Surface already has role".to_string(),
+                        },
+                    );
+                    return;
+                }
+
+                let xwl = data_init.init(id, key);
+                data.xwl = Some(xwl);
+            }
+            Request::Destroy => {}
+            _ => unreachable!(),
+        }
+    }
+}
+
+impl<C: XConnection> Dispatch<XwaylandSurfaceV1, ObjectKey> for ServerState<C> {
+    fn request(
+        state: &mut Self,
+        _: &wayland_server::Client,
+        _: &XwaylandSurfaceV1,
+        request: <XwaylandSurfaceV1 as Resource>::Request,
+        key: &ObjectKey,
+        _: &DisplayHandle,
+        _: &mut wayland_server::DataInit<'_, Self>,
+    ) {
+        use xwayland_surface_v1::Request;
+        let data: &mut SurfaceData = state.objects[*key].as_mut();
+        match request {
+            Request::SetSerial {
+                serial_lo,
+                serial_hi,
+            } => {
+                let serial = [serial_lo, serial_hi];
+                data.serial = Some(serial);
+                if let Some((win, window_data)) =
+                    state.windows.iter_mut().find_map(|(win, data)| {
+                        Some(*win)
+                            .zip((data.surface_serial.is_some_and(|s| s == serial)).then_some(data))
+                    })
+                {
+                    debug!("associate surface {} with {:?}", data.server.id().protocol_id(), win);
+                    window_data.surface_key = Some(*key);
+                    state.associated_windows.insert(*key, win);
+                    if window_data.mapped {
+                        state.create_role_window(win, *key);
+                    }
+                }
+            }
+            Request::Destroy => {
+                data.xwl.take();
+            }
+            _ => unreachable!(),
+        }
+    }
+}

src/server/event.rs

@@ -395,6 +395,9 @@ impl HandleEvent for Pointer {
                 }
             }
             client::wl_pointer::Event::Leave { serial, surface } => {
+                if !surface.is_alive() {
+                    return;
+                }
                 debug!("leaving surface ({serial})");
                 self.pending_enter.0.take();
                 self.server
@@ -504,7 +507,12 @@ impl HandleEvent for Keyboard {
                 },
                 Leave {
                     serial,
-                    |surface| state.get_server_surface_from_client(surface)
+                    |surface| {
+                        if !surface.is_alive() {
+                            return;
+                        }
+                        state.get_server_surface_from_client(surface)
+                    }
                 },
                 Key {
                     serial,

src/server/mod.rs

@@ -33,6 +33,9 @@ use wayland_protocols::{
         },
         xdg_output::zv1::server::zxdg_output_manager_v1::ZxdgOutputManagerV1,
     },
+    xwayland::shell::v1::server::{
+        xwayland_shell_v1::XwaylandShellV1, xwayland_surface_v1::XwaylandSurfaceV1,
+    },
 };
 use wayland_server::{
     protocol::{
@@ -81,7 +84,7 @@ pub struct WindowAttributes {
 #[derive(Debug)]
 struct WindowData {
     window: x::Window,
-    surface_id: u32,
+    surface_serial: Option<[u32; 2]>,
     surface_key: Option<ObjectKey>,
     mapped: bool,
     attrs: WindowAttributes,
@@ -96,8 +99,8 @@ impl WindowData {
     ) -> Self {
         Self {
             window,
-            surface_id: 0,
             surface_key: None,
+            surface_serial: None,
             mapped: false,
             attrs: WindowAttributes {
                 override_redirect,
@@ -119,9 +122,12 @@ pub struct SurfaceData {
     client: client::wl_surface::WlSurface,
     server: WlSurface,
     key: ObjectKey,
+    serial: Option<[u32; 2]>,
     frame_callback: Option<WlCallback>,
     attach: Option<SurfaceAttach>,
     role: Option<SurfaceRole>,
+    xwl: Option<XwaylandSurfaceV1>,
+    window: Option<x::Window>,
 }
 
 impl SurfaceData {
@@ -410,6 +416,8 @@ impl<C: XConnection> ServerState<C> {
                 .registry
                 .bind::<XdgWmBase, _, _>(data.name, XDG_WM_BASE_VERSION, &qh, ());
 
+        dh.create_global::<Self, XwaylandShellV1, _>(1, ());
+
         let mut ret = Self {
             windows: HashMap::new(),
             clientside,
@@ -558,27 +566,9 @@ impl<C: XConnection> ServerState<C> {
         }
     }
 
-    pub fn associate_window(&mut self, window: x::Window, surface_id: u32) {
+    pub fn set_window_serial(&mut self, window: x::Window, serial: [u32; 2]) {
         let win = self.windows.get_mut(&window).unwrap();
-        win.surface_id = surface_id;
+        win.surface_serial = Some(serial);
-
-        if let Some(key) = self
-            .objects
-            .iter_mut()
-            .filter_map(|(key, obj)| {
-                Some(key).zip(<&mut SurfaceData>::try_from(obj.0.as_mut().unwrap()).ok())
-            })
-            .find_map(|(key, surface)| {
-                (surface_id == surface.server.id().protocol_id()).then_some(key)
-            })
-        {
-            win.surface_key = Some(key);
-            self.associated_windows.insert(key, window);
-            debug!("associate {:?} with surface {surface_id}", window);
-            if win.mapped {
-                self.create_role_window(window, key);
-            }
-        }
     }
 
     pub fn reconfigure_window(&mut self, event: x::ConfigureNotifyEvent) {
@@ -689,7 +679,8 @@ impl<C: XConnection> ServerState<C> {
     }
 
     fn create_role_window(&mut self, window: x::Window, surface_key: ObjectKey) {
-        let surface: &SurfaceData = self.objects[surface_key].as_ref();
+        let surface: &mut SurfaceData = self.objects[surface_key].as_mut();
+        surface.window = Some(window);
         let client = &surface.client;
         client.attach(None, 0, 0);
         client.commit();
@@ -714,7 +705,7 @@ impl<C: XConnection> ServerState<C> {
                 window.window,
                 parent,
                 window.attrs.dims,
-                surface.client.id()
+                client.id()
             );
 
             let parent_window = self.windows.get(&parent).unwrap();

src/server/tests.rs

@@ -31,6 +31,9 @@ use wayland_protocols::{
         shell::server::{xdg_positioner, xdg_toplevel},
         xdg_output::zv1::client::zxdg_output_manager_v1::ZxdgOutputManagerV1,
     },
+    xwayland::shell::v1::client::{
+        xwayland_shell_v1::XwaylandShellV1, xwayland_surface_v1::XwaylandSurfaceV1,
+    },
 };
 use wayland_server::{protocol as s_proto, Display, Resource};
 use wl_drm::client::wl_drm::WlDrm;
@@ -82,6 +85,7 @@ with_optional! {
 struct Compositor {
     compositor: TestObject<WlCompositor>,
     shm: TestObject<WlShm>,
+    shell: TestObject<XwaylandShellV1>
 }
 
 }
@@ -202,6 +206,7 @@ struct TestFixture {
     exwl_connection: Arc<Connection>,
     /// Exwayland's display - must dispatch this for our server state to advance
     exwl_display: Display<FakeServerState>,
+    surface_serial: u64,
 }
 
 static INIT: std::sync::Once = std::sync::Once::new();
@@ -240,6 +245,7 @@ impl TestFixture {
             exwayland,
             exwl_connection: Connection::from_socket(fake_client).unwrap().into(),
             exwl_display: display,
+            surface_serial: 1,
         };
         f.run();
         f
@@ -291,6 +297,12 @@ impl TestFixture {
                             bind_req(name, WlShm::interface(), version),
                         ));
                     }
+                    x if x == XwaylandShellV1::interface().name => {
+                        ret.shell = Some(TestObject::from_request(
+                            &registry.obj,
+                            bind_req(name, XwaylandShellV1::interface(), version),
+                        ));
+                    }
                     _ => {}
                 }
             }
@@ -339,44 +351,68 @@ impl TestFixture {
             .insert(window, data);
     }
 
-    fn create_and_map_window(
+    fn new_window(
+        &mut self,
+        window: Window,
+        override_redirect: bool,
+        data: WindowData,
+        parent: Option<Window>,
+    ) {
+        let dims = data.dims;
+        self.register_window(window, data);
+        self.exwayland
+            .new_window(window, override_redirect, dims, parent);
+    }
+
+    fn map_window(
         &mut self,
         comp: &Compositor,
         window: Window,
-        override_redirect: bool,
+        surface: &WlSurface,
-    ) -> (TestObject<WlSurface>, testwl::SurfaceId) {
+        buffer: &TestObject<WlBuffer>,
-        let (_buffer, surface) = comp.create_surface();
+    ) {
-
+        self.exwayland.map_window(window);
-        let data = WindowData {
+        self.associate_window(comp, window, surface);
-            mapped: true,
+        self.run();
-            dims: WindowDims {
+        surface
+            .send_request(Req::<WlSurface>::Attach {
+                buffer: Some(buffer.obj.clone()),
                 x: 0,
                 y: 0,
-                width: 50,
+            })
-                height: 50,
+            .unwrap();
+    }
+
+    fn associate_window(&mut self, comp: &Compositor, window: Window, surface: &WlSurface) {
+        let xwl = TestObject::<XwaylandSurfaceV1>::from_request(
+            &comp.shell.obj,
+            Req::<XwaylandShellV1>::GetXwaylandSurface {
+                surface: surface.clone(),
             },
-            fullscreen: false,
+        );
-        };
 
-        let dims = data.dims;
+        let serial = self.surface_serial;
+        let serial_lo = (serial & 0xFF) as u32;
+        let serial_hi = (serial >> 8 & 0xFF) as u32;
+        self.surface_serial += 1;
 
-        self.register_window(window, data);
+        xwl.send_request(Req::<XwaylandSurfaceV1>::SetSerial {
+            serial_lo,
+            serial_hi,
+        })
+        .unwrap();
         self.exwayland
-            .new_window(window, override_redirect, dims, None);
+            .set_window_serial(window, [serial_lo, serial_hi]);
-        self.exwayland.map_window(window);
+    }
-        self.exwayland
-            .associate_window(window, surface.id().protocol_id());
 
-        self.run();
+    #[track_caller]
-
+    fn check_new_surface(&mut self) -> testwl::SurfaceId {
-        let testwl_id = self
+        let id = self
             .testwl
             .last_created_surface_id()
             .expect("Surface not created");
-
+        assert!(self.testwl.get_surface_data(id).is_some());
-        assert!(self.testwl.get_surface_data(testwl_id).is_some());
+        id
-
-        (surface, testwl_id)
     }
 
     fn create_toplevel(
@@ -384,7 +420,7 @@ impl TestFixture {
         comp: &Compositor,
         window: Window,
     ) -> (TestObject<WlSurface>, testwl::SurfaceId) {
-        let (_buffer, surface) = comp.create_surface();
+        let (buffer, surface) = comp.create_surface();
 
         let data = WindowData {
             mapped: true,
@@ -397,27 +433,18 @@ impl TestFixture {
             fullscreen: false,
         };
 
-        let dims = data.dims;
+        self.new_window(window, false, data, None);
-
+        self.map_window(comp, window, &surface.obj, &buffer);
-        self.register_window(window, data);
-        self.exwayland.new_window(window, false, dims, None);
-        self.exwayland.map_window(window);
-        self.exwayland
-            .associate_window(window, surface.id().protocol_id());
-
         self.run();
+        let id = self.check_new_surface();
 
-        let testwl_id = self
-            .testwl
-            .last_created_surface_id()
-            .expect("Toplevel surface not created");
         {
-            let surface_data = self.testwl.get_surface_data(testwl_id).unwrap();
+            let surface_data = self.testwl.get_surface_data(id).unwrap();
             assert!(
                 surface_data.surface
                     == self
                         .testwl
-                        .get_object::<s_proto::wl_surface::WlSurface>(testwl_id)
+                        .get_object::<s_proto::wl_surface::WlSurface>(id)
                         .unwrap()
             );
             assert!(surface_data.buffer.is_none());
@@ -429,11 +456,11 @@ impl TestFixture {
         }
 
         self.testwl
-            .configure_toplevel(testwl_id, 100, 100, vec![xdg_toplevel::State::Activated]);
+            .configure_toplevel(id, 100, 100, vec![xdg_toplevel::State::Activated]);
         self.run();
 
         {
-            let surface_data = self.testwl.get_surface_data(testwl_id).unwrap();
+            let surface_data = self.testwl.get_surface_data(id).unwrap();
             assert!(surface_data.buffer.is_some());
         }
 
@@ -451,7 +478,7 @@ impl TestFixture {
             "Incorrect window geometry: {win_data:?}"
         );
 
-        (surface, testwl_id)
+        (surface, id)
     }
 
     fn create_popup(
@@ -460,7 +487,7 @@ impl TestFixture {
         window: Window,
         parent_id: testwl::SurfaceId,
     ) -> (TestObject<WlSurface>, testwl::SurfaceId) {
-        let (_, popup_surface) = comp.create_surface();
+        let (buffer, surface) = comp.create_surface();
         let data = WindowData {
             mapped: true,
             dims: WindowDims {
@@ -472,13 +499,11 @@ impl TestFixture {
             fullscreen: false,
         };
         let dims = data.dims;
-        self.register_window(window, data);
+        self.new_window(window, true, data, None);
-        self.exwayland.new_window(window, true, dims, None);
+        self.map_window(&comp, window, &surface.obj, &buffer);
-        self.exwayland.map_window(window);
-        self.exwayland
-            .associate_window(window, popup_surface.id().protocol_id());
         self.run();
-        let popup_id = self.testwl.last_created_surface_id().unwrap();
+
+        let popup_id = self.check_new_surface();
         assert_ne!(popup_id, parent_id);
 
         {
@@ -533,7 +558,7 @@ impl TestFixture {
             assert!(surface_data.buffer.is_some());
         }
 
-        (popup_surface, popup_id)
+        (surface, popup_id)
     }
 }
 
@@ -597,8 +622,6 @@ type Ev<T> = <T as Proxy>::Event;
 
 // TODO: tests to add
 // - destroy window before surface
-// - destroy surface before window
-// - destroy popup and reassociate with new surface
 // - reconfigure window (popup) before mapping
 // - associate window after surface is already created
 
@@ -701,7 +724,8 @@ fn pass_through_globals() {
         ZxdgOutputManagerV1,
         WpViewporter,
         WlDrm,
-        ZwpPointerConstraintsV1
+        ZwpPointerConstraintsV1,
+        XwaylandShellV1
     }
 
     let mut globals = SupportedGlobals::default();
@@ -777,8 +801,7 @@ fn popup_window_changes_surface() {
     assert!(f.testwl.get_surface_data(id).is_some());
 
     f.exwayland.map_window(win);
-    f.exwayland
+    f.associate_window(&comp, win, &surface.obj);
-        .associate_window(win, surface.id().protocol_id());
     f.run();
 
     let data = f.testwl.get_surface_data(id).unwrap();
@@ -807,7 +830,10 @@ fn override_redirect_window_after_toplevel_close() {
     assert!(f.testwl.get_surface_data(first).is_none());
 
     let win2 = unsafe { Window::new(2) };
-    let (_, second) = f.create_and_map_window(&comp, win2, true);
+    let (buffer, surface) = comp.create_surface();
+    f.new_window(win2, true, WindowData::default(), None);
+    f.map_window(&comp, win2, &surface.obj, &buffer);
+    let second = f.check_new_surface();
     let data = f.testwl.get_surface_data(second).unwrap();
     assert!(
         matches!(data.role, Some(testwl::SurfaceRole::Toplevel(_))),
@@ -934,8 +960,7 @@ fn window_group_properties() {
         },
     );
     f.exwayland.map_window(win);
-    f.exwayland
+    f.associate_window(&comp, win, &surface.obj);
-        .associate_window(win, surface.id().protocol_id());
     f.run();
 
     let id = f.testwl.last_created_surface_id().unwrap();

src/xstate.rs

@@ -263,11 +263,13 @@ impl XState {
                 }
                 xcb::Event::X(x::Event::ClientMessage(e)) => match e.r#type() {
                     x if x == self.atoms.wl_surface_id => {
+                        panic!("Xserver should be using WL_SURFACE_SERIAL, not WL_SURFACE_ID");
+                    }
+                    x if x == self.atoms.wl_surface_serial => {
                         let x::ClientMessageData::Data32(data) = e.data() else {
                             unreachable!();
                         };
-                        let id: u32 = (data[0] as u64 | ((data[1] as u64) << 32)) as u32;
+                        server_state.set_window_serial(e.window(), [data[0], data[1]]);
-                        server_state.associate_window(e.window(), id);
                     }
                     x if x == self.atoms.net_wm_state => {
                         let x::ClientMessageData::Data32(data) = e.data() else {
@@ -544,6 +546,7 @@ xcb::atoms_struct! {
     #[derive(Clone, Debug)]
     pub struct Atoms {
         pub wl_surface_id => b"WL_SURFACE_ID" only_if_exists = false,
+        pub wl_surface_serial => b"WL_SURFACE_SERIAL" only_if_exists = false,
         pub wm_protocols => b"WM_PROTOCOLS" only_if_exists = false,
         pub wm_delete_window => b"WM_DELETE_WINDOW" only_if_exists = false,
         pub wm_transient_for => b"WM_TRANSIENT_FOR" only_if_exists = false,

tests/integration.rs

@@ -300,11 +300,6 @@ impl Connection {
             .unwrap();
     }
 
-    fn unmap_window(&self, window: x::Window) {
-        self.send_and_check_request(&x::UnmapWindow { window })
-            .unwrap();
-    }
-
     fn set_property<P: x::PropEl>(
         &self,
         window: x::Window,
@@ -404,6 +399,7 @@ fn toplevel_flow() {
     );
     f.wait_and_dispatch();
     let data = f.testwl.get_surface_data(surface).unwrap();
+    let toplevel = data.toplevel().toplevel.clone();
     assert_eq!(data.toplevel().title, Some("bindow".into()));
     assert_eq!(data.toplevel().app_id, Some("ssalc".into()));
     assert_eq!(
@@ -421,8 +417,7 @@ fn toplevel_flow() {
     drop(connection);
     f.wait_and_dispatch();
 
-    let data = f.testwl.get_surface_data(surface).expect("No surface data");
+    assert!(!toplevel.is_alive());
-    assert!(!data.toplevel().toplevel.is_alive());
 }
 
 #[test]