# Notes for the codebase review Collected while building M5 to M7 and the follow-ups. Each item names where to look. Ordered roughly by how much it would pay back. ## Structure - **`State` is a god object.** It has 70 public fields in `state.rs`. They cover backend handles, every smithay protocol state, layout, animation, capture, imports, Xwayland, lock, idle, activation and launch tracking. Every module is `impl State`, so nothing has a boundary and any function can touch anything. The protocol states are mostly inert after construction. They could live in one `Protocols` struct, with the live subsystems (lock, captures, imports, xwayland, toplevel handles) owning their own data and taking narrow arguments. - **Three copies of "draw an output".** `drm::render_surface`, `nested::render` and `capture::output_elements` each assemble cursor plus lock-or-scene, and each destructures `State` field by field to satisfy the borrow checker. `capture::output_elements` is the general version. The backends should call one function and keep only presentation. - **`scene::build` takes ten arguments** and carries `#[expect(clippy::too_many_arguments)]`. Most of them are one borrowed view of the world. A `SceneInput` struct, or the split above, removes it. - **`drm::render_surface` does too much.** It handles VRR switching, timing and budget, scene assembly, lock confirmation, capture damage, frame callbacks and the slow-frame log. VRR and stats could be methods on `Surface`. - **`handlers.rs` is a grab bag.** It holds compositor commit logic, xdg-shell, decorations, seat, selection, DnD, syncobj, `hold_until_readable`, workspace actions and fullscreen. The commit handler alone runs import, window commit, unmapped mapping, first-buffer, popups, layers and capture damage in sequence. A per-role dispatch would read better. - **`layout/src/lib.rs` is 1000 lines.** Tags, outputs, parking, scratchpads, focus history and arrangement share one `impl Layout`. Scratchpads and output parking look separable. - **`render-vk/src/renderer.rs` is 1100 lines.** It mixes the smithay trait impls, submission and stall handling, the shm cache, read-back and the new `DmabufImporter`. ## Duplication - `CLEAR` is defined identically in `drm.rs`, `nested/mod.rs` and `capture.rs`. - Closing a window (xdg `send_close` or X11 `close`) is written out in both `commands.rs` and `toplevels.rs`. It belongs on a window helper next to `root()` and `identity()`. - Window lookup is a linear scan: `window_id` by surface, `x11_window_id`, the toplevel key lookup in `toplevels.rs`, and `ListHandles::window`. That's fine at desktop window counts, but four spellings of the same search should be one index. - Smoke-test shell scripts were rewritten for every protocol (`/tmp/m4fix/smoke_*.sh`). A small `xtask` or `.nu` harness that starts nested monolith with a client list and collects logs would make protocol checks repeatable, and could gate CI later. ## Code quality - **Per-dispatch work that doesn't need to run.** `after_dispatch` runs `publish_workspaces` and `publish_toplevels` after every batch of events. They rebuild full snapshots and lock every toplevel's data for titles even when nothing changed. A dirty flag set by commits, layout changes and focus changes would make them nearly free. - **Silent `let _ =`** on X11 `configure` and `set_*` calls, on `notifier.successful`, and on several protocol sends. Most are fine, but the X11 ones hide real failures. - `configure()` in `state.rs` handles both xdg and X11 windows with two inline branches. Once more window kinds exist (none planned), it wants a small trait. - **`Xwayland` lifecycle.** A SIGKILLed monolith leaves its `/tmp/.X*-lock` and socket behind. `claim` handles stale locks by pid, but a recycled pid makes it skip that display for good. Low priority. - `Focus::X11(Box)` exists only to satisfy `large_enum_variant`. That's fine, but `X11Surface` is 432 bytes, which is worth knowing. - Comment density is uneven. Some modules carry a doc line on most items, others none. Check against the comment rules in one sweep. ## Tests - **The layout proptest pulls its weight.** It caught the hidden root-leaf bug the moment the "hidden tiles have no area" invariant was added. More invariants in `check_geometry` (focus always visible, tags always viewed somewhere) would be cheap and high-yield. - **Nothing tests the protocol servers.** `workspace.rs` and `toplevel.rs` diff snapshots by hand, which is exactly the strange, racy logic the test policy wants covered. Candidates are a snapshot-diff test driving two `update` calls through a client, or a pure function that computes the diff and a test of that. - `render-vk` has golden and dmabuf tests. Nothing pins the submit-stall absorption logic, since `StallPhase::saw` is pure and tiny. It's a candidate only if it ever changes. - `monolith` itself has no tests, so behaviour such as lock confirmation, capture waiting and import blockers is only smoke-tested. Most of it needs a display. The pure parts (the frame budget, `floats()`, workspace filtering) could be pulled out and tested if they ever break. ## Smaller observations - `state.rs` free functions `root`, `identity`, `configure`, `gaps` and `xkb_config` are window, layout and config helpers living in the state module. - The `layers.rs` debug lines added to chase the quickshell report can be dropped or kept at trace. - `design/parked.md` and this file overlap a little. Parked is for behaviour and this is for structure.