Toplevel parent fixes, checks, and callback

src/wayland/shell/mod.rs

@@ -18,6 +18,7 @@
 use crate::{utils::Serial, wayland::compositor};
 use thiserror::Error;
 use wayland_server::protocol::wl_surface::WlSurface;
+use xdg::XdgToplevelSurfaceData;
 
 pub mod kde;
 pub mod wlr_layer;
@@ -43,5 +44,38 @@ pub fn is_toplevel_equivalent(surface: &WlSurface) -> bool {
     // xdg_toplevel is toplevel like, so verify if the role matches.
     let role = compositor::get_role(surface);
 
+    // When changing this, don't forget to change the check in is_valid_parent() below.
     matches!(role, Some(xdg::XDG_TOPLEVEL_ROLE))
 }
+
+/// Returns true if the `parent` is valid to set for `child`.
+///
+/// This will check that `parent` is toplevel equivalent, then make sure that it doesn't introduce
+/// a parent loop.
+pub fn is_valid_parent(child: &WlSurface, parent: &WlSurface) -> bool {
+    if !is_toplevel_equivalent(parent) {
+        return false;
+    }
+
+    // Check that we're not making a parent loop.
+    let mut next_parent = Some(parent.clone());
+    while let Some(parent) = next_parent.clone() {
+        // Did we find a cycle?
+        if *child == parent {
+            return false;
+        }
+
+        compositor::with_states(&parent, |states| {
+            if let Some(data) = states.data_map.get::<XdgToplevelSurfaceData>() {
+                // Get xdg-toplevel parent.
+                let role = data.lock().unwrap();
+                next_parent = role.parent.clone();
+            } else {
+                // Reached a surface we don't know how to get a parent of.
+                next_parent = None;
+            }
+        });
+    }
+
+    true
+}

src/wayland/shell/xdg/handlers.rs

@@ -11,5 +11,5 @@ pub use positioner::XdgPositionerUserData;
 
 mod surface;
 pub(in crate::wayland::shell) use surface::make_popup_handle;
-pub(super) use surface::{get_parent, send_popup_configure, send_toplevel_configure, set_parent};
+pub(super) use surface::{get_parent, send_popup_configure, send_toplevel_configure};
 pub use surface::{XdgShellSurfaceUserData, XdgSurfaceUserData};

src/wayland/shell/xdg/handlers/surface.rs

@@ -31,7 +31,7 @@ use super::{
 
 mod toplevel;
 use toplevel::make_toplevel_handle;
-pub use toplevel::{get_parent, send_toplevel_configure, set_parent};
+pub use toplevel::{get_parent, send_toplevel_configure};
 
 mod popup;
 pub use popup::{make_popup_handle, send_popup_configure};

src/wayland/shell/xdg/handlers/surface/toplevel.rs

@@ -2,7 +2,10 @@ use std::sync::atomic::Ordering;
 
 use crate::{
     utils::Serial,
-    wayland::{compositor, shell::xdg::XdgToplevelSurfaceData},
+    wayland::{
+        compositor,
+        shell::{is_valid_parent, xdg::XdgToplevelSurfaceData},
+    },
 };
 
 use wayland_protocols::xdg::shell::server::xdg_toplevel::{self, XdgToplevel};
@@ -46,8 +49,18 @@ where
                         .clone()
                 });
 
+                let old_parent = get_parent(toplevel);
+                let changed = old_parent != parent_surface;
+
                 // Parent is not double buffered, we can set it directly
-                set_parent(toplevel, parent_surface);
+                if !set_parent(toplevel, parent_surface) {
+                    toplevel.post_error(xdg_toplevel::Error::InvalidParent, "invalid parent toplevel");
+                }
+
+                if changed {
+                    let handle = make_toplevel_handle(toplevel);
+                    XdgShellHandler::parent_changed(state, handle);
+                }
             }
             xdg_toplevel::Request::SetTitle { title } => {
                 // Title is not double buffered, we can set it directly
@@ -204,17 +217,26 @@ pub fn get_parent(toplevel: &xdg_toplevel::XdgToplevel) -> Option<wl_surface::Wl
     with_surface_toplevel_role_data(toplevel, |data| data.parent.clone())
 }
 
-/// Sets the parent of the specified toplevel surface.
+/// Sets the parent of the specified toplevel surface and returns whether the parent was successfully set.
 ///
 /// The parent must be a toplevel surface.
 ///
 /// The parent of a surface is not double buffered and therefore may be set directly.
 ///
 /// If the parent is `None`, the parent-child relationship is removed.
-pub fn set_parent(toplevel: &xdg_toplevel::XdgToplevel, parent: Option<wl_surface::WlSurface>) {
+pub fn set_parent(toplevel: &xdg_toplevel::XdgToplevel, parent: Option<wl_surface::WlSurface>) -> bool {
+    if let Some(parent) = &parent {
+        let data = toplevel.data::<XdgShellSurfaceUserData>().unwrap();
+        if !is_valid_parent(&data.wl_surface, parent) {
+            return false;
+        }
+    }
+
     with_surface_toplevel_role_data(toplevel, |data| {
         data.parent = parent;
     });
+
+    true
 }
 
 fn with_toplevel_pending_state<F, T>(data: &XdgShellSurfaceUserData, f: F) -> T

src/wayland/shell/xdg/mod.rs

@@ -123,7 +123,6 @@ use crate::utils::{user_data::UserDataMap, Logical, Point, Rectangle, Size};
 use crate::utils::{Serial, SERIAL_COUNTER};
 use crate::wayland::compositor;
 use crate::wayland::compositor::Cacheable;
-use crate::wayland::shell::is_toplevel_equivalent;
 use std::cmp::min;
 use std::{collections::HashSet, fmt::Debug, sync::Mutex};
 
@@ -1175,6 +1174,9 @@ pub trait XdgShellHandler {
 
     /// The toplevel surface set a different title.
     fn title_changed(&mut self, surface: ToplevelSurface) {}
+
+    /// The parent of a toplevel surface has changed.
+    fn parent_changed(&mut self, surface: ToplevelSurface) {}
 }
 
 /// Shell global state
@@ -1714,24 +1716,6 @@ impl ToplevelSurface {
     pub fn parent(&self) -> Option<wl_surface::WlSurface> {
         handlers::get_parent(&self.shell_surface)
     }
-
-    /// Sets the parent of this toplevel surface and returns whether the parent was successfully set.
-    ///
-    /// The parent must be another toplevel equivalent surface.
-    ///
-    /// If the parent is `None`, the parent-child relationship is removed.
-    pub fn set_parent(&self, parent: Option<&wl_surface::WlSurface>) -> bool {
-        if let Some(parent) = parent {
-            if !is_toplevel_equivalent(parent) {
-                return false;
-            }
-        }
-
-        // Unset the parent
-        handlers::set_parent(&self.shell_surface, None);
-
-        true
-    }
 }
 
 /// Represents the possible errors that

src/wayland/xdg_foreign/handlers.rs

@@ -6,9 +6,17 @@ use wayland_protocols::xdg::foreign::zv2::server::{
     zxdg_imported_v2::{self, ZxdgImportedV2},
     zxdg_importer_v2::{self, ZxdgImporterV2},
 };
-use wayland_server::{backend::ClientId, Client, DataInit, Dispatch, DisplayHandle, GlobalDispatch, New};
+use wayland_server::{
+    backend::ClientId, Client, DataInit, Dispatch, DisplayHandle, GlobalDispatch, New, Resource,
+};
 
-use crate::wayland::{compositor, shell::xdg::XdgToplevelSurfaceData};
+use crate::wayland::{
+    compositor,
+    shell::{
+        is_valid_parent,
+        xdg::{XdgShellHandler, XdgToplevelSurfaceData},
+    },
+};
 
 use super::{
     ExportedState, XdgExportedUserData, XdgForeignHandle, XdgForeignHandler, XdgForeignState,
@@ -64,7 +72,7 @@ where
                     handle,
                     ExportedState {
                         exported_surface: surface,
-                        requested_parent: None,
+                        requested_child: None,
                         imported_by: HashSet::new(),
                     },
                 );
@@ -75,7 +83,10 @@ where
     }
 }
 
