Fix Wayland layout shift from wl-copy dummy windows#243
Fix Wayland layout shift from wl-copy dummy windows#243paperbenni wants to merge 1 commit intomainfrom
Conversation
The recent fix for GTK windows (sending an initial configure unconditionally) caused clipboard utilities like `wl-copy` to respond by committing a tiny 1x1 dummy buffer to gain keyboard focus. Because this dummy buffer was treated as a normal tiled window, it momentarily participated in the layout and caused a spurious layout shift before exiting. This patch fixes the issue by explicitly floating `wl-clipboard` windows in `apply_rules`, which prevents them from being tiled. It also adds a fallback check in `handlers.rs` to automatically float any window that commits a dummy buffer size of 1x1 or smaller, ensuring similar utilities don't cause layout shifts in the future. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideMarks wl-clipboard dummy Wayland toplevels as floating to prevent tiny 1x1 focus windows from disrupting the tiling layout, using both a static client rule and a dynamic size-based fallback when mapping new toplevels. Sequence diagram for Wayland toplevel mapping with dummy wl-clipboard window floatingsequenceDiagram
participant Client as WlClipboardClient
participant Compositor as WaylandState
participant Surface as WlSurface
participant Renderer as RendererSurfaceState
participant Globals as Globals
Client->>Compositor: create xdg_toplevel (wl-clipboard dummy)
Client->>Surface: commit 1x1 transparent buffer
Compositor->>Surface: check has_buffer
Surface-->>Compositor: has_buffer = true
Compositor->>Renderer: with_renderer_surface_state(surface)
Renderer-->>Compositor: surface_size = (w <= 1, h <= 1)
Compositor->>Compositor: toplevel = pending_toplevels.swap_remove(pos)
Compositor->>Compositor: win = map_new_toplevel(toplevel)
alt surface_size <= 1x1
Compositor->>Globals: globals_mut()
Globals-->>Compositor: Some(globals)
Compositor->>Globals: clients.get_mut(win)
Globals-->>Compositor: Some(client)
Compositor->>Globals: client.is_floating = true
end
Compositor->>Client: focus dummy floating window
Client->>Compositor: clipboard operation completes
Client->>Compositor: destroy dummy window
Class diagram for Globals, client rules, and Wayland toplevel floating logicclassDiagram
class Globals {
+HashMap~WindowId, Client~ clients
+Behavior behavior
+Option~Globals~ globals_mut()
}
class Behavior {
+bool specialnext
}
class Client {
+String name
+bool is_floating
}
class WindowProperties {
+String class
+String title
}
class WindowId
class WaylandState {
+Vec~Toplevel~ pending_toplevels
+Option~Globals~ globals_mut()
+WindowId map_new_toplevel(Toplevel toplevel)
}
class Toplevel
class SurfaceState {
+Option~Size~ surface_size()
}
class Size {
+i32 w
+i32 h
}
class RendererSurfaceUtils {
+static with_renderer_surface_state(Surface surface, fn callback)
}
class Surface
Globals "1" --> "*" Client : clients
Globals "1" --> "1" Behavior : behavior
WaylandState "1" --> "*" Toplevel : pending_toplevels
WaylandState ..> Globals : uses globals_mut
WaylandState ..> RendererSurfaceUtils : uses with_renderer_surface_state
RendererSurfaceUtils ..> SurfaceState : callback receives
SurfaceState --> Size : surface_size
class RulesModule {
+apply_rules(Globals g, WindowId win, WindowProperties props)
}
RulesModule ..> Globals : modifies clients
RulesModule ..> WindowProperties : reads class, title
RulesModule ..> WindowId
RulesModule ..> Client : sets is_floating
%% apply_rules logic
class ApplyRulesLogic {
+void apply_rules(Globals g, WindowId win, WindowProperties props)
}
ApplyRulesLogic ..> Client : set name from title
ApplyRulesLogic ..> Client : set is_floating when wl_clipboard
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughTwo modifications implement floating window detection: in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- You now have two separate mechanisms to special-case dummy windows (hardcoded wl-clipboard rule and the 1x1-size heuristic); consider centralizing this logic (e.g. a helper or a dedicated predicate) to avoid divergence in behavior over time.
- The size-based heuristic uses
<= 1x1withunwrap_or((0, 0)), which conflates ‘no size yet’ and ‘dummy 1x1 window’; if that distinction matters, you might want to treatNoneor0x0separately from an explicit 1x1 buffer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You now have two separate mechanisms to special-case dummy windows (hardcoded wl-clipboard rule and the 1x1-size heuristic); consider centralizing this logic (e.g. a helper or a dedicated predicate) to avoid divergence in behavior over time.
- The size-based heuristic uses `<= 1x1` with `unwrap_or((0, 0))`, which conflates ‘no size yet’ and ‘dummy 1x1 window’; if that distinction matters, you might want to treat `None` or `0x0` separately from an explicit 1x1 buffer.
## Individual Comments
### Comment 1
<location path="src/backend/wayland/compositor/handlers.rs" line_range="85-86" />
<code_context>
})
.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))
+ }).unwrap_or((0, 0));
+
</code_context>
<issue_to_address>
**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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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)) |
There was a problem hiding this comment.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/wayland/compositor/handlers.rs`:
- Around line 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).
In `@src/client/rules.rs`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1c1aa5b-31fa-4a22-95a7-1fed1aa5a33f
📒 Files selected for processing (2)
src/backend/wayland/compositor/handlers.rssrc/client/rules.rs
| 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)); | ||
|
|
||
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 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:
- 1: https://docs.rs/smithay/latest/src/smithay/backend/renderer/element/surface.rs.html
- 2: https://smithay.github.io/smithay/smithay/backend/renderer/element/surface/struct.WaylandSurfaceRenderElement.html
- 3: https://smithay.github.io/smithay/smithay/wayland/compositor/struct.SurfaceAttributes.html
🏁 Script executed:
# First, let's view the exact code at lines 85-102
sed -n '75,110p' src/backend/wayland/compositor/handlers.rs | cat -nRepository: 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 -nRepository: 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 -50Repository: 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).
| // 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; | ||
| } |
There was a problem hiding this comment.
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.
1 participant
The previous GTK window visibility fix caused
wl-copyto break the layout by spawning a tiny tiled dummy window.When an initial
send_configureis fired,wl-copy(and potentially other clipboard utilities) responds by attaching a 1x1 transparent buffer to an XDG toplevel. It does this solely to gain keyboard focus. Because instantWM previously treated this as a standard tiled window, it disrupted the layout momentarily before the tool finished and exited.This pull request resolves the problem in two ways:
src/client/rules.rsto immediately set any window with class/title"wl-clipboard"to floating.src/backend/wayland/compositor/handlers.rsthat checks the size of the initial committed buffer viawith_renderer_surface_state. If the buffer dimensions are<= 1x1, it marks the window as floating.Floating windows bypass the tiling arrangement algorithm, so
wl-copycan now gain focus and work correctly without causing any layout flicker.PR created automatically by Jules for task 757461423317328864 started by @paperbenni
Summary by Sourcery
Prevent tiny Wayland clipboard helper windows from disrupting tiling layouts by treating them as floating.
Bug Fixes:
Enhancements:
Summary by CodeRabbit