From e20d49b0ee3dbd83499445d61eb2d65904d74311 Mon Sep 17 00:00:00 2001 From: srdusr <99972264+srdusr@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:25:00 +0200 Subject: Hide a window only when srdwm knows it has not drawn, not when a lookup says so Regression I introduced two commits ago, reported live: "I can click close where the button would normally be and it does close, but it is still invisible." The gate that stops an empty frame being drawn before a client paints asked the renderer, from inside the render loop, whether a window's surface had a buffer attached right now - and treated "no" as "do not draw". That question is only meaningful for a native xdg-shell toplevel. An XWayland window's surface state does not describe it the same way, so the answer came back no on every frame and the window was never drawn again, while srdwm's own hit-testing carried on working perfectly: an invisible window that still takes clicks, which is a worse failure than the empty frame it was meant to prevent. Inverted to the fail-safe direction. `new_managed_window` - the one path that creates a native toplevel - puts the window into `awaiting_first_buffer`, and `commit` takes it out on the first commit that carries a buffer. The render and capture paths test that set and nothing else. A window is now hidden only when srdwm itself put it there, so no window whose plumbing works differently can be hidden by a lookup that did not apply to it: the XWayland map path never touches the set, and neither can anything else. The buffer question still gets asked, but only in `commit`, about a surface it was just handed, where it is the right question. Verified both halves: an ordinary spawn still shows no frame before content (26 captured frames with content, 0 without), and the only way into the set is one line in one function. --- crates/wayland/src/protocols/compositor.rs | 6 ++-- crates/wayland/src/state/geometry.rs | 42 +++++++++++----------------- crates/wayland/src/state/lifecycle.rs | 18 ++++++------ crates/wayland/src/state/mod.rs | 45 ++++++++++++++++++------------ crates/wayland/src/udev/capture.rs | 2 +- crates/wayland/src/udev/platform.rs | 2 +- crates/wayland/src/udev/render.rs | 4 +-- crates/wayland/src/winit/capture.rs | 2 +- crates/wayland/src/winit/connect.rs | 2 +- crates/wayland/src/winit/render.rs | 2 +- 10 files changed, 64 insertions(+), 61 deletions(-) (limited to 'crates/wayland/src') diff --git a/crates/wayland/src/protocols/compositor.rs b/crates/wayland/src/protocols/compositor.rs index 7be5d7e..cf5a048 100644 --- a/crates/wayland/src/protocols/compositor.rs +++ b/crates/wayland/src/protocols/compositor.rs @@ -91,14 +91,14 @@ impl CompositorHandler for CompState { // against the guessed placeholder for one more round-trip. // The first commit that actually carries a buffer is when this // window becomes visible, so it is also when the open-slide - // should start - see `windows_shown_once`. Registered here + // should start - see `awaiting_first_buffer`. Registered here // rather than in `new_managed_window` because a role is created // well before a client paints (measured at ~800ms for a cold // terminal), which is long enough for the whole tween to finish // against an empty frame and for the window to simply appear, // already at rest, with no animation at all. - if !self.windows_shown_once.contains(&id) && self.window_has_content(id) { - self.windows_shown_once.insert(id); + if self.awaiting_first_buffer.contains(&id) && self.surface_has_buffer(id) { + self.awaiting_first_buffer.remove(&id); let mut wm = self.wm.borrow_mut(); if wm.animations_enabled { if let Some(win) = wm.window_mut(id) { diff --git a/crates/wayland/src/state/geometry.rs b/crates/wayland/src/state/geometry.rs index d3c7ab8..9780ae3 100644 --- a/crates/wayland/src/state/geometry.rs +++ b/crates/wayland/src/state/geometry.rs @@ -315,29 +315,25 @@ impl CompState { } /// True when this window has something on screen to draw a frame - /// around - it has committed a buffer at least once. + /// around, i.e. its client has painted at least once. /// - /// A toplevel is placed and decorated the moment its role is created, - /// which is well before the client paints. Drawing it then puts a - /// border, a titlebar and a shadow around bare desktop, at the guessed - /// placeholder size (`Window::size_is_provisional`), and that empty - /// frame then jumps when the real buffer arrives at the real size. - /// - /// A window that cannot be resolved to a surface at all counts as - /// drawable, deliberately: this hides a window only on positive - /// evidence that it has never drawn, so nothing whose surface plumbing - /// works differently (an XWayland window, say) can be hidden by a - /// lookup that simply did not apply to it. - /// - /// See `windows_shown_once` for why the answer latches once true. + /// Reads one set and nothing else - see `awaiting_first_buffer` for + /// why the answer must never depend on a lookup that can fail. Takes + /// the set rather than `&self` so a render loop can call it while it + /// already holds `self.udev` mutably borrowed. + pub(crate) fn has_content(awaiting_first_buffer: &HashSet, id: WindowId) -> bool { + !awaiting_first_buffer.contains(&id) + } + + /// Whether this window's own surface has a buffer attached right now. /// - /// Takes the two maps rather than `&self` so a render loop can call it - /// while it already holds `self.udev` mutably borrowed. - pub(crate) fn has_content(shown_once: &HashSet, id_to_window: &HashMap, id: WindowId) -> bool { - if shown_once.contains(&id) { - return true; - } - let Some(surface) = id_to_window.get(&id).and_then(crate::elements::window_wl_surface) else { return true }; + /// Only `CompositorHandler::commit` asks this, about a surface it was + /// just handed, to decide whether a window can stop + /// `awaiting_first_buffer`. Nothing on the render path may ask it: a + /// window whose surface state this does not describe would answer + /// "no" and be hidden for ever. + pub(crate) fn surface_has_buffer(&self, id: WindowId) -> bool { + let Some(surface) = self.id_to_window.get(&id).and_then(crate::elements::window_wl_surface) else { return false }; smithay::backend::renderer::utils::with_renderer_surface_state( &surface, |state: &mut smithay::backend::renderer::utils::RendererSurfaceState| state.buffer().is_some(), @@ -345,10 +341,6 @@ impl CompState { .unwrap_or(false) } - pub(crate) fn window_has_content(&self, id: WindowId) -> bool { - Self::has_content(&self.windows_shown_once, &self.id_to_window, id) - } - pub(crate) fn effective_frame(&self, id: WindowId, geom: srdwm_core::Rect) -> srdwm_core::Rect { Self::effective_frame_of(&self.wm, &self.id_to_window, &self.pending_size_configure, id, geom) } diff --git a/crates/wayland/src/state/lifecycle.rs b/crates/wayland/src/state/lifecycle.rs index b6c7f09..f2cddd9 100644 --- a/crates/wayland/src/state/lifecycle.rs +++ b/crates/wayland/src/state/lifecycle.rs @@ -19,13 +19,15 @@ impl CompState { if wm.window(id).is_some_and(|w| w.size_is_provisional) { self.provisional_size.insert(id); } - // The open-slide tween is NOT started here, deliberately. - // A toplevel role exists well before its client paints - // anything, so starting it here ran the animation against an - // empty frame and left the window simply appearing, already at - // rest. `CompositorHandler::commit` starts it at the first - // commit that carries a buffer instead - see - // `windows_shown_once`. + // Nothing is drawn for this window until its client paints, + // and the open-slide tween starts then rather than here - a + // toplevel role exists well before a client's first buffer, so + // starting the animation here ran it against an empty frame and + // left the window simply appearing, already at rest. + // `CompositorHandler::commit` does both. This is the only place + // anything is ever put into `awaiting_first_buffer`; see that + // field for why that matters. + self.awaiting_first_buffer.insert(id); id }; @@ -378,7 +380,7 @@ impl CompState { self.shadow_buffers.remove(&id); self.border_side_buffers.remove(&id); self.decoration_signatures.remove(&id); - self.windows_shown_once.remove(&id); + self.awaiting_first_buffer.remove(&id); self.last_synced_size.remove(&id); self.content_epoch.remove(&id); self.rounded_content_buffers.remove(&id); diff --git a/crates/wayland/src/state/mod.rs b/crates/wayland/src/state/mod.rs index 44ef772..64990c3 100644 --- a/crates/wayland/src/state/mod.rs +++ b/crates/wayland/src/state/mod.rs @@ -461,27 +461,36 @@ pub(crate) struct CompState { /// this set is ever consulted, so a surface only reaches the unmap path /// once it has legitimately shown something. pub(crate) layer_surfaces_shown_once: HashSet, - /// Windows whose surface has committed a buffer at least once. + /// Native Wayland toplevels that srdwm has created but whose client + /// has not yet drawn anything. /// - /// A toplevel exists, and is placed and decorated, from the moment its - /// role is created - which is well before the client has drawn - /// anything. Rendering it at that point paints a border and a titlebar - /// around empty desktop: an empty frame stands there on its own, then - /// snaps to a different size once the real buffer arrives and the - /// guessed `800x600` placeholder (`Window::size_is_provisional`) is - /// replaced. Measured in a nested session: the frame was drawn ~800ms - /// before any content, one full `TITLEBAR_HEIGHT` too tall, which is - /// what "the border corners look funny before a window spawns" is. + /// A toplevel is placed and decorated from the moment its role is + /// created, which is well before the client paints. Rendering it then + /// paints a border and a titlebar around empty desktop: an empty frame + /// stands there on its own, then snaps to a different size once the + /// real buffer arrives and the guessed `800x600` placeholder + /// (`Window::size_is_provisional`) is replaced. Measured in a nested + /// session: ~800ms of empty frame, one full `TITLEBAR_HEIGHT` too tall. /// - /// So this gates two things: nothing is drawn for a window that has - /// never had a buffer, and the open-slide starts at the first buffer - /// rather than at role creation, so the animation plays where it can - /// actually be seen instead of finishing against an empty frame. + /// This gates two things: nothing is drawn for a window still in this + /// set, and the open-slide starts when a window leaves it, so the + /// animation plays where it can be seen instead of finishing against + /// an empty frame. /// - /// Same shape, and the same reason, as `layer_surfaces_shown_once` - /// above: a window that has legitimately shown something once is never - /// hidden again by this, however its buffer state changes afterward. - pub(crate) windows_shown_once: HashSet, + /// Membership is the fail-safe direction, and that is the whole point + /// of the design. A window is hidden only when srdwm itself put it + /// here - `new_managed_window`, the one path that creates a native + /// toplevel - and `CompositorHandler::commit` takes it out again on + /// the first commit that carries a buffer. Nothing else can ever land + /// in it, so no window whose plumbing works differently can be hidden + /// by a lookup that did not apply to it. The first version of this + /// asked the renderer "does this surface have a buffer right now" from + /// inside the render loop, defaulting to hidden when the answer was + /// no; an XWayland window, whose surface state that lookup does not + /// describe, went invisible in the owner's live session while staying + /// clickable - reported as "I can click close where the button would + /// be and it does close, but it is still invisible". + pub(crate) awaiting_first_buffer: HashSet, pub(crate) decorations: HashMap, /// The top border strip's rounded-corner bitmap, cached the same way /// and at the same trigger points as `decorations` (built in diff --git a/crates/wayland/src/udev/capture.rs b/crates/wayland/src/udev/capture.rs index 287c003..310c9f7 100644 --- a/crates/wayland/src/udev/capture.rs +++ b/crates/wayland/src/udev/capture.rs @@ -71,7 +71,7 @@ impl CompState { for id in ids { // Matches the render loops: a capture must not show a frame the // screen does not (see `window_has_content`). - if !Self::has_content(&self.windows_shown_once, &self.id_to_window, id) { + if !Self::has_content(&self.awaiting_first_buffer, id) { continue; } let Some(w) = self.id_to_window.get(&id) else { continue }; diff --git a/crates/wayland/src/udev/platform.rs b/crates/wayland/src/udev/platform.rs index 86307dc..d3c091c 100644 --- a/crates/wayland/src/udev/platform.rs +++ b/crates/wayland/src/udev/platform.rs @@ -256,7 +256,7 @@ impl UdevPlatform { dead_layer_surfaces: HashSet::new(), hidden_layer_surfaces: HashMap::new(), layer_surfaces_shown_once: HashSet::new(), - windows_shown_once: HashSet::new(), + awaiting_first_buffer: HashSet::new(), decorations: HashMap::new(), border_top_decorations: HashMap::new(), border_bottom_decorations: HashMap::new(), diff --git a/crates/wayland/src/udev/render.rs b/crates/wayland/src/udev/render.rs index a2cf367..cefd759 100644 --- a/crates/wayland/src/udev/render.rs +++ b/crates/wayland/src/udev/render.rs @@ -308,7 +308,7 @@ impl CompState { // stand around empty desktop until the client paints, // then jump when the placeholder size is replaced. See // `window_has_content`. - if !Self::has_content(&self.windows_shown_once, &self.id_to_window, id) { + if !Self::has_content(&self.awaiting_first_buffer, id) { continue; } let Some(w) = self.wm.borrow().window(id).cloned() else { continue }; @@ -558,7 +558,7 @@ impl CompState { for &id in &ids { // See the GPU loop above: a window with no buffer yet has // nothing for a frame to go around. - if !Self::has_content(&self.windows_shown_once, &self.id_to_window, id) { + if !Self::has_content(&self.awaiting_first_buffer, id) { continue; } let Some(w) = self.wm.borrow().window(id).cloned() else { continue }; diff --git a/crates/wayland/src/winit/capture.rs b/crates/wayland/src/winit/capture.rs index 6e03dd8..5eeabe1 100644 --- a/crates/wayland/src/winit/capture.rs +++ b/crates/wayland/src/winit/capture.rs @@ -100,7 +100,7 @@ impl WaylandPlatform { for id in self.wm.borrow().visible_windows_front_to_back().map(|w| w.id).collect::>() { // Matches the render loops: a capture must not show a frame the // screen does not (see `window_has_content`). - if !crate::state::CompState::has_content(&self.state.windows_shown_once, &self.state.id_to_window, id) { + if !crate::state::CompState::has_content(&self.state.awaiting_first_buffer, id) { continue; } let Some(w) = self.wm.borrow().window(id).cloned() else { continue }; diff --git a/crates/wayland/src/winit/connect.rs b/crates/wayland/src/winit/connect.rs index cca991c..1f2f876 100644 --- a/crates/wayland/src/winit/connect.rs +++ b/crates/wayland/src/winit/connect.rs @@ -168,7 +168,7 @@ impl WaylandPlatform { dead_layer_surfaces: HashSet::new(), hidden_layer_surfaces: HashMap::new(), layer_surfaces_shown_once: HashSet::new(), - windows_shown_once: HashSet::new(), + awaiting_first_buffer: HashSet::new(), decorations: HashMap::new(), border_top_decorations: HashMap::new(), border_bottom_decorations: HashMap::new(), diff --git a/crates/wayland/src/winit/render.rs b/crates/wayland/src/winit/render.rs index 02ee322..3138d45 100644 --- a/crates/wayland/src/winit/render.rs +++ b/crates/wayland/src/winit/render.rs @@ -237,7 +237,7 @@ impl WaylandPlatform { let mut occluders: Vec = Vec::with_capacity(ids.len()); for id in ids { // See `window_has_content`: no buffer yet means no frame yet. - if !crate::state::CompState::has_content(&self.state.windows_shown_once, &self.state.id_to_window, id) { + if !crate::state::CompState::has_content(&self.state.awaiting_first_buffer, id) { continue; } let Some(w) = self.wm.borrow().window(id).cloned() else { continue }; -- cgit v1.2.3