Skip to content

Fix Wayland layout shift from wl-copy dummy windows #243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paperbenni wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix Wayland layout shift from wl-copy dummy windows #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/backend/wayland/compositor/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,25 @@ impl CompositorHandler for WaylandState {
state.buffer().is_some()
})
.unwrap_or(false);

let (w, h) = smithay::backend::renderer::utils::with_renderer_surface_state(surface, |state| {
state.surface_size().map(|s| (s.w, s.h)).unwrap_or((0, 0))
Comment on lines +85 to +86
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Avoid treating an unknown surface size as 0x0 to prevent misclassifying normal windows as dummy ones.

unwrap_or((0, 0)) makes an unknown size indistinguishable from a real 0x0 surface. As a result, any surface whose size isn’t known yet will appear as (w, h) = (0, 0) and may be treated as a dummy window (w <= 1 && h <= 1), incorrectly forcing real windows into floating mode. Please propagate Option<(u32, u32)> (or equivalent) and only apply the dummy-window heuristic when the size is known and small.

}).unwrap_or((0, 0));

if has_buffer {
let toplevel = self.pending_toplevels.swap_remove(pos);
let _ = self.map_new_toplevel(toplevel);
let win = self.map_new_toplevel(toplevel);

// If the requested dimensions are 1x1 or 0x0, it's likely a dummy
// window (e.g. some clipboard tools use this to gain focus).
// Force it to float so it doesn't cause a layout shift.
if w <= 1 && h <= 1 {
if let Some(g) = self.globals_mut() {
if let Some(client) = g.clients.get_mut(&win) {
client.is_floating = true;
}
}
}
Comment on lines +85 to +102
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Smithay, under what conditions can with_renderer_surface_state(surface, ...)orstate.surface_size()returnNoneduring a commit wherestate.buffer().is_some() is true?

💡 Result:

In Smithay, with_renderer_surface_state(surface, ...) returns None if the RendererSurfaceStateUserData is not present in the surface's SurfaceData data_map. This user data is typically inserted by the compositor during surface commit processing if a buffer is attached and successfully imported into a texture by the renderer (via ImportAll trait methods like import_shm_buffer, import_egl_buffer, etc.). Even if state.buffer.is_some is true (indicating a valid buffer attachment), the user data (and thus the RendererSurfaceState) may be absent if: - The compositor failed to retrieve or insert the RendererSurfaceStateUserData during commit (e.g., due to a rendering/import error not propagated as a protocol error). - The surface state was not fully processed for rendering in this commit cycle (e.g., custom compositor logic skips renderer state update despite buffer presence). - The buffer import failed silently or the texture creation errored, preventing state population. Similarly, state.surface_size returns None if the SurfaceView is invalid or missing, which occurs if the surface has no valid destination geometry defined (e.g., unset or zero-sized window geometry from shell protocols like xdg_shell during initial commits before configure acknowledgment). The SurfaceView relies on committed surface attributes (size from buffer or shell geometry), so during commits where state.buffer.is_some but the surface lacks mapped geometry (e.g., pre-configure), data.view? fails, leading to None. These conditions are evident during Wayland surface commits where a buffer is attached (SurfaceAttributes.buffer.is_some), but rendering state or geometry is incomplete. Smithay's WaylandSurfaceRenderElement::from_surface explicitly returns None without the user data or if data.view fails, mirroring these checks. Compositors must handle None by skipping rendering for that surface in the frame.

Citations:

🏁 Script executed:

# First, let's view the exact code at lines 85-102
sed -n '75,110p' src/backend/wayland/compositor/handlers.rs | cat -n

Repository: instantOS/instantWM

Length of output: 1807

🏁 Script executed:

# Let's also check the broader context to understand has_buffer and map_new_toplevel
sed -n '50,120p' src/backend/wayland/compositor/handlers.rs | cat -n

Repository: instantOS/instantWM

Length of output: 3247

🏁 Script executed:

# Check if there's any validation of surface state before the problematic code
rg -B 10 "with_renderer_surface_state" src/backend/wayland/compositor/handlers.rs | head -50

Repository: instantOS/instantWM

Length of output: 740

Avoid coercing unknown surface size to (0, 0) in dummy-window detection.

Even when has_buffer is true, state.surface_size() can return None if the surface lacks valid destination geometry (e.g., during pre-configure state or before window geometry is set). Lines 36–38 treat this case as (0, 0), causing line 47 to misclassify real windows as dummy windows.

The heuristic should only apply when size is actually known:

Proposed fix 
-            let (w, h) = smithay::backend::renderer::utils::with_renderer_surface_state(surface, |state| {
-                state.surface_size().map(|s| (s.w, s.h)).unwrap_or((0, 0))
-            }).unwrap_or((0, 0));
+            let surface_size =
+                smithay::backend::renderer::utils::with_renderer_surface_state(surface, |state| {
+                    state.surface_size().map(|s| (s.w, s.h))
+                })
+                .flatten();
 
             if has_buffer {
                 let toplevel = self.pending_toplevels.swap_remove(pos);
                 let win = self.map_new_toplevel(toplevel);
 
                 // If the requested dimensions are 1x1 or 0x0, it's likely a dummy
                 // window (e.g. some clipboard tools use this to gain focus).
                 // Force it to float so it doesn't cause a layout shift.
-                if w <= 1 && h <= 1 {
+                if let Some((w, h)) = surface_size
+                    && w <= 1
+                    && h <= 1
+                {
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/backend/wayland/compositor/handlers.rs` around lines 85 - 102, The
dummy-window detection currently treats unknown surface sizes as (0,0) by
unwrapping the result of with_renderer_surface_state, which can misclassify real
windows; change the logic so you call with_renderer_surface_state and inspect
the Option returned by state.surface_size() directly and only run the `if w <= 1
&& h <= 1 { ... }` block when that Option is Some((w,h)); keep the early flow
(checking has_buffer, swap_remove on self.pending_toplevels, map_new_toplevel)
the same but move or guard the floating-flag mutation (access via
self.globals_mut() and g.clients.get_mut(&win).is_floating) behind a check that
surface_size returned Some, rather than treating None as (0,0).

}
// No else: the surface stays in pending_toplevels until it
// either commits a buffer or is destroyed.
Expand Down
5 changes: 5 additions & 0 deletions src/client/rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ pub fn apply_rules(g: &mut Globals, win: WindowId, props: &WindowProperties) {
if !props.title.is_empty() {
c.name = props.title.clone();
}
// wl-clipboard (wl-copy) creates dummy windows just to get keyboard focus.
// Float them immediately so they don't cause layout shifts.
if props.class == "wl-clipboard" || props.title == "wl-clipboard" {
c.is_floating = true;
}
Comment on lines +35 to +39
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

wl-clipboard floating override can be undone by later rules.

Line 37-Line 39 sets is_floating = true, but later matched rules can reset it to tiled (RuleFloat::Tiled), so this is not a guaranteed override.

💡 Proposed fix (make the override final) 
 pub fn apply_rules(g: &mut Globals, win: WindowId, props: &WindowProperties) {
+    let force_wl_clipboard_float =
+        props.class == "wl-clipboard" || props.title == "wl-clipboard";
+
     // --- Initialise fields we are about to set -------------------------------
     if let Some(c) = g.clients.get_mut(&win) {
-        c.is_floating = false;
+        c.is_floating = force_wl_clipboard_float;
         c.tags = 0;
         // Also update the client name from properties if it's not empty
         if !props.title.is_empty() {
             c.name = props.title.clone();
         }
-        // wl-clipboard (wl-copy) creates dummy windows just to get keyboard focus.
-        // Float them immediately so they don't cause layout shifts.
-        if props.class == "wl-clipboard" || props.title == "wl-clipboard" {
-            c.is_floating = true;
-        }
     }
@@
     }
 
+    // Keep wl-clipboard windows floating even if later rules requested tiled.
+    if force_wl_clipboard_float
+        && let Some(c) = g.clients.get_mut(&win)
+    {
+        c.is_floating = true;
+    }
+
     // --- Clamp tags to the valid tag mask ------------------------------------
     clamp_client_tags(g, win, tag_mask);
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/client/rules.rs` around lines 35 - 39, The wl-clipboard floating flag is
currently reversible because later rule processing can set RuleFloat::Tiled;
change the wl-clipboard special-case so it not only sets c.is_floating = true
but also marks this decision as final so subsequent rule application skips
overriding it (e.g., set a new boolean like c.rule_floating_override = true or
set c.rule_floating = Some(RuleFloat::FloatingFinal)); then update the
rule-application logic that applies RuleFloat (the code that inspects
RuleFloat::Tiled/Floating) to respect that override flag and no-op when the
override is present.

}

let special_next = g.behavior.specialnext;
Expand Down
Loading