refactor(compositor): decompose compositor server implementation#112
Merged
refactor(compositor): decompose compositor server implementation#112
Conversation
- Replace the all-in-one compositor source with subsystem-oriented files - Centralize shared compositor state while reducing file-level coupling - Isolate protocol, input, and presentation paths behind clearer boundaries - Shrink compositor_server.cpp to facade and orchestration duties
📝 WalkthroughWalkthroughRefactors the monolithic compositor into modular subsystems (core, input, focus, cursor, layer-shell, xdg, xwayland, present), adds compile-safe state/target/hook headers, moves implementations into new source files, and updates CMake to include the new compilation units. CompositorServer remains the external facade. Changes
Sequence Diagram(s)mermaid Client->>SDL: user input Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
src/compositor/compositor_core.cpp (1)
128-131: Guard against multiple wlroots logging initialization.
initialize_wlroots_logging()is called fromsetup_base_components(). If setup fails and is retried,wlr_log_initwould be called again. Consider adding a static flag to ensure one-time initialization.♻️ Suggested improvement
auto initialize_wlroots_logging() -> void { + static bool initialized = false; + if (initialized) { + return; + } + initialized = true; const auto level = goggles::get_logger()->level(); wlr_log_init(wlr_importance_from_log_level(level), wlr_log_bridge); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_core.cpp` around lines 128 - 131, The initialize_wlroots_logging() function should guard against repeated calls to wlr_log_init; add a static bool (or std::once_flag) inside initialize_wlroots_logging() to record if wlroots logging was already initialized and return early if so, ensuring only the first call (from setup_base_components() or any retry) invokes wlr_log_init; reference the existing functions/idents initialize_wlroots_logging, setup_base_components, and the call to wlr_log_init/wlr_log_bridge when implementing the guard.src/compositor/compositor_input.cpp (3)
22-28: Duplicateget_time_msechelper.This function is duplicated in
compositor_cursor.cpp. Consider extracting to a shared utility header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_input.cpp` around lines 22 - 28, Duplicate helper get_time_msec should be extracted into a shared utility header (e.g., time_utils.h) and the duplicate definitions removed: create a single implementation (preferably static inline or non-ODR-violating function) named get_time_msec in the new header, add include guards, include that header from both compositor_input.cpp and compositor_cursor.cpp, and remove the anonymous-namespace copies so both compilation units use the shared symbol.
411-456: Duplicated cursor reset logic in button and axis handlers.Lines 422-424 and 444-446 contain identical cursor reset logic:
if (cursor_surface != cursor_reference || !cursor_initialized) { reset_cursor_for_surface(cursor_reference); }Consider extracting to a helper method to reduce duplication.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_input.cpp` around lines 411 - 456, Both handle_pointer_button_event and handle_pointer_axis_event duplicate the same cursor-reset logic; extract that check into a small helper (e.g., ensure_cursor_for_surface or update_cursor_reference) that accepts the resolved cursor_reference (wlr_surface*) and calls reset_cursor_for_surface(cursor_reference) when (cursor_surface != cursor_reference || !cursor_initialized). Replace the duplicated if-block in CompositorState::handle_pointer_button_event and CompositorState::handle_pointer_axis_event with a single call to the new helper to remove duplication and keep behavior identical.
152-164: Add clarifying comments for SDL button index mappings.Buttons 6, 7, and 8 lack SDL3 named constants—SDL3 only defines constants for buttons 1–5 (
SDL_BUTTON_LEFTthroughSDL_BUTTON_X2). Higher button indices are expected to be handled numerically. Adding comments explaining what these hardware button indices represent (e.g., forward/back navigation) would improve clarity without requiring constant definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_input.cpp` around lines 152 - 164, The mapping for sdl_button to BTN_* values (symbols: sdl_button, BTN_FORWARD, BTN_BACK, BTN_TASK, BTN_MISC, GOGGLES_LOG_TRACE) is missing clarifying comments; update the block in compositor_input.cpp that handles sdl_button == 6/7/8 and sdl_button > 8 to add short comments explaining that SDL3 only provides named constants for buttons 1–5 (SDL_BUTTON_LEFT..SDL_BUTTON_X2), that indices 6/7/8 are being mapped to forward/back/task hardware buttons respectively, and that any index >8 is handled numerically as BTN_MISC + (sdl_button - 8); keep the code logic unchanged and place the comments immediately above each if-case to document the hardware meaning and why numeric handling is used.src/compositor/compositor_layer_shell.cpp (1)
34-71: DuplicateRenderSurfaceContextandrender_surface_iteratoracross modules.This struct and function are duplicated verbatim in
compositor_present.cpp(lines 52-89). Consider extracting to a shared internal header to avoid code duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_layer_shell.cpp` around lines 34 - 71, Extract the duplicated definitions of RenderSurfaceContext and render_surface_iterator into a single shared internal header, declare the struct and the function there (preserving the exact symbol names RenderSurfaceContext and render_surface_iterator), include that header from both compositor_layer_shell.cpp and compositor_present.cpp, and remove the duplicate definitions from those .cpp files; ensure the header has proper include guards (or `#pragma` once) and the function signature remains compatible with existing usage so no call sites need changing.src/compositor/compositor_protocol_hooks.hpp (1)
105-110: Usewl_list_empty()for idiomatic listener state checking.The current check
listener.link.next != nullptr && listener.link.prev != nullptris unreliable: afterwl_list_init(), the pointers point to the node itself (notnullptr), so the check doesn't distinguish between "in a list" and "detached". While safe (remove on a self-linked list is a no-op), the idiomatic wlroots pattern useswl_list_empty():♻️ Suggested improvement
inline void detach_listener(wl_listener& listener) { - if (listener.link.next != nullptr && listener.link.prev != nullptr) { + if (!wl_list_empty(&listener.link)) { wl_list_remove(&listener.link); } wl_list_init(&listener.link); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_protocol_hooks.hpp` around lines 105 - 110, The detach_listener function should use the idiomatic wl_list_empty() check instead of inspecting next/prev pointers directly; update detach_listener to call wl_list_remove(&listener.link) only when !wl_list_empty(&listener.link), then always call wl_list_init(&listener.link) to reinitialize the listener; reference the function detach_listener and the wl_list_empty/wl_list_remove/wl_list_init calls when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openspec/changes/archive/2026-03-07-refactor-compositor-server-modules/proposal.md`:
- Line 40: The list item for "compositor_layer_shell.*" is malformed due to a
doubled list marker; edit the proposal markdown and fix the bullet so it uses a
single list marker (remove the extra "-" at the start) so the
`compositor_layer_shell.*` entry becomes a normal list item and renders within
the surrounding boundary list.
In
`@openspec/changes/archive/2026-03-07-refactor-compositor-server-modules/tasks.md`:
- Around line 103-108: The table rows include unescaped pipe characters inside
code spans (e.g. the regexes
"goggles_tests|goggles_test_child_death_signal|goggles_test_headless_child_exit",
"goggles_auto_input_forwarding_(x11|wayland)", and
"goggles_headless_integration(_png_exists)?") which break the Markdown table;
fix by escaping each '|' inside those table cells (use '\|' or replace with
'|') or wrap the entire command in an HTML code element that uses '|'
for pipes so the cell remains a single column and the traceability matrix
renders correctly.
- Line 89: Update the broken archived references in the checklist: locate the
checklist items that currently reference
openspec/changes/refactor-compositor-server-modules/... (the lines containing
"8.1 Confirm implementation matches
`openspec/changes/refactor-compositor-server-modules/specs/compositor-module-layout/spec.md`
and `openspec/changes/refactor-compositor-server-modules/design.md`" and the
similar entry at line 91) and change those paths to the new dated archive
location
(openspec/changes/archive/2026-03-07-refactor-compositor-server-modules/...) so
both links point to the archived spec and design files instead of the old
non-archived paths.
In `@openspec/specs/compositor-module-layout/spec.md`:
- Around line 3-4: Replace the placeholder "TBD" under the "## Purpose" heading
in spec.md with a concrete, final purpose statement describing the intent and
scope of the compositor-module-layout spec (e.g., what the module layout
standardizes, who it affects, and any high-level goals). Update the "## Purpose"
section text (the block currently containing "TBD - created by archiving change
refactor-compositor-server-modules. Update Purpose after archive.") to a
complete sentence or short paragraph that serves as the canonical description
for the published spec.
In `@src/compositor/compositor_core.cpp`:
- Around line 212-237: CompositorState::setup_event_loop_fd currently creates a
new eventfd unconditionally which can overwrite and leak the existing event_fd;
before creating a new fd, check if event_fd already holds a valid descriptor
(via util::UniqueFd's validity check or event_fd.get() >= 0) and if so either
return success immediately or close/remove the existing fd and its
wl_event_source (event_source) before creating a new one; ensure you also avoid
adding a duplicate wl_event_loop_add_fd by checking/clearing event_source, and
only call wl_event_loop_add_fd when no valid event_fd/event_source exists.
In `@src/compositor/compositor_focus.cpp`:
- Around line 59-62: get_surfaces() currently calls
m_impl->state.get_surfaces_snapshot() directly from the caller thread, causing a
data race because the compositor thread mutates focused_surface,
focused_xsurface and hook fields under a different synchronization boundary; fix
by routing the snapshot request to the compositor thread (e.g. post a task to
the compositor's event loop and return the snapshot from that task) or ensure
both readers and writers use the same mutex by taking hooks_mutex (or the
state’s single mutex) for both get_surfaces() and all compositor-side mutations;
update get_surfaces(), get_surfaces_snapshot(), and any compositor mutation
sites that touch focused_surface/focused_xsurface/hooks to use the chosen
synchronization approach so reads and writes share a single boundary.
- Around line 241-245: Previous focused toplevels aren't being deactivated
before being overwritten, causing multiple clients to remain active; before
assigning the new targets in the focus-switch paths (the blocks that set
focused_xsurface and focused_surface and call deactivate_constraint()),
explicitly call the appropriate deactivate function on the currently stored
focus (e.g., if focused_xsurface != nullptr call its
deactivate/toplevel_deactivate method, or if focused_surface != nullptr call its
deactivate method) then clear them and proceed to activate the incoming surface;
apply the same fix to the other focus-switch locations referenced (around the
blocks at 281-290 and 344-346) so the old toplevel always receives a deactivate
call prior to being replaced.
In `@src/compositor/compositor_input.cpp`:
- Around line 271-294: The two separate pointer_axis injections in
CompositorServer::forward_mouse_wheel can enqueue only one axis when the queue
is near capacity, causing inconsistent diagonal scrolls; change this to emit a
single consolidated event (e.g., add a new InputEventType::pointer_axis2 or
extend InputEvent with axis_x and axis_y fields) and populate both vertical (y)
and horizontal (x) values in one InputEvent before calling inject_event once, or
alternatively modify inject_event to support an atomic batch enqueue so both
axis values are accepted together; update any handling code that reads
InputEvent to consume the new combined event representation accordingly
(references: CompositorServer::forward_mouse_wheel, inject_event, InputEvent,
InputEventType::pointer_axis).
In `@src/compositor/compositor_layer_shell.cpp`:
- Around line 234-245: The unmap handler
CompositorState::handle_layer_surface_unmap uses keyboard without checking for
null; update the conditional that currently checks seat, hooks->surface, and
focused_surface to also verify keyboard is non-null (e.g., keyboard != nullptr)
before calling wlr_seat_set_keyboard(seat, keyboard.get()) and
wlr_seat_keyboard_notify_enter(..., keyboard->keycodes, keyboard->num_keycodes,
&keyboard->modifiers); ensure the early-return or conditional scope prevents
dereferencing keyboard when it's null.
- Around line 253-258: The destroy handler is missing a null check for keyboard
before dereferencing it; update the handler to ensure keyboard is non-null (in
addition to existing checks for seat, hooks->surface, focused_surface) before
calling wlr_seat_set_keyboard and wlr_seat_keyboard_notify_enter and before
accessing keyboard->keycodes, keyboard->num_keycodes, or &keyboard->modifiers so
you don’t dereference a null pointer (i.e., mirror the null-check logic used
here for keyboard when handling destruction in compositor_layer_shell.cpp).
- Around line 221-232: In CompositorState::handle_layer_surface_map, add a null
check for the keyboard pointer before calling wlr_seat_set_keyboard and
wlr_seat_keyboard_notify_enter: verify keyboard is non-null (e.g., if (keyboard)
or if (keyboard.get())) and only then call wlr_seat_set_keyboard(seat,
keyboard.get()) and wlr_seat_keyboard_notify_enter(seat, hooks->surface,
keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers); otherwise
skip those calls so no dereference of keyboard occurs; keep LayerSurfaceHooks
handling and request_present_reset() unchanged.
In `@src/compositor/compositor_present.cpp`:
- Around line 93-139: The initialize_present_output() function can leak the
previous swapchain when called multiple times; before creating a new
present_swapchain in CompositorState::initialize_present_output(), check if
present_swapchain is non-null and call wlr_swapchain_destroy(present_swapchain)
(then set present_swapchain = nullptr) to free the old resource, ensuring you
clear any dependent state (e.g., present_width/present_height if needed) before
creating the new swapchain.
In `@src/compositor/compositor_xdg.cpp`:
- Around line 234-260: The code erases the unique_ptr owner in xdg_popup_hooks
(inside the scoped_lock using hooks_mutex) while still dereferencing hooks
afterwards to compare hooks->surface against keyboard_entered_surface and
pointer_entered_surface; move those dereferences before destroying the owner or
stash the needed data first: capture hooks->surface into a local variable (e.g.
auto* surface = hooks->surface) or perform the
keyboard_entered_surface/pointer_entered_surface comparisons and resets prior to
the block that finds/erases the XdgPopupHooks unique_ptr (functions/vars
involved: XdgPopupHooks, xdg_popup_hooks, hooks, hooks_mutex,
keyboard_entered_surface, pointer_entered_surface, detach_listener).
In `@src/compositor/compositor_xwayland.cpp`:
- Around line 223-275: The code currently caches XWayland metadata
(title/class_name) only during the associate handler in XWaylandSurfaceHooks so
later renames never propagate; either update those cached fields whenever the
XWayland surface metadata changes (add a listener in XWaylandSurfaceHooks for
metadata/title/class change and refresh the cached fields there, e.g., in the
associate lambda or a new metadata notify) or change get_surfaces_snapshot() to
read h->xsurface->title and h->xsurface->class_name live instead of using the
cached copies—modify the associate/destroy/commit-related handlers
(XWaylandSurfaceHooks::associate, ::destroy, and the commit notify) or the
get_surfaces_snapshot implementation to ensure title/class_name are kept
current.
---
Nitpick comments:
In `@src/compositor/compositor_core.cpp`:
- Around line 128-131: The initialize_wlroots_logging() function should guard
against repeated calls to wlr_log_init; add a static bool (or std::once_flag)
inside initialize_wlroots_logging() to record if wlroots logging was already
initialized and return early if so, ensuring only the first call (from
setup_base_components() or any retry) invokes wlr_log_init; reference the
existing functions/idents initialize_wlroots_logging, setup_base_components, and
the call to wlr_log_init/wlr_log_bridge when implementing the guard.
In `@src/compositor/compositor_input.cpp`:
- Around line 22-28: Duplicate helper get_time_msec should be extracted into a
shared utility header (e.g., time_utils.h) and the duplicate definitions
removed: create a single implementation (preferably static inline or
non-ODR-violating function) named get_time_msec in the new header, add include
guards, include that header from both compositor_input.cpp and
compositor_cursor.cpp, and remove the anonymous-namespace copies so both
compilation units use the shared symbol.
- Around line 411-456: Both handle_pointer_button_event and
handle_pointer_axis_event duplicate the same cursor-reset logic; extract that
check into a small helper (e.g., ensure_cursor_for_surface or
update_cursor_reference) that accepts the resolved cursor_reference
(wlr_surface*) and calls reset_cursor_for_surface(cursor_reference) when
(cursor_surface != cursor_reference || !cursor_initialized). Replace the
duplicated if-block in CompositorState::handle_pointer_button_event and
CompositorState::handle_pointer_axis_event with a single call to the new helper
to remove duplication and keep behavior identical.
- Around line 152-164: The mapping for sdl_button to BTN_* values (symbols:
sdl_button, BTN_FORWARD, BTN_BACK, BTN_TASK, BTN_MISC, GOGGLES_LOG_TRACE) is
missing clarifying comments; update the block in compositor_input.cpp that
handles sdl_button == 6/7/8 and sdl_button > 8 to add short comments explaining
that SDL3 only provides named constants for buttons 1–5
(SDL_BUTTON_LEFT..SDL_BUTTON_X2), that indices 6/7/8 are being mapped to
forward/back/task hardware buttons respectively, and that any index >8 is
handled numerically as BTN_MISC + (sdl_button - 8); keep the code logic
unchanged and place the comments immediately above each if-case to document the
hardware meaning and why numeric handling is used.
In `@src/compositor/compositor_layer_shell.cpp`:
- Around line 34-71: Extract the duplicated definitions of RenderSurfaceContext
and render_surface_iterator into a single shared internal header, declare the
struct and the function there (preserving the exact symbol names
RenderSurfaceContext and render_surface_iterator), include that header from both
compositor_layer_shell.cpp and compositor_present.cpp, and remove the duplicate
definitions from those .cpp files; ensure the header has proper include guards
(or `#pragma` once) and the function signature remains compatible with existing
usage so no call sites need changing.
In `@src/compositor/compositor_protocol_hooks.hpp`:
- Around line 105-110: The detach_listener function should use the idiomatic
wl_list_empty() check instead of inspecting next/prev pointers directly; update
detach_listener to call wl_list_remove(&listener.link) only when
!wl_list_empty(&listener.link), then always call wl_list_init(&listener.link) to
reinitialize the listener; reference the function detach_listener and the
wl_list_empty/wl_list_remove/wl_list_init calls when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5570b1b4-d8d0-4cbc-a8cd-23e2668fbe60
📒 Files selected for processing (20)
openspec/changes/archive/2026-03-07-refactor-compositor-server-modules/.openspec.yamlopenspec/changes/archive/2026-03-07-refactor-compositor-server-modules/design.mdopenspec/changes/archive/2026-03-07-refactor-compositor-server-modules/implementation-handoff.mdopenspec/changes/archive/2026-03-07-refactor-compositor-server-modules/proposal.mdopenspec/changes/archive/2026-03-07-refactor-compositor-server-modules/specs/compositor-module-layout/spec.mdopenspec/changes/archive/2026-03-07-refactor-compositor-server-modules/tasks.mdopenspec/specs/compositor-module-layout/spec.mdsrc/compositor/CMakeLists.txtsrc/compositor/compositor_core.cppsrc/compositor/compositor_cursor.cppsrc/compositor/compositor_focus.cppsrc/compositor/compositor_input.cppsrc/compositor/compositor_layer_shell.cppsrc/compositor/compositor_present.cppsrc/compositor/compositor_protocol_hooks.hppsrc/compositor/compositor_server.cppsrc/compositor/compositor_state.hppsrc/compositor/compositor_targets.hppsrc/compositor/compositor_xdg.cppsrc/compositor/compositor_xwayland.cpp
openspec/changes/archive/2026-03-07-refactor-compositor-server-modules/proposal.md
Show resolved
Hide resolved
openspec/changes/archive/2026-03-07-refactor-compositor-server-modules/tasks.md
Show resolved
Hide resolved
openspec/changes/archive/2026-03-07-refactor-compositor-server-modules/tasks.md
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/compositor/compositor_core.cpp (1)
261-277: Consider making output resolution configurable.The hardcoded 1920x1080 resolution works for headless mode but may limit flexibility. Consider accepting dimensions as parameters or reading from configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_core.cpp` around lines 261 - 277, CompositorState::setup_output currently hardcodes 1920x1080 when calling wlr_headless_add_output; change it to accept configurable dimensions (either parameters on CompositorState::setup_output or loading width/height from the existing configuration object) and pass those values to wlr_headless_add_output instead of literals. Update any callers of setup_output (or CompositorState constructor) to provide or propagate the configured width/height, and validate ranges before calling wlr_headless_add_output; keep the fallback default to 1920x1080 if no config is provided. Ensure identifiers referenced: CompositorState::setup_output, output, wlr_headless_add_output, and any config accessor you add/modify.src/compositor/compositor_xwayland.cpp (1)
388-393: Minor: Redundant clearing of entered surfaces.When
clear_focusis true,keyboard_entered_surfaceandpointer_entered_surfaceare cleared twice: first conditionally at lines 388-393 (if they matchsurface), then unconditionally at lines 409-410. The second clear is redundant when the first already matched.♻️ Suggested simplification
if (keyboard_entered_surface == surface) { keyboard_entered_surface = nullptr; } if (pointer_entered_surface == surface) { pointer_entered_surface = nullptr; } if (presented_surface == surface) { clear_presented_frame(); } if (xsurface && xsurface->override_redirect) { request_present_reset(); } if (!clear_focus) { return; } GOGGLES_LOG_DEBUG("Focused XWayland surface destroyed: ptr={}", static_cast<void*>(xsurface)); deactivate_constraint(); - keyboard_entered_surface = nullptr; - pointer_entered_surface = nullptr; cursor_surface = nullptr;Also applies to: 409-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_xwayland.cpp` around lines 388 - 393, keyboard_entered_surface and pointer_entered_surface are being cleared twice when clear_focus is true; remove the redundant early conditional clears (the block that does "if (keyboard_entered_surface == surface) { keyboard_entered_surface = nullptr; }" and the pointer equivalent) or instead guard that early block with a "!clear_focus" check so the later unconditional clears (when clear_focus is true) are the only path that clears them; update the code around the surface handling to either delete the first conditional clear block or wrap it in if (!clear_focus) to avoid double-clearing of keyboard_entered_surface and pointer_entered_surface.
🤖 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/compositor/compositor_focus.cpp`:
- Around line 727-755: auto_focus_next_surface iterates xwayland_hooks and
xdg_hooks without locking hooks_mutex which risks data races; either acquire
hooks_mutex for the duration of the two iterations (lock before iterating
xwayland_hooks and xdg_hooks and unlock after) to protect access to the
containers, or if the function is guaranteed to run only on the compositor
thread, add a clear comment in CompositorState::auto_focus_next_surface stating
that the compositor thread exclusively owns xwayland_hooks/xdg_hooks and
hooks_mutex need not be taken; apply the change near the start of
auto_focus_next_surface and ensure use of the existing hooks_mutex name and that
calls to focus_xwayland_surface, wlr_xdg_toplevel_set_activated, and
focus_surface remain executed while holding or after documenting the ownership
contract consistently.
- Around line 93-96: Multiple functions read the non-atomic pointers
focused_surface and focused_xsurface without synchronization, causing data
races; update all accesses (reads and writes) to those pointers to be protected
by the existing hooks_mutex (or convert the pointers to std::atomic if you
prefer an atomic approach). Specifically, wrap the early-exit checks in
focus_surface() and focus_xwayland_surface(), the reads in
handle_new_pointer_constraint(), the code paths around the reads in
compositor_focus.cpp (those at lines noted in the review), and the readers in
handle_layer_surface_unmap() and handle_layer_surface_destroy() so they take
hooks_mutex before reading focused_surface/focused_xsurface; also ensure
destructors and shutdown() similarly acquire hooks_mutex or use the atomic
variant to avoid races. Ensure you lock hooks_mutex consistently wherever
focused_surface or focused_xsurface are read or written (or replace those
pointers with atomic pointers and adjust reads/writes accordingly).
---
Nitpick comments:
In `@src/compositor/compositor_core.cpp`:
- Around line 261-277: CompositorState::setup_output currently hardcodes
1920x1080 when calling wlr_headless_add_output; change it to accept configurable
dimensions (either parameters on CompositorState::setup_output or loading
width/height from the existing configuration object) and pass those values to
wlr_headless_add_output instead of literals. Update any callers of setup_output
(or CompositorState constructor) to provide or propagate the configured
width/height, and validate ranges before calling wlr_headless_add_output; keep
the fallback default to 1920x1080 if no config is provided. Ensure identifiers
referenced: CompositorState::setup_output, output, wlr_headless_add_output, and
any config accessor you add/modify.
In `@src/compositor/compositor_xwayland.cpp`:
- Around line 388-393: keyboard_entered_surface and pointer_entered_surface are
being cleared twice when clear_focus is true; remove the redundant early
conditional clears (the block that does "if (keyboard_entered_surface ==
surface) { keyboard_entered_surface = nullptr; }" and the pointer equivalent) or
instead guard that early block with a "!clear_focus" check so the later
unconditional clears (when clear_focus is true) are the only path that clears
them; update the code around the surface handling to either delete the first
conditional clear block or wrap it in if (!clear_focus) to avoid double-clearing
of keyboard_entered_surface and pointer_entered_surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7879f5b-4a62-4be2-821e-7552bcacb396
📒 Files selected for processing (4)
src/compositor/compositor_core.cppsrc/compositor/compositor_focus.cppsrc/compositor/compositor_xdg.cppsrc/compositor/compositor_xwayland.cpp
K1ngst0m
force-pushed
the
dev/split-compositor-src
branch
from
19879ac to
7e247ab
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/compositor/compositor_layer_shell.cpp (3)
243-248:⚠️ Potential issue | 🟠 MajorSame null check needed for
keyboardin unmap handler.🐛 Proposed fix
if (seat && hooks->surface && seat->keyboard_state.focused_surface == hooks->surface && - next_focus_surface) { + next_focus_surface && keyboard) { wlr_seat_set_keyboard(seat, keyboard.get()); wlr_seat_keyboard_notify_enter(seat, next_focus_surface, keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_layer_shell.cpp` around lines 243 - 248, In the unmap handler add a null check for the keyboard before using it: ensure keyboard is non-null along with seat, hooks->surface, and next_focus_surface before calling wlr_seat_set_keyboard and wlr_seat_keyboard_notify_enter (the same pattern as the map handler), i.e. only call wlr_seat_set_keyboard(seat, keyboard.get()) and wlr_seat_keyboard_notify_enter(...) when keyboard != nullptr to avoid dereferencing a null keyboard.
265-270:⚠️ Potential issue | 🟠 MajorSame null check needed for
keyboardin destroy handler.🐛 Proposed fix
if (seat && hooks->surface && seat->keyboard_state.focused_surface == hooks->surface && - next_focus_surface) { + next_focus_surface && keyboard) { wlr_seat_set_keyboard(seat, keyboard.get()); wlr_seat_keyboard_notify_enter(seat, next_focus_surface, keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_layer_shell.cpp` around lines 265 - 270, The destroy handler uses keyboard without null-checking it; update the conditional that currently checks seat, hooks->surface, seat->keyboard_state.focused_surface and next_focus_surface to also require keyboard (e.g., keyboard != nullptr) before calling wlr_seat_set_keyboard(seat, keyboard.get()) and wlr_seat_keyboard_notify_enter(... keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers). Ensure the same combined null-check (keyboard) is added so wlr_seat_set_keyboard and wlr_seat_keyboard_notify_enter are only invoked when keyboard is valid.
221-232:⚠️ Potential issue | 🟠 MajorAdd null check for
keyboardbefore dereferencing.The code accesses
keyboard->keycodes,keyboard->num_keycodes, andkeyboard->modifierswithout verifyingkeyboardis non-null. If input device setup fails or is deferred, this could cause a null pointer dereference.🐛 Proposed fix
void CompositorState::handle_layer_surface_map(LayerSurfaceHooks* hooks) { hooks->mapped = true; - if (hooks->layer_surface && hooks->layer_surface->current.keyboard_interactive == + if (keyboard && hooks->layer_surface && hooks->layer_surface->current.keyboard_interactive == ZWLR_LAYER_SURFACE_V1_KEYBOARD_INTERACTIVITY_EXCLUSIVE) { wlr_seat_set_keyboard(seat, keyboard.get()); wlr_seat_keyboard_notify_enter(seat, hooks->surface, keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers); } request_present_reset(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_layer_shell.cpp` around lines 221 - 232, In CompositorState::handle_layer_surface_map, avoid dereferencing keyboard when it may be null: before calling wlr_seat_set_keyboard and wlr_seat_keyboard_notify_enter, add a null-check (e.g. if (keyboard) { ... }) and only call wlr_seat_set_keyboard(seat, keyboard.get()) and wlr_seat_keyboard_notify_enter(seat, hooks->surface, keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers) inside that branch; leave request_present_reset() and the mapped flag behavior unchanged.src/compositor/compositor_core.cpp (1)
212-237:⚠️ Potential issue | 🟡 MinorCheck for existing eventfd before creating a new one.
If
setup_event_loop_fd()is called whenevent_fdalready holds a valid descriptor, the old fd would be leaked (overwritten by the newUniqueFd). While this may not happen in practice with the current initialization sequence, adding a guard makes the code more robust.🐛 Proposed fix
auto CompositorState::setup_event_loop_fd() -> Result<void> { GOGGLES_PROFILE_FUNCTION(); + if (event_fd.valid()) { + return {}; // Already initialized + } int efd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_core.cpp` around lines 212 - 237, The setup_event_loop_fd function can overwrite an existing event_fd and leak the old descriptor; update CompositorState::setup_event_loop_fd to first check whether event_fd already holds a valid descriptor (e.g., by testing event_fd.get() or a valid()/is_valid() helper) and if so either return success immediately or reuse/close it before creating a new eventfd; ensure you also avoid adding a second wl_event_loop_add_fd when event_source is already set (check event_source) and only call wl_event_loop_add_fd when creating a new event_fd so event_source and event_fd remain consistent.src/compositor/compositor_xwayland.cpp (1)
291-292:⚠️ Potential issue | 🟡 MinorXWayland metadata cached only once during associate.
The hook captures
titleandclass_nameonce during associate, butget_surfaces_snapshot()later serves those cached strings. Any later rename on the same XWayland window will never reach callers, so the surface list can drift until the window is recreated.Consider refreshing the cache on metadata changes or reading live fields when building the snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_xwayland.cpp` around lines 291 - 292, The current associate() stores XWayland metadata into hooks->title and hooks->class_name once (from xsurface->title / xsurface->class_) so later renames don't propagate to callers of get_surfaces_snapshot(); instead, either update the cache when metadata changes or stop using the cached strings: modify get_surfaces_snapshot() to read the live xsurface->title and xsurface->class_ fields (or register a metadata-change callback in associate() that updates hooks->title/hooks->class_name when xsurface metadata changes) so snapshot generation always reflects current window title/class.
🧹 Nitpick comments (3)
src/compositor/compositor_focus.cpp (1)
475-482: Consider makingget_surface_extentstatic or moving to anonymous namespace.This helper function is defined at file scope without
staticor being inside the anonymous namespace, which means it has external linkage. Since it appears to be an internal helper, consider moving it inside the anonymous namespace (likeget_root_xdg_surfaceand others below) or marking itstaticto prevent ODR violations if a similar function exists elsewhere.♻️ Suggested fix
+namespace { + auto get_surface_extent(wlr_surface* surface) -> std::optional<std::pair<uint32_t, uint32_t>> { if (surface && surface->current.width > 0 && surface->current.height > 0) { return std::pair<uint32_t, uint32_t>(static_cast<uint32_t>(surface->current.width), static_cast<uint32_t>(surface->current.height)); } return std::nullopt; } + +} // namespace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_focus.cpp` around lines 475 - 482, The helper get_surface_extent has external linkage but is only used internally; change its linkage by either marking the function static (add the static keyword to the get_surface_extent declaration/definition) or move its definition into the file's anonymous namespace alongside helpers like get_root_xdg_surface so it has internal linkage; update any references if you relocate it and ensure the signature and return type remain unchanged.src/compositor/compositor_core.cpp (1)
334-336: Consider removing redundant conditional.The
if (keyboard)check beforekeyboard.reset()is unnecessary sincereset()on a nullunique_ptris a no-op. This is a minor style nit.♻️ Suggested simplification
- if (keyboard) { - keyboard.reset(); - } + keyboard.reset();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_core.cpp` around lines 334 - 336, Remove the redundant conditional guarding the unique_ptr reset: replace the `if (keyboard) { keyboard.reset(); }` pattern with a single unconditional `keyboard.reset();` call (the `keyboard` unique_ptr already no-ops on null). Locate the usage in compositor_core.cpp where the `keyboard` unique_ptr is reset (search for the `keyboard` identifier in the function surrounding the shown diff) and simplify accordingly.src/compositor/compositor_xdg.cpp (1)
170-203: Consider reducing lock scope when calling wlroots functions.The
hooks_mutexis acquired at line 179 to protect access tostate.xdg_popup_hookswithinget_popup_owner_root_surface. However, the lock is held throughout the call towlr_xdg_popup_unconstrain_from_boxat line 192. Since this is an external wlroots library function that modifies popup state, it could potentially emit signals that trigger callbacks. If any such callbacks attempt to acquirehooks_mutex, a deadlock would occur. Consider restructuring to release the lock after gathering the constraint box parameters but before callingunconstrain_from_box.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compositor/compositor_xdg.cpp` around lines 170 - 203, The lock on hooks_mutex in CompositorState::handle_xdg_popup_commit is held across the wlroots call wlr_xdg_popup_unconstrain_from_box which can emit signals and cause deadlocks; change the code to acquire hooks_mutex only to call get_popup_owner_root_surface and compute a local wlr_box constraint_box (and copy any needed pointers/flags such as hooks->popup) then release the lock before calling wlr_xdg_popup_unconstrain_from_box, and only call the wlroots function when not holding hooks_mutex (ensure any pointers used remain valid after the lock is released).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/compositor/compositor_core.cpp`:
- Around line 212-237: The setup_event_loop_fd function can overwrite an
existing event_fd and leak the old descriptor; update
CompositorState::setup_event_loop_fd to first check whether event_fd already
holds a valid descriptor (e.g., by testing event_fd.get() or a
valid()/is_valid() helper) and if so either return success immediately or
reuse/close it before creating a new eventfd; ensure you also avoid adding a
second wl_event_loop_add_fd when event_source is already set (check
event_source) and only call wl_event_loop_add_fd when creating a new event_fd so
event_source and event_fd remain consistent.
In `@src/compositor/compositor_layer_shell.cpp`:
- Around line 243-248: In the unmap handler add a null check for the keyboard
before using it: ensure keyboard is non-null along with seat, hooks->surface,
and next_focus_surface before calling wlr_seat_set_keyboard and
wlr_seat_keyboard_notify_enter (the same pattern as the map handler), i.e. only
call wlr_seat_set_keyboard(seat, keyboard.get()) and
wlr_seat_keyboard_notify_enter(...) when keyboard != nullptr to avoid
dereferencing a null keyboard.
- Around line 265-270: The destroy handler uses keyboard without null-checking
it; update the conditional that currently checks seat, hooks->surface,
seat->keyboard_state.focused_surface and next_focus_surface to also require
keyboard (e.g., keyboard != nullptr) before calling wlr_seat_set_keyboard(seat,
keyboard.get()) and wlr_seat_keyboard_notify_enter(... keyboard->keycodes,
keyboard->num_keycodes, &keyboard->modifiers). Ensure the same combined
null-check (keyboard) is added so wlr_seat_set_keyboard and
wlr_seat_keyboard_notify_enter are only invoked when keyboard is valid.
- Around line 221-232: In CompositorState::handle_layer_surface_map, avoid
dereferencing keyboard when it may be null: before calling wlr_seat_set_keyboard
and wlr_seat_keyboard_notify_enter, add a null-check (e.g. if (keyboard) { ...
}) and only call wlr_seat_set_keyboard(seat, keyboard.get()) and
wlr_seat_keyboard_notify_enter(seat, hooks->surface, keyboard->keycodes,
keyboard->num_keycodes, &keyboard->modifiers) inside that branch; leave
request_present_reset() and the mapped flag behavior unchanged.
In `@src/compositor/compositor_xwayland.cpp`:
- Around line 291-292: The current associate() stores XWayland metadata into
hooks->title and hooks->class_name once (from xsurface->title /
xsurface->class_) so later renames don't propagate to callers of
get_surfaces_snapshot(); instead, either update the cache when metadata changes
or stop using the cached strings: modify get_surfaces_snapshot() to read the
live xsurface->title and xsurface->class_ fields (or register a metadata-change
callback in associate() that updates hooks->title/hooks->class_name when
xsurface metadata changes) so snapshot generation always reflects current window
title/class.
---
Nitpick comments:
In `@src/compositor/compositor_core.cpp`:
- Around line 334-336: Remove the redundant conditional guarding the unique_ptr
reset: replace the `if (keyboard) { keyboard.reset(); }` pattern with a single
unconditional `keyboard.reset();` call (the `keyboard` unique_ptr already no-ops
on null). Locate the usage in compositor_core.cpp where the `keyboard`
unique_ptr is reset (search for the `keyboard` identifier in the function
surrounding the shown diff) and simplify accordingly.
In `@src/compositor/compositor_focus.cpp`:
- Around line 475-482: The helper get_surface_extent has external linkage but is
only used internally; change its linkage by either marking the function static
(add the static keyword to the get_surface_extent declaration/definition) or
move its definition into the file's anonymous namespace alongside helpers like
get_root_xdg_surface so it has internal linkage; update any references if you
relocate it and ensure the signature and return type remain unchanged.
In `@src/compositor/compositor_xdg.cpp`:
- Around line 170-203: The lock on hooks_mutex in
CompositorState::handle_xdg_popup_commit is held across the wlroots call
wlr_xdg_popup_unconstrain_from_box which can emit signals and cause deadlocks;
change the code to acquire hooks_mutex only to call get_popup_owner_root_surface
and compute a local wlr_box constraint_box (and copy any needed pointers/flags
such as hooks->popup) then release the lock before calling
wlr_xdg_popup_unconstrain_from_box, and only call the wlroots function when not
holding hooks_mutex (ensure any pointers used remain valid after the lock is
released).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24c12bd1-0a48-4ea9-a035-d105c20d32bd
📒 Files selected for processing (5)
src/compositor/compositor_core.cppsrc/compositor/compositor_focus.cppsrc/compositor/compositor_layer_shell.cppsrc/compositor/compositor_xdg.cppsrc/compositor/compositor_xwayland.cpp
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.
Summary by CodeRabbit
Refactor
New Features
Documentation