Fix layer-shell initial commit logic

I didn't properly update it for the Smithay refactor. It was reading
initial_configure_sent too early. This worked before when niri had to reset it
manually, but it no longer works now that it is automatically reset already
before entering this function.

3 files changed, 271 insertions, 93 deletions

diff --git a/src/handlers/layer_shell.rs b/src/handlers/layer_shell.rs
index 7678ccc1..a1e66d1a 100644
--- a/src/handlers/layer_shell.rs
+++ b/src/handlers/layer_shell.rs
@@ -97,104 +97,110 @@ impl State {
             return false;
         };
 
-        if surface == &root_surface {
-            let initial_configure_sent = with_states(surface, |states| {
-                states
-                    .data_map
-                    .get::<LayerSurfaceData>()
-                    .unwrap()
-                    .lock()
-                    .unwrap()
-                    .initial_configure_sent
-            });
-
-            let mut map = layer_map_for_output(&output);
-
-            // Arrange the layers before sending the initial configure to respect any size the
-            // client may have sent.
-            map.arrange();
-
-            let layer = map
-                .layer_for_surface(surface, WindowSurfaceType::TOPLEVEL)
-                .unwrap();
-
-            if initial_configure_sent {
-                if is_mapped(surface) {
-                    let was_unmapped = self.niri.unmapped_layer_surfaces.remove(surface);
-
-                    // Resolve rules for newly mapped layer surfaces.
-                    if was_unmapped {
-                        let config = self.niri.config.borrow();
-
-                        let rules = &config.layer_rules;
-                        let rules =
-                            ResolvedLayerRules::compute(rules, layer, self.niri.is_at_startup);
-
-                        let output_size = output_size(&output);
-                        let scale = output.current_scale().fractional_scale();
-
-                        let mapped = MappedLayer::new(
-                            layer.clone(),
-                            rules,
-                            output_size,
-                            scale,
-                            self.niri.clock.clone(),
-                            &config,
-                        );
-
-                        let prev = self
-                            .niri
-                            .mapped_layer_surfaces
-                            .insert(layer.clone(), mapped);
-                        if prev.is_some() {
-                            error!("MappedLayer was present for an unmapped surface");
-                        }
-                    }
-
-                    // Give focus to newly mapped on-demand surfaces. Some launchers like
-                    // lxqt-runner rely on this behavior. While this behavior doesn't make much
-                    // sense for other clients like panels, the consensus seems to be that it's not
-                    // a big deal since panels generally only open once at the start of the
-                    // session.
-                    //
-                    // Note that:
-                    // 1) Exclusive layer surfaces already get focus automatically in
-                    //    update_keyboard_focus().
-                    // 2) Same-layer exclusive layer surfaces are already preferred to on-demand
-                    //    surfaces in update_keyboard_focus(), so we don't need to check for that
-                    //    here.
-                    //
-                    // https://github.com/YaLTeR/niri/issues/641
-                    let on_demand = layer.cached_state().keyboard_interactivity
-                        == wlr_layer::KeyboardInteractivity::OnDemand;
-                    if was_unmapped && on_demand {
-                        // I guess it'd make sense to check that no higher-layer on-demand surface
-                        // has focus, but Smithay's Layer doesn't implement Ord so this would be a
-                        // little annoying.
-                        self.niri.layer_shell_on_demand_focus = Some(layer.clone());
-                    }
-                } else {
-                    self.niri.mapped_layer_surfaces.remove(layer);
-                    self.niri.unmapped_layer_surfaces.insert(surface.clone());
-                }
-            } else {
-                let scale = output.current_scale();
-                let transform = output.current_transform();
-                with_states(surface, |data| {
-                    send_scale_transform(surface, data, scale, transform);
-                });
+        if surface != &root_surface {
+            // This is an unsync layer-shell subsurface.
+            self.niri.queue_redraw(&output);
+            return true;
+        }
+
+        let mut map = layer_map_for_output(&output);
+
+        // Arrange the layers before sending the initial configure to respect any size the
+        // client may have sent.
+        map.arrange();
+
+        let layer = map
+            .layer_for_surface(surface, WindowSurfaceType::TOPLEVEL)
+            .unwrap();
 
-                layer.layer_surface().send_configure();
+        if is_mapped(surface) {
+            let was_unmapped = self.niri.unmapped_layer_surfaces.remove(surface);
+
+            // Resolve rules for newly mapped layer surfaces.
+            if was_unmapped {
+                let config = self.niri.config.borrow();
+
+                let rules = &config.layer_rules;
+                let rules = ResolvedLayerRules::compute(rules, layer, self.niri.is_at_startup);
+
+                let output_size = output_size(&output);
+                let scale = output.current_scale().fractional_scale();
+
+                let mapped = MappedLayer::new(
+                    layer.clone(),
+                    rules,
+                    output_size,
+                    scale,
+                    self.niri.clock.clone(),
+                    &config,
+                );
+
+                let prev = self
+                    .niri
+                    .mapped_layer_surfaces
+                    .insert(layer.clone(), mapped);
+                if prev.is_some() {
+                    error!("MappedLayer was present for an unmapped surface");
+                }
             }
-            drop(map);
 
-            // This will call queue_redraw() inside.
-            self.niri.output_resized(&output);
+            // Give focus to newly mapped on-demand surfaces. Some launchers like lxqt-runner rely
+            // on this behavior. While this behavior doesn't make much sense for other clients like
+            // panels, the consensus seems to be that it's not a big deal since panels generally
+            // only open once at the start of the session.
+            //
+            // Note that:
+            // 1) Exclusive layer surfaces already get focus automatically in
+            //    update_keyboard_focus().
+            // 2) Same-layer exclusive layer surfaces are already preferred to on-demand surfaces in
+            //    update_keyboard_focus(), so we don't need to check for that here.
+            //
+            // https://github.com/YaLTeR/niri/issues/641
+            let on_demand = layer.cached_state().keyboard_interactivity
+                == wlr_layer::KeyboardInteractivity::OnDemand;
+            if was_unmapped && on_demand {
+                // I guess it'd make sense to check that no higher-layer on-demand surface
+                // has focus, but Smithay's Layer doesn't implement Ord so this would be a
+                // little annoying.
+                self.niri.layer_shell_on_demand_focus = Some(layer.clone());
+            }
         } else {
-            // This is an unsync layer-shell subsurface.
-            self.niri.queue_redraw(&output);
+            // The surface is unmapped.
+            if self.niri.mapped_layer_surfaces.remove(layer).is_some() {
+                // A mapped surface got unmapped via a null commit. Now it needs to do a new
+                // initial commit again.
+                self.niri.unmapped_layer_surfaces.insert(surface.clone());
+            } else {
+                // An unmapped surface remains unmapped. If we haven't sent an initial configure
+                // yet, we should do so.
+                let initial_configure_sent = with_states(surface, |states| {
+                    states
+                        .data_map
+                        .get::<LayerSurfaceData>()
+                        .unwrap()
+                        .lock()
+                        .unwrap()
+                        .initial_configure_sent
+                });
+                if !initial_configure_sent {
+                    let scale = output.current_scale();
+                    let transform = output.current_transform();
+                    with_states(surface, |data| {
+                        send_scale_transform(surface, data, scale, transform);
+                    });
+
+                    layer.layer_surface().send_configure();
+                }
+                // If we already sent an initial configure, then map.arange() above had just sent
+                // it a new configure, if needed.
+            }
         }
 
+        drop(map);
+
+        // This will call queue_redraw() inside.
+        self.niri.output_resized(&output);
+
         true
     }
 }
