wayland.compositor: rework the subsurface tree

Rework the subsurface tree by:

- forbidding subsurface loops
- storing the relative depth of a parent to its children,
  finally respecting the wl_subsurface specification.

closes #23

anvil/src/glium_drawer.rs

@@ -294,9 +294,11 @@ impl<F: GLGraphicsBackend + 'static> GliumDrawer<F> {
         compositor_token: MyCompositorToken,
         screen_dimensions: (u32, u32),
     ) {
-        compositor_token
+        compositor_token.with_surface_tree_upward(
-            .with_surface_tree_upward(root, location, |_surface, attributes, role, &(mut x, mut y)| {
+            root,
-                // there is actually something to draw !
+            location,
+            |_surface, attributes, role, &(mut x, mut y)| {
+                // Pull a new buffer if available
                 if attributes.user_data.texture.is_none() {
                     if let Some(buffer) = attributes.user_data.buffer.take() {
                         if let Ok(m) = self.texture_from_buffer(buffer.clone()) {
@@ -307,7 +309,23 @@ impl<F: GLGraphicsBackend + 'static> GliumDrawer<F> {
                         buffer.release();
                     }
                 }
+                // Now, should we be drawn ?
+                if attributes.user_data.texture.is_some() {
+                    // if yes, also process the children
+                    if let Ok(subdata) = Role::<SubsurfaceRole>::data(role) {
+                        x += subdata.location.0;
+                        y += subdata.location.1;
+                    }
+                    TraversalAction::DoChildren((x, y))
+                } else {
+                    // we are not display, so our children are neither
+                    TraversalAction::SkipChildren
+                }
+            },
+            |_surface, attributes, role, &(mut x, mut y)| {
                 if let Some(ref metadata) = attributes.user_data.texture {
+                    // we need to re-extract the subsurface offset, as the previous closure
+                    // only passes it to our children
                     if let Ok(subdata) = Role::<SubsurfaceRole>::data(role) {
                         x += subdata.location.0;
                         y += subdata.location.1;
@@ -332,13 +350,10 @@ impl<F: GLGraphicsBackend + 'static> GliumDrawer<F> {
                             ..Default::default()
                         },
                     );
-                    TraversalAction::DoChildren((x, y))
-                } else {
-                    // we are not display, so our children are neither
-                    TraversalAction::SkipChildren
                 }
-            })
+            },
-            .unwrap();
+            |_, _, _, _| true,
+        );
     }
 
     pub fn draw_windows(

anvil/src/window_map.rs

@@ -1,3 +1,5 @@
+use std::cell::RefCell;
+
 use smithay::{
     reexports::wayland_server::protocol::wl_surface,
     utils::Rectangle,
@@ -63,7 +65,7 @@ where
             return None;
         }
         // need to check more carefully
-        let mut found = None;
+        let found = RefCell::new(None);
         if let Some(wl_surface) = self.toplevel.get_surface() {
             let _ = ctoken.with_surface_tree_downward(
                 wl_surface,
@@ -81,18 +83,22 @@ where
                             height: h,
                         };
                         if my_rect.contains((point.0 as i32, point.1 as i32)) {
-                            found = Some((wl_surface.clone(), (my_rect.x as f64, my_rect.y as f64)));
+                            *found.borrow_mut() =
-                            TraversalAction::Break
+                                Some((wl_surface.clone(), (my_rect.x as f64, my_rect.y as f64)));
-                        } else {
-                            TraversalAction::DoChildren((x, y))
                         }
+                        TraversalAction::DoChildren((x, y))
                     } else {
                         TraversalAction::SkipChildren
                     }
                 },
+                |_, _, _, _| {},
+                |_, _, _, _| {
+                    // only continue if the point is not found
+                    found.borrow().is_none()
+                },
             );
         }
-        found
+        found.into_inner()
     }
 
     fn self_update<F>(&mut self, ctoken: CompositorToken<U, R>, get_size: F)
@@ -129,6 +135,8 @@ where
                         TraversalAction::SkipChildren
                     }
                 },
