Fix cursor visibility over windows in winit backend#236
Conversation
Move `render_cursor_overlays` before `assemble_scene_elements!` in the winit backend renderer so that the cursor elements are pushed into the render elements vector first. Smithay processes these front-to-back, so appending them first ensures the cursor is drawn on top of windows instead of being hidden behind them. Also slightly increased the vector capacity pre-allocation to account for the cursor elements. 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 guide (collapsed on small PRs)Reviewer's GuideAdjusts the winit Wayland backend’s render ordering so cursor overlays are pushed into the render_elements list before scene/window elements, ensuring cursors are drawn on top, and slightly increases the pre-allocation capacity to account for the extra elements. Sequence diagram for updated winit render_frame cursor orderingsequenceDiagram
participant RenderFrame
participant Renderer
participant CursorPresentation
participant SceneAssembler
participant DamageTracker
RenderFrame->>RenderFrame: get_render_element_counts(scene, space_render_elements_len, num_upper)
RenderFrame->>RenderFrame: render_elements = Vec_with_capacity(counts_total_plus_2)
RenderFrame->>Renderer: render_cursor_overlays(renderer, cursor_presentation, pointer_current_location, render_elements_mut)
Renderer->>CursorPresentation: read_cursor_and_dnd_surfaces
CursorPresentation-->>Renderer: cursor_overlay_elements
Renderer-->>RenderFrame: append_cursor_overlays_to_render_elements
RenderFrame->>SceneAssembler: assemble_scene_elements(scene, space_render_elements, num_upper, render_elements_mut)
SceneAssembler-->>RenderFrame: append_window_and_scene_elements
RenderFrame->>DamageTracker: render_output(renderer, render_elements, output, age)
DamageTracker-->>RenderFrame: render_result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughReorders cursor overlay rendering in Wayland window management to execute before scene assembly instead of after. Increases render_elements pre-allocation capacity by 2 and repositions the explicit backend-specific overlay rendering call, modifying control flow for z-order handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 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)
📝 Coding Plan
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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![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 left some high level feedback:
- The
+ 2inVec::with_capacity(counts.total() + 2)is a bit of a magic number; consider either deriving this from the number of potential cursor overlays or adding a short comment explaining why2is sufficient. - Since this change relies on cursor elements being processed first in
render_elementsfor correct z-ordering, consider adding a brief comment nearrender_cursor_overlaysexplaining that it must be called beforeassemble_scene_elements!to keep cursors on top.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `+ 2` in `Vec::with_capacity(counts.total() + 2)` is a bit of a magic number; consider either deriving this from the number of potential cursor overlays or adding a short comment explaining why `2` is sufficient.
- Since this change relies on cursor elements being processed first in `render_elements` for correct z-ordering, consider adding a brief comment near `render_cursor_overlays` explaining that it must be called before `assemble_scene_elements!` to keep cursors on top.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/wayland/render/winit.rs (1)
62-62: Consider making the cursor capacity hint data-driven.
+ 2at Line 62 is safe, but surface-tree cursors can produce more than two elements. If you want to avoid avoidable reallocations on hot paths, prebuild overlays into a temporary vec and sizerender_elementswithcounts.total() + cursor_elements.len().♻️ Optional refactor
- let mut render_elements = Vec::with_capacity(counts.total() + 2); - - // Backend-specific: render cursor overlays (client surface cursors and DnD icons) - render_cursor_overlays( - renderer, - &cursor_presentation, - state.pointer.current_location(), - &mut render_elements, - ); + let mut cursor_elements = Vec::new(); + // Backend-specific: render cursor overlays (client surface cursors and DnD icons) + render_cursor_overlays( + renderer, + &cursor_presentation, + state.pointer.current_location(), + &mut cursor_elements, + ); + let mut render_elements = + Vec::with_capacity(counts.total() + cursor_elements.len()); + render_elements.extend(cursor_elements);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wayland/render/winit.rs` at line 62, The allocation for render_elements uses a fixed "+ 2" buffer which can under-estimate surface-tree cursor overlays; before creating render_elements, prebuild the cursor/overlay elements into a temporary Vec (e.g., cursor_elements) and then allocate render_elements with Vec::with_capacity(counts.total() + cursor_elements.len()), then extend or append cursor_elements into render_elements when populating; update the code around the render_elements allocation (the render_elements variable and any cursor/overlay construction logic) to compute cursor_elements first and use its length for the capacity hint to avoid unnecessary reallocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/wayland/render/winit.rs`:
- Line 62: The allocation for render_elements uses a fixed "+ 2" buffer which
can under-estimate surface-tree cursor overlays; before creating
render_elements, prebuild the cursor/overlay elements into a temporary Vec
(e.g., cursor_elements) and then allocate render_elements with
Vec::with_capacity(counts.total() + cursor_elements.len()), then extend or
append cursor_elements into render_elements when populating; update the code
around the render_elements allocation (the render_elements variable and any
cursor/overlay construction logic) to compute cursor_elements first and use its
length for the capacity hint to avoid unnecessary reallocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8da9ae1-e518-4ba2-af80-4234a021718d
📒 Files selected for processing (1)
src/wayland/render/winit.rs
1 participant
This PR fixes an issue where the mouse cursor becomes invisible when hovering over windows in the
winit(nested) Wayland backend.The root cause was that in
src/wayland/render/winit.rs, the cursor elements were being appended to therender_elementsarray after the window and scene elements were appended. Because Smithay processes these render elements in a front-to-back (top-to-bottom) order, this meant the cursor was being rendered at the bottom of the z-index stack, behind the windows.By moving the call to
render_cursor_overlaysto happen before theassemble_scene_elements!macro, the cursor elements are now the first ones in the array, correctly rendering them on top of everything else. The array capacity pre-allocation was also slightly bumped (+2) to account for these extra elements.PR created automatically by Jules for task 10457518865414828820 started by @paperbenni
Summary by Sourcery
Ensure cursors render above windows in the winit Wayland backend by adjusting render element ordering and capacity preallocation.
Bug Fixes:
Enhancements:
Summary by CodeRabbit