diff --git a/src/tests/client.rs b/src/tests/client.rs
index fc61c2ac..89fa15a3 100644
--- a/src/tests/client.rs
+++ b/src/tests/client.rs
@@ -438,6 +438,10 @@ impl LayerSurface {
         self.surface.attach(Some(&buffer), 0, 0);
     }
 
+    pub fn attach_null(&self) {
+        self.surface.attach(None, 0, 0);
+    }
+
     pub fn set_size(&self, w: u16, h: u16) {
         self.viewport.set_destination(i32::from(w), i32::from(h));
     }
diff --git a/src/tests/layer_shell.rs b/src/tests/layer_shell.rs
index 65014942..72caf724 100644
--- a/src/tests/layer_shell.rs
+++ b/src/tests/layer_shell.rs
@@ -1,6 +1,8 @@
 use insta::assert_snapshot;
 use smithay::reexports::wayland_protocols_wlr::layer_shell::v1::client::zwlr_layer_shell_v1::Layer;
-use smithay::reexports::wayland_protocols_wlr::layer_shell::v1::client::zwlr_layer_surface_v1::Anchor;
+use smithay::reexports::wayland_protocols_wlr::layer_shell::v1::client::zwlr_layer_surface_v1::{
+    Anchor, KeyboardInteractivity,
+};
 
 use super::*;
 use crate::tests::client::{LayerConfigureProps, LayerMargin};
@@ -88,3 +90,169 @@ fn margin_overflow() {
     let layer = f.client(id).layer(&surface);
     assert_snapshot!(layer.format_recent_configures(), @"size: 0 × 0");
 }
+
+#[test]
+fn unmap_through_null_buffer() {
+    let mut f = Fixture::new();
+    f.add_output(1, (1920, 1080));
+    let id = f.add_client();
+
+    let layer = f.client(id).create_layer(None, Layer::Top, "");
+    let surface = layer.surface.clone();
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 50)),
+        ..Default::default()
+    });
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    assert_snapshot!(layer.format_recent_configures(), @"size: 1920 × 50");
+
+    layer.attach_new_buffer();
+    layer.set_size(100, 100);
+    layer.ack_last_and_commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // No new configure since nothing changed.
+    assert_snapshot!(layer.format_recent_configures(), @"");
+
+    // Unmap by attaching a null buffer. This moves the surface back to pre-initial-commit stage.
+    layer.attach_null();
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // Configures must be empty because we haven't done an initial commit yet.
+    assert_snapshot!(layer.format_recent_configures(), @"");
+
+    // Do the initial commit again.
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 100)),
+        ..Default::default()
+    });
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // This is the new initial configure.
+    assert_snapshot!(layer.format_recent_configures(), @"size: 1920 × 100");
+
+    layer.attach_new_buffer();
+    layer.set_size(100, 100);
+    layer.ack_last_and_commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    assert_snapshot!(layer.format_recent_configures(), @"");
+}
+
+#[test]
+fn multiple_commits_before_mapping() {
+    let mut f = Fixture::new();
+    f.add_output(1, (1920, 1080));
+    let id = f.add_client();
+
+    let layer = f.client(id).create_layer(None, Layer::Top, "");
+    let surface = layer.surface.clone();
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 50)),
+        ..Default::default()
+    });
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    assert_snapshot!(layer.format_recent_configures(), @"size: 1920 × 50");
+
+    // Change something that won't cause a configure.
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 50)),
+        kb_interactivity: Some(KeyboardInteractivity::OnDemand),
+        ..Default::default()
+    });
+    layer.ack_last_and_commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // No new configure since the size hasn't changed.
+    assert_snapshot!(layer.format_recent_configures(), @"");
+
+    // Change something that will cause a configure.
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 100)),
+        ..Default::default()
+    });
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // Configure with new size.
+    assert_snapshot!(layer.format_recent_configures(), @"size: 1920 × 100");
+
+    // Map.
+    layer.attach_new_buffer();
+    layer.set_size(100, 100);
+    layer.ack_last_and_commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // No new configure since nothing changed.
+    assert_snapshot!(layer.format_recent_configures(), @"");
+
+    // Unmap by attaching a null buffer. This moves the surface back to pre-initial-commit stage.
+    layer.attach_null();
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // Configures must be empty because we haven't done an initial commit yet.
+    assert_snapshot!(layer.format_recent_configures(), @"");
+
+    // Same configure props as before, but since we unmapped, we should get a new initial
+    // configure (that will happen to match the previous configure we had got while mapped).
+    let surface = layer.surface.clone();
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 100)),
+        ..Default::default()
+    });
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    assert_snapshot!(layer.format_recent_configures(), @"size: 1920 × 100");
+
+    // Change something that won't cause a configure.
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 100)),
+        kb_interactivity: Some(KeyboardInteractivity::OnDemand),
+        ..Default::default()
+    });
+    layer.ack_last_and_commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // No new configure since the size hasn't changed.
+    assert_snapshot!(layer.format_recent_configures(), @"");
+
+    // Change something that will cause a configure.
+    layer.set_configure_props(LayerConfigureProps {
+        anchor: Some(Anchor::Left | Anchor::Right | Anchor::Top),
+        size: Some((0, 50)),
+        ..Default::default()
+    });
+    layer.commit();
+    f.double_roundtrip(id);
+
+    let layer = f.client(id).layer(&surface);
+    // Configure with new size.
+    assert_snapshot!(layer.format_recent_configures(), @"size: 1920 × 50");
+}