Pocket Player — Architecture Review

Files

• MainWindow.h:159–235 — ~60 members
• MainWindow.cpp:290–390 — startLibraryThread (2 worker threads, ~20 connects)
• MainWindow.cpp:339–389 — import pipeline wired as lambdas
• MainWindow.cpp:2272–2305 — startSubsonicSync lifecycle

Problem

A UI class belongs here — but dissolved into it are whole subsystems that aren't UI: the two-phase import coordination (enumerate → hydrate → write m3u8 → notify), the worker-thread ownership for the library + probe, and the Subsonic-sync lifecycle. The tell is 25 project includes: MainWindow depends on nearly the whole codebase because it is the wiring harness. It changes for a dozen unrelated reasons (Divergent Change).

Solution (sketch — no interfaces yet)

Shed the non-UI coordinators into their own deep modules — an import coordinator owning :339–389, a library host owning the thread wiring, a Subsonic sync host owning :2272–2305. MainWindow keeps layout + user-intent routing and talks to each through a narrow seam.

Benefits

• Locality: "how does an import finish?" lives in one module, not smeared across constructor lambdas.
• Leverage: each coordinator becomes reusable/mockable; the pop-out and CLI paths could drive the same seam.
• Testability: the import + sync state machines gain an interface to drive — today they're only reachable by constructing the whole window.

Files

• Stringly-typed QSettings keys in 14 files
• SettingsDialog.h:29–32 — 11-arg positional ctor
• MainWindow.cpp:1402–1406 — re-reads keys the dialog already wrote
• SettingsDialog.h:105–136 — some controls self-persist, others don't

Problem

Two contradictory persistence contracts coexist. Getter-based settings (folders(), restoreQueue()…) are read by the host on Accept; other controls write QSettings themselves. So after one dialog round-trip, MainWindow must re-read playback/preferHq and playback/ignoreTitles back out to push them into the controller (:1402–1406). Keys like ui/restoreQueue are duplicated 2–3× as bare strings; a typo in one site fails silently. This is Primitive Obsession at codebase scale — a domain concept (a setting) smeared as loose strings instead of a type.

Solution (sketch)

One deep Settings module owns every key and its default; both the dialog and the app read/write through it. The QSettings backend hides behind that single seam (so an in-memory adapter becomes possible for tests). The 11-arg ctor collapses when the dialog reads settings itself rather than being handed them positionally.

Benefits

• Locality: a key is defined once; typos become compile errors, not silent no-ops.
• Leverage: one place gives every caller defaults, migration, and change-notification.
• Testability: the seam admits a fake store — settings-dependent logic becomes unit-testable without a real QSettings file.

Files

• MainWindow.cpp:870–917 — resolveSubsonicCover
• MainWindow.cpp:858 & 872 — subsonic-cover: token parsed twice
• SubsonicClient.h:72–76 — "covers NOT downloaded here"

Problem

The window parses the subsonic-cover:<server>:<id> token, SHA-1s it to a cache filename, checks the disk cache, resolves the server, builds the auth URL via SubsonicClient::coverArtUrl(), issues a raw QNetworkAccessManager::get, and writes the bytes — all Subsonic-client work implemented in the UI, guarded by an ad-hoc m_currentCoverToken race flag. The token format is a shared secret with no single owner. "How does a Subsonic cover appear?" forces a bounce across MainWindow ↔ SubsonicClient ↔ the DB's art_url token — Feature Envy across a leaky seam.

Solution (sketch)

Move cover resolution behind the Subsonic seam: the token, cache path, URL, and fetch become one module's business. The window asks for "the cover for this track" and receives a path — it stops knowing the token grammar or the auth scheme.

Benefits

• Locality: one owner for the token grammar + cache + auth — no more parse-in-two-places.
• Leverage: any surface (pop-out, notifications, MPRIS art) reuses the same cover resolver.
• Testability: cover-path logic becomes testable off a fake network; today it's welded to a live QNetworkAccessManager in the UI.

Pairs naturally with SubsonicClient's split-brain: it already mixes a static server config store (servers()/saveServers()) with an async sync client — the same module wanting to be two.

Files

• PlayerController.h:163–168 — 7 coupled members
• PlayerController.cpp:221–265 — playInternal 5-way branch
• PlayerController.cpp:284–296 — decideAutoNext (shuffle dedup)
• PlayerController.cpp:298–311, 603–627 — two consumers kept in lockstep

Problem

The remote-play plan is spread across m_autoNext, m_prefetchUrl, m_prefetchStream, m_prefetchTriggered, m_history, m_historyPos, m_index — invariants enforced only by prose comments. playInternal branches 5 ways (remote / subsonic / prefetched / in-flight-prefetch / fresh), each poking prefetch state differently; decideAutoNext rolls the shuffle choice "once here so it isn't repeated by the prefetch and the actual advance." This is exactly where real bugs live (gapless prefetch, double-rolled shuffle) — and it's reachable only by driving QMediaPlayer::MediaStatus transitions through a worker thread. Meanwhile the pure helpers that got extracted (applyPreferHq, qualityTier) are the parts that need testing least. Testability was extracted where the bugs aren't.

Solution (sketch)

Pull the queue/history/prefetch decision into a deep module that takes the current state + an event ("track ended", "prefetch resolved") and returns what to do next — no QMediaPlayer, no threads. The controller keeps the I/O; the planner keeps the logic.

Benefits

• Testability: "prefetched track loads without a gap" and "shuffle doesn't double-roll" become plain unit tests — no event loop.
• Locality: the seven fields + their invariants live in one place instead of four methods and a comment thread.
• Leverage: a pure planner is trivial to reason about and to extend (e.g. new repeat modes).

Files

• YtDlp.h (52) · RemoteResolver.h/.cpp (34 / 43)
• SubsonicClient.h (103) · Importer.h (119)

Problem

Four modules all mean "run something async, hand back results," each re-deriving the same shell: an in-flight guard, a supersede-previous rule, a status(QString) progress signal, and a (done, failed) terminal pair. Importer and SubsonicClient independently re-invent "job queue + in-flight counter + emit-once-drained." That's Duplicated Code at the shape level.

Note too that RemoteResolver is borderline shallow: its whole .cpp is 43 lines — set the URL, run yt-dlp -g, split stdout, emit the first line. Its header even calls ytDlpPath() "a thin wrapper." Its interface is nearly as big as its body; it earns only a little keep (DRM-message mapping + pageUrl guard).