Fix crash when opening rofi in wayland backend#239
Merged
paperbenni merged 2 commits intomainfrom Mar 20, 2026
Merged
Conversation
When `KeyboardFocusTarget` is converted to `PointerFocusTarget` for layer shells or XWayland unmanaged windows like rofi, `w.wl_surface()` can return `None`. Calling `.unwrap()` caused a panic that crashed the compositor. This commit safely handles `Window(w)` by adding it as a variant to `PointerFocusTarget`. This defers the retrieval of the surface until the pointer or touch events are actually dispatched, where it can safely check `if let Some(surface) = w.wl_surface()`. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
Contributor
|
👋 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 GuideThis PR makes pointer and touch focus handling robust for windows whose Wayland surface may be absent (e.g., layer shells or XWayland unmanaged windows like rofi) by introducing a Window variant to PointerFocusTarget and deferring wl_surface resolution until event dispatch, avoiding panics from unwrap(). Sequence diagram for pointer motion with lazy wl_surface resolutionsequenceDiagram
participant Seat
participant WaylandState
participant PointerFocusTarget
participant Window
participant WlSurface
Seat->>WaylandState: motion_event
WaylandState->>PointerFocusTarget: handle_motion(seat, data, event)
alt PointerFocusTarget_Window
PointerFocusTarget->>Window: wl_surface()
alt wl_surface_some
Window-->>PointerFocusTarget: Some(surface)
PointerFocusTarget->>WlSurface: pointer_motion(seat, data, event)
WlSurface-->>Seat: motion_delivered
else wl_surface_none
Window-->>PointerFocusTarget: None
PointerFocusTarget-->>WaylandState: no_op_avoid_panic
end
else PointerFocusTarget_WlSurface
PointerFocusTarget->>WlSurface: pointer_motion(seat, data, event)
WlSurface-->>Seat: motion_delivered
else PointerFocusTarget_Popup
PointerFocusTarget->>WlSurface: popup_wl_surface()
WlSurface-->>Seat: motion_delivered
end
Class diagram for updated focus targets and event dispatchclassDiagram
class Window {
+wl_surface() Option_Cow_WlSurface
+alive() bool
}
class WlSurface {
+alive() bool
}
class PopupKind {
+wl_surface() WlSurface
+alive() bool
}
class KeyboardFocusTarget {
<<enum>>
Window
WlSurface
Popup
}
class PointerFocusTarget {
<<enum>>
Window
WlSurface
Popup
}
class IsAlive {
<<interface>>
+alive() bool
}
class WaylandFocus {
<<interface>>
+wl_surface() Option_Cow_WlSurface
}
class PointerTarget_WaylandState {
<<interface>>
+enter(seat, data, event)
+motion(seat, data, event)
+relative_motion(seat, data, event)
+button(seat, data, event)
+axis(seat, data, frame)
+frame(seat, data)
+gesture_swipe_begin(seat, data, event)
+gesture_swipe_update(seat, data, event)
+gesture_swipe_end(seat, data, event)
+gesture_pinch_begin(seat, data, event)
+gesture_pinch_update(seat, data, event)
+gesture_pinch_end(seat, data, event)
+gesture_hold_begin(seat, data, event)
+gesture_hold_end(seat, data, event)
+leave(seat, data, serial, time)
}
class TouchTarget_WaylandState {
<<interface>>
+down(seat, data, event, seq)
+up(seat, data, event, seq)
+motion(seat, data, event, seq)
+frame(seat, data, seq)
+cancel(seat, data, seq)
+shape(seat, data, event, seq)
+orientation(seat, data, event, seq)
}
KeyboardFocusTarget ..> Window
KeyboardFocusTarget ..> WlSurface
KeyboardFocusTarget ..> PopupKind
PointerFocusTarget ..> Window
PointerFocusTarget ..> WlSurface
PointerFocusTarget ..> PopupKind
KeyboardFocusTarget ..|> IsAlive
PointerFocusTarget ..|> IsAlive
KeyboardFocusTarget ..|> WaylandFocus
PointerFocusTarget ..|> WaylandFocus
PointerFocusTarget ..|> PointerTarget_WaylandState
PointerFocusTarget ..|> TouchTarget_WaylandState
KeyboardFocusTarget --> PointerFocusTarget : From_KeyboardFocusTarget
PopupKind --> PointerFocusTarget : From_PopupKind
PopupKind --> KeyboardFocusTarget : From_PopupKind
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input Device
participant Comp as Compositor / PointerFocusTarget
participant Win as Window
participant Smith as Smithay
Input->>Comp: pointer/touch event
Comp->>Comp: resolve PointerFocusTarget (Surface/Popup/Window)
alt target is Window
Comp->>Win: w.wl_surface()
alt wl_surface exists
Comp->>Smith: forward event to Smithay with surface
else no surface
Comp-->>Input: drop event (trace)
end
else target is Surface/Popup
Comp->>Smith: forward event to Smithay
end
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 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 |
![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:
- There is a lot of repeated
match PointerFocusTarget::Window(w) { if let Some(surface) = w.wl_surface() { … } }boilerplate across all pointer and touch event methods; consider factoring this into a small helper (e.g. a method onPointerFocusTargetor a closure-takingwith_surfacefunction) to centralize thewl_surfacelookup and reduce duplication. - When
w.wl_surface()returnsNonethe events are now silently dropped; if this is expected but rare, you may want to add a trace-level log in theWindowbranch to make diagnosing unexpectedNonecases easier without reintroducing a crash.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is a lot of repeated `match PointerFocusTarget::Window(w) { if let Some(surface) = w.wl_surface() { … } }` boilerplate across all pointer and touch event methods; consider factoring this into a small helper (e.g. a method on `PointerFocusTarget` or a closure-taking `with_surface` function) to centralize the `wl_surface` lookup and reduce duplication.
- When `w.wl_surface()` returns `None` the events are now silently dropped; if this is expected but rare, you may want to add a trace-level log in the `Window` branch to make diagnosing unexpected `None` cases easier without reintroducing a crash.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
Please address the comments from this code review: Overall Comments
|
![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/backend/wayland/compositor/focus.rs (1)
257-661: Nice guard; consider extracting theWindowforwarding pattern.The
if let Some(surface)checks are the right safety net, but the same block is now repeated across every pointer/touch callback. A tiny helper would make future event additions less error-prone.♻️ Refactor sketch
+fn with_window_surface(window: &Window, f: impl FnOnce(&WlSurface)) { + if let Some(surface) = window.wl_surface() { + f(surface.as_ref()); + } +} + impl smithay::input::pointer::PointerTarget<WaylandState> for PointerFocusTarget { fn enter( &self, seat: &Seat<WaylandState>, data: &mut WaylandState, event: &smithay::input::pointer::MotionEvent, ) { match self { - PointerFocusTarget::Window(w) => { - if let Some(surface) = w.wl_surface() { - smithay::input::pointer::PointerTarget::enter( - surface.as_ref(), - seat, - data, - event, - ); - } - } + PointerFocusTarget::Window(w) => with_window_surface(w, |surface| { + smithay::input::pointer::PointerTarget::enter(surface, seat, data, event); + }), PointerFocusTarget::WlSurface(s) => { smithay::input::pointer::PointerTarget::enter(s, seat, data, event); } PointerFocusTarget::Popup(p) => { smithay::input::pointer::PointerTarget::enter(p.wl_surface(), seat, data, event); } } }Apply the same helper to the remaining pointer and touch branches.
Also applies to: 689-843
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/wayland/compositor/focus.rs` around lines 257 - 661, Extract a small helper on PointerFocusTarget (e.g. fn as_surface(&self) -> Option<&wl_surface::WlSurface> or Option<&T> matching your wl_surface type) that returns Some(surface) for PointerFocusTarget::Window (calling w.wl_surface().map(|s| s.as_ref())) and for PointerFocusTarget::WlSurface/Popup returns the surface directly, then replace the repeated if let Some(surface) { smithay::input::pointer::PointerTarget::<event>(surface.as_ref(), seat, data, event) } blocks in methods enter, motion, relative_motion, button, axis, frame, gesture_*, leave, etc., with a single call guarded by the helper (e.g. if let Some(surface) = self.as_surface() { PointerTarget::enter(surface, seat, data, event) }), applying the same change to the pointer and touch branches mentioned.
🤖 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/backend/wayland/compositor/focus.rs`:
- Around line 257-661: Extract a small helper on PointerFocusTarget (e.g. fn
as_surface(&self) -> Option<&wl_surface::WlSurface> or Option<&T> matching your
wl_surface type) that returns Some(surface) for PointerFocusTarget::Window
(calling w.wl_surface().map(|s| s.as_ref())) and for
PointerFocusTarget::WlSurface/Popup returns the surface directly, then replace
the repeated if let Some(surface) {
smithay::input::pointer::PointerTarget::<event>(surface.as_ref(), seat, data,
event) } blocks in methods enter, motion, relative_motion, button, axis, frame,
gesture_*, leave, etc., with a single call guarded by the helper (e.g. if let
Some(surface) = self.as_surface() { PointerTarget::enter(surface, seat, data,
event) }), applying the same change to the pointer and touch branches mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63c4c53e-54e4-40f5-9528-f2fe207f94e8
📒 Files selected for processing (1)
src/backend/wayland/compositor/focus.rs
Contributor
I have factored the logic into a |
Following code review feedback, a `with_surface` helper function was added to `PointerFocusTarget`. This centralizes the lookup for `wl_surface()` and reduces the boilerplate previously repeated across all pointer and touch event handlers. It additionally logs a trace message when a surface is missing instead of silently dropping the event, making future diagnostics easier. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix crash when opening rofi in wayland backend
When
KeyboardFocusTargetis converted toPointerFocusTargetfor layer shells or XWayland unmanaged windows like rofi,w.wl_surface()can returnNone. Calling.unwrap()caused a panic that crashed the compositor.This commit safely handles
Window(w)by adding it as a variant toPointerFocusTarget. This defers the retrieval of the surface until the pointer or touch events are actually dispatched, where it can safely checkif let Some(surface) = w.wl_surface().PR created automatically by Jules for task 4843312196377472437 started by @paperbenni
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit