#+TITLE: Review: music-config Without EMMS -- Implementation Readiness #+AUTHOR: Craig Jennings #+DATE: 2026-05-15 #+OPTIONS: toc:nil num:nil Spec reviewed: =docs/design/music-config-without-emms.org= Third pass. This review answers the single question: is the spec in shape to start =/start-work= against? Prior reviews (architectural critique, test/perf/parity/packaging analysis) are in git history and remain accurate. This pass distills them into a go / no-go and a punch list of exactly what must change before implementation starts. * Verdict **No, not quite.** Step 1 of the Migration Plan can start today. Steps 2-7 need five interlocked decisions resolved in the spec first. Without them, the implementer would be making those decisions mid-flight -- exactly when the cost of being wrong is highest. Estimated spec work to reach implementable: **1-2 hours of focused editing.** No new analysis needed; the answers are already in the prior reviews. * What is implementable today The pure-helper extraction (Migration Plan step 1) does not depend on any of the blocking decisions. Specifically: - =cj/music--valid-file-p= (with =webm= and =ape= added per the prior format audit) - =cj/music--valid-directory-p= - =cj/music--collect-entries-recursive= - =cj/music--completion-table= - =cj/music--m3u-file-tracks= - =cj/music--get-m3u-files= / =--get-m3u-basenames= - =cj/music--safe-filename= - =cj/music--format-duration= - =cj/music--append-track-to-m3u-file= - =cj/music--assert-valid-playlist-file= - =cj/music-create-radio-station= These are already EMMS-free in spirit; their tests already pass without EMMS mocking. Extracting them is roughly a 2-3 day commit with characterization tests as the safety net. That work can start before the spec revisions land. But it should not be open-ended -- it is one phase, after which work stops until the spec is revised. * What is blocking the rest Five interlocked decisions. Each is small to resolve; the cost of not resolving them is large. ** B1: mpv IPC transport Spec lines 125-131 punt on this with "Pause/seek/volume can use mpv's JSON IPC socket if available; otherwise v1 may support only play/stop/next/previous and leave pause/seek/volume as explicit follow-up work." The fallback path produces a worse player than today's EMMS+mpv (which has working pause). Per-track-restart pause is an 80-200 ms spawn cost plus the loss of the playing position. That is a user-visible regression. **Decide**: long-running mpv + JSON-IPC socket from day one. This blocks Migration Plan step 4 (mpv backend). ** B2: state-change hook contract Today's module subscribes to six EMMS hooks (=started=, =stopped=, =paused=, =finished=, =cleared=, plus advice on the three toggle commands). The header overlay, consume-after-finish, random-history, auto-advance, and modeline updates all depend on them. Spec lists these hooks under "Existing EMMS Coupling" but the Proposed Architecture has no replacement -- the backend protocol on spec lines 100-111 is read-side only. **Decide**: abnormal hook =cj/music-state-change-functions= fired by the backend with a plist =(:event STARTED|STOPPED|PAUSED|RESUMED|FINISHED :track TRACK :error MAYBE)=. Five events. Dispatched from the mpv IPC process filter on the corresponding mpv events (=playback-restart=, =pause=, =end-file=). This blocks Migration Plan step 5 (rewire public commands). Without it, "rewire" has no API to rewire to. ** B3: selected-track representation Spec proposes =selected-index= in the playlist struct -- fine for state. But the buffer view still needs a marker / overlay / face on the playing line. The current code uses =emms-playlist-selected-marker= (a buffer marker) plus =emms-playlist-selected-face=. **Decide**: overlay at the line whose index matches =selected-index=, face =cj/music-playlist-selected-face=, repositioned on each =STARTED= event. Single overlay reused across track changes. This blocks playlist-view rendering and reorder behavior. ** B4: test architecture ~110 of the 252 existing tests mock EMMS at the boundary (=emms-next=, =emms-random=, =emms-playlist-track-at=, =emms-playlist-mode-kill-track=, etc.). Without a shared fake-backend testutil, the rewrite happens 110 times in 110 different shapes. **Decide**: =tests/testutil-music-backend.el= provides =cj/test-music-fake-backend= (matching the real backend's plist shape) plus an =:events= ledger that records each call as =(verb . args)=. Tests bind =cj/music-current-backend= to the fake and assert sequences against the ledger. Build this first; rewrite tests against it second. Without this, effort balloons by roughly a factor of three. ** B5: cross-platform stance mpv's IPC works over unix domain sockets on Linux+macOS but uses named pipes on Windows. Emacs's =make-network-process= does not natively support Windows named pipes. **Decided this session (Craig, 2026-05-15)**: **best-effort on Windows.** v1 ships full-feature on Linux + macOS; Windows gets a degraded mode with play / stop / next / previous only (no pause, no seek, no volume) via =start-process= + stdin or one-shot =call-process= per command. The README documents the limitation. Anyone who wants to fund Option B (=mpvc.exe= shellout) or Option C (=w32-*= named-pipe client) can do it as a follow-up. This unblocks the implementer to write Linux+macOS code without spending the v1 budget on Windows IPC plumbing. Quote Craig in the spec body so the rationale survives. * Smaller items the implementer should not have to invent ** S1: feature-parity scope 13 EMMS features are missing from the spec entirely (catalogued in the prior parity review): seek forward (=f=), seek backward (=b=), volume up (=+= / ===), volume down (=-=), one-shot shuffle (=Z=), info popup (=i=), center-on-current (=o=), kill-track (=d= or similar), bury buffer (=q=), append-to-M3U (=A=), active-window background tint, dired/dirvish add (=+= in dired). **Decide**: preserve all 13 in the playlist-mode keymap. Volume and seek depend on B1 (IPC); if B1 is decided in favor of IPC, they all come along for free. Dired/Dirvish add is independent and worth explicitly listing in the Goals section. ** S2: URL-vs-file routing Today's mpv player detects URLs vs files via =emms-player-mpv-regexp=. Spec does not carry this forward. **Decide**: small helper #+begin_src elisp (defun cj/music--track-type-from-name (name) (cond ((string-match-p "\\`\\(?:https?\\|mms\\)://" name) 'url) ((cj/music--valid-file-p name) 'file) (t nil))) #+end_src Used when constructing tracks; populates the existing =type= slot. ** S3: metadata strategy Track struct includes =title=, =artist=, =duration= but the spec says "tag extraction can be added later." **Decide one of two**: - Drop the metadata fields from v1's struct. Track description falls back to filename-without-extension (matches current =cj/music--track-description= behavior for untagged files). - Keep the fields; populate via mpv IPC =get_property metadata= on the =STARTED= event. Lazy, free, fits the IPC story from B1. Recommend the second. It's two lines of code given B1's IPC infrastructure is already there. ** S4: scope declaration Spec is ambiguous on whether this stays a personal module or ships as a public package. **Decide**: personal Phase 1 (=modules/music-config.el=, =cj/= namespace). Re-evaluate Phase 3 (=cadenza=, GitHub, MELPA) after ~4 weeks of daily use. Quote Craig's preference for this phasing in the spec body. * The exact spec edits Concrete inserts and deletions, in order. Roughly 1-2 hours of focused work, no new analysis. 1. **Rewrite "Initial Backend"** (spec lines 118-133) -- drop the "if available" / "otherwise" fork. Single decision: long-running mpv with =--input-ipc-server==, JSON commands over =make-network-process :family 'local=, async event filter on the socket's process buffer. 2. **Insert "Process Lifecycle"** subsection after "Initial Backend" -- one mpv per Emacs session, killed on =kill-emacs-hook=, socket file removed in the same hook. Stale-socket cleanup on startup (loop =/tmp/cj-music-mpv-*.sock= or the platform equivalent and unlink). 3. **Insert "State-Change Hooks"** subsection after "Backend Protocol" -- =cj/music-state-change-functions= abnormal hook with the 5-event plist contract. List the mpv events that trigger each. 4. **Insert "Selected-Track Representation"** subsection in the Playlist Buffer section -- single overlay at the line whose track-index matches =selected-index=, face =cj/music-playlist-selected-face=, repositioned on the =STARTED= event. 5. **Insert "Test Architecture"** subsection at the top of the Migration Plan -- =testutil-music-backend.el=, fake-backend plist shape, events ledger, characterization tests for the six user-facing commands (=next=, =previous=, =toggle-consume=, =playlist-toggle=, =playlist-load=, =playlist-clear=) before any non-pure code moves. 6. **Insert "Platform Support"** subsection -- quote Craig's 2026-05-15 decision: full features on Linux + macOS; Windows is best-effort with play/stop/next/previous only. Document in the README and surface in =M-x cj/music-doctor= when run on Windows. 7. **Rewrite the track struct definition** (spec line 72-79) per S3 -- either drop metadata fields or keep with a one-line note about lazy population via mpv IPC. 8. **Expand "Goals"** to list the 13 missing features per S1 with "preserve" disposition. Mark volume / seek as "preserve if B1=IPC, drop otherwise" -- but B1 is IPC, so they all stay. 9. **Insert URL-detection helper** (S2) in the M3U Handling section or alongside the track struct. Cite where it's used. 10. **Insert "Scope"** subsection before Goals -- this is a personal module first; potential public package later. Quote Craig's preference. Name the future public name candidate (=cadenza=). 11. **Update "Existing EMMS Coupling"** inventory with the surfaces missed in the prior review (kill-track, shift-track, bury, selected-marker, =emms-source-file-default-directory=, =emms-player-mpv-parameters=, =emms-player-mpv-regexp=, =emms-track-description-function=, the custom-set-faces block, and the =emms-all= / =emms-mode-line-mode= setup calls). 12. **Strengthen "Acceptance Criteria"** with a parity-walk entry (the 22-step user workflow from the prior review). 13. **Add "Effort estimate"** and "Risks" sections to match the format of the other =docs/design/= specs. 14. **Add ~webm~ and ~ape~** to =cj/music-file-extensions= (or note that the migration step does so). Two extensions, two-line edit; the audit confirms files of both types exist in =~/music=. * What stays as-is The spec's data model (track struct, playlist struct), backend plist shape, M3U-as-persistence stance, pure-helpers-first migration ordering, and the overall layering (data / backend / view) are all sound. Don't rework them. The fixes above are inserts and clarifications, not architectural changes. * Recommended next step Two paths, pick one: 1. **Apply the 14 edits directly.** I draft them as concrete inserts into the spec, you review the final spec end-to-end, then =/start-work= against the revised spec. Roughly the same 1-2 hours but spent on review rather than writing. 2. **You apply the edits.** This review is the punch list. Work through items 1-14, ping me when ready, I do one more pass to verify, then =/start-work=. Either way, the next milestone is "the spec passes Phase 3 (Approach) of =/start-work= without me having to invent the missing decisions on the fly." Until then, only the pure-helper extraction (Migration Plan step 1) is safe to start.