refactor(compositor): remove pimpl wrapper from CompositorServer

[K1ngst0m]

Summary by CodeRabbit

• Refactor
  • Improved internal state management architecture in the compositor server for enhanced code organization and maintainability.

[qodo-code-review]

Review Summary by Qodo

Remove pimpl wrapper from CompositorServer

✨ Enhancement

Walkthroughs

Description

• Remove pimpl wrapper pattern from CompositorServer class
• Replace m_impl->state with direct m_state pointer
• Simplify member variable access throughout codebase
• Add forward declaration of CompositorState struct

Diagram

flowchart LR
  A["CompositorServer with Impl pimpl"] -->|Remove Impl wrapper| B["CompositorServer with direct CompositorState"]
  C["m_impl->state access pattern"] -->|Simplify to| D["m_state access pattern"]

Loading

File Changes

1. src/compositor/compositor_server.hpp Refactoring +3/-2

Remove pimpl wrapper and add forward declaration

• Remove nested Impl struct definition
• Replace std::unique_ptr<Impl> m_impl with std::unique_ptr<CompositorState> m_state
• Add forward declaration of CompositorState struct

src/compositor/compositor_server.hpp

---

2. src/compositor/compositor_server.cpp Refactoring +9/-9

Update all member variable references to use m_state

• Update constructor to initialize m_state directly instead of m_impl
• Replace all m_impl->state references with m_state or *m_state throughout file
• Simplify state access in start(), stop(), and getter methods

src/compositor/compositor_server.cpp

---

3. src/compositor/compositor_cursor.cpp Refactoring +1/-1

Update cursor visibility method call

• Replace m_impl->state.set_cursor_visible() with m_state->set_cursor_visible()

src/compositor/compositor_cursor.cpp

---

View more (5)

4. src/compositor/compositor_focus.cpp Refactoring +4/-4

Update focus management method references

• Replace all m_impl->state references with m_state in focus-related methods
• Update is_pointer_locked(), get_surfaces(), set_input_target(), and
 request_surface_resize() methods

src/compositor/compositor_focus.cpp

---

5. src/compositor/compositor_input.cpp Refactoring +2/-2

Update input event injection references

• Replace m_impl->state.event_queue with m_state->event_queue
• Replace m_impl->state.wake_event_loop() with m_state->wake_event_loop()

src/compositor/compositor_input.cpp

---

6. src/compositor/compositor_present.cpp Refactoring +3/-3

Update presentation frame access references

• Replace m_impl->state.present_mutex with m_state->present_mutex
• Replace m_impl->state.presented_frame with m_state->presented_frame in frame retrieval method

src/compositor/compositor_present.cpp

---

7. src/compositor/compositor_state.hpp Refactoring +0/-4

Remove Impl wrapper struct definition

• Remove CompositorServer::Impl struct definition that wrapped CompositorState

src/compositor/compositor_state.hpp

---

8. tests/render/test_filter_boundary_contracts.cpp 🧪 Tests +2/-2

Update test string search patterns

• Update test assertions to search for m_state->target_fps instead of m_impl->state.target_fps
• Update test assertions to search for m_state->wake_event_loop() instead of
 m_impl->state.wake_event_loop()

tests/render/test_filter_boundary_contracts.cpp

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

ⓘ The new review experience is currently in Beta. Learn more

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request refactors CompositorServer to replace an internal pimpl wrapper pattern (m_impl) with direct ownership of CompositorState via m_state. All method implementations are updated to reference state through the new member, and the nested Impl struct is removed. Public API signatures remain unchanged.

Changes

Cohort / File(s)	Summary
State member migration src/compositor/compositor_server.hpp, src/compositor/compositor_state.hpp	Replaces std::unique_ptr<Impl> m_impl with std::unique_ptr<CompositorState> m_state; adds forward declaration of CompositorState and removes nested Impl struct definition.
Method implementation updates src/compositor/compositor_cursor.cpp, src/compositor/compositor_focus.cpp, src/compositor/compositor_input.cpp, src/compositor/compositor_present.cpp, src/compositor/compositor_server.cpp	Updates all member accesses in CompositorServer methods from m_impl->state to m_state; no control flow or logic changes.
Test boundary contracts tests/render/test_filter_boundary_contracts.cpp	Updates expected member access patterns in boundary-contract string checks from m_impl->state to m_state.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(present): unify frame pacing across app-compositor-viewer workflow #119: Directly related as it added the compositor state members (e.g., target_fps, wake_event_loop) via m_impl->state that this PR now accesses through m_state.
• refactor(compositor): decompose compositor server implementation #112: Related to the broader refactor of extracting and centralizing CompositorState from the nested Impl structure.
• feat(compositor): unify surface selection #80: Related as it modifies internal state object and access patterns within CompositorServer::Impl that are now consolidated into direct m_state ownership.

Suggested labels

review-effort-3/5, refactoring

Poem

🐰 A wrapper falls away so clean,
Direct and true, no in-between,
The state hops forth through m_state's gate,
Simplicity—a rabbit's fate!
✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: refactoring CompositorServer to remove its pimpl wrapper (Impl) and directly use CompositorState via m_state instead of m_impl->state.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch dev/refactor-impl

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/render/test_filter_boundary_contracts.cpp (1)

183-186: Optional: reduce brittleness of source-text contract checks.

These assertions are valid for this PR, but they tightly couple tests to member names. Consider matching higher-level intent (e.g., ordering of store + wake call) with less exact string dependency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/render/test_filter_boundary_contracts.cpp` around lines 183 - 186, The
test currently couples to exact member names by searching for
"m_state->target_fps.store(...)" and "m_state->wake_event_loop()", so change the
assertions to instead detect the intended ordering (a store followed by a wake)
using less brittle text checks: use compositor_server_text->find to search for a
more general substring like "store(" or "target_fps" and "wake_event_loop" (or a
regex matching "store\\s*\\(") and assert compositor_set_target_pos <
compositor_wake_pos, keeping the existing variables compositor_set_target_pos
and compositor_wake_pos to locate positions without depending on the full member
name.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/render/test_filter_boundary_contracts.cpp`:
- Around line 183-186: The test currently couples to exact member names by
searching for "m_state->target_fps.store(...)" and "m_state->wake_event_loop()",
so change the assertions to instead detect the intended ordering (a store
followed by a wake) using less brittle text checks: use
compositor_server_text->find to search for a more general substring like
"store(" or "target_fps" and "wake_event_loop" (or a regex matching
"store\\s*\\(") and assert compositor_set_target_pos < compositor_wake_pos,
keeping the existing variables compositor_set_target_pos and compositor_wake_pos
to locate positions without depending on the full member name.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 244ea4a5-c294-4265-88f2-180f3ac393bc

📥 Commits

Reviewing files that changed from the base of the PR and between 2f00089 and 69ca53a.

📒 Files selected for processing (8)

• src/compositor/compositor_cursor.cpp
• src/compositor/compositor_focus.cpp
• src/compositor/compositor_input.cpp
• src/compositor/compositor_present.cpp
• src/compositor/compositor_server.cpp
• src/compositor/compositor_server.hpp
• src/compositor/compositor_state.hpp
• tests/render/test_filter_boundary_contracts.cpp

💤 Files with no reviewable changes (1)

• src/compositor/compositor_state.hpp