-impl<D: XdgForeignHandler> Dispatch<ZxdgExportedV2, XdgExportedUserData, D> for XdgForeignState {
+impl<D> Dispatch<ZxdgExportedV2, XdgExportedUserData, D> for XdgForeignState
+where
+    D: XdgForeignHandler + XdgShellHandler,
+{
     fn request(
         _state: &mut D,
         _client: &Client,
@@ -90,9 +101,8 @@ impl<D: XdgForeignHandler> Dispatch<ZxdgExportedV2, XdgExportedUserData, D> for
     fn destroyed(state: &mut D, _client: ClientId, _resource: &ZxdgExportedV2, data: &XdgExportedUserData) {
         // Revoke the previously exported surface.
         // This invalidates any relationship the importer may have set up using the xdg_imported created given the handle sent via xdg_exported.handle.
-        if let Some(mut state) = state.xdg_foreign_state().exported.remove(&data.handle) {
-            invalidate_all_relationships(&mut state);
-        }
+        invalidate_all_relationships(state, &data.handle);
+        state.xdg_foreign_state().exported.remove(&data.handle);
     }
 }
 
@@ -159,7 +169,10 @@ where
     }
 }
 
-impl<D: XdgForeignHandler> Dispatch<ZxdgImportedV2, XdgImportedUserData, D> for XdgForeignState {
+impl<D> Dispatch<ZxdgImportedV2, XdgImportedUserData, D> for XdgForeignState
+where
+    D: XdgForeignHandler + XdgShellHandler,
+{
     fn request(
         state: &mut D,
         _client: &Client,
@@ -170,20 +183,50 @@ impl<D: XdgForeignHandler> Dispatch<ZxdgImportedV2, XdgImportedUserData, D> for
         _data_init: &mut DataInit<'_, D>,
     ) {
         match request {
-            zxdg_imported_v2::Request::SetParentOf { surface } => {
-                if let Some((_, state)) = state
+            zxdg_imported_v2::Request::SetParentOf { surface: child } => {
+                if let Some((_, exported_state)) = state
                     .xdg_foreign_state()
                     .exported
                     .iter_mut()
                     .find(|(key, _)| key.as_str() == data.handle.as_str())
                 {
-                    compositor::with_states(&state.exported_surface, |states| {
+                    let parent = &exported_state.exported_surface;
+
+                    let mut invalid = false;
+                    let mut changed = false;
+                    compositor::with_states(&child, |states| {
                         if let Some(data) = states.data_map.get::<XdgToplevelSurfaceData>() {
-                            data.lock().unwrap().parent = Some(surface.clone());
+                            if is_valid_parent(&child, parent) {
+                                let mut role = data.lock().unwrap();
+                                changed = role.parent.as_ref() != Some(parent);
+                                role.parent = Some(parent.clone());
+                            } else {
+                                invalid = true;
+                            }
                         }
                     });
 
-                    state.requested_parent = Some((surface, resource.clone()));
+                    if invalid {
+                        resource.post_error(
+                            zxdg_imported_v2::Error::InvalidSurface,
+                            "invalid parent relationship",
+                        );
+                        return;
+                    }
+
+                    exported_state.requested_child = Some((child.clone(), resource.clone()));
+
+                    if changed {
+                        if let Some(toplevel) = state
+                            .xdg_shell_state()
+                            .toplevel_surfaces()
+                            .iter()
+                            .find(|toplevel| *toplevel.wl_surface() == child)
+                            .cloned()
+                        {
+                            XdgShellHandler::parent_changed(state, toplevel);
+                        }
+                    }
                 }
             }
             zxdg_imported_v2::Request::Destroy => {}
@@ -192,24 +235,43 @@ impl<D: XdgForeignHandler> Dispatch<ZxdgImportedV2, XdgImportedUserData, D> for
     }
 
     fn destroyed(state: &mut D, _client: ClientId, resource: &ZxdgImportedV2, data: &XdgImportedUserData) {
-        if let Some((_, state)) = state
+        if let Some((_, exported_state)) = state
             .xdg_foreign_state()
             .exported
             .iter_mut()
             .find(|(key, _)| key.as_str() == data.handle.as_str())
         {
-            state.imported_by.remove(resource);
-            invalidate_relationship_for(state, Some(resource));
+            exported_state.imported_by.remove(resource);
         }
+
+        invalidate_relationship_for(state, &data.handle, Some(resource));
     }
 }
 
-fn invalidate_all_relationships(state: &mut ExportedState) {
-    invalidate_relationship_for(state, None);
+fn invalidate_all_relationships<D>(state: &mut D, handle: &XdgForeignHandle)
+where
+    D: XdgForeignHandler + XdgShellHandler,
+{
+    invalidate_relationship_for(state, handle, None);
 }
 
-fn invalidate_relationship_for(state: &mut ExportedState, invalidate_for: Option<&ZxdgImportedV2>) {
-    let Some((requested_parent, requested_by)) = state.requested_parent.as_ref() else {
+fn invalidate_relationship_for<D>(
+    state: &mut D,
+    handle: &XdgForeignHandle,
+    invalidate_for: Option<&ZxdgImportedV2>,
+) where
+    D: XdgForeignHandler + XdgShellHandler,
+{
+    let Some((_, exported_state)) = state
+        .xdg_foreign_state()
+        .exported
+        .iter_mut()
+        .find(|(key, _)| key.as_str() == handle.as_str())
+    else {
+        return;
+    };
+
+    let Some((requested_child, requested_by)) = exported_state.requested_child.as_ref() else {
         return;
     };
 
@@ -219,16 +281,31 @@ fn invalidate_relationship_for(state: &mut ExportedState, invalidate_for: Option
         }
     }
 
-    compositor::with_states(&state.exported_surface, |states| {
+    let mut changed = false;
+    compositor::with_states(requested_child, |states| {
         let Some(data) = states.data_map.get::<XdgToplevelSurfaceData>() else {
             return;
         };
 
         let data = &mut *data.lock().unwrap();
-        if data.parent.as_ref() == Some(requested_parent) {
+        if data.parent.as_ref() == Some(&exported_state.exported_surface) {
             data.parent = None;
+            changed = true;
         }
     });
 
-    state.requested_parent = None;
+    let requested_child = requested_child.clone();
+    exported_state.requested_child = None;
+
+    if changed {
+        if let Some(toplevel) = state
+            .xdg_shell_state()
+            .toplevel_surfaces()
+            .iter()
+            .find(|toplevel| *toplevel.wl_surface() == requested_child)
+            .cloned()
+        {
+            XdgShellHandler::parent_changed(state, toplevel);
+        }
+    }
 }

src/wayland/xdg_foreign/mod.rs

@@ -86,7 +86,7 @@ pub struct XdgImportedUserData {
 #[derive(Debug)]
 struct ExportedState {
     exported_surface: WlSurface,
-    requested_parent: Option<(WlSurface, ZxdgImportedV2)>,
+    requested_child: Option<(WlSurface, ZxdgImportedV2)>,
     imported_by: HashSet<ZxdgImportedV2>,
 }
 
@@ -131,7 +131,8 @@ impl XdgForeignState {
 
 /// Macro to delegate implementation of the xdg foreign to [`XdgForeignState`].
 ///
-/// You must also implement [`XdgForeignHandler`] to use this.
+/// You must also implement [`XdgForeignHandler`] and
+/// [`XdgShellHandler`](crate::wayland::shell::xdg::XdgShellHandler) to use this.
 #[macro_export]
 macro_rules! delegate_xdg_foreign {
     ($(@<$( $lt:tt $( : $clt:tt $(+ $dlt:tt )* )? ),+>)? $ty: ty) => {