+                |_, _, _, _| {},
+                |_, _, _, _| true,
             );
         }
         self.surface = Rectangle {

src/backend/egl/mod.rs

@@ -300,6 +300,7 @@ impl Drop for EGLImages {
                 }
             }
         }
+        println!("RELEASING EGL BUFFER");
         self.buffer.release();
     }
 }

src/wayland/compositor/handlers.rs

@@ -139,6 +139,7 @@ where
         Some(|surface| SurfaceData::<U, R>::cleanup(&surface)),
         SurfaceData::<U, R>::new(),
     );
+    SurfaceData::<U, R>::init(&surface);
     surface
 }
 

src/wayland/compositor/mod.rs

@@ -287,11 +287,21 @@ where
 {
     /// Access the data of a surface tree from bottom to top
     ///
-    /// The provided closure is called successively on the surface and all its child subsurfaces,
+    /// You provide three closures, a "filter", a "processor" and a "post filter".
-    /// in a depth-first order. This matches the order in which the surfaces are supposed to be
-    /// drawn: top-most last.
     ///
-    /// The arguments provided to the closure are, in this order:
+    /// The first closure is initially called on a surface to determine if its children
+    /// should be processed as well. It returns a `TraversalAction<T>` reflecting that.
+    ///
+    /// The second closure is supposed to do the actual processing. The processing closure for
+    /// a surface may be called after the processing closure of some of its children, depending
+    /// on the stack ordering the client requested. Here the surfaces are processed in the same
+    /// order as they are supposed to be drawn: from the farthest of the screen to the nearest.
+    ///
+    /// The third closure is called once all the subtree of a node has been processed, and gives
+    /// an opportunity for early-stopping. If it returns `true` the processing will continue,
+    /// while if it returns `false` it'll stop.
+    ///
+    /// The arguments provided to the closures are, in this order:
     ///
     /// - The surface object itself
     /// - a mutable reference to its surface attribute data
@@ -301,27 +311,40 @@ where
     ///
     /// If the surface not managed by the `CompositorGlobal` that provided this token, this
     /// will panic (having more than one compositor is not supported).
-    pub fn with_surface_tree_upward<F, T>(&self, surface: &WlSurface, initial: T, f: F) -> Result<(), ()>
+    pub fn with_surface_tree_upward<F1, F2, F3, T>(
-    where
+        &self,
-        F: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
+        surface: &WlSurface,
+        initial: T,
+        filter: F1,
+        processor: F2,
+        post_filter: F3,
+    ) where
+        F1: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
+        F2: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T),
+        F3: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> bool,
     {
-        SurfaceData::<U, R>::map_tree(surface, initial, f, false);
+        SurfaceData::<U, R>::map_tree(surface, &initial, filter, processor, post_filter, false);
-        Ok(())
     }
 
     /// Access the data of a surface tree from top to bottom
     ///
-    /// The provided closure is called successively on the surface and all its child subsurfaces,
+    /// Behavior is the same as [`with_surface_tree_upward`](CompositorToken::with_surface_tree_upward), but
-    /// in a depth-first order. This matches the reverse of the order in which the surfaces are
+    /// the processing is done in the reverse order, from the nearest of the screen to the deepest.
-    /// supposed to be drawn: top-most first.
     ///
-    /// Behavior is the same as [`with_surface_tree_upward`](CompositorToken::with_surface_tree_upward).
+    /// This would typically be used to find out which surface of a subsurface tree has been clicked for example.
-    pub fn with_surface_tree_downward<F, T>(&self, surface: &WlSurface, initial: T, f: F) -> Result<(), ()>
+    pub fn with_surface_tree_downward<F1, F2, F3, T>(
-    where
+        &self,
-        F: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
+        surface: &WlSurface,
+        initial: T,
+        filter: F1,
+        processor: F2,
+        post_filter: F3,
+    ) where
+        F1: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
+        F2: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T),
+        F3: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> bool,
     {
-        SurfaceData::<U, R>::map_tree(surface, initial, f, true);
+        SurfaceData::<U, R>::map_tree(surface, &initial, filter, processor, post_filter, true);
-        Ok(())
     }
 
     /// Retrieve the parent of this surface

