fix(wayland): explicitly restack XWayland surfaces in WaylandState::restack#247
Conversation
Added `xwm.raise_window(surface)` inside `WaylandState::restack` to ensure that XWayland windows visually update their Z-order when focus changes occur in layouts like monocle. Without this, Smithay would internally reorder the surfaces but X11 clients would continue compositing their buffers based on stale X11 stacking orders. 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 GuideAdds an explicit XWayland/X11 restack when raising elements in WaylandState::restack so that XWayland windows visually reflect focus changes, particularly in monocle layouts. Sequence diagram for WaylandState restack with explicit XWayland restacksequenceDiagram
actor User
participant Compositor
participant WaylandState
participant Space
participant Element
participant Xwm
User->>Compositor: request_focus_change(target_window)
Compositor->>WaylandState: restack()
loop for_each_element_in_new_stack
WaylandState->>Space: raise_element(element, false)
activate Element
WaylandState->>Element: x11_surface()
alt element_has_x11_surface_and_xwm_present
Element-->>WaylandState: X11Surface
WaylandState->>Xwm: raise_window(X11Surface)
Xwm-->>WaylandState: result
else
Element-->>WaylandState: None
end
deactivate Element
end
WaylandState->>Xwm: raise_unmanaged_x11_windows()
Xwm-->>WaylandState: done
WaylandState-->>Compositor: restack_complete
Compositor-->>User: focused_window_visually_raised
Class diagram for updated WaylandState restack XWayland integrationclassDiagram
class WaylandState {
+Space space
+Option_Xwm xwm
+restack()
+raise_unmanaged_x11_windows()
}
class Space {
+raise_element(element, activate)
}
class Element {
+x11_surface() Option_X11Surface
}
class Xwm {
+raise_window(surface)
+raise_unmanaged_x11_windows()
}
class Option_Xwm
class Option_X11Surface
class X11Surface
WaylandState --> Space : has
WaylandState --> Option_Xwm : has
Space --> Element : manages
Element --> Option_X11Surface : may_expose
Option_X11Surface --> X11Surface : wraps
Option_Xwm --> Xwm : wraps
WaylandState ..> Xwm : uses_raise_window
WaylandState ..> Element : iterates_in_restack
WaylandState ..> X11Surface : passes_to_raise_window
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request enhances error handling in Wayland window management functions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ 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 left some high level feedback:
- Right now the result of
xwm.raise_window(surface)is silently ignored; consider at least logging failures so unexpected XWayland restack errors can be diagnosed if the visual stacking issue reappears.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Right now the result of `xwm.raise_window(surface)` is silently ignored; consider at least logging failures so unexpected XWayland restack errors can be diagnosed if the visual stacking issue reappears.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@jules Overall Comments
|
Good point. I have updated the |
Updated `WaylandState::restack` to explicitly restack X11 surfaces via `xwm.raise_window(surface)`. Also added error logging when this restack request fails, which will assist with diagnosing any future XWayland depth/stacking-related visual bugs in overlapping or monocle layouts. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
paperbenni
deleted the
fix-xwayland-restack-monocle-17793732228212661052
branch
1 participant
This commit fixes an issue where switching focus (e.g.
super j/super k) in a monocle layout under Wayland would sometimes fail to visually raise the focused window. The problem was caused by a missing explicit X11 restack call for XWayland surfaces insideWaylandState::restack, which allowed the embedded X server to keep compositing windows with an outdated stacking order.PR created automatically by Jules for task 17793732228212661052 started by @paperbenni
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit