Conversation
…lications that request an exclusive keyboard grab right after mapping would often fail because their surface hasn't been assigned to a managed window yet. Now `keyboard_focus_for_xsurface` also searches `self.space.elements()` for unmanaged surfaces. 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 Wayland XWaylandKeyboardGrabHandler focus resolution to fall back to scanning space elements for the wl_surface when the window_index lookup fails, preventing dmenu crashes when its unmanaged overlay window is not yet indexed. Sequence diagram for updated XWayland keyboard grab focus resolutionsequenceDiagram
actor DmenuProcess
participant XWayland as XWaylandServer
participant Compositor as WaylandState
participant WindowIndex as window_index
participant Space as space
DmenuProcess->>XWayland: Request keyboard grab
XWayland->>Compositor: keyboard_focus_for(surface)
alt Surface is indexed window
Compositor->>WindowIndex: get(win_id_for_surface(surface))
WindowIndex-->>Compositor: Window
Compositor-->>XWayland: KeyboardFocusTarget_Window
XWayland-->>DmenuProcess: Keyboard grab granted
else Surface is unmanaged overlay (not in window_index)
Compositor->>WindowIndex: get(win_id_for_surface(surface))
WindowIndex-->>Compositor: None
Compositor->>Space: elements()
Space-->>Compositor: iterator over windows
Compositor->>Space: find window with wl_surface == surface
Space-->>Compositor: Window (cloned)
Compositor-->>XWayland: KeyboardFocusTarget_Window
XWayland-->>DmenuProcess: Keyboard grab granted (no crash)
end
Class diagram for updated WaylandState XWaylandKeyboardGrabHandler implementationclassDiagram
class WaylandState {
window_index
space
keyboard_focus_for(surface: WlSurface) Option_KeyboardFocus
}
class XWaylandKeyboardGrabHandler {
<<interface>>
keyboard_focus_for(surface: WlSurface) Option_KeyboardFocus
}
class KeyboardFocusTarget {
Window
}
class Window {
wl_surface() WlSurface
}
class Space {
elements() Iterator_Window
}
WaylandState ..|> XWaylandKeyboardGrabHandler
WaylandState o--> Space
WaylandState o--> WindowIndex
WindowIndex --> Window
Space --> Window
KeyboardFocusTarget --> Window
class WindowIndex {
get(win_id: WindowId) Window
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 new fallback search over
self.space.elements()introduces a linear scan on every keyboard grab; consider whether this handler is called frequently enough to warrant caching or a helper that avoids repeated full scans. - The early-return logic in
keyboard_focus_for_surfacecould be simplified by factoring the two lookup steps into a small helper (e.g.,window_for_surface) to keep the control flow tighter and avoid duplicated lookup logic in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new fallback search over `self.space.elements()` introduces a linear scan on every keyboard grab; consider whether this handler is called frequently enough to warrant caching or a helper that avoids repeated full scans.
- The early-return logic in `keyboard_focus_for_surface` could be simplified by factoring the two lookup steps into a small helper (e.g., `window_for_surface`) to keep the control flow tighter and avoid duplicated lookup logic in the future.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReworks surface→window lookup and keyboard focus resolution: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@jules Overall Comments
|
…lications that request an exclusive keyboard grab right after mapping would often fail because their surface hasn't been assigned to a managed window yet. Now `keyboard_focus_for_xsurface` also searches `self.space.elements()` for unmanaged surfaces. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
update_script_3.sh (1)
1-2: Consider dropping this one-off rewrite script before merge.This script is PR-scoped migration tooling tied to exact source formatting; keeping it in-tree adds maintenance noise and accidental re-run risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@update_script_3.sh` around lines 1 - 2, The one-off rewrite script update_script_3.sh should be removed from the PR to avoid keeping PR-scoped, formatting-dependent tooling in-tree; delete the file (or move it out of the repository to private/archival location) and ensure the actual code change it produced (the rename of window_id_for_surface to window_for_surface and the new implementation matching smithay::desktop::WindowSurfaceType usage) is committed separately in the src/backend/wayland/compositor/state.rs change set; also verify there are no CI or project references to update_script_3.sh left behind.
🤖 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/state.rs`:
- Around line 1074-1080: The current check in the closure over
self.space.elements() uses Window::surface_under((0.0,0.0), ...) which is a
point hit-test and misses descendant surfaces not covering that point; replace
that logic with Window::with_surfaces() to iterate the window's surface tree and
compare each surfaced wl_surface to the target surface. Concretely, inside the
find closure for each window value w (referencing w.wl_surface()), call
w.with_surfaces(|surf, _| surf == surface) (or equivalent) and return true if
any match; keep the direct w.wl_surface() == Some(surface) check as before but
remove the surface_under(...) branch. Ensure you reference the existing symbols
w.with_surfaces, w.wl_surface, and surface in the updated condition.
---
Nitpick comments:
In `@update_script_3.sh`:
- Around line 1-2: The one-off rewrite script update_script_3.sh should be
removed from the PR to avoid keeping PR-scoped, formatting-dependent tooling
in-tree; delete the file (or move it out of the repository to private/archival
location) and ensure the actual code change it produced (the rename of
window_id_for_surface to window_for_surface and the new implementation matching
smithay::desktop::WindowSurfaceType usage) is committed separately in the
src/backend/wayland/compositor/state.rs change set; also verify there are no CI
or project references to update_script_3.sh left behind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc518a38-6770-4a84-ab7c-88edc7f9a98d
📒 Files selected for processing (3)
src/backend/wayland/compositor/handlers.rssrc/backend/wayland/compositor/state.rsupdate_script_3.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/wayland/compositor/handlers.rs
| self.space.elements().find(|w| { | ||
| if w.wl_surface().as_deref() == Some(surface) { | ||
| return true; | ||
| } | ||
| w.surface_under((0.0, 0.0), WindowSurfaceType::ALL) | ||
| .map(|(hit_surface, _)| hit_surface == *surface) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In smithay 0.7.0, does Window::surface_under((0.0, 0.0), WindowSurfaceType::ALL)only test that coordinate, or can it reliably identify any descendantWlSurfaceof the window? What API is recommended to verify whether an arbitraryWlSurfacebelongs to aWindow surface tree?
💡 Result:
Window::surface_under(point, WindowSurfaceType::ALL) is strictly a point-based hit-test: it walks the window’s relevant surface trees (toplevel and, if enabled, popups; plus subsurfaces depending on the flags) and returns the topmost surface whose input region contains that point. So calling surface_under((0.0, 0.0), ...) only tells you what (if anything) is hit at (0,0); it is not a reliable way to identify “any descendant WlSurface of the window” unless that descendant happens to be under the tested coordinate. [1]
To verify whether an arbitrary WlSurface belongs to a given Window’s surface tree, the intended API is to iterate the window’s surfaces and compare against the target surface, via:
Window::with_surfaces(|wl_surface, _surface_data| { ... })(includes the window’s surface tree and, if you usePopupManager, also its popups). [1][2]
Sources:
[1] smithay Window implementation (shows surface_under is a point hit-test and with_surfaces traverses the surface trees) https://docs.rs/smithay/latest/src/smithay/desktop/wayland/window.rs.html
[2] smithay Window docs (documents with_surfaces / surface_under) https://docs.rs/smithay/latest/smithay/desktop/struct.Window.html
Switch to Window::with_surfaces() for reliable surface tree membership check.
The code uses surface_under((0.0, 0.0), WindowSurfaceType::ALL) to verify if a surface belongs to a window. However, surface_under() is strictly a point-based hit-test—it only identifies surfaces whose input region contains that coordinate. Any descendant surface not covering (0,0) will be missed, breaking grab fallback for such surfaces.
Use Window::with_surfaces() instead, which iterates the window's surface tree and enables reliable membership checking regardless of surface position.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/wayland/compositor/state.rs` around lines 1074 - 1080, The
current check in the closure over self.space.elements() uses
Window::surface_under((0.0,0.0), ...) which is a point hit-test and misses
descendant surfaces not covering that point; replace that logic with
Window::with_surfaces() to iterate the window's surface tree and compare each
surfaced wl_surface to the target surface. Concretely, inside the find closure
for each window value w (referencing w.wl_surface()), call
w.with_surfaces(|surf, _| surf == surface) (or equivalent) and return true if
any match; keep the direct w.wl_surface() == Some(surface) check as before but
remove the surface_under(...) branch. Ensure you reference the existing symbols
w.with_surfaces, w.wl_surface, and surface in the updated condition.
|
@jules Inline comments:
Nitpick comments:
|
Thank you for the review! I have updated |
…lications that request an exclusive keyboard grab right after mapping would often fail because their surface hasn't been assigned to a managed window yet. Now `keyboard_focus_for_xsurface` also searches `self.space.elements()` for unmanaged surfaces, checking their whole surface tree. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
1 participant
Fixes an issue where
dmenuwould occasionally crash when trying to start on Wayland because it failed to acquire the keyboard grab. The problem happened becausedmenurequests a keyboard grab right after creating its window, but the window might not be fully tracked in thewindow_indexyet since it's an unmanaged overlay. We now also searchself.space.elements()to find the surface for the keyboard grab.PR created automatically by Jules for task 7409422313332858039 started by @paperbenni
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
Bug Fixes
Refactor