From d17d750091935eea43222cd4a7d85c36d0ee4d40 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Fri, 5 Sep 2025 19:37:35 +0300 Subject: [PATCH 01/14] anvil: Send configure unconditionally in unfullscreen & unmaximize Mandated by the protocol, same as fullscreen/maximize. --- anvil/src/shell/xdg.rs | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/anvil/src/shell/xdg.rs b/anvil/src/shell/xdg.rs index 3b53483b2fe0..df2422b6f7bf 100644 --- a/anvil/src/shell/xdg.rs +++ b/anvil/src/shell/xdg.rs @@ -319,14 +319,6 @@ impl XdgShellHandler for AnvilState { } fn unfullscreen_request(&mut self, surface: ToplevelSurface) { - if !surface - .current_state() - .states - .contains(xdg_toplevel::State::Fullscreen) - { - return; - } - let ret = surface.with_pending_state(|state| { state.states.unset(xdg_toplevel::State::Fullscreen); state.size = None; @@ -341,7 +333,13 @@ impl XdgShellHandler for AnvilState { } } - surface.send_pending_configure(); + // The protocol demands us to always reply with a configure, + // regardless of we fulfilled the request or not + if surface.is_initial_configure_sent() { + surface.send_configure(); + } else { + // Will be sent during initial configure + } } fn maximize_request(&mut self, surface: ToplevelSurface) { @@ -379,19 +377,18 @@ impl XdgShellHandler for AnvilState { } fn unmaximize_request(&mut self, surface: ToplevelSurface) { - if !surface - .current_state() - .states - .contains(xdg_toplevel::State::Maximized) - { - return; - } - surface.with_pending_state(|state| { state.states.unset(xdg_toplevel::State::Maximized); state.size = None; }); - surface.send_pending_configure(); + + // The protocol demands us to always reply with a configure, + // regardless of we fulfilled the request or not + if surface.is_initial_configure_sent() { + surface.send_configure(); + } else { + // Will be sent during initial configure + } } fn grab(&mut self, surface: PopupSurface, seat: wl_seat::WlSeat, serial: Serial) { From 82d95a842a7f39a75b471a0cb711ae273439db0b Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Fri, 5 Sep 2025 19:38:48 +0300 Subject: [PATCH 02/14] anvil: Remove WmCapabilities checks Unclear what they were for. This is compositor-side state that Anvil doesn't change after startup. --- anvil/src/shell/xdg.rs | 112 ++++++++++++++++++----------------------- 1 file changed, 50 insertions(+), 62 deletions(-) diff --git a/anvil/src/shell/xdg.rs b/anvil/src/shell/xdg.rs index df2422b6f7bf..96eed27abea1 100644 --- a/anvil/src/shell/xdg.rs +++ b/anvil/src/shell/xdg.rs @@ -264,49 +264,43 @@ impl XdgShellHandler for AnvilState { } fn fullscreen_request(&mut self, surface: ToplevelSurface, mut wl_output: Option) { - if surface - .current_state() - .capabilities - .contains(xdg_toplevel::WmCapabilities::Fullscreen) - { - // NOTE: This is only one part of the solution. We can set the - // location and configure size here, but the surface should be rendered fullscreen - // independently from its buffer size - let wl_surface = surface.wl_surface(); - - let output_geometry = fullscreen_output_geometry(wl_surface, wl_output.as_ref(), &mut self.space); - - if let Some(geometry) = output_geometry { - let output = wl_output - .as_ref() - .and_then(Output::from_resource) - .unwrap_or_else(|| self.space.outputs().next().unwrap().clone()); - let client = match self.display_handle.get_client(wl_surface.id()) { - Ok(client) => client, - Err(_) => return, - }; - for output in output.client_outputs(&client) { - wl_output = Some(output); - } - let window = self - .space - .elements() - .find(|window| window.wl_surface().map(|s| &*s == wl_surface).unwrap_or(false)) - .unwrap(); - - surface.with_pending_state(|state| { - state.states.set(xdg_toplevel::State::Fullscreen); - state.size = Some(geometry.size); - state.fullscreen_output = wl_output; - }); - output.user_data().insert_if_missing(FullscreenSurface::default); - output - .user_data() - .get::() - .unwrap() - .set(window.clone()); - trace!("Fullscreening: {:?}", window); + // NOTE: This is only one part of the solution. We can set the + // location and configure size here, but the surface should be rendered fullscreen + // independently from its buffer size + let wl_surface = surface.wl_surface(); + + let output_geometry = fullscreen_output_geometry(wl_surface, wl_output.as_ref(), &mut self.space); + + if let Some(geometry) = output_geometry { + let output = wl_output + .as_ref() + .and_then(Output::from_resource) + .unwrap_or_else(|| self.space.outputs().next().unwrap().clone()); + let client = match self.display_handle.get_client(wl_surface.id()) { + Ok(client) => client, + Err(_) => return, + }; + for output in output.client_outputs(&client) { + wl_output = Some(output); } + let window = self + .space + .elements() + .find(|window| window.wl_surface().map(|s| &*s == wl_surface).unwrap_or(false)) + .unwrap(); + + surface.with_pending_state(|state| { + state.states.set(xdg_toplevel::State::Fullscreen); + state.size = Some(geometry.size); + state.fullscreen_output = wl_output; + }); + output.user_data().insert_if_missing(FullscreenSurface::default); + output + .user_data() + .get::() + .unwrap() + .set(window.clone()); + trace!("Fullscreening: {:?}", window); } // The protocol demands us to always reply with a configure, @@ -345,27 +339,21 @@ impl XdgShellHandler for AnvilState { fn maximize_request(&mut self, surface: ToplevelSurface) { // NOTE: This should use layer-shell when it is implemented to // get the correct maximum size - if surface - .current_state() - .capabilities - .contains(xdg_toplevel::WmCapabilities::Maximize) - { - let window = self.window_for_surface(surface.wl_surface()).unwrap(); - let outputs_for_window = self.space.outputs_for_element(&window); - let output = outputs_for_window - .first() - // The window hasn't been mapped yet, use the primary output instead - .or_else(|| self.space.outputs().next()) - // Assumes that at least one output exists - .expect("No outputs found"); - let geometry = self.space.output_geometry(output).unwrap(); + let window = self.window_for_surface(surface.wl_surface()).unwrap(); + let outputs_for_window = self.space.outputs_for_element(&window); + let output = outputs_for_window + .first() + // The window hasn't been mapped yet, use the primary output instead + .or_else(|| self.space.outputs().next()) + // Assumes that at least one output exists + .expect("No outputs found"); + let geometry = self.space.output_geometry(output).unwrap(); - surface.with_pending_state(|state| { - state.states.set(xdg_toplevel::State::Maximized); - state.size = Some(geometry.size); - }); - self.space.map_element(window, geometry.loc, true); - } + surface.with_pending_state(|state| { + state.states.set(xdg_toplevel::State::Maximized); + state.size = Some(geometry.size); + }); + self.space.map_element(window, geometry.loc, true); // The protocol demands us to always reply with a configure, // regardless of we fulfilled the request or not From e47aceac9b089fd68ed9e557900ea865a88aafb7 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Fri, 5 Sep 2025 19:39:27 +0300 Subject: [PATCH 03/14] anvil: Correct checks of "current" to pending state --- anvil/src/shell/xdg.rs | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/anvil/src/shell/xdg.rs b/anvil/src/shell/xdg.rs index 96eed27abea1..47306565e8e9 100644 --- a/anvil/src/shell/xdg.rs +++ b/anvil/src/shell/xdg.rs @@ -454,13 +454,15 @@ impl AnvilState { let mut initial_window_location = self.space.element_location(&window).unwrap(); // If surface is maximized then unmaximize it - let current_state = surface.current_state(); - if current_state.states.contains(xdg_toplevel::State::Maximized) { - surface.with_pending_state(|state| { - state.states.unset(xdg_toplevel::State::Maximized); + let changed = surface.with_pending_state(|state| { + if state.states.unset(xdg_toplevel::State::Maximized) { state.size = None; - }); - + true + } else { + false + } + }); + if changed { surface.send_configure(); // NOTE: In real compositor mouse location should be mapped to a new window size @@ -518,13 +520,15 @@ impl AnvilState { let mut initial_window_location = self.space.element_location(&window).unwrap(); // If surface is maximized then unmaximize it - let current_state = surface.current_state(); - if current_state.states.contains(xdg_toplevel::State::Maximized) { - surface.with_pending_state(|state| { - state.states.unset(xdg_toplevel::State::Maximized); + let changed = surface.with_pending_state(|state| { + if state.states.unset(xdg_toplevel::State::Maximized) { state.size = None; - }); - + true + } else { + false + } + }); + if changed { surface.send_configure(); // NOTE: In real compositor mouse location should be mapped to a new window size From c2f476388a77a37ae75a57af4715656061ed6dfb Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Fri, 5 Sep 2025 19:55:10 +0300 Subject: [PATCH 04/14] wayland/shell/xdg: Track last acked configure in MultiCache The last-acked configure is part of the committed state, and should be tracked as such. Before this commit, it would be possible to observe a newer "current state" in a commit handler if the surface acked the next state while the compositor was waiting on a blocker set in a pre-commit handler. As part of the change, the current_state() getter was replaced by with_committed_state(). Since the "current" state wasn't really a correct thing most of the time, most existing uses of current_state() actually wanted something different, and were corrected in previous commits. --- anvil/src/shell/xdg.rs | 18 ++- src/desktop/wayland/popup/manager.rs | 16 +-- src/desktop/wayland/popup/mod.rs | 18 +-- src/wayland/shell/xdg/handlers/surface.rs | 26 +++- .../shell/xdg/handlers/surface/popup.rs | 6 +- .../shell/xdg/handlers/surface/toplevel.rs | 9 +- src/wayland/shell/xdg/mod.rs | 127 +++++++++++------- 7 files changed, 134 insertions(+), 86 deletions(-) diff --git a/anvil/src/shell/xdg.rs b/anvil/src/shell/xdg.rs index 47306565e8e9..7b31b5af8b83 100644 --- a/anvil/src/shell/xdg.rs +++ b/anvil/src/shell/xdg.rs @@ -20,8 +20,8 @@ use smithay::{ compositor::{self, with_states}, seat::WaylandFocus, shell::xdg::{ - Configure, PopupSurface, PositionerState, ToplevelSurface, XdgShellHandler, XdgShellState, - XdgToplevelSurfaceData, + Configure, PopupSurface, PositionerState, ToplevelCachedState, ToplevelSurface, XdgShellHandler, + XdgShellState, }, }, }; @@ -221,14 +221,12 @@ impl XdgShellHandler for AnvilState { // will no longer have the resize state set let is_resizing = with_states(&surface, |states| { states - .data_map - .get::() - .unwrap() - .lock() - .unwrap() - .current - .states - .contains(xdg_toplevel::State::Resizing) + .cached_state + .get::() + .current() + .last_acked + .as_ref() + .is_some_and(|c| c.state.states.contains(xdg_toplevel::State::Resizing)) }); if configure.serial >= serial && is_resizing { diff --git a/src/desktop/wayland/popup/manager.rs b/src/desktop/wayland/popup/manager.rs index 8a2a0250ab6e..c65676f14d85 100644 --- a/src/desktop/wayland/popup/manager.rs +++ b/src/desktop/wayland/popup/manager.rs @@ -4,7 +4,7 @@ use crate::{ wayland::{ compositor::{get_role, with_states}, seat::WaylandFocus, - shell::xdg::{XdgPopupSurfaceData, XdgPopupSurfaceRoleAttributes, XDG_POPUP_ROLE}, + shell::xdg::{PopupCachedState, XdgPopupSurfaceData, XdgPopupSurfaceRoleAttributes, XDG_POPUP_ROLE}, }, }; use std::sync::{Arc, Mutex}; @@ -249,14 +249,12 @@ pub fn get_popup_toplevel_coords(popup: &PopupKind) -> Point { while get_role(&parent) == Some(XDG_POPUP_ROLE) { offset += with_states(&parent, |states| { states - .data_map - .get::>() - .unwrap() - .lock() - .unwrap() - .current - .geometry - .loc + .cached_state + .get::() + .current() + .last_acked + .map(|c| c.state.geometry.loc) + .unwrap_or_default() }); parent = with_states(&parent, |states| { states diff --git a/src/desktop/wayland/popup/mod.rs b/src/desktop/wayland/popup/mod.rs index 6ea9a3957a39..666702aab786 100644 --- a/src/desktop/wayland/popup/mod.rs +++ b/src/desktop/wayland/popup/mod.rs @@ -10,7 +10,7 @@ use crate::{ wayland::{ compositor::with_states, input_method, - shell::xdg::{self, SurfaceCachedState, XdgPopupSurfaceData}, + shell::xdg::{self, SurfaceCachedState}, }, }; @@ -81,21 +81,9 @@ impl PopupKind { } fn location(&self) -> Point { - let wl_surface = self.wl_surface(); - match *self { - PopupKind::Xdg(_) => { - with_states(wl_surface, |states| { - states - .data_map - .get::() - .unwrap() - .lock() - .unwrap() - .current - .geometry - }) - .loc + PopupKind::Xdg(ref t) => { + t.with_committed_state(|current| current.map(|state| state.geometry.loc).unwrap_or_default()) } PopupKind::InputMethod(ref t) => t.location(), } diff --git a/src/wayland/shell/xdg/handlers/surface.rs b/src/wayland/shell/xdg/handlers/surface.rs index 3ae3a79e954b..a85eef0f96c4 100644 --- a/src/wayland/shell/xdg/handlers/surface.rs +++ b/src/wayland/shell/xdg/handlers/surface.rs @@ -5,7 +5,9 @@ use std::sync::Mutex; use indexmap::IndexSet; use crate::utils::alive_tracker::{AliveTracker, IsAlive}; -use crate::wayland::shell::xdg::{XdgPopupSurfaceData, XdgToplevelSurfaceData}; +use crate::wayland::shell::xdg::{ + Configure, PopupCachedState, ToplevelCachedState, XdgPopupSurfaceData, XdgToplevelSurfaceData, +}; use crate::{ utils::{Rectangle, Serial}, wayland::{ @@ -107,14 +109,17 @@ where // Initialize the toplevel capabilities from the default capabilities let default_capabilities = &state.xdg_shell_state().default_capabilities; - let current_capabilties = &mut states + let mut attributes = states .data_map .get::>() .unwrap() .lock() - .unwrap() - .current - .capabilities; + .unwrap(); + if attributes.server_pending.is_none() { + let current = attributes.current_server_state(); + attributes.server_pending = Some(current); + } + let current_capabilties = &mut attributes.server_pending.as_mut().unwrap().capabilities; current_capabilties.replace(default_capabilities.capabilities.iter().copied()); initial @@ -325,6 +330,17 @@ where } }; + compositor::with_states(surface, |states| match &configure { + Configure::Toplevel(configure) => { + let mut state = states.cached_state.get::(); + state.pending().last_acked = Some(configure.clone()); + } + Configure::Popup(configure) => { + let mut state = states.cached_state.get::(); + state.pending().last_acked = Some(*configure); + } + }); + XdgShellHandler::ack_configure(state, surface.clone(), configure); } _ => unreachable!(), diff --git a/src/wayland/shell/xdg/handlers/surface/popup.rs b/src/wayland/shell/xdg/handlers/surface/popup.rs index 227744c909f6..0942fd153a1d 100644 --- a/src/wayland/shell/xdg/handlers/surface/popup.rs +++ b/src/wayland/shell/xdg/handlers/surface/popup.rs @@ -5,7 +5,7 @@ use crate::{ utils::Serial, wayland::{ compositor, - shell::xdg::{SurfaceCachedState, XdgPopupSurfaceData, XdgPositionerUserData}, + shell::xdg::{PopupCachedState, SurfaceCachedState, XdgPopupSurfaceData, XdgPositionerUserData}, }, }; @@ -90,6 +90,10 @@ where let mut guard = states.cached_state.get::(); *guard.pending() = Default::default(); *guard.current() = Default::default(); + + let mut guard = states.cached_state.get::(); + *guard.pending() = Default::default(); + *guard.current() = Default::default(); }) } } diff --git a/src/wayland/shell/xdg/handlers/surface/toplevel.rs b/src/wayland/shell/xdg/handlers/surface/toplevel.rs index 17bd3b9b4d4d..c91f4991b1fb 100644 --- a/src/wayland/shell/xdg/handlers/surface/toplevel.rs +++ b/src/wayland/shell/xdg/handlers/surface/toplevel.rs @@ -4,7 +4,10 @@ use crate::{ utils::Serial, wayland::{ compositor, - shell::{is_valid_parent, xdg::XdgToplevelSurfaceData}, + shell::{ + is_valid_parent, + xdg::{ToplevelCachedState, XdgToplevelSurfaceData}, + }, }, }; @@ -182,6 +185,10 @@ where let mut guard = states.cached_state.get::(); *guard.pending() = Default::default(); *guard.current() = Default::default(); + + let mut guard = states.cached_state.get::(); + *guard.pending() = Default::default(); + *guard.current() = Default::default(); }) } } diff --git a/src/wayland/shell/xdg/mod.rs b/src/wayland/shell/xdg/mod.rs index d427d3060204..98e152ddecdd 100644 --- a/src/wayland/shell/xdg/mod.rs +++ b/src/wayland/shell/xdg/mod.rs @@ -159,7 +159,8 @@ const XDG_TOPLEVEL_STATE_SUSPENDED_SINCE: u32 = 6; macro_rules! xdg_role { ($state:ty, $(#[$configure_meta:meta])* $configure_name:ident $({$($(#[$configure_field_meta:meta])* $configure_field_vis:vis$configure_field_name:ident:$configure_field_type:ty),*}),*, - $(#[$attributes_meta:meta])* $attributes_name:ident {$($(#[$attributes_field_meta:meta])* $attributes_field_vis:vis$attributes_field_name:ident:$attributes_field_type:ty),*}) => { + $(#[$attributes_meta:meta])* $attributes_name:ident {$($(#[$attributes_field_meta:meta])* $attributes_field_vis:vis$attributes_field_name:ident:$attributes_field_type:ty),*}, + $(#[$cached_state_meta:meta])* $cached_state_name:ident) => { $(#[$configure_meta])* pub struct $configure_name { @@ -200,13 +201,9 @@ macro_rules! xdg_role { pub server_pending: Option<$state>, /// Holds the last server_pending state that has been acknowledged /// by the client. This state should be cloned to the current - /// during a commit. + /// during a commit. Note that this state can be newer than the last acked state at + /// the time of the last commit. pub last_acked: Option<$state>, - /// Holds the current state after a successful commit. - pub current: $state, - /// Holds the last acked configure serial at the time of the last successful - /// commit. This serial corresponds to the current state. - pub current_serial: Option, /// Does the surface have a buffer (updated on every commit) has_buffer: bool, @@ -255,7 +252,7 @@ macro_rules! xdg_role { /// This can be used for example to check if the /// [`server_pending`](#structfield.server_pending) state /// is different from the last configured. - pub fn current_server_state(&self) -> &$state { + pub fn current_server_state(&self) -> $state { // We check if there is already a non-acked pending // configure and use its state or otherwise we could // loose some state that was previously configured @@ -263,9 +260,6 @@ macro_rules! xdg_role { // again. If there is no pending state we try to use the // last acked state which could contain state changes // already acked but not committed to the current state. - // In case no last acked state is available, which is - // the case on the first configure we fallback to the - // current state. // In both cases the state already contains all previous // sent states. This way all pending state is accumulated // into the current state. @@ -273,7 +267,8 @@ macro_rules! xdg_role { .last() .map(|c| &c.state) .or_else(|| self.last_acked.as_ref()) - .unwrap_or(&self.current) + .cloned() + .unwrap_or_default() } /// Check if the state has pending changes that have @@ -283,7 +278,7 @@ macro_rules! xdg_role { /// state is [`Some`] in that it also checks if a current pending /// state is different from the [`current_server_state`](#method.current_server_state). pub fn has_pending_changes(&self) -> bool { - self.server_pending.as_ref().map(|s| s != self.current_server_state()).unwrap_or(false) + self.server_pending.as_ref().map(|s| *s != self.current_server_state()).unwrap_or(false) } /// Returns a list of configures sent to, but not yet acknowledged by the client. @@ -304,8 +299,6 @@ macro_rules! xdg_role { initial_configure_sent: false, server_pending: None, last_acked: None, - current: Default::default(), - current_serial: None, has_buffer: false, $( @@ -314,6 +307,23 @@ macro_rules! xdg_role { } } } + + $(#[$cached_state_meta])* + pub struct $cached_state_name { + /// Configure last acknowledged by the client at the time of the commit. + /// + /// Reset to `None` when the surface unmaps. + pub last_acked: Option<$configure_name>, + } + + impl Cacheable for $cached_state_name { + fn commit(&mut self, _dh: &DisplayHandle) -> Self { + self.clone() + } + fn merge_into(self, into: &mut Self, _dh: &DisplayHandle) { + *into = self; + } + } }; } @@ -375,7 +385,10 @@ xdg_role!( /// An `zxdg_toplevel_decoration_v1::configure` event has been sent /// to the client. pub initial_decoration_configure_sent: bool - } + }, + /// Represents the xdg_toplevel pending state + #[derive(Debug, Default, Clone)] + ToplevelCachedState ); /// Data associated with XDG toplevel surface @@ -449,7 +462,10 @@ xdg_role!( pub committed: bool, popup_handle: Option - } + }, + /// Represents the xdg_popup pending state + #[derive(Debug, Default, Clone)] + PopupCachedState ); /// Data associated with XDG popup surface @@ -1449,7 +1465,7 @@ impl ToplevelSurface { attributes .server_pending .take() - .unwrap_or_else(|| attributes.current_server_state().clone()), + .unwrap_or_else(|| attributes.current_server_state()), ); } @@ -1503,7 +1519,7 @@ impl ToplevelSurface { let pending = self .get_pending_state(&mut attributes) - .unwrap_or_else(|| attributes.current_server_state().clone()); + .unwrap_or_else(|| attributes.current_server_state()); // retrieve the current state before adding it to the // pending state so that we can compare what has changed let current = attributes.current_server_state(); @@ -1633,11 +1649,6 @@ impl ToplevelSurface { guard.has_buffer = has_buffer; } - - if let Some(state) = guard.last_acked.clone() { - guard.current = state; - guard.current_serial = guard.configure_serial; - } }); } @@ -1707,7 +1718,7 @@ impl ToplevelSurface { .lock() .unwrap(); if attributes.server_pending.is_none() { - attributes.server_pending = Some(attributes.current_server_state().clone()); + attributes.server_pending = Some(attributes.current_server_state()); } let server_pending = attributes.server_pending.as_mut().unwrap(); @@ -1732,20 +1743,27 @@ impl ToplevelSurface { }) } - /// Gets a copy of the current state of this toplevel - pub fn current_state(&self) -> ToplevelState { + /// Provides access to the current committed cached state. + pub fn with_cached_state(&self, f: F) -> T + where + F: FnOnce(&ToplevelCachedState) -> T, + { compositor::with_states(&self.wl_surface, |states| { - let attributes = states - .data_map - .get::() - .unwrap() - .lock() - .unwrap(); - - attributes.current.clone() + let mut guard = states.cached_state.get::(); + f(guard.current()) }) } + /// Provides access to the current committed state. + /// + /// This is the state that the client last acked before making the current commit. + pub fn with_committed_state(&self, f: F) -> T + where + F: FnOnce(Option<&ToplevelState>) -> T, + { + self.with_cached_state(move |state| f(state.last_acked.as_ref().map(|c| &c.state))) + } + /// Returns the parent of this toplevel surface. pub fn parent(&self) -> Option { handlers::get_parent(&self.shell_surface) @@ -1839,7 +1857,7 @@ impl PopupSurface { let pending = attributes .server_pending .take() - .unwrap_or_else(|| *attributes.current_server_state()); + .unwrap_or_else(|| attributes.current_server_state()); let configure = PopupConfigure { state: pending, @@ -1902,7 +1920,12 @@ impl PopupSurface { return Err(PopupConfigureError::AlreadyConfigured); } - let is_reactive = attributes.current.positioner.reactive; + let mut cached = states.cached_state.get::(); + let is_reactive = cached + .current() + .last_acked + .as_ref() + .is_some_and(|c| c.state.positioner.reactive); if attributes.initial_configure_sent && !is_reactive { // Return error, the positioner does not allow re-configure @@ -2024,13 +2047,6 @@ impl PopupSurface { attributes.has_buffer = has_buffer; } - - if attributes.initial_configure_sent { - if let Some(state) = attributes.last_acked { - attributes.current = state; - attributes.current_serial = attributes.configure_serial; - } - } }); } @@ -2103,7 +2119,7 @@ impl PopupSurface { .lock() .unwrap(); if attributes.server_pending.is_none() { - attributes.server_pending = Some(*attributes.current_server_state()); + attributes.server_pending = Some(attributes.current_server_state()); } let server_pending = attributes.server_pending.as_mut().unwrap(); @@ -2127,6 +2143,27 @@ impl PopupSurface { !attributes.initial_configure_sent || attributes.has_pending_changes() }) } + + /// Provides access to the current committed cached state. + pub fn with_cached_state(&self, f: F) -> T + where + F: FnOnce(&PopupCachedState) -> T, + { + compositor::with_states(&self.wl_surface, |states| { + let mut guard = states.cached_state.get::(); + f(guard.current()) + }) + } + + /// Provides access to the current committed state. + /// + /// This is the state that the client last acked before making the current commit. + pub fn with_committed_state(&self, f: F) -> T + where + F: FnOnce(Option<&PopupState>) -> T, + { + self.with_cached_state(move |state| f(state.last_acked.as_ref().map(|c| &c.state))) + } } /// Defines the possible configure variants From cc99b75bc7f134ea112c671fb4a6c6c319f18bf7 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sat, 6 Sep 2025 10:15:06 +0300 Subject: [PATCH 05/14] wayland/shell/xdg: Track configured state through MultiCache last_acked Also: - Removes dependency on renderer abstractions for tracking unmaps. - Removes manual reset_initial_configure() because it's now always automatic. - Makes the check for map before initial configure automatic. - Adds missing role attributes reset on unmap (it only happened on role destroy previously). --- src/wayland/shell/xdg/handlers/surface.rs | 23 +- src/wayland/shell/xdg/mod.rs | 347 ++++++++++------------ 2 files changed, 160 insertions(+), 210 deletions(-) diff --git a/src/wayland/shell/xdg/handlers/surface.rs b/src/wayland/shell/xdg/handlers/surface.rs index a85eef0f96c4..61ffe1119043 100644 --- a/src/wayland/shell/xdg/handlers/surface.rs +++ b/src/wayland/shell/xdg/handlers/surface.rs @@ -5,9 +5,7 @@ use std::sync::Mutex; use indexmap::IndexSet; use crate::utils::alive_tracker::{AliveTracker, IsAlive}; -use crate::wayland::shell::xdg::{ - Configure, PopupCachedState, ToplevelCachedState, XdgPopupSurfaceData, XdgToplevelSurfaceData, -}; +use crate::wayland::shell::xdg::{XdgPopupSurfaceData, XdgToplevelSurfaceData}; use crate::{ utils::{Rectangle, Serial}, wayland::{ @@ -126,9 +124,9 @@ where }); if initial { - compositor::add_post_commit_hook::( + compositor::add_pre_commit_hook::( surface, - super::super::ToplevelSurface::commit_hook, + super::super::ToplevelSurface::pre_commit_hook, ); } @@ -208,10 +206,6 @@ where surface, super::super::PopupSurface::pre_commit_hook, ); - compositor::add_post_commit_hook::( - surface, - super::super::PopupSurface::post_commit_hook, - ); } let popup = data_init.init( @@ -330,17 +324,6 @@ where } }; - compositor::with_states(surface, |states| match &configure { - Configure::Toplevel(configure) => { - let mut state = states.cached_state.get::(); - state.pending().last_acked = Some(configure.clone()); - } - Configure::Popup(configure) => { - let mut state = states.cached_state.get::(); - state.pending().last_acked = Some(*configure); - } - }); - XdgShellHandler::ack_configure(state, surface.clone(), configure); } _ => unreachable!(), diff --git a/src/wayland/shell/xdg/mod.rs b/src/wayland/shell/xdg/mod.rs index 98e152ddecdd..927360aaf413 100644 --- a/src/wayland/shell/xdg/mod.rs +++ b/src/wayland/shell/xdg/mod.rs @@ -121,11 +121,12 @@ use crate::utils::alive_tracker::IsAlive; 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::compositor::{self, BufferAssignment, SurfaceAttributes}; use std::cmp::min; use std::{collections::HashSet, fmt::Debug, sync::Mutex}; +use tracing::{error, trace, trace_span}; use wayland_protocols::xdg::decoration::zv1::server::zxdg_toplevel_decoration_v1; use wayland_protocols::xdg::shell::server::xdg_positioner::{Anchor, ConstraintAdjustment, Gravity}; use wayland_protocols::xdg::shell::server::xdg_surface; @@ -181,11 +182,6 @@ macro_rules! xdg_role { $(#[$attributes_meta])* pub struct $attributes_name { - /// Defines if the surface has received at least one - /// xdg_surface.ack_configure from the client - pub configured: bool, - /// The serial of the last acked configure - pub configure_serial: Option, /// Holds the state if the surface has sent the initial /// configure event to the client. It is expected that /// during the first commit a initial @@ -199,13 +195,10 @@ macro_rules! xdg_role { pending_configures: Vec<$configure_name>, /// Holds the pending state as set by the server. pub server_pending: Option<$state>, - /// Holds the last server_pending state that has been acknowledged - /// by the client. This state should be cloned to the current - /// during a commit. Note that this state can be newer than the last acked state at - /// the time of the last commit. - pub last_acked: Option<$state>, - /// Does the surface have a buffer (updated on every commit) - has_buffer: bool, + /// Holds the last configure that has been acknowledged by the client. This state + /// should be cloned to the current during a commit. Note that this state can be + /// newer than the last acked state at the time of the last commit. + pub last_acked: Option<$configure_name>, $( $(#[$attributes_field_meta])* @@ -226,14 +219,8 @@ macro_rules! xdg_role { } }; - // Save the state as the last acked state - self.last_acked = Some(configure.state.clone()); - - // Set the xdg_surface to configured - self.configured = true; - - // Save the last configure serial as a reference - self.configure_serial = Some(Serial::from(serial)); + // Save the configure as the last acked configure + self.last_acked = Some(configure.clone()); // Clean old configures self.pending_configures.retain(|c| c.serial > serial); @@ -266,7 +253,7 @@ macro_rules! xdg_role { self.pending_configures .last() .map(|c| &c.state) - .or_else(|| self.last_acked.as_ref()) + .or_else(|| self.last_acked.as_ref().map(|c| &c.state)) .cloned() .unwrap_or_default() } @@ -293,13 +280,10 @@ macro_rules! xdg_role { impl Default for $attributes_name { fn default() -> Self { Self { - configured: false, - configure_serial: None, pending_configures: Vec::new(), initial_configure_sent: false, server_pending: None, last_acked: None, - has_buffer: false, $( $attributes_field_name: Default::default(), @@ -442,6 +426,11 @@ xdg_role!( /// for the xdg_popup state to take effect. #[derive(Debug)] XdgPopupSurfaceRoleAttributes { + // FIXME: "parent" should move into PopupCachedState, and "committed" should be removed in + // favor of something like PopupCachedState::last_acked.is_some(). Along the way, popup + // grabs should be made to apply at commit time, rather than immediately upon + // xdg_popup.grab(). This is required to fix Qt layer-shell popup grabs for example. + /// Holds the parent for the xdg_popup. /// /// The parent is allowed to remain unset as long @@ -459,9 +448,7 @@ xdg_role!( /// This can be used to check for protocol errors, like /// checking if a popup requested a grab after it has been /// mapped. - pub committed: bool, - - popup_handle: Option + pub committed: bool }, /// Represents the xdg_popup pending state #[derive(Debug, Default, Clone)] @@ -1596,91 +1583,98 @@ impl ToplevelSurface { }) } - /// A newly-unmapped toplevel surface has to perform the initial commit-configure sequence as if it was - /// a new toplevel. - /// - /// This method is used to mark a surface for reinitialization. - /// - /// NOTE: If you are using smithay's rendering abstractions you don't have to call this manually - /// - /// Calls [`compositor::with_states`] internally. - pub fn reset_initial_configure_sent(&self) { - compositor::with_states(&self.wl_surface, |states| { - let mut data = states - .data_map - .get::() - .unwrap() - .lock() - .unwrap(); - data.initial_configure_sent = false; - data.initial_decoration_configure_sent = false; - }); - } - - /// Handles the role specific commit logic + /// Handles the role specific commit error checking /// /// This should be called when the underlying WlSurface /// handles a wl_surface.commit request. - pub(crate) fn commit_hook( - _state: &mut D, + pub(crate) fn pre_commit_hook( + state: &mut D, _dh: &DisplayHandle, surface: &wl_surface::WlSurface, ) { - let has_buffer = crate::backend::renderer::utils::with_renderer_surface_state(surface, |state| { - state.buffer().is_some() - }); + let _span = trace_span!("xdg-toplevel pre-commit", surface = %surface.id()).entered(); - compositor::with_states(surface, |states| { - let mut guard = states + let error = compositor::with_states(surface, |states| { + let mut role = states .data_map .get::() .unwrap() .lock() .unwrap(); - // This can be None if rendering utils are not used by the user - if let Some(has_buffer) = has_buffer { - // The surface was mapped in the past, and now got unmapped - if guard.has_buffer && !has_buffer { - // After xdg surface unmaps it has to perform the initial commit-configure sequence again - guard.initial_configure_sent = false; - guard.initial_decoration_configure_sent = false; - } + let mut guard_toplevel = states.cached_state.get::(); + let pending = guard_toplevel.pending(); + + // The presence of last_acked always follows the buffer assignment because the + // surface is not allowed to attach a buffer without acking the initial configure. + let had_buffer_before = pending.last_acked.is_some(); - guard.has_buffer = has_buffer; + let mut guard_surface = states.cached_state.get::(); + let has_buffer = match &guard_surface.pending().buffer { + Some(BufferAssignment::NewBuffer(_)) => true, + Some(BufferAssignment::Removed) => false, + None => had_buffer_before, + }; + // Need to check had_buffer_before in case the client attaches a null buffer for the + // initial commit---we don't want to consider that as "got unmapped" and reset role. + let got_unmapped = had_buffer_before && !has_buffer; + + if has_buffer { + let Some(last_acked) = role.last_acked.clone() else { + return Some(( + xdg_surface::Error::UnconfiguredBuffer, + "must ack the initial configure before attaching buffer", + )); + }; + + // The surface remains, or became mapped, track the last acked state. + pending.last_acked = Some(last_acked); + } else { + // The surface remains, or became, unmapped, meaning that it's in the initial + // configure stage. + pending.last_acked = None; } - }); - } - /// Make sure this surface was configured - /// - /// Returns `true` if it was, if not, returns `false` and raise - /// a protocol error to the associated client. Also returns `false` - /// if the surface is already destroyed. - /// - /// `xdg_shell` mandates that a client acks a configure before committing - /// anything. - pub fn ensure_configured(&self) -> bool { - let configured = compositor::with_states(&self.wl_surface, |states| { - states - .data_map - .get::() - .unwrap() - .lock() - .unwrap() - .configured + if got_unmapped { + trace!( + "got unmapped; resetting role and cached state; have {} pending configures", + role.pending_configures.len() + ); + + // All attributes are discarded when an xdg_surface is unmapped. Though, we keep + // the list of pending configures because there's no way for a surface to tell + // an in-flight configure apart from our next initial configure after unmapping. + // + // This can be reproduced with wleird-unmap if, say, and Activated configure is + // timed right. + let pending_configures = std::mem::take(&mut role.pending_configures); + *role = Default::default(); + role.pending_configures = pending_configures; + *guard_toplevel.pending() = Default::default(); + + // Don't forget to prepare the default capabilities though. + let default_capabilities = &state.xdg_shell_state().default_capabilities; + role.server_pending = Some(Default::default()); + let current_capabilties = &mut role.server_pending.as_mut().unwrap().capabilities; + current_capabilties.replace(default_capabilities.capabilities.iter().copied()); + } + + None }); - if !configured { - let data = self - .shell_surface - .data::() - .unwrap(); - data.xdg_surface.post_error( - xdg_surface::Error::NotConstructed, - "Surface has not been configured yet.", - ); + + if let Some((error, msg)) = error { + // The surface attached a buffer without acking the initial configure. + let toplevels = &state.xdg_shell_state().known_toplevels; + if let Some(toplevel) = toplevels.iter().find(|handle| handle.wl_surface == *surface) { + let data = toplevel + .shell_surface + .data::() + .unwrap(); + data.xdg_surface.post_error(error, msg); + } else { + error!("surface missing from known toplevels"); + } } - configured } /// Send a "close" event to the client @@ -1956,26 +1950,6 @@ impl PopupSurface { }) } - /// A newly-unmapped popup surface has to perform the initial commit-configure sequence as if it was - /// a new popup. - /// - /// This method is used to mark a surface for reinitialization. - /// - /// NOTE: If you are using smithay's rendering abstractions you don't have to call this manually - /// - /// Calls [`compositor::with_states`] internally. - pub fn reset_initial_configure_sent(&self) { - compositor::with_states(&self.wl_surface, |states| { - let mut data = states - .data_map - .get::() - .unwrap() - .lock() - .unwrap(); - data.initial_configure_sent = false; - }); - } - /// Send a configure event, including the `repositioned` event to the client /// in response to a `reposition` request. /// @@ -1988,97 +1962,90 @@ impl PopupSurface { /// /// This should be called when the underlying WlSurface /// handles a wl_surface.commit request. - pub(crate) fn pre_commit_hook( - _state: &mut D, + pub(crate) fn pre_commit_hook( + state: &mut D, _dh: &DisplayHandle, surface: &wl_surface::WlSurface, ) { - let send_error_to = compositor::with_states(surface, |states| { - let attributes = states + let _span = trace_span!("xdg-popup pre-commit", surface = %surface.id()).entered(); + + let error = compositor::with_states(surface, |states| { + let mut role = states .data_map .get::() .unwrap() .lock() .unwrap(); - if attributes.parent.is_none() { - attributes.popup_handle.clone() - } else { - None + if role.parent.is_none() { + return Some(( + xdg_surface::Error::NotConstructed, + "xdg_popup must have parent before mapping", + )); } - }); - if let Some(handle) = send_error_to { - let data = handle.data::().unwrap(); - data.xdg_surface.post_error( - xdg_surface::Error::NotConstructed, - "Surface has not been configured yet.", - ); - } - } - /// Handles the role specific commit state application - /// - /// This should be called when the underlying WlSurface - /// applies a wl_surface.commit state. - pub(crate) fn post_commit_hook( - _state: &mut D, - _dh: &DisplayHandle, - surface: &wl_surface::WlSurface, - ) { - let has_buffer = crate::backend::renderer::utils::with_renderer_surface_state(surface, |state| { - state.buffer().is_some() - }); + let mut guard_popup = states.cached_state.get::(); + let pending = guard_popup.pending(); - compositor::with_states(surface, |states| { - let mut attributes = states - .data_map - .get::() - .unwrap() - .lock() - .unwrap(); - attributes.committed = true; - - // This can be None if rendering utils are not used by the user - if let Some(has_buffer) = has_buffer { - // The surface was mapped in the past, and now got unmapped - if attributes.has_buffer && !has_buffer { - // After xdg surface unmaps it has to perform the initial commit-configure sequence again - attributes.initial_configure_sent = false; - } + // The presence of last_acked always follows the buffer assignment because the + // surface is not allowed to attach a buffer without acking the initial configure. + let had_buffer_before = pending.last_acked.is_some(); - attributes.has_buffer = has_buffer; + let mut guard_surface = states.cached_state.get::(); + let has_buffer = match &guard_surface.pending().buffer { + Some(BufferAssignment::NewBuffer(_)) => true, + Some(BufferAssignment::Removed) => false, + None => had_buffer_before, + }; + // Need to check had_buffer_before in case the client attaches a null buffer for the + // initial commit---we don't want to consider that as "got unmapped" and reset role. + let got_unmapped = had_buffer_before && !has_buffer; + + if has_buffer { + let Some(last_acked) = role.last_acked else { + return Some(( + xdg_surface::Error::UnconfiguredBuffer, + "must ack the initial configure before attaching buffer", + )); + }; + + // The surface remains, or became mapped, track the last acked state. + pending.last_acked = Some(last_acked); + } else { + // The surface remains, or became, unmapped, meaning that it's in the initial + // configure stage. + pending.last_acked = None; } - }); - } - /// Make sure this surface was configured - /// - /// Returns `true` if it was, if not, returns `false` and raise - /// a protocol error to the associated client. Also returns `false` - /// if the surface is already destroyed. - /// - /// xdg_shell mandates that a client acks a configure before committing - /// anything. - pub fn ensure_configured(&self) -> bool { - let configured = compositor::with_states(&self.wl_surface, |states| { - states - .data_map - .get::() - .unwrap() - .lock() - .unwrap() - .configured + if got_unmapped { + trace!( + "got unmapped; resetting role and cached state; have {} pending configures", + role.pending_configures.len() + ); + + // All attributes are discarded when an xdg_surface is unmapped. Though, we keep + // the list of pending configures because there's no way for a surface to tell + // an in-flight configure apart from our next initial configure after unmapping. + let pending_configures = std::mem::take(&mut role.pending_configures); + *role = Default::default(); + role.pending_configures = pending_configures; + *guard_popup.pending() = Default::default(); + } + + None }); - if !configured { - let data = self - .shell_surface - .data::() - .unwrap(); - data.xdg_surface.post_error( - xdg_surface::Error::NotConstructed, - "Surface has not been configured yet.", - ); + + if let Some((error, msg)) = error { + let popups = &state.xdg_shell_state().known_popups; + if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) { + let data = popup + .shell_surface + .data::() + .unwrap(); + data.xdg_surface.post_error(error, msg); + } else { + error!("surface missing from known popups"); + } } - configured } /// Send a `popup_done` event to the popup surface From 023e2ed76c73a8ed27291ef66aa88f2c8ffd6c62 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 09:34:33 +0300 Subject: [PATCH 06/14] wayland/shell/layer: Fix comment --- src/wayland/shell/wlr_layer/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wayland/shell/wlr_layer/mod.rs b/src/wayland/shell/wlr_layer/mod.rs index 4dd819978a95..946e6edaf1e2 100644 --- a/src/wayland/shell/wlr_layer/mod.rs +++ b/src/wayland/shell/wlr_layer/mod.rs @@ -74,7 +74,7 @@ pub use types::{Anchor, ExclusiveZone, KeyboardInteractivity, Layer, Margins}; /// The role of a wlr_layer_shell_surface pub const LAYER_SURFACE_ROLE: &str = "zwlr_layer_surface_v1"; -/// Data associated with XDG popup surface +/// Data associated with layer surface /// /// ```no_run /// use smithay::wayland::compositor; From c64f5dd33e3630f2494e18cb883d71719d9bed65 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 08:10:24 +0300 Subject: [PATCH 07/14] wayland/shell/layer: Add and use with_cached_state() helper --- anvil/src/input_handler.rs | 12 +++++------- src/desktop/wayland/layer.rs | 31 ++++++++---------------------- src/wayland/shell/wlr_layer/mod.rs | 11 +++++++++++ 3 files changed, 24 insertions(+), 30 deletions(-) diff --git a/anvil/src/input_handler.rs b/anvil/src/input_handler.rs index 14a113fbc4a9..1681f75f8685 100644 --- a/anvil/src/input_handler.rs +++ b/anvil/src/input_handler.rs @@ -24,10 +24,9 @@ use smithay::{ }, utils::{Logical, Point, Serial, Transform, SERIAL_COUNTER as SCOUNTER}, wayland::{ - compositor::with_states, input_method::InputMethodSeat, keyboard_shortcuts_inhibit::KeyboardShortcutsInhibitorSeat, - shell::wlr_layer::{KeyboardInteractivity, Layer as WlrLayer, LayerSurfaceCachedState}, + shell::wlr_layer::{KeyboardInteractivity, Layer as WlrLayer}, }, }; @@ -146,12 +145,11 @@ impl AnvilState { let keyboard = self.seat.get_keyboard().unwrap(); for layer in self.layer_shell_state.layer_surfaces().rev() { - let data = with_states(layer.wl_surface(), |states| { - *states.cached_state.get::().current() + let exclusive = layer.with_cached_state(|data| { + data.keyboard_interactivity == KeyboardInteractivity::Exclusive + && (data.layer == WlrLayer::Top || data.layer == WlrLayer::Overlay) }); - if data.keyboard_interactivity == KeyboardInteractivity::Exclusive - && (data.layer == WlrLayer::Top || data.layer == WlrLayer::Overlay) - { + if exclusive { let surface = self.space.outputs().find_map(|o| { let map = layer_map_for_output(o); let cloned = map.layers().find(|l| l.layer_surface() == &layer).cloned(); diff --git a/src/desktop/wayland/layer.rs b/src/desktop/wayland/layer.rs index 7031f4c603e1..497e80b0654b 100644 --- a/src/desktop/wayland/layer.rs +++ b/src/desktop/wayland/layer.rs @@ -297,9 +297,7 @@ impl LayerMap { ) } - let data = with_states(surface, |states| { - *states.cached_state.get::().current() - }); + let data = layer.cached_state(); let mut source = match data.exclusive_zone { ExclusiveZone::Exclusive(_) | ExclusiveZone::Neutral => zone, @@ -551,37 +549,24 @@ impl LayerSurface { self.0.surface.wl_surface() } - /// Returns the cached protocol state + /// Returns a clone of the cached protocol state pub fn cached_state(&self) -> LayerSurfaceCachedState { - with_states(self.0.surface.wl_surface(), |states| { - *states.cached_state.get::().current() - }) + self.0.surface.with_cached_state(Clone::clone) } /// Returns true, if the surface has indicated, that it is able to process keyboard events. pub fn can_receive_keyboard_focus(&self) -> bool { - with_states(self.0.surface.wl_surface(), |states| { - match states - .cached_state - .get::() - .current() - .keyboard_interactivity - { + self.0 + .surface + .with_cached_state(|state| match state.keyboard_interactivity { KeyboardInteractivity::Exclusive | KeyboardInteractivity::OnDemand => true, KeyboardInteractivity::None => false, - } - }) + }) } /// Returns the layer this surface resides on, if any yet. pub fn layer(&self) -> WlrLayer { - with_states(self.0.surface.wl_surface(), |states| { - states - .cached_state - .get::() - .current() - .layer - }) + self.0.surface.with_cached_state(|state| state.layer) } /// Returns the namespace of this surface diff --git a/src/wayland/shell/wlr_layer/mod.rs b/src/wayland/shell/wlr_layer/mod.rs index 946e6edaf1e2..b1588aaf628a 100644 --- a/src/wayland/shell/wlr_layer/mod.rs +++ b/src/wayland/shell/wlr_layer/mod.rs @@ -496,6 +496,17 @@ impl LayerSurface { }) } + /// Provides access to the current committed cached state. + pub fn with_cached_state(&self, f: F) -> T + where + F: FnOnce(&LayerSurfaceCachedState) -> T, + { + compositor::with_states(&self.wl_surface, |states| { + let mut guard = states.cached_state.get::(); + f(guard.current()) + }) + } + /// Access the underlying `zwlr_layer_surface_v1` of this layer surface /// pub fn shell_surface(&self) -> &zwlr_layer_surface_v1::ZwlrLayerSurfaceV1 { From 5a89fb57d2f91d80e7a2b3de9b2cfe6c9f3326cf Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 08:11:20 +0300 Subject: [PATCH 08/14] wayland/shell/layer: Move last_acked tracking to cached state Same idea as with xdg-shell in the previous commit. --- src/wayland/shell/wlr_layer/handlers.rs | 47 +----- src/wayland/shell/wlr_layer/mod.rs | 184 ++++++++++++++---------- 2 files changed, 116 insertions(+), 115 deletions(-) diff --git a/src/wayland/shell/wlr_layer/handlers.rs b/src/wayland/shell/wlr_layer/handlers.rs index d3de45852850..5eb3bc9c7291 100644 --- a/src/wayland/shell/wlr_layer/handlers.rs +++ b/src/wayland/shell/wlr_layer/handlers.rs @@ -122,49 +122,10 @@ where }); if initial { - compositor::add_pre_commit_hook::(&wl_surface, |_state, _dh, surface| { - compositor::with_states(surface, |states| { - let guard = states - .data_map - .get::>() - .unwrap() - .lock() - .unwrap(); - - let mut cached_guard = states.cached_state.get::(); - let pending = cached_guard.pending(); - - if pending.size.w == 0 && !pending.anchor.anchored_horizontally() { - guard.surface.post_error( - zwlr_layer_surface_v1::Error::InvalidSize, - "width 0 requested without setting left and right anchors", - ); - return; - } - - if pending.size.h == 0 && !pending.anchor.anchored_vertically() { - guard.surface.post_error( - zwlr_layer_surface_v1::Error::InvalidSize, - "height 0 requested without setting top and bottom anchors", - ); - } - }); - }); - - compositor::add_post_commit_hook::(&wl_surface, |_state, _dh, surface| { - compositor::with_states(surface, |states| { - let mut guard = states - .data_map - .get::>() - .unwrap() - .lock() - .unwrap(); - - if let Some(state) = guard.last_acked.clone() { - guard.current = state; - } - }); - }); + compositor::add_pre_commit_hook::( + &wl_surface, + super::LayerSurface::pre_commit_hook, + ); } let handle = super::LayerSurface { diff --git a/src/wayland/shell/wlr_layer/mod.rs b/src/wayland/shell/wlr_layer/mod.rs index b1588aaf628a..ba2aed561f4e 100644 --- a/src/wayland/shell/wlr_layer/mod.rs +++ b/src/wayland/shell/wlr_layer/mod.rs @@ -47,20 +47,20 @@ use std::sync::{Arc, Mutex}; +use tracing::{trace, trace_span}; use wayland_protocols_wlr::layer_shell::v1::server::{ - zwlr_layer_shell_v1::{self, ZwlrLayerShellV1}, - zwlr_layer_surface_v1, + zwlr_layer_shell_v1::ZwlrLayerShellV1, zwlr_layer_surface_v1, }; use wayland_server::{ backend::GlobalId, protocol::{wl_output::WlOutput, wl_surface}, - Client, DisplayHandle, GlobalDispatch, Resource, + Client, DisplayHandle, GlobalDispatch, Resource as _, }; use crate::{ utils::{alive_tracker::IsAlive, Logical, Serial, Size, SERIAL_COUNTER}, wayland::{ - compositor::{self, Cacheable}, + compositor::{self, BufferAssignment, Cacheable, SurfaceAttributes}, shell::xdg, }, }; @@ -91,11 +91,6 @@ pub type LayerSurfaceData = Mutex; #[derive(Debug)] pub struct LayerSurfaceAttributes { surface: zwlr_layer_surface_v1::ZwlrLayerSurfaceV1, - /// Defines if the surface has received at least one - /// layer_surface.ack_configure from the client - pub configured: bool, - /// The serial of the last acked configure - pub configure_serial: Option, /// Holds the state if the surface has sent the initial /// configure event to the client. It is expected that /// during the first commit a initial @@ -109,26 +104,20 @@ pub struct LayerSurfaceAttributes { pending_configures: Vec, /// Holds the pending state as set by the server. pub server_pending: Option, - /// Holds the last server_pending state that has been acknowledged - /// by the client. This state should be cloned to the current - /// during a commit. - pub last_acked: Option, - /// Holds the current state of the layer after a successful - /// commit. - pub current: LayerSurfaceState, + /// Holds the last configure that has been acknowledged by the client. This state should be + /// cloned to the current during a commit. Note that this state can be newer than the last + /// acked state at the time of the last commit. + pub last_acked: Option, } impl LayerSurfaceAttributes { fn new(surface: zwlr_layer_surface_v1::ZwlrLayerSurfaceV1) -> Self { Self { surface, - configured: false, - configure_serial: None, initial_configure_sent: false, pending_configures: Vec::new(), server_pending: None, last_acked: None, - current: Default::default(), } } @@ -139,36 +128,32 @@ impl LayerSurfaceAttributes { .find(|configure| configure.serial == serial) .cloned()?; - self.last_acked = Some(configure.state.clone()); + self.last_acked = Some(configure.clone()); - self.configured = true; - self.configure_serial = Some(serial); self.pending_configures.retain(|c| c.serial > serial); Some(configure) } fn reset(&mut self) { - self.configured = false; - self.configure_serial = None; self.initial_configure_sent = false; self.pending_configures = Vec::new(); self.server_pending = None; self.last_acked = None; - self.current = Default::default(); } - fn current_server_state(&self) -> &LayerSurfaceState { + fn current_server_state(&self) -> LayerSurfaceState { self.pending_configures .last() .map(|c| &c.state) - .or(self.last_acked.as_ref()) - .unwrap_or(&self.current) + .or(self.last_acked.as_ref().map(|c| &c.state)) + .cloned() + .unwrap_or_default() } fn has_pending_changes(&self) -> bool { self.server_pending .as_ref() - .map(|s| s != self.current_server_state()) + .map(|s| *s != self.current_server_state()) .unwrap_or(false) } } @@ -181,7 +166,7 @@ pub struct LayerSurfaceState { } /// Represents the client pending state -#[derive(Debug, Default, Clone, Copy)] +#[derive(Debug, Default, Clone)] pub struct LayerSurfaceCachedState { /// The size requested by the client pub size: Size, @@ -195,11 +180,15 @@ pub struct LayerSurfaceCachedState { pub keyboard_interactivity: KeyboardInteractivity, /// The layer that the surface is rendered on pub layer: Layer, + /// Configure last acknowledged by the client at the time of the commit. + /// + /// Reset to `None` when the surface unmaps. + pub last_acked: Option, } impl Cacheable for LayerSurfaceCachedState { fn commit(&mut self, _dh: &DisplayHandle) -> Self { - *self + self.clone() } fn merge_into(self, into: &mut Self, _dh: &DisplayHandle) { *into = self; @@ -398,30 +387,6 @@ impl LayerSurface { serial } - /// Make sure this surface was configured - /// - /// Returns `true` if it was, if not, returns `false` and raise - /// a protocol error to the associated layer surface. Also returns `false` - /// if the surface is already destroyed. - pub fn ensure_configured(&self) -> bool { - let configured = compositor::with_states(&self.wl_surface, |states| { - states - .data_map - .get::>() - .unwrap() - .lock() - .unwrap() - .configured - }); - if !configured { - self.shell_surface.post_error( - zwlr_layer_shell_v1::Error::AlreadyConstructed, - "layer_surface has never been configured", - ); - } - configured - } - /// Send a "close" event to the client pub fn send_close(&self) { self.shell_surface.closed() @@ -479,23 +444,6 @@ impl LayerSurface { }) } - /// Gets a copy of the current state of this layer - /// - /// Returns `None` if the underlying surface has been - /// destroyed - pub fn current_state(&self) -> LayerSurfaceState { - compositor::with_states(&self.wl_surface, |states| { - let attributes = states - .data_map - .get::>() - .unwrap() - .lock() - .unwrap(); - - attributes.current.clone() - }) - } - /// Provides access to the current committed cached state. pub fn with_cached_state(&self, f: F) -> T where @@ -507,6 +455,98 @@ impl LayerSurface { }) } + /// Provides access to the current committed state. + /// + /// This is the state that the client last acked before making the current commit. + pub fn with_committed_state(&self, f: F) -> T + where + F: FnOnce(Option<&LayerSurfaceState>) -> T, + { + self.with_cached_state(move |state| f(state.last_acked.as_ref().map(|c| &c.state))) + } + + /// Handles the role specific commit error checking + /// + /// This should be called when the underlying WlSurface + /// handles a wl_surface.commit request. + pub(crate) fn pre_commit_hook( + _state: &mut D, + _dh: &DisplayHandle, + surface: &wl_surface::WlSurface, + ) { + let _span = trace_span!("layer-surface pre-commit", surface = %surface.id()).entered(); + + compositor::with_states(surface, |states| { + let mut role = states.data_map.get::().unwrap().lock().unwrap(); + + let mut guard_layer = states.cached_state.get::(); + let pending = guard_layer.pending(); + + if pending.size.w == 0 && !pending.anchor.anchored_horizontally() { + role.surface.post_error( + zwlr_layer_surface_v1::Error::InvalidSize, + "width 0 requested without setting left and right anchors", + ); + return; + } + + if pending.size.h == 0 && !pending.anchor.anchored_vertically() { + role.surface.post_error( + zwlr_layer_surface_v1::Error::InvalidSize, + "height 0 requested without setting top and bottom anchors", + ); + return; + } + + // The presence of last_acked always follows the buffer assignment because the + // surface is not allowed to attach a buffer without acking the initial configure. + let had_buffer_before = pending.last_acked.is_some(); + + let mut guard_surface = states.cached_state.get::(); + let has_buffer = match &guard_surface.pending().buffer { + Some(BufferAssignment::NewBuffer(_)) => true, + Some(BufferAssignment::Removed) => false, + None => had_buffer_before, + }; + // Need to check had_buffer_before in case the client attaches a null buffer for the + // initial commit---we don't want to consider that as "got unmapped" and reset role. + // Reproducer: waybar. + let got_unmapped = had_buffer_before && !has_buffer; + + if has_buffer { + let Some(last_acked) = role.last_acked.clone() else { + role.surface.post_error( + zwlr_layer_surface_v1::Error::InvalidSurfaceState, + "must ack the initial configure before attaching buffer", + ); + return; + }; + + // The surface remains, or became mapped, track the last acked state. + pending.last_acked = Some(last_acked); + } else { + // The surface remains, or became, unmapped, meaning that it's in the initial + // configure stage. + pending.last_acked = None; + } + + if got_unmapped { + trace!( + "got unmapped; resetting role and cached state; have {} pending configures", + role.pending_configures.len() + ); + + // All attributes are discarded when an xdg_surface is unmapped. Though, we keep + // the list of pending configures because there's no way for a surface to tell + // an in-flight configure apart from our next initial configure after unmapping. + let pending_configures = std::mem::take(&mut role.pending_configures); + role.reset(); + role.pending_configures = pending_configures; + *guard_layer.pending() = Default::default(); + } + }); + } + /// Access the underlying `zwlr_layer_surface_v1` of this layer surface /// pub fn shell_surface(&self) -> &zwlr_layer_surface_v1::ZwlrLayerSurfaceV1 { From e6d3633168ef291001ba8c7b8eed6d616cfa10ed Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 09:01:46 +0300 Subject: [PATCH 09/14] wayland/session_lock: Reset attributes on destroy In theory, allows reusing the wl_surface, consistent with other shells. --- src/wayland/session_lock/surface.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/wayland/session_lock/surface.rs b/src/wayland/session_lock/surface.rs index 8865a88df800..e9bdbb43ae51 100644 --- a/src/wayland/session_lock/surface.rs +++ b/src/wayland/session_lock/surface.rs @@ -60,6 +60,25 @@ where _ => unreachable!(), } } + + fn destroyed( + _state: &mut D, + _client: wayland_server::backend::ClientId, + _resource: &ExtSessionLockSurfaceV1, + data: &ExtLockSurfaceUserData, + ) { + if let Ok(surface) = data.surface.upgrade() { + compositor::with_states(&surface, |states| { + let mut attributes = states + .data_map + .get::>() + .unwrap() + .lock() + .unwrap(); + attributes.reset(); + }); + } + } } /// Attributes for ext-session-lock surfaces. @@ -113,6 +132,14 @@ impl LockSurfaceAttributes { Some(configure) } + + fn reset(&mut self) { + self.configure_serial = None; + self.server_pending = None; + self.pending_configures = Vec::new(); + self.last_acked = None; + self.current = Default::default(); + } } /// Handle for a ext-session-lock surface. From 215726fefb2858011971304de9dd5e843a4c3d99 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 09:52:00 +0300 Subject: [PATCH 10/14] wayland/session_lock: Move last_acked tracking to cached state Same idea as with xdg-shell in the previous commit. --- src/wayland/session_lock/lock.rs | 75 +---------- src/wayland/session_lock/mod.rs | 7 +- src/wayland/session_lock/surface.rs | 190 +++++++++++++++++++++++----- 3 files changed, 163 insertions(+), 109 deletions(-) diff --git a/src/wayland/session_lock/lock.rs b/src/wayland/session_lock/lock.rs index 7867e17e511b..9f4492a7e00b 100644 --- a/src/wayland/session_lock/lock.rs +++ b/src/wayland/session_lock/lock.rs @@ -3,14 +3,11 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; -use crate::backend::renderer::buffer_dimensions; -use crate::utils::Size; use crate::wayland::compositor::SurfaceAttributes; use crate::wayland::compositor::{self, BufferAssignment}; -use crate::wayland::viewporter::{ViewportCachedState, ViewporterSurfaceState}; use _session_lock::ext_session_lock_surface_v1::ExtSessionLockSurfaceV1; use _session_lock::ext_session_lock_v1::{Error, ExtSessionLockV1, Request}; -use wayland_protocols::ext::session_lock::v1::server::{self as _session_lock, ext_session_lock_surface_v1}; +use wayland_protocols::ext::session_lock::v1::server::{self as _session_lock}; use wayland_server::{Client, DataInit, Dispatch, DisplayHandle, Resource}; use crate::wayland::session_lock::surface::{ExtLockSurfaceUserData, LockSurface, LockSurfaceAttributes}; @@ -101,75 +98,7 @@ where }); // Add pre-commit hook for updating surface state. - compositor::add_pre_commit_hook::(&surface, |_state, _dh, surface| { - compositor::with_states(surface, |states| { - let attributes = states.data_map.get::>(); - let attributes = attributes.unwrap().lock().unwrap(); - - let Some(state) = attributes.last_acked else { - attributes.surface.post_error( - ext_session_lock_surface_v1::Error::CommitBeforeFirstAck, - "Committed before the first ack_configure.", - ); - return; - }; - - // Verify the attached buffer: ext-session-lock requires no NULL buffers - // and an exact dimentions match. - let mut guard = states.cached_state.get::(); - let surface_attrs = guard.pending(); - if let Some(assignment) = surface_attrs.buffer.as_ref() { - match assignment { - BufferAssignment::Removed => { - attributes.surface.post_error( - ext_session_lock_surface_v1::Error::NullBuffer, - "Surface attached a NULL buffer.", - ); - } - BufferAssignment::NewBuffer(buffer) => { - if let Some(buf_size) = buffer_dimensions(buffer) { - let viewport = states - .data_map - .get::() - .map(|v| v.lock().unwrap()); - let surface_size = if let Some(dest) = - viewport.as_ref().and_then(|_| { - let mut guard = - states.cached_state.get::(); - let viewport_state = guard.pending(); - viewport_state.dst - }) { - Size::from((dest.w as u32, dest.h as u32)) - } else { - let scale = surface_attrs.buffer_scale; - let transform = surface_attrs.buffer_transform.into(); - let surface_size = buf_size.to_logical(scale, transform); - - Size::from((surface_size.w as u32, surface_size.h as u32)) - }; - - if Some(surface_size) != state.size { - attributes.surface.post_error( - ext_session_lock_surface_v1::Error::DimensionsMismatch, - "Surface dimensions do not match acked configure.", - ); - } - } - } - } - } - }); - }); - compositor::add_post_commit_hook::(&surface, |_state, _dh, surface| { - compositor::with_states(surface, |states| { - let attributes = states.data_map.get::>(); - let mut attributes = attributes.unwrap().lock().unwrap(); - - if let Some(state) = attributes.last_acked { - attributes.current = state; - } - }); - }); + compositor::add_pre_commit_hook::(&surface, LockSurface::pre_commit_hook); // Call compositor handler. let lock_surface = LockSurface::new(surface, lock_surface); diff --git a/src/wayland/session_lock/mod.rs b/src/wayland/session_lock/mod.rs index 4981dd080c95..bdac9a125d33 100644 --- a/src/wayland/session_lock/mod.rs +++ b/src/wayland/session_lock/mod.rs @@ -58,13 +58,14 @@ use wayland_server::protocol::wl_output::WlOutput; use wayland_server::protocol::wl_surface::WlSurface; use wayland_server::{Client, DataInit, Dispatch, DisplayHandle, GlobalDispatch, New}; -use crate::wayland::session_lock::surface::LockSurfaceConfigure; - mod lock; mod surface; pub use lock::SessionLockState; -pub use surface::{ExtLockSurfaceUserData, LockSurface, LockSurfaceState}; +pub use surface::{ + ExtLockSurfaceUserData, LockSurface, LockSurfaceAttributes, LockSurfaceCachedState, LockSurfaceConfigure, + LockSurfaceData, LockSurfaceState, +}; const MANAGER_VERSION: u32 = 1; diff --git a/src/wayland/session_lock/surface.rs b/src/wayland/session_lock/surface.rs index e9bdbb43ae51..92e2f10b0731 100644 --- a/src/wayland/session_lock/surface.rs +++ b/src/wayland/session_lock/surface.rs @@ -2,9 +2,12 @@ use std::sync::Mutex; +use crate::backend::renderer::buffer_dimensions; use crate::utils::{IsAlive, Logical, Serial, Size, SERIAL_COUNTER}; -use crate::wayland::compositor; +use crate::wayland::compositor::{self, BufferAssignment, Cacheable, SurfaceAttributes}; +use crate::wayland::viewporter::{ViewportCachedState, ViewporterSurfaceState}; use _session_lock::ext_session_lock_surface_v1::{Error, ExtSessionLockSurfaceV1, Request}; +use tracing::trace_span; use wayland_protocols::ext::session_lock::v1::server::{self as _session_lock, ext_session_lock_surface_v1}; use wayland_server::protocol::wl_surface::WlSurface; use wayland_server::{Client, DataInit, Dispatch, DisplayHandle, Resource, Weak}; @@ -76,19 +79,33 @@ where .lock() .unwrap(); attributes.reset(); + + let mut guard = states.cached_state.get::(); + *guard.pending() = Default::default(); + *guard.current() = Default::default(); }); } } } +/// Data associated with session lock surface +/// +/// ```no_run +/// use smithay::wayland::compositor; +/// use smithay::wayland::session_lock::LockSurfaceData; +/// +/// # let wl_surface = todo!(); +/// compositor::with_states(&wl_surface, |states| { +/// states.data_map.get::(); +/// }); +/// ``` +pub type LockSurfaceData = Mutex; + /// Attributes for ext-session-lock surfaces. #[derive(Debug)] pub struct LockSurfaceAttributes { pub(crate) surface: ext_session_lock_surface_v1::ExtSessionLockSurfaceV1, - /// The serial of the last acked configure - pub configure_serial: Option, - /// Holds the pending state as set by the server. pub server_pending: Option, @@ -98,23 +115,19 @@ pub struct LockSurfaceAttributes { /// layer_surface.ack_configure. pub pending_configures: Vec, - /// Holds the last server_pending state that has been acknowledged by the - /// client. This state should be cloned to the current during a commit. - pub last_acked: Option, - - /// Holds the current state of the layer after a successful commit. - pub current: LockSurfaceState, + /// Holds the last configure that has been acknowledged by the client. This state should be + /// cloned to the current during a commit. Note that this state can be newer than the last + /// acked state at the time of the last commit. + pub last_acked: Option, } impl LockSurfaceAttributes { pub(crate) fn new(surface: ext_session_lock_surface_v1::ExtSessionLockSurfaceV1) -> Self { Self { surface, - configure_serial: None, server_pending: None, pending_configures: vec![], last_acked: None, - current: Default::default(), } } @@ -127,18 +140,22 @@ impl LockSurfaceAttributes { self.pending_configures .retain(|configure| configure.serial > serial); - self.last_acked = Some(configure.state); - self.configure_serial = Some(serial); + self.last_acked = Some(configure); Some(configure) } fn reset(&mut self) { - self.configure_serial = None; self.server_pending = None; self.pending_configures = Vec::new(); self.last_acked = None; - self.current = Default::default(); + } + + fn current_server_state(&self) -> Option<&LockSurfaceState> { + self.pending_configures + .last() + .map(|c| &c.state) + .or(self.last_acked.as_ref().map(|c| &c.state)) } } @@ -174,12 +191,8 @@ impl LockSurface { pub fn get_pending_state(&self, attributes: &mut LockSurfaceAttributes) -> Option { let server_pending = attributes.server_pending.take()?; - // Get the last pending state. - let pending = attributes.pending_configures.last(); - let last_state = pending.map(|c| &c.state).or(attributes.last_acked.as_ref()); - // Check if last state matches pending state. - match last_state { + match attributes.current_server_state() { Some(state) if state == &server_pending => None, _ => Some(server_pending), } @@ -230,26 +243,119 @@ impl LockSurface { let mut attributes = attributes.unwrap().lock().unwrap(); // Ensure pending state is initialized. - let current = attributes.current; - let server_pending = attributes.server_pending.get_or_insert(current); + if attributes.server_pending.is_none() { + attributes.server_pending = + Some(attributes.current_server_state().cloned().unwrap_or_default()); + } + let server_pending = attributes.server_pending.as_mut().unwrap(); f(server_pending) }) } - /// Get the current pending state. - #[allow(unused)] - pub fn current_state(&self) -> LockSurfaceState { + /// Provides access to the current committed cached state. + pub fn with_cached_state(&self, f: F) -> T + where + F: FnOnce(&LockSurfaceCachedState) -> T, + { compositor::with_states(&self.surface, |states| { - states - .data_map - .get::>() - .unwrap() - .lock() - .unwrap() - .current + let mut guard = states.cached_state.get::(); + f(guard.current()) }) } + + /// Provides access to the current committed state. + /// + /// This is the state that the client last acked before making the current commit. + pub fn with_committed_state(&self, f: F) -> T + where + F: FnOnce(Option<&LockSurfaceState>) -> T, + { + self.with_cached_state(move |state| f(state.last_acked.as_ref().map(|c| &c.state))) + } + + /// Handles the role specific commit error checking + /// + /// This should be called when the underlying WlSurface + /// handles a wl_surface.commit request. + pub(crate) fn pre_commit_hook(_state: &mut D, _dh: &DisplayHandle, surface: &WlSurface) { + let _span = trace_span!("session-lock-surface pre-commit", surface = %surface.id()).entered(); + + compositor::with_states(surface, |states| { + let role = states.data_map.get::().unwrap().lock().unwrap(); + + let Some(last_acked) = role.last_acked else { + role.surface.post_error( + ext_session_lock_surface_v1::Error::CommitBeforeFirstAck, + "Committed before the first ack_configure.", + ); + return; + }; + let LockSurfaceConfigure { state, serial: _ } = &last_acked; + + let mut guard_layer = states.cached_state.get::(); + let pending = guard_layer.pending(); + + // The presence of last_acked always follows the buffer assignment because the + // surface is not allowed to attach a buffer without acking the initial configure. + let had_buffer_before = pending.last_acked.is_some(); + + let mut guard_surface = states.cached_state.get::(); + let surface_attrs = guard_surface.pending(); + let has_buffer = match &surface_attrs.buffer { + Some(BufferAssignment::NewBuffer(buffer)) => { + // Verify buffer size. + if let Some(buf_size) = buffer_dimensions(buffer) { + let viewport = states + .data_map + .get::() + .map(|v| v.lock().unwrap()); + let surface_size = if let Some(dest) = viewport.as_ref().and_then(|_| { + let mut guard = states.cached_state.get::(); + let viewport_state = guard.pending(); + viewport_state.dst + }) { + Size::from((dest.w as u32, dest.h as u32)) + } else { + let scale = surface_attrs.buffer_scale; + let transform = surface_attrs.buffer_transform.into(); + let surface_size = buf_size.to_logical(scale, transform); + + Size::from((surface_size.w as u32, surface_size.h as u32)) + }; + + if Some(surface_size) != state.size { + role.surface.post_error( + ext_session_lock_surface_v1::Error::DimensionsMismatch, + "Surface dimensions do not match acked configure.", + ); + return; + } + } + + true + } + Some(BufferAssignment::Removed) => { + role.surface.post_error( + ext_session_lock_surface_v1::Error::NullBuffer, + "Surface attached a NULL buffer.", + ); + return; + } + None => had_buffer_before, + }; + + if has_buffer { + // The surface remains, or became mapped, track the last acked state. + pending.last_acked = Some(last_acked); + } else { + // The surface remains unmapped, meaning that it's in the initial configure stage. + pending.last_acked = None; + } + + // Lock surfaces aren't allowed to attach a null buffer, and therefore to unmap. + }); + } } /// State of an ext-session-lock surface. @@ -277,3 +383,21 @@ impl LockSurfaceConfigure { } } } + +/// Represents the client pending state +#[derive(Debug, Default, Copy, Clone)] +pub struct LockSurfaceCachedState { + /// Configure last acknowledged by the client at the time of the commit. + /// + /// Reset to `None` when the surface unmaps. + pub last_acked: Option, +} + +impl Cacheable for LockSurfaceCachedState { + fn commit(&mut self, _dh: &DisplayHandle) -> Self { + *self + } + fn merge_into(self, into: &mut Self, _dh: &DisplayHandle) { + *into = self; + } +} From 44f881a3ba6d97a22819d57efb837275bd20fb74 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 10:03:00 +0300 Subject: [PATCH 11/14] Add /wlcs to .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 0bf1ddb2ec3b..84f58efc62ff 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ Cargo.lock *.bk .vscode .vagga +/wlcs From 3b516122a593bf41b75b7cde85a80ca7eeff0ff1 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 14:01:36 +0300 Subject: [PATCH 12/14] Bump WLCS to v1.8.1 Contains fixes required for enforcing ack before commit as mandated by the protocols. --- compile_wlcs.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compile_wlcs.sh b/compile_wlcs.sh index b6d9ae3a8105..81760d871a5c 100755 --- a/compile_wlcs.sh +++ b/compile_wlcs.sh @@ -1,6 +1,6 @@ #!/bin/sh -WLCS_SHA=12234affdc0a4cc104fbaf8a502efc5f822b973b +WLCS_SHA=2b385328366de5db19521b9b3eef81861430d984 if [ -f "./wlcs/wlcs" ] && [ "$(cd wlcs; git rev-parse HEAD)" = "${WLCS_SHA}" ] ; then echo "Using cached WLCS." From 1efb6951bc9f4956a0bea399b760cc94874d9487 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sun, 7 Sep 2025 14:01:36 +0300 Subject: [PATCH 13/14] Add changelog entries for the protocol changes --- CHANGELOG.md | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index da4d3a59eb4b..61acfa10b03c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,132 @@ enables accepting dmabufs originating from any node. All other methods have moved to this new type, allowing compositors to take the same lock across multiple operations. +xdg_shell, layer_shell, and session_lock surfaces now track `last_acked` state for each commit. +You can find it in the `last_acked` field of the corresponding `...CachedState` structs. + +The "current state" and "current serial" attribute fields were removed; use `last_acked` from the new `with_committed_state()` accessors instead (and verify that you didn't actually want `with_pending_state()` instead where you used the "current state"). + +```diff +-ToplevelSurface::current_state(); ++ToplevelSurface::with_committed_state(|state| { ++ // ... ++}); + +-PopupSurface::current_state(); ++PopupSurface::with_committed_state(|state| { ++ // ... ++}); + +-LayerSurface::current_state(); ++LayerSurface::with_committed_state(|state| { ++ // ... ++}); + +-LockSurface::current_state(); ++LockSurface::with_committed_state(|state| { ++ // ... ++}); + +struct XdgToplevelSurfaceRoleAttributes { +- configured: bool, +- configure_serial: Option, +- current: ToplevelState, +- current_serial: Option, +- last_acked: Option, ++ last_acked: Option, + // ... +} + +struct XdgPopupSurfaceRoleAttributes { +- configured: bool, +- configure_serial: Option, +- current: PopupState, +- current_serial: Option, +- last_acked: Option, ++ last_acked: Option, + // ... +} + +struct LayerSurfaceAttributes { +- configured: bool, +- configure_serial: Option, +- current: LayerSurfaceState, +- last_acked: Option, ++ last_acked: Option, + // ... +} + +-impl Copy for LayerSurfaceCachedState; +``` + +The following methods are no longer needed as Smithay does them automatically now: +```diff +-ToplevelSurface::reset_initial_configure_sent(); +-PopupSurface::reset_initial_configure_sent(); +``` + +You also no longer need to manually set `LayerSurfaceAttributes::initial_configure_sent`, Smithay handles it automatically. + +### Additions + +xdg_shell and layer_shell now enforce the client acking a configure before committing a buffer, as required by the protocols. + +```rs +struct ToplevelCachedState { + /// Configure last acknowledged by the client at the time of the commit. + last_acked: Option, +} + +/// Provides access to the current committed cached state. +fn ToplevelSurface::with_cached_state(&self, f: impl FnOnce(&ToplevelCachedState) -> T) -> T; +/// Provides access to the current committed state. +fn ToplevelSurface::with_committed_state(&self, f: impl FnOnce(Option<&ToplevelState>) -> T) -> T; + +struct PopupCachedState { + /// Configure last acknowledged by the client at the time of the commit. + last_acked: Option, +} + +/// Provides access to the current committed cached state. +fn PopupSurface::with_cached_state(&self, f: impl FnOnce(&PopupCachedState) -> T) -> T; +/// Provides access to the current committed state. +fn PopupSurface::with_committed_state(&self, f: impl FnOnce(Option<&PopupState>) -> T) -> T; + +struct LayerSurfaceCachedState { + /// Configure last acknowledged by the client at the time of the commit. + last_acked: Option, + // ... +} + +/// Provides access to the current committed cached state. +fn LayerSurface::with_cached_state(&self, f: impl FnOnce(&LayerSurfaceCachedState) -> T) -> T; +/// Provides access to the current committed state. +fn LayerSurface::with_committed_state(&self, f: impl FnOnce(Option<&LayerSurfaceState>) -> T) -> T; + +struct LockSurfaceCachedState { + /// Configure last acknowledged by the client at the time of the commit. + last_acked: Option, +} + +/// Provides access to the current committed cached state. +fn LockSurface::with_cached_state(&self, f: impl FnOnce(&LockSurfaceCachedState) -> T) -> T; +/// Provides access to the current committed state. +fn LockSurface::with_committed_state(&self, f: impl FnOnce(Option<&LockSurfaceState>) -> T) -> T; + +struct LockSurfaceAttributes { + server_pending: Option, + pending_configures: Vec, + last_acked: Option, +} + +type LockSurfaceData = Mutex; + +struct LockSurfaceConfigure { + state: LockSurfaceState, + serial: Serial, +} +``` + ## 0.7.0 ### Breaking changes From 62da70bc846fcc0cb775df1eb7ee116ad7d56168 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Mon, 15 Sep 2025 13:50:59 +0300 Subject: [PATCH 14/14] wayland/shell/xdg: Delay grab handler until pre-commit Qt layer-shell calls grab() first, only then sets popup parent via zwlr_layer_surface_v1::get_popup(). Before this commit, grab() would call the handler immediately, which would see that the popup has no parent, and fail. However, it works on other compositors. The xdg_popup docs don't mention the desired behavior explicitly, though this line could be interpreted in favor of "applying" the grab request at commit time: > The client must call wl_surface.commit on the corresponding wl_surface for > the xdg_popup state to take effect. Fixes grabs from first-level popups on layer-shell Qt clients such as pcmanfm-qt --desktop and lxqt-panel. --- CHANGELOG.md | 1 + src/desktop/wayland/popup/manager.rs | 20 +----- .../shell/xdg/handlers/surface/popup.rs | 12 +++- src/wayland/shell/xdg/mod.rs | 66 ++++++++++++------- 4 files changed, 54 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61acfa10b03c..194b7985b61b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ struct XdgPopupSurfaceRoleAttributes { - configure_serial: Option, - current: PopupState, - current_serial: Option, +- committed: bool, - last_acked: Option, + last_acked: Option, // ... diff --git a/src/desktop/wayland/popup/manager.rs b/src/desktop/wayland/popup/manager.rs index c65676f14d85..56fe5fae99f2 100644 --- a/src/desktop/wayland/popup/manager.rs +++ b/src/desktop/wayland/popup/manager.rs @@ -9,7 +9,7 @@ use crate::{ }; use std::sync::{Arc, Mutex}; use tracing::trace; -use wayland_protocols::xdg::shell::server::{xdg_popup, xdg_wm_base}; +use wayland_protocols::xdg::shell::server::xdg_wm_base; use wayland_server::{protocol::wl_surface::WlSurface, Resource}; use super::{PopupGrab, PopupGrabError, PopupGrabInner, PopupKind}; @@ -74,23 +74,7 @@ impl PopupManager { ); match popup { - PopupKind::Xdg(ref xdg) => { - let surface = xdg.wl_surface(); - let committed = with_states(surface, |states| { - states - .data_map - .get::() - .unwrap() - .lock() - .unwrap() - .committed - }); - - if committed { - surface.post_error(xdg_popup::Error::InvalidGrab, "xdg_popup already is mapped"); - return Err(PopupGrabError::InvalidGrab); - } - } + PopupKind::Xdg(ref _xdg) => (), PopupKind::InputMethod(ref _input_method) => { return Err(PopupGrabError::InvalidGrab); } diff --git a/src/wayland/shell/xdg/handlers/surface/popup.rs b/src/wayland/shell/xdg/handlers/surface/popup.rs index 0942fd153a1d..08b50c4e72b2 100644 --- a/src/wayland/shell/xdg/handlers/surface/popup.rs +++ b/src/wayland/shell/xdg/handlers/surface/popup.rs @@ -4,7 +4,7 @@ use crate::{ input::SeatHandler, utils::Serial, wayland::{ - compositor, + compositor::{self, with_states}, shell::xdg::{PopupCachedState, SurfaceCachedState, XdgPopupSurfaceData, XdgPositionerUserData}, }, }; @@ -45,7 +45,15 @@ where let serial = Serial::from(serial); - XdgShellHandler::grab(state, handle, seat, serial); + with_states(handle.wl_surface(), |states| { + states + .data_map + .get::() + .unwrap() + .lock() + .unwrap() + .requested_grab = Some((seat, serial)); + }); } xdg_popup::Request::Reposition { positioner, token } => { let handle = crate::wayland::shell::xdg::PopupSurface { diff --git a/src/wayland/shell/xdg/mod.rs b/src/wayland/shell/xdg/mod.rs index 927360aaf413..0341ad317498 100644 --- a/src/wayland/shell/xdg/mod.rs +++ b/src/wayland/shell/xdg/mod.rs @@ -426,11 +426,6 @@ xdg_role!( /// for the xdg_popup state to take effect. #[derive(Debug)] XdgPopupSurfaceRoleAttributes { - // FIXME: "parent" should move into PopupCachedState, and "committed" should be removed in - // favor of something like PopupCachedState::last_acked.is_some(). Along the way, popup - // grabs should be made to apply at commit time, rather than immediately upon - // xdg_popup.grab(). This is required to fix Qt layer-shell popup grabs for example. - /// Holds the parent for the xdg_popup. /// /// The parent is allowed to remain unset as long @@ -443,12 +438,8 @@ xdg_role!( /// the xdg_popup role when no parent is set. pub parent: Option, - /// Defines if the surface has received at least one commit - /// - /// This can be used to check for protocol errors, like - /// checking if a popup requested a grab after it has been - /// mapped. - pub committed: bool + /// If the popup requested a grab, this is the seat and serial. + requested_grab: Option<(wl_seat::WlSeat, Serial)> }, /// Represents the xdg_popup pending state #[derive(Debug, Default, Clone)] @@ -1969,7 +1960,7 @@ impl PopupSurface { ) { let _span = trace_span!("xdg-popup pre-commit", surface = %surface.id()).entered(); - let error = compositor::with_states(surface, |states| { + let res = compositor::with_states(surface, |states| { let mut role = states .data_map .get::() @@ -1977,7 +1968,7 @@ impl PopupSurface { .lock() .unwrap(); if role.parent.is_none() { - return Some(( + return Err(( xdg_surface::Error::NotConstructed, "xdg_popup must have parent before mapping", )); @@ -1990,6 +1981,19 @@ impl PopupSurface { // surface is not allowed to attach a buffer without acking the initial configure. let had_buffer_before = pending.last_acked.is_some(); + let grab = role.requested_grab.take(); + if grab.is_some() && had_buffer_before { + let popups = &state.xdg_shell_state().known_popups; + if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) { + popup + .shell_surface + .post_error(xdg_popup::Error::InvalidGrab, "tried to grab after being mapped"); + } else { + error!("surface missing from known popups"); + } + return Ok(None); + } + let mut guard_surface = states.cached_state.get::(); let has_buffer = match &guard_surface.pending().buffer { Some(BufferAssignment::NewBuffer(_)) => true, @@ -2002,7 +2006,7 @@ impl PopupSurface { if has_buffer { let Some(last_acked) = role.last_acked else { - return Some(( + return Err(( xdg_surface::Error::UnconfiguredBuffer, "must ack the initial configure before attaching buffer", )); @@ -2031,19 +2035,31 @@ impl PopupSurface { *guard_popup.pending() = Default::default(); } - None + Ok(grab) }); - if let Some((error, msg)) = error { - let popups = &state.xdg_shell_state().known_popups; - if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) { - let data = popup - .shell_surface - .data::() - .unwrap(); - data.xdg_surface.post_error(error, msg); - } else { - error!("surface missing from known popups"); + match res { + Ok(Some((seat, serial))) => { + let popups = &state.xdg_shell_state().known_popups; + if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) { + let popup = popup.clone(); + XdgShellHandler::grab(state, popup, seat, serial); + } else { + error!("surface missing from known popups"); + } + } + Ok(None) => (), + Err((error, msg)) => { + let popups = &state.xdg_shell_state().known_popups; + if let Some(popup) = popups.iter().find(|handle| handle.wl_surface == *surface) { + let data = popup + .shell_surface + .data::() + .unwrap(); + data.xdg_surface.post_error(error, msg); + } else { + error!("surface missing from known popups"); + } } } }