surface: commit subsurface state during parent commit

src/ifs/wl_surface.rs

@@ -287,8 +287,8 @@ trait SurfaceExt {
         Ok(())
     }
 
-    fn after_apply_commit(self: Rc<Self>, pending: &mut PendingState) {
+    fn after_apply_commit(self: Rc<Self>) {
-        let _ = pending;
+        // nothing
     }
 
     fn is_some(&self) -> bool {
@@ -336,7 +336,7 @@ trait SurfaceExt {
         surface: &WlSurface,
         child: SubsurfaceId,
         consume: &mut dyn FnMut(
-            OccupiedEntry<SubsurfaceId, CommittedSubsurface>,
+            OccupiedEntry<SubsurfaceId, AttachedSubsurfaceState>,
         ) -> Result<(), WlSurfaceError>,
     ) -> Result<(), WlSurfaceError> {
         surface.pending.borrow_mut().consume_child(child, consume)
@@ -367,18 +367,17 @@ struct PendingState {
     xwayland_serial: Option<u64>,
     tearing: Option<bool>,
     content_type: Option<Option<ContentType>>,
-    subsurface: Option<Box<PendingSubsurfaceData>>,
     xdg_surface: Option<Box<PendingXdgSurfaceData>>,
     layer_surface: Option<Box<PendingLayerSurfaceData>>,
-    subsurfaces: AHashMap<SubsurfaceId, CommittedSubsurface>,
+    subsurfaces: AHashMap<SubsurfaceId, AttachedSubsurfaceState>,
     acquire_point: Option<(Rc<SyncObj>, SyncObjPoint)>,
     release_point: Option<(Rc<SyncObj>, SyncObjPoint)>,
     explicit_sync: bool,
 }
 
-struct CommittedSubsurface {
+struct AttachedSubsurfaceState {
     subsurface: Rc<WlSubsurface>,
-    state: Box<PendingState>,
+    pending: PendingSubsurfaceData,
 }
 
 impl PendingState {
@@ -445,13 +444,12 @@ impl PendingState {
                 }
             };
         }
-        merge_ext!(subsurface);
         merge_ext!(xdg_surface);
         merge_ext!(layer_surface);
         for (id, mut state) in next.subsurfaces.drain() {
             match self.subsurfaces.entry(id) {
                 Entry::Occupied(mut o) => {
-                    o.get_mut().state.merge(&mut state.state, client);
+                    o.get_mut().pending.merge(&mut state.pending, client);
                 }
                 Entry::Vacant(v) => {
                     v.insert(state);
@@ -464,7 +462,7 @@ impl PendingState {
         &mut self,
         child: SubsurfaceId,
         consume: impl FnOnce(
-            OccupiedEntry<SubsurfaceId, CommittedSubsurface>,
+            OccupiedEntry<SubsurfaceId, AttachedSubsurfaceState>,
         ) -> Result<(), WlSurfaceError>,
     ) -> Result<(), WlSurfaceError> {
         match self.subsurfaces.entry(child) {
@@ -851,11 +849,8 @@ impl WlSurface {
     }
 
     fn apply_state(self: &Rc<Self>, pending: &mut PendingState) -> Result<(), WlSurfaceError> {
-        for (_, mut subsurface) in pending.subsurfaces.drain() {
+        for (_, pending) in &mut pending.subsurfaces {
-            subsurface
+            pending.subsurface.apply_state(&mut pending.pending)?;
-                .subsurface
-                .surface
-                .apply_state(&mut subsurface.state)?;
         }
         if self.destroyed.get() {
             return Ok(());
@@ -1056,7 +1051,7 @@ impl WlSurface {
                 cursor.update_hardware_cursor();
             }
         }
-        self.ext.get().after_apply_commit(pending);
+        self.ext.get().after_apply_commit();
         self.client.state.damage();
         Ok(())
     }
@@ -1274,7 +1269,7 @@ impl WlSurface {
         &self,
         child: SubsurfaceId,
         mut consume: impl FnMut(
-            OccupiedEntry<SubsurfaceId, CommittedSubsurface>,
+            OccupiedEntry<SubsurfaceId, AttachedSubsurfaceState>,
         ) -> Result<(), WlSurfaceError>,
     ) -> Result<(), WlSurfaceError> {
         self.ext

src/ifs/wl_surface/commit_timeline.rs

@@ -269,7 +269,9 @@ fn consume_acquire_points(pending: &mut PendingState, points: &mut SmallVec<[Poi
         points.push(point);
     }
     for ss in pending.subsurfaces.values_mut() {
-        consume_acquire_points(&mut ss.state, points);
+        if let Some(state) = &mut ss.pending.state {
+            consume_acquire_points(state, points);
+        }
     }
 }
 
@@ -290,6 +292,8 @@ fn set_effective_timeline(
         }
     }
     for ss in pending.subsurfaces.values() {
-        set_effective_timeline(&ss.subsurface.surface.commit_timeline, &ss.state, effective);
+        if let Some(state) = &ss.pending.state {
+            set_effective_timeline(&ss.subsurface.surface.commit_timeline, state, effective);
+        }
     }
 }

src/ifs/wl_surface/wl_subsurface.rs

@@ -1,9 +1,9 @@
 use {
     crate::{
-        client::ClientError,
+        client::{Client, ClientError},
         ifs::wl_surface::{
-            CommitAction, CommittedSubsurface, PendingState, StackElement, SurfaceExt, SurfaceRole,
+            AttachedSubsurfaceState, CommitAction, PendingState, StackElement, SurfaceExt,
-            WlSurface, WlSurfaceError, WlSurfaceId,
+            SurfaceRole, WlSurface, WlSurfaceError, WlSurfaceId,
         },
         leaks::Tracker,
         object::Object,
@@ -13,13 +13,12 @@ use {
             clonecell::CloneCell,
             linkedlist::{LinkedNode, NodeRef},
             numcell::NumCell,
-            option_ext::OptionExt,
         },
         wire::{wl_subsurface::*, WlSubsurfaceId},
     },
     std::{
         cell::{Cell, RefCell, RefMut},
-        collections::hash_map::{Entry, OccupiedEntry},
+        collections::hash_map::OccupiedEntry,
         mem,
         ops::Deref,
         rc::Rc,
@@ -51,12 +50,20 @@ pub struct WlSubsurface {
 
 #[derive(Default)]
 pub struct PendingSubsurfaceData {
+    pub(super) state: Option<Box<PendingState>>,
     node: Option<LinkedNode<StackElement>>,
     position: Option<(i32, i32)>,
 }
 
 impl PendingSubsurfaceData {
-    pub fn merge(&mut self, next: &mut Self) {
+    pub fn merge(&mut self, next: &mut Self, client: &Rc<Client>) {
+        if let Some(mut new) = next.state.take() {
+            match &mut self.state {
+                Some(old) => old.merge(&mut new, client),
+                _ => self.state = Some(new),
+            }
+        }
+
         macro_rules! opt {
             ($name:ident) => {
                 if let Some(n) = next.$name.take() {
@@ -107,12 +114,35 @@ impl WlSubsurface {
         }
     }
 
-    fn pending(&self) -> RefMut<Box<PendingSubsurfaceData>> {
+    fn pending<'a>(self: &'a Rc<Self>) -> RefMut<'a, PendingSubsurfaceData> {
-        RefMut::map(self.surface.pending.borrow_mut(), |m| {
+        RefMut::map(self.parent.pending.borrow_mut(), |m| {
-            m.subsurface.get_or_insert_default_ext()
+            &mut m
+                .subsurfaces
+                .entry(self.unique_id)
+                .or_insert_with(|| AttachedSubsurfaceState {
+                    subsurface: self.clone(),
+                    pending: Default::default(),
+                })
+                .pending
         })
     }
 
+    pub fn apply_state(&self, pending: &mut PendingSubsurfaceData) -> Result<(), WlSurfaceError> {
+        if let Some(state) = &mut pending.state.take() {
+            self.surface.apply_state(state)?;
+        }
+        if let Some(v) = pending.node.take() {
+            v.pending.set(false);
+            self.node.borrow_mut().replace(v);
+        }
+        if let Some((x, y)) = pending.position.take() {
+            self.position
+                .set(self.surface.buffer_abs_pos.get().at_point(x, y));
+            self.parent.need_extents_update.set(true);
+        }
+        Ok(())
+    }
+
     pub fn install(self: &Rc<Self>) -> Result<(), WlSubsurfaceError> {
         if self.surface.id == self.parent.id {
             return Err(WlSubsurfaceError::OwnParent(self.surface.id));
@@ -158,9 +188,12 @@ impl WlSubsurface {
         let _req: Destroy = self.surface.client.parse(self, parser)?;
         self.surface.unset_ext();
         self.parent.consume_pending_child(self.unique_id, |oe| {
-            self.surface.apply_state(&mut oe.remove().state)
+            let oe = oe.remove();
+            if let Some(mut state) = oe.pending.state {
+                self.surface.apply_state(&mut state)?;
+            }
+            Ok(())
         })?;
-        self.surface.pending.borrow_mut().subsurface.take();
         *self.node.borrow_mut() = None;
         self.latest_node.take();
         {
@@ -184,8 +217,8 @@ impl WlSubsurface {
         Ok(())
     }
 
-    fn set_position(&self, parser: MsgParser<'_, '_>) -> Result<(), WlSubsurfaceError> {
+    fn set_position(self: &Rc<Self>, parser: MsgParser<'_, '_>) -> Result<(), WlSubsurfaceError> {
-        let req: SetPosition = self.surface.client.parse(self, parser)?;
+        let req: SetPosition = self.surface.client.parse(&**self, parser)?;
         self.pending().position = Some((req.x, req.y));
         Ok(())
     }
@@ -270,14 +303,12 @@ impl WlSubsurface {
     }
 
     fn on_desync(&self) -> Result<(), WlSurfaceError> {
-        let committed = self
+        let committed = &mut *self.parent.pending.borrow_mut();
-            .parent
+        let committed = committed.subsurfaces.get_mut(&self.unique_id);
-            .pending
+        if let Some(ps) = committed {
-            .borrow_mut()
+            if let Some(mut state) = ps.pending.state.take() {
-            .subsurfaces
+                self.surface.apply_state(&mut state)?;
-            .remove(&self.unique_id);
+            }
-        if let Some(mut ps) = committed {
-            self.surface.apply_state(&mut ps.state)?;
         }
         Ok(())
     }
@@ -318,35 +349,17 @@ simple_add_obj!(WlSubsurface);
 impl SurfaceExt for WlSubsurface {
     fn commit_requested(self: Rc<Self>, pending: &mut Box<PendingState>) -> CommitAction {
         if self.sync() {
-            let mut parent_pending = self.parent.pending.borrow_mut();
+            let mut parent_pending = self.pending();
-            match parent_pending.subsurfaces.entry(self.unique_id) {
+            match &mut parent_pending.state {
-                Entry::Occupied(mut o) => {
+                None => parent_pending.state = Some(mem::take(&mut *pending)),
-                    o.get_mut().state.merge(pending, &self.surface.client);
+                Some(state) => state.merge(pending, &self.surface.client),
-                }
-                Entry::Vacant(v) => {
-                    v.insert(CommittedSubsurface {
-                        subsurface: self.clone(),
-                        state: mem::take(&mut *pending),
-                    });
-                }
             }
             return CommitAction::AbortCommit;
         }
         CommitAction::ContinueCommit
     }
 
-    fn after_apply_commit(self: Rc<Self>, pending: &mut PendingState) {
+    fn after_apply_commit(self: Rc<Self>) {
-        if let Some(pending) = &mut pending.subsurface {
-            if let Some(v) = pending.node.take() {
-                v.pending.set(false);
-                self.node.borrow_mut().replace(v);
-            }
-            if let Some((x, y)) = pending.position.take() {
-                self.position
-                    .set(self.surface.buffer_abs_pos.get().at_point(x, y));
-                self.parent.need_extents_update.set(true);
-            }
-        }
         let has_buffer = self.surface.buffer.is_some();
         if self.had_buffer.replace(has_buffer) != has_buffer {
             if has_buffer {
@@ -376,12 +389,16 @@ impl SurfaceExt for WlSubsurface {
         surface: &WlSurface,
         child: SubsurfaceId,
         consume: &mut dyn FnMut(
-            OccupiedEntry<SubsurfaceId, CommittedSubsurface>,
+            OccupiedEntry<SubsurfaceId, AttachedSubsurfaceState>,
         ) -> Result<(), WlSurfaceError>,
     ) -> Result<(), WlSurfaceError> {
         self.parent
             .consume_pending_child(self.unique_id, |mut oe| {
-                oe.get_mut().state.consume_child(child, &mut *consume)
+                let oe = oe.get_mut();
+                match &mut oe.pending.state {
+                    Some(state) => state.consume_child(child, &mut *consume),
+                    _ => Ok(()),
+                }
             })?;
         surface.pending.borrow_mut().consume_child(child, consume)
     }

src/ifs/wl_surface/x_surface.rs

@@ -2,7 +2,7 @@ use {
     crate::{
         ifs::wl_surface::{
             x_surface::{xwayland_surface_v1::XwaylandSurfaceV1, xwindow::Xwindow},
-            PendingState, SurfaceExt, WlSurface, WlSurfaceError,
+            SurfaceExt, WlSurface, WlSurfaceError,
         },
         leaks::Tracker,
         tree::ToplevelNode,
@@ -23,7 +23,7 @@ pub struct XSurface {
 }
 
 impl SurfaceExt for XSurface {
-    fn after_apply_commit(self: Rc<Self>, _pending: &mut PendingState) {
+    fn after_apply_commit(self: Rc<Self>) {
         if let Some(xwindow) = self.xwindow.get() {
             xwindow.map_status_changed();
         }

src/ifs/wl_surface/xdg_surface.rs

@@ -404,7 +404,7 @@ impl SurfaceExt for XdgSurface {
         Ok(())
     }
 
-    fn after_apply_commit(self: Rc<Self>, _pending: &mut PendingState) {
+    fn after_apply_commit(self: Rc<Self>) {
         if let Some(ext) = self.ext.get() {
             ext.post_commit();
         }

src/ifs/wl_surface/zwlr_layer_surface_v1.rs

@@ -349,7 +349,7 @@ impl SurfaceExt for ZwlrLayerSurfaceV1 {
         Ok(())
     }
 
-    fn after_apply_commit(self: Rc<Self>, _pending: &mut PendingState) {
+    fn after_apply_commit(self: Rc<Self>) {
         let buffer_is_some = self.surface.buffer.is_some();
         let was_mapped = self.mapped.get();
         if self.mapped.get() {

src/it/tests.rs

@@ -69,6 +69,7 @@ mod t0034_workspace_restoration;
 mod t0035_scanout_feedback;
 mod t0036_idle;
 mod t0037_toplevel_drag;
+mod t0038_subsurface_parent_state;
 
 pub trait TestCase: Sync {
     fn name(&self) -> &'static str;
@@ -125,5 +126,6 @@ pub fn tests() -> Vec<&'static dyn TestCase> {
         t0035_scanout_feedback,
         t0036_idle,
         t0037_toplevel_drag,
+        t0038_subsurface_parent_state,
     }
 }

src/it/tests/t0038_subsurface_parent_state.rs

@@ -0,0 +1,38 @@
+use {
+    crate::{
+        it::{test_error::TestResult, testrun::TestRun},
+        theme::Color,
+    },
+    std::rc::Rc,
+};
+
+testcase!();
+
+async fn test(run: Rc<TestRun>) -> TestResult {
+    let _ds = run.create_default_setup().await?;
+
+    let client = run.create_client().await?;
+    let win = client.create_window().await?;
+    win.set_color(255, 255, 255, 255);
+    win.map2().await?;
+
+    let ss = client.comp.create_surface().await?;
+    let vp = client.viewporter.get_viewport(&ss)?;
+    vp.set_destination(100, 100)?;
+    let buf = client.spbm.create_buffer(Color::SOLID_BLACK)?;
+    ss.attach(buf.id)?;
+    ss.commit()?;
+
+    let ss = client.sub.get_subsurface(ss.id, win.surface.id).await?;
+    ss.set_position(0, 0)?;
+    win.surface.commit()?;
+
+    client.compare_screenshot("1", false).await?;
+
+    ss.set_position(100, 100)?;
+    win.surface.commit()?;
+
+    client.compare_screenshot("2", false).await?;
+
+    Ok(())
+}