src/wayland/compositor/tree.rs

@@ -13,11 +13,8 @@ use wayland_server::protocol::wl_surface::WlSurface;
 /// fact that lifetime of objects are decided by Wayland-server to ensure
 /// the cleanup will be done properly, and we won't leak anything.
 ///
-/// This implementation is not strictly a tree, but rather a directed graph
+/// Each node also appears within its children list, to allow relative placement
-/// with the constraint that node can have at most one incoming edge. Aka like
+/// between them.
-/// a tree, but with loops allowed. This is because the Wayland protocol does not
-/// have a failure case to forbid this. Note that if any node in such a graph does not
-/// have a parent, then the graph is a tree and this node is its root.
 pub struct SurfaceData<U, R> {
     parent: Option<WlSurface>,
     children: Vec<WlSurface>,
@@ -44,7 +41,7 @@ impl<U: Default, R: Default> SurfaceData<U, R> {
     pub fn new() -> Mutex<SurfaceData<U, R>> {
         Mutex::new(SurfaceData {
             parent: None,
-            children: Vec::new(),
+            children: vec![],
             role: Default::default(),
             attributes: Default::default(),
         })
@@ -56,22 +53,28 @@ where
     U: 'static,
     R: 'static,
 {
+    /// Initializes the surface, must be called at creation for state coherence
+    pub fn init(surface: &WlSurface) {
+        let my_data_mutex = surface.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
+        let mut my_data = my_data_mutex.lock().unwrap();
+        debug_assert!(my_data.children.len() == 0);
+        my_data.children.push(surface.clone());
+    }
+
     /// Cleans the `as_ref().user_data` of that surface, must be called when it is destroyed
     pub fn cleanup(surface: &WlSurface) {
         let my_data_mutex = surface.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
         let mut my_data = my_data_mutex.lock().unwrap();
         if let Some(old_parent) = my_data.parent.take() {
-            if !old_parent.as_ref().equals(surface.as_ref()) {
+            // We had a parent, lets unregister ourselves from it
-                // We had a parent that is not ourselves, lets unregister ourselves from it
+            let old_parent_mutex = old_parent
-                let old_parent_mutex = old_parent
+                .as_ref()
-                    .as_ref()
+                .user_data::<Mutex<SurfaceData<U, R>>>()
-                    .user_data::<Mutex<SurfaceData<U, R>>>()
+                .unwrap();
-                    .unwrap();
+            let mut old_parent_guard = old_parent_mutex.lock().unwrap();
-                let mut old_parent_guard = old_parent_mutex.lock().unwrap();
+            old_parent_guard
-                old_parent_guard
+                .children
-                    .children
+                .retain(|c| !c.as_ref().equals(surface.as_ref()));
-                    .retain(|c| !c.as_ref().equals(surface.as_ref()));
-            }
         }
         // orphan all our children
         for child in &my_data.children {
@@ -159,6 +162,21 @@ impl<U: 'static, R: RoleType + 'static> SurfaceData<U, R> {
 }
 
 impl<U: 'static, R: RoleType + Role<SubsurfaceRole> + 'static> SurfaceData<U, R> {
+    /// Checks if the first surface is an ancestor of the second
+    pub fn is_ancestor(a: &WlSurface, b: &WlSurface) -> bool {
+        let b_mutex = b.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
+        let b_guard = b_mutex.lock().unwrap();
+        if let Some(ref parent) = b_guard.parent {
+            if parent.as_ref().equals(a.as_ref()) {
+                return true;
+            } else {
+                return Self::is_ancestor(a, parent);
+            }
+        } else {
+            return false;
+        }
+    }
+
     /// Sets the parent of a surface
     ///
     /// if this surface already has a role, does nothing and fails, otherwise
@@ -166,6 +184,10 @@ impl<U: 'static, R: RoleType + Role<SubsurfaceRole> + 'static> SurfaceData<U, R>
     pub fn set_parent(child: &WlSurface, parent: &WlSurface) -> Result<(), ()> {
         debug_assert!(child.as_ref().is_alive());
         debug_assert!(parent.as_ref().is_alive());
+        // ensure the child is not already a parent of the parent
+        if Self::is_ancestor(child, parent) {
+            return Err(());
+        }
 
         // change child's parent
         {
@@ -177,7 +199,6 @@ impl<U: 'static, R: RoleType + Role<SubsurfaceRole> + 'static> SurfaceData<U, R>
             child_guard.parent = Some(parent.clone());
         }
         // register child to new parent
-        // double scoping is to be robust to have a child be its own parent
         {
             let parent_mutex = parent.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
             let mut parent_guard = parent_mutex.lock().unwrap();
@@ -222,11 +243,16 @@ impl<U: 'static, R: RoleType + Role<SubsurfaceRole> + 'static> SurfaceData<U, R>
         child_guard.parent.as_ref().cloned()
     }
 
-    /// Retrieve the parent surface (if any) of this surface
+    /// Retrieve the children surface (if any) of this surface
-    pub fn get_children(child: &WlSurface) -> Vec<WlSurface> {
+    pub fn get_children(parent: &WlSurface) -> Vec<WlSurface> {
-        let child_mutex = child.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
+        let parent_mutex = parent.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
-        let child_guard = child_mutex.lock().unwrap();
+        let parent_guard = parent_mutex.lock().unwrap();
-        child_guard.children.to_vec()
+        parent_guard
+            .children
+            .iter()
+            .filter(|s| !s.as_ref().equals(parent.as_ref()))
+            .cloned()
+            .collect()
     }
 
     /// Reorders a surface relative to one of its sibling
@@ -238,10 +264,6 @@ impl<U: 'static, R: RoleType + Role<SubsurfaceRole> + 'static> SurfaceData<U, R>
             let data_guard = data_mutex.lock().unwrap();
             data_guard.parent.as_ref().cloned().unwrap()
         };
-        if parent.as_ref().equals(relative_to.as_ref()) {
-            // TODO: handle positioning relative to parent
-            return Ok(());
-        }
 
         fn index_of(surface: &WlSurface, slice: &[WlSurface]) -> Option<usize> {
             for (i, s) in slice.iter().enumerate() {
@@ -255,7 +277,7 @@ impl<U: 'static, R: RoleType + Role<SubsurfaceRole> + 'static> SurfaceData<U, R>
         let parent_mutex = parent.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
         let mut parent_guard = parent_mutex.lock().unwrap();
         let my_index = index_of(surface, &parent_guard.children).unwrap();
-        let mut other_index = match index_of(surface, &parent_guard.children) {
+        let mut other_index = match index_of(relative_to, &parent_guard.children) {
             Some(idx) => idx,
             None => return Err(()),
         };
@@ -295,76 +317,81 @@ impl<U: 'static, R: 'static> SurfaceData<U, R> {
     /// Note that an internal lock is taken during access of this data,
     /// so the tree cannot be manipulated at the same time.
     ///
-    /// The callback returns whether the traversal should continue or not. Returning
+    /// The first callback determines if this node children should be processed or not.
-    /// false will cause an early-stopping.
+    ///
-    pub fn map_tree<F, T>(root: &WlSurface, initial: T, mut f: F, reverse: bool)
+    /// The second actually does the processing, being called on children in display depth
-    where
+    /// order.
-        F: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
+    ///
+    /// The third is called once all the children of a node has been processed (including itself), only if the first
+    /// returned `DoChildren`, and gives an opportunity to early stop
+    pub fn map_tree<F1, F2, F3, T>(
+        surface: &WlSurface,
+        initial: &T,
+        mut filter: F1,
+        mut processor: F2,
+        mut post_filter: F3,
+        reverse: bool,
+    ) where
+        F1: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
+        F2: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T),
+        F3: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> bool,
     {
-        // helper function for recursion
+        Self::map(
-        fn map<U: 'static, R: 'static, F, T>(
+            surface,
-            surface: &WlSurface,
+            initial,
-            root: &WlSurface,
+            &mut filter,
-            initial: &T,
+            &mut processor,
-            f: &mut F,
+            &mut post_filter,
-            reverse: bool,
+            reverse,
-        ) -> bool
+        );
-        where
+    }
-            F: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
-        {
-            // stop if we met the root, so to not deadlock/inifinte loop
-            if surface.as_ref().equals(root.as_ref()) {
-                return true;
-            }
 
-            let data_mutex = surface.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
+    // helper function for map_tree
-            let mut data_guard = data_mutex.lock().unwrap();
+    fn map<F1, F2, F3, T>(
-            let data_guard = &mut *data_guard;
+        surface: &WlSurface,
-            // call the callback on ourselves
+        initial: &T,
-            match f(surface, &mut data_guard.attributes, &mut data_guard.role, initial) {
+        filter: &mut F1,
-                TraversalAction::DoChildren(t) => {
+        processor: &mut F2,
-                    // loop over children
+        post_filter: &mut F3,
-                    if reverse {
+        reverse: bool,
-                        for c in data_guard.children.iter().rev() {
+    ) -> bool
-                            if !map::<U, R, _, _>(c, root, &t, f, true) {
+    where
-                                return false;
+        F1: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> TraversalAction<T>,
-                            }
+        F2: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T),
-                        }
+        F3: FnMut(&WlSurface, &mut SurfaceAttributes<U>, &mut R, &T) -> bool,
-                    } else {
+    {
-                        for c in &data_guard.children {
+        let data_mutex = surface.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
-                            if !map::<U, R, _, _>(c, root, &t, f, false) {
-                                return false;
-                            }
-                        }
-                    }
-                    true
-                }
-                TraversalAction::SkipChildren => true,
-                TraversalAction::Break => false,
-            }
-        }
-
-        let data_mutex = root.as_ref().user_data::<Mutex<SurfaceData<U, R>>>().unwrap();
         let mut data_guard = data_mutex.lock().unwrap();
         let data_guard = &mut *data_guard;
-        // call the callback on ourselves
+        // call the filter on ourselves
-        if let TraversalAction::DoChildren(t) =
+        match filter(surface, &mut data_guard.attributes, &mut data_guard.role, initial) {
-            f(root, &mut data_guard.attributes, &mut data_guard.role, &initial)
+            TraversalAction::DoChildren(t) => {
-        {
+                // loop over children
-            // loop over children
+                if reverse {
-            if reverse {
+                    for c in data_guard.children.iter().rev() {
-                for c in data_guard.children.iter().rev() {
+                        if c.as_ref().equals(surface.as_ref()) {
-                    if !map::<U, R, _, _>(c, root, &t, &mut f, true) {
+                            processor(surface, &mut data_guard.attributes, &mut data_guard.role, initial);
-                        break;
+                        } else if !Self::map(c, &t, filter, processor, post_filter, true) {
-                    }
+                            return false;
-                }
+                        }
-            } else {
+                    }
-                for c in &data_guard.children {
+                } else {
-                    if !map::<U, R, _, _>(c, root, &t, &mut f, false) {
+                    for c in &data_guard.children {
-                        break;
+                        if c.as_ref().equals(surface.as_ref()) {
+                            processor(surface, &mut data_guard.attributes, &mut data_guard.role, initial);
+                        } else if !Self::map(c, &t, filter, processor, post_filter, false) {
+                            return false;
+                        }
                     }
                 }
+                post_filter(surface, &mut data_guard.attributes, &mut data_guard.role, initial)
             }
+            TraversalAction::SkipChildren => {
+                // still process ourselves
+                processor(surface, &mut data_guard.attributes, &mut data_guard.role, initial);
+                true
+            }
+            TraversalAction::Break => false,
         }
     }
 }