feat(ui): add surface selector for multi-surface input routing#64
feat(ui): add surface selector for multi-surface input routing#64
Conversation
- Add F4 toggle for ImGui surface selector window - Display surface list with ID, title, dimensions, and input target indicator - Support click-to-select for manual input targeting - Add "Reset to Auto" button to revert to automatic selection - Query X11 WM_NAME/WM_CLASS for XWayland surface metadata
📝 WalkthroughWalkthroughAdds multi-surface input management: track surfaces with SurfaceInfo and unique IDs; CompositorServer enumerates surfaces and supports manual input-target override; InputForwarder exposes these APIs; ImGui adds a toggleable Surface Selector (F4) and Application wires UI callbacks to forwarding APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant ImGui as ImGuiLayer
participant Forwarder as InputForwarder
participant Server as CompositorServer
Note over App,ImGui: F4 toggles selector
App->>ImGui: toggle_surface_selector()
ImGui-->>ImGui: m_surface_selector_visible = true
Note over App,Server: Poll surfaces for UI
App->>Forwarder: get_surfaces()
Forwarder->>Server: get_surfaces()
Server-->>Forwarder: list<SurfaceInfo>
Forwarder-->>App: list<SurfaceInfo>
App->>ImGui: set_surfaces(list)
Note over ImGui,App: User selects surface
ImGui->>App: on_surface_select(surface_id)
App->>Forwarder: set_input_target(surface_id)
Forwarder->>Server: set_input_target(surface_id)
Server-->>Server: manual_input_target = surface_id
Note over ImGui,App: User resets to auto
ImGui->>App: on_surface_reset()
App->>Forwarder: clear_input_override()
Forwarder->>Server: clear_input_override()
Server-->>Server: manual_input_target cleared
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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
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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/input/compositor_server.cpp (1)
620-639: Manual targeting doesn't update pointer-constraint state.When
set_input_target()changes the input target to a different surface,focus_surface()is not called, leavingactive_constraintpointing to the previous surface's constraint. This causeshandle_pointer_motion_event()to use stale constraint state: if the previous surface had a locked constraint, the new target won't receive absolute motion updates, and any constraint on the new surface won't be recognized or activated.Call
focus_surface()when manual targeting changes, or explicitly query and activate the constraint for the new target surface usingwlr_pointer_constraints_v1_constraint_for_surface().
🤖 Fix all issues with AI agents
In `@openspec/changes/add-surface-selector/proposal.md`:
- Around line 7-15: Update the "What Changes" section to explicitly list spec
deltas using ADDED/MODIFIED notation: mark SurfaceInfo struct as ADDED, the
CompositorServer tracking of connected surfaces and the get_surfaces() API as
MODIFIED/ADDED as appropriate, and mark set_input_target(id) and
clear_input_override() as ADDED (and note the ImGui F4 surface selector UI as
ADDED); include a short line describing each delta (e.g., "ADDED: SurfaceInfo
struct (id, title, class, dimensions, type)"; "ADDED: CompositorServer surface
tracking and get_surfaces() API"; "ADDED: set_input_target(id) /
clear_input_override() APIs"; "ADDED: ImGui F4 surface list UI with
click-to-select and auto/manual indicator") so the proposal conforms to the
required format.
In `@openspec/changes/add-surface-selector/specs/input-forwarding/spec.md`:
- Around line 98-109: The wording is contradictory: it says the system tracks
both xdg_shell and XWayland surfaces but then specifies a "unified list …
(Wayland surfaces only)"; update the spec to clearly state whether there's a
single unified active-surface list including both xdg_shell and XWayland or two
separate trackers (e.g., a unified active_surfaces list for Wayland-native
surfaces and a separate xwayland_windows registry for XWayland metadata), and
clarify behaviors tied to each (e.g., listen for new_toplevel and new_surface,
register destroy listeners only for Wayland surfaces, assign unique IDs on
creation, query WM_NAME/WM_CLASS for XWayland entries, and apply manual
target/auto-focus and mutual exclusion semantics consistently across the two
trackers).
In `@openspec/changes/add-surface-selector/tasks.md`:
- Around line 64-68: The checklist item "9.5 Manual test with Steam: verify
overlay surfaces appear when invoked" is still unchecked — either perform that
Steam overlay manual test and mark item 9.5 done in tasks.md (verifying overlay
surfaces appear and behave correctly), or create a follow-up issue referencing
this test gap (mentioning "9.5 Manual test with Steam" and expected verification
steps) and then update tasks.md to link the new issue and mark the checklist
entry as deferred; ensure the change touches the "9.5 Manual test with Steam"
line so reviewers see the resolution.
In `@src/input/compositor_server.cpp`:
- Around line 1095-1195: The UI-thread/data-race issue: protect access to
xwayland_hooks, xdg_hooks and manual_input_target by adding a std::mutex (e.g.
hooks_mutex) to CompositorServer::Impl and take a std::lock_guard<std::mutex>
(or std::scoped_lock) around all reads/writes in get_surfaces(),
set_input_target(), clear_input_override() and in all mutation points
(handle_new_*, handle_*_destroy and any place that pushes/pops into
xwayland_hooks/xdg_hooks); alternatively marshal these calls to the compositor
thread using the existing compositor task queue—but do not leave those
containers unguarded across threads. Ensure every access uses the same mutex
name so the lock consistently prevents UAFs/races.
In `@src/ui/imgui_layer.cpp`:
- Around line 547-560: The mode label is wrong because m_surfaces’
is_input_target is set for the active surface in both auto and manual cases;
update the UI to rely on an explicit manual flag from the compositor instead of
scanning is_input_target. Add or expose a bool like
SurfaceInfo::is_manual_override (or a CompositorServer getter that returns
whether input selection is manual) and use that flag when computing the mode
label and enabling/disabling the "Reset to Auto" button (replace the
has_manual_override check in the block using m_surfaces and the ImGui::Text call
so the UI reflects the compositor-provided is_manual_override).
| ## What Changes | ||
|
|
||
| - Add `SurfaceInfo` struct to expose surface metadata (id, title, class, dimensions, type) | ||
| - Track all connected surfaces with unique IDs in CompositorServer | ||
| - Add `get_surfaces()` API to enumerate connected surfaces | ||
| - Add `set_input_target(id)` and `clear_input_override()` for manual selection | ||
| - Add new ImGui window (F4) displaying surface list with click-to-select | ||
| - Show current input target with "(auto)" or "(manual)" indicator | ||
|
|
There was a problem hiding this comment.
Add explicit spec deltas under “What Changes.”
The section lists features but doesn’t explicitly enumerate the spec deltas (ADDED/MODIFIED), which the proposal format requires.
✏️ Suggested addition
## What Changes
+- Spec deltas:
+ - ADDED Requirements: Surface Enumeration; Manual Input Target Selection; Surface Selector UI
+ - MODIFIED Requirements: Surface Tracking
- Add `SurfaceInfo` struct to expose surface metadata (id, title, class, dimensions, type)
- Track all connected surfaces with unique IDs in CompositorServer
- Add `get_surfaces()` API to enumerate connected surfaces
- Add `set_input_target(id)` and `clear_input_override()` for manual selection
- Add new ImGui window (F4) displaying surface list with click-to-select
- Show current input target with "(auto)" or "(manual)" indicatorAs per coding guidelines, please list spec deltas in the “What Changes” section.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## What Changes | |
| - Add `SurfaceInfo` struct to expose surface metadata (id, title, class, dimensions, type) | |
| - Track all connected surfaces with unique IDs in CompositorServer | |
| - Add `get_surfaces()` API to enumerate connected surfaces | |
| - Add `set_input_target(id)` and `clear_input_override()` for manual selection | |
| - Add new ImGui window (F4) displaying surface list with click-to-select | |
| - Show current input target with "(auto)" or "(manual)" indicator | |
| ## What Changes | |
| - Spec deltas: | |
| - ADDED Requirements: Surface Enumeration; Manual Input Target Selection; Surface Selector UI | |
| - MODIFIED Requirements: Surface Tracking | |
| - Add `SurfaceInfo` struct to expose surface metadata (id, title, class, dimensions, type) | |
| - Track all connected surfaces with unique IDs in CompositorServer | |
| - Add `get_surfaces()` API to enumerate connected surfaces | |
| - Add `set_input_target(id)` and `clear_input_override()` for manual selection | |
| - Add new ImGui window (F4) displaying surface list with click-to-select | |
| - Show current input target with "(auto)" or "(manual)" indicator |
🤖 Prompt for AI Agents
In `@openspec/changes/add-surface-selector/proposal.md` around lines 7 - 15,
Update the "What Changes" section to explicitly list spec deltas using
ADDED/MODIFIED notation: mark SurfaceInfo struct as ADDED, the CompositorServer
tracking of connected surfaces and the get_surfaces() API as MODIFIED/ADDED as
appropriate, and mark set_input_target(id) and clear_input_override() as ADDED
(and note the ImGui F4 surface selector UI as ADDED); include a short line
describing each delta (e.g., "ADDED: SurfaceInfo struct (id, title, class,
dimensions, type)"; "ADDED: CompositorServer surface tracking and get_surfaces()
API"; "ADDED: set_input_target(id) / clear_input_override() APIs"; "ADDED: ImGui
F4 surface list UI with click-to-select and auto/manual indicator") so the
proposal conforms to the required format.
| The system SHALL track surfaces from both xdg_shell (Wayland native) and XWayland clients. | ||
|
|
||
| The compositor server SHALL: | ||
| - Listen for `new_toplevel` signals from xdg_shell | ||
| - Listen for `new_surface` signals from XWayland | ||
| - Maintain a unified list of active surfaces (Wayland surfaces only) | ||
| - Register destroy listeners for Wayland surfaces only | ||
| - Assign a unique ID to each surface on creation | ||
| - Query X11 window properties (`WM_NAME`, `WM_CLASS`) for XWayland surfaces | ||
| - Use manual target if set, otherwise auto-focus the first connected surface | ||
| - Use mutual exclusion to manage focus between Wayland and XWayland surfaces | ||
|
|
There was a problem hiding this comment.
Clarify the surface list wording to avoid contradiction.
Line 103 says “unified list … (Wayland surfaces only)” which conflicts with tracking both Wayland and XWayland. Consider clarifying the Wayland list vs. XWayland tracking split.
✏️ Suggested wording fix
- - Maintain a unified list of active surfaces (Wayland surfaces only)
+ - Maintain a unified list of active Wayland surfaces (xdg_shell)
+ - Track active XWayland surfaces separately from the Wayland list🤖 Prompt for AI Agents
In `@openspec/changes/add-surface-selector/specs/input-forwarding/spec.md` around
lines 98 - 109, The wording is contradictory: it says the system tracks both
xdg_shell and XWayland surfaces but then specifies a "unified list … (Wayland
surfaces only)"; update the spec to clearly state whether there's a single
unified active-surface list including both xdg_shell and XWayland or two
separate trackers (e.g., a unified active_surfaces list for Wayland-native
surfaces and a separate xwayland_windows registry for XWayland metadata), and
clarify behaviors tied to each (e.g., listen for new_toplevel and new_surface,
register destroy listeners only for Wayland surfaces, assign unique IDs on
creation, query WM_NAME/WM_CLASS for XWayland entries, and apply manual
target/auto-focus and mutual exclusion semantics consistently across the two
trackers).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/input/compositor_server.cpp`:
- Around line 1173-1200: set_input_target currently only sets
m_impl->manual_input_target but leaves m_impl->focused_surface /
m_impl->focused_xsurface and m_impl->active_constraint unchanged, so pointer
lock and motion may still target the old surface; update set_input_target to,
after verifying the surface via xwayland_hooks/xdg_hooks, also update the
compositor's focus state and active constraint to point to that surface (e.g.,
set m_impl->focused_surface or focused_xsurface accordingly and update/clear
m_impl->active_constraint as needed) and ensure the update runs on the
compositor thread / under the same guard used when accessing
xwayland_hooks/xdg_hooks; likewise update clear_input_override to reset
manual_input_target and restore or recompute focus/constraint state (e.g., clear
focused_surface/focused_xsurface or recalc focus based on current pointer/seat)
so pointer lock/motion behavior is consistent.
- Around line 1125-1167: get_surfaces() currently auto-selects the "first"
surface as the input target which can drift from what actual routing uses;
update the auto-selection to derive target_id from the real focused/input target
used by the compositor (e.g. call get_input_target() from within get_surfaces()
or read the implementation's focused ids like m_impl->focused_xwayland /
m_impl->focused_xdg) and use that id for info.is_input_target comparisons so the
UI indicator matches actual focus/routing; preserve manual override via
m_impl->manual_input_target when present.
♻️ Duplicate comments (1)
src/input/compositor_server.cpp (1)
183-190: Guard hook containers and manual target across threads.
get_surfaces/set_input_target/clear_input_overridecan run on the UI thread while the compositor thread mutatesxdg_hooks/xwayland_hooksandmanual_input_target. That’s a race/UAF risk—please add synchronization or marshal these operations to the compositor thread.🔒 Minimal locking sketch
struct CompositorServer::Impl { + mutable std::mutex surface_mutex; ... }; auto CompositorServer::get_surfaces() const -> std::vector<SurfaceInfo> { + std::scoped_lock lock(m_impl->surface_mutex); ... } void CompositorServer::set_input_target(uint32_t surface_id) { + std::scoped_lock lock(m_impl->surface_mutex); ... } void CompositorServer::clear_input_override() { + std::scoped_lock lock(m_impl->surface_mutex); m_impl->manual_input_target.reset(); }(Also apply the same guard around hook mutations in handle_new_* / handle_*_destroy.)
| auto CompositorServer::is_manual_override_active() const -> bool { | ||
| return m_impl->manual_input_target.has_value(); | ||
| } | ||
|
|
||
| void CompositorServer::set_input_target(uint32_t surface_id) { | ||
| bool found = false; | ||
| for (const auto* hooks : m_impl->xwayland_hooks) { | ||
| if (hooks->id == surface_id) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!found) { | ||
| for (const auto* hooks : m_impl->xdg_hooks) { | ||
| if (hooks->id == surface_id) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (found) { | ||
| m_impl->manual_input_target = surface_id; | ||
| } | ||
| } | ||
|
|
||
| void CompositorServer::clear_input_override() { | ||
| m_impl->manual_input_target.reset(); | ||
| } |
There was a problem hiding this comment.
Manual override doesn’t update focus/constraint state.
set_input_target only stores an ID; focused_surface/focused_xsurface and active_constraint stay tied to the prior focus. Pointer lock/motion behavior can remain bound to the old surface even after manual selection. Consider updating focus/constraints when manual target changes (ideally on the compositor thread / under the same guard as hook access).
🤖 Prompt for AI Agents
In `@src/input/compositor_server.cpp` around lines 1173 - 1200, set_input_target
currently only sets m_impl->manual_input_target but leaves
m_impl->focused_surface / m_impl->focused_xsurface and m_impl->active_constraint
unchanged, so pointer lock and motion may still target the old surface; update
set_input_target to, after verifying the surface via xwayland_hooks/xdg_hooks,
also update the compositor's focus state and active constraint to point to that
surface (e.g., set m_impl->focused_surface or focused_xsurface accordingly and
update/clear m_impl->active_constraint as needed) and ensure the update runs on
the compositor thread / under the same guard used when accessing
xwayland_hooks/xdg_hooks; likewise update clear_input_override to reset
manual_input_target and restore or recompute focus/constraint state (e.g., clear
focused_surface/focused_xsurface or recalc focus based on current pointer/seat)
so pointer lock/motion behavior is consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/input/compositor_server.cpp (1)
639-642:active_constraintcheck may apply wrong constraint after manual target switch.The constraint check here uses the global
active_constraint, which is tied tofocused_surfacerather than the manual input target. If the user manually selects a different surface, pointer motion could be incorrectly constrained (or not constrained when it should be).This relates to the broader issue that
set_input_target()doesn't synchronize constraint state with the new target.
♻️ Duplicate comments (2)
src/input/compositor_server.cpp (2)
1199-1219: Manual override only stores ID without updating compositor focus state.This method validates and stores the surface ID but doesn't update
focused_surface,focused_xsurface, or constraint state. As noted in a previous review, this can cause pointer lock/motion behavior to remain bound to the old surface.Consider either:
- Marshaling focus updates to the compositor thread (e.g., via the event queue)
- Documenting this as intentional "soft" targeting that only affects input routing via
get_input_target()without full focus transfer
1149-1159: Auto target indicator may not match actual input routing.When no manual override is active,
get_surfaces()marks the first surface as the input target (lines 1154-1158), butget_input_target()routes input tofocused_xsurface/focused_surface(lines 1135-1140). This can cause the UI to highlight a different surface than the one actually receiving input.Consider aligning the auto-selection logic:
🛠️ Suggested fix
uint32_t target_id = 0; if (m_impl->manual_input_target) { target_id = *m_impl->manual_input_target; - } else { - // Auto-select: first XWayland surface if available, else first XDG surface - if (!m_impl->xwayland_hooks.empty()) { - target_id = m_impl->xwayland_hooks.front()->id; - } else if (!m_impl->xdg_hooks.empty()) { - target_id = m_impl->xdg_hooks.front()->id; + } else if (m_impl->focused_xsurface && m_impl->focused_xsurface->surface) { + for (const auto* hooks : m_impl->xwayland_hooks) { + if (hooks->xsurface == m_impl->focused_xsurface) { + target_id = hooks->id; + break; + } + } + } else if (m_impl->focused_surface) { + for (const auto* hooks : m_impl->xdg_hooks) { + if (hooks->surface == m_impl->focused_surface) { + target_id = hooks->id; + break; + } } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/CMakeLists.txt`:
- Around line 131-155: These manual SDL/Vulkan test executables (targets
goggles_manual_surface_selector_x11 and goggles_manual_surface_selector_wayland,
sources input/manual_surface_selector_x11.cpp and
input/manual_surface_selector_wayland.cpp) live under tests/ and violate the
Catch2/non-GPU unit-test policy; move their add_executable and
target_link_libraries blocks out of tests/CMakeLists.txt into a new
CMakeLists.txt under a tools/ or examples/manual-tests directory (or
alternatively remove them from tests/ and document an explicit exception for
tests/input/), and update whichever top-level CMakeLists or project registration
that references these targets so they are built as manual tools/examples rather
than unit tests.
♻️ Duplicate comments (2)
tests/input/manual_surface_selector_wayland.cpp (2)
113-174: Same optional improvement as X11: avoidstd::exitfor cleaner RAII teardown.
189-239: Same optional improvement as X11: guard unknown window IDs in logs.
🧹 Nitpick comments (2)
tests/input/manual_surface_selector_x11.cpp (2)
113-174: Consider propagating init failures instead ofstd::exitto keep RAII cleanup.
std::exitskips stack unwinding, so partially created windows/renderers won’t be cleaned up. Returning an error (orstd::optional) tomainlets destructors run and keeps the test harness-friendly.
189-239: Handle unknown window IDs in logs to avoid “window=0”.
If an event arrives without a matching window ID,window_idxstays-1and logs0, which is ambiguous. Consider logging “unknown” or skipping the print.
1 participant
User description
PR Type
Enhancement
Description
Add surface enumeration API with metadata (ID, title, class, dimensions)
Implement manual input target selection with auto-fallback behavior
Create ImGui surface selector window (F4 toggle) for click-to-select routing
Refactor input event handling to support manual override via
get_input_target()Track XWayland and XDG surfaces with unique IDs and X11 property queries
Diagram Walkthrough
File Walkthrough
7 files
Add F4 handler and surface polling integrationAdd SurfaceInfo struct and surface enumeration APIImplement surface tracking, manual override, and input targetresolutionExpose surface enumeration and selection methodsForward surface API calls to compositor serverAdd surface selector state and callback typesImplement surface selector window with click-to-select UI3 files
Document design rationale and implementation approachDefine surface enumeration and manual selection requirementsTrack implementation tasks and validation checklistSummary by CodeRabbit
New Features
Public API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.