Split PlayerSession into PlaybackEngine + three stores (fixes #60, #62) - #79
Open
KyNorthstar wants to merge 4 commits into
Open
Split PlayerSession into PlaybackEngine + three stores (fixes #60, #62)#79KyNorthstar wants to merge 4 commits into
KyNorthstar wants to merge 4 commits into
Conversation
`displayName`, `systemImageName_menuItem`, and `systemImageName_preview` lived in a private extension at the bottom of LibraryView.swift instead of beside RepeatMode itself. Moved them to a new RepeatMode + display.swift, following the same Type + purpose.swift pattern already used by NativeImage + placeholder.swift and NativeImage + thumbnail.swift. No logic changes. Both call sites in LibraryView.swift still resolve against the new public extension. Fixes #58 Co-authored-by: Claude 5 Opus <feedback+claude@kynorthstar.me>
This was
linked to
issues
Sep 7, 2026
MediaPlayerView owned the AVPlayer as @State, with isPlaying fed by a rate publisher and then consumed to command the player right back — both an input and an output on the same property. Moving AVPlayer ownership into the model layer fixes that: PlaybackEngine now owns the player and everything around it (remote commands, Now Playing info, PIP-adjacent state), and isPlaying is written in exactly one place. That's a fourth domain PlayerSession would otherwise have absorbed on top of the three it already had (now-playing/queue, history, saved playlists), so this splits those into NowPlayingStore, HistoryStore, and PlaylistLibraryStore at the same time. PlayerSession stays as a thin facade composing all four — still the one object views hold. requestPlaybackOnNextLoad() / takePlaybackIntent() / takePendingRestoredSeek() are gone: they existed only because the session didn't own the player, parking intent across a SwiftUI update cycle. NowPlayingStore drives the engine directly instead. MediaPlayerView now owns no playback state — it reads and displays session.engine. Gains init(playing: URL) / init(playing: [URL]) for drop-in single-file use: builds a memory-only PlayerSession, so playback batteries (remote controls, Now Playing info, PIP) work without writing history or a saved playlist anyone didn't ask for. Behavior changes, both intentional: - Tapping the already-current queue entry now plays it if paused (the old no-op only existed to avoid stranding parked intent) - Remote-control-event registration now follows the engine's load/ unload rather than MediaPlayerView's onAppear/onDisappear currentPlaylist: dropped from MediaPlayerView's initializer — the view has no use for it once the store drives the engine. One call site (ContentView) updated. No compiler available in this environment; verified by inspection and tracing every call site by hand, not by building. Said so in the PR. Fixes #60 Fixes #62 Co-authored-by: Claude 5 Opus <feedback+claude@kynorthstar.me>
KyNorthstar
force-pushed
the
feature/62-Great-Player-Session-Restructuring
branch
from
September 7, 2026 06:16
841c138 to
88ac41c
Compare
KyNorthstar
added a commit
that referenced
this pull request
Sep 7, 2026
Hoist guard let self out of the nested assumeIsolated closure into the outer one that actually declares [weak self] — implicit-self-after- guard only exempts a closure that unwraps in its own scope. Co-authored-by: Claude 5 Opus <feedback+claude@kynorthstar.me>
Hoist guard let self out of the nested assumeIsolated closure into the outer one that actually declares [weak self] — implicit-self-after- guard only exempts a closure that unwraps in its own scope. Co-authored-by: Claude 5 Opus <feedback+claude@kynorthstar.me>
KyNorthstar
force-pushed
the
feature/62-Great-Player-Session-Restructuring
branch
from
September 7, 2026 06:25
482c457 to
55221e3
Compare
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
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.
What
PlaybackEngine— owns theAVPlayer, remote transportcommands, Now Playing info, the periodic time observer, per-item
end/time-jump sinks, restored-seek readiness KVO.
isPlaying(andevery other fact) is written in exactly one place.
NowPlayingStore,HistoryStore,PlaylistLibraryStore— thethree domains
PlayerSessionis a god-object — three unrelated domains in one class #62 already named, each owning its own persistence.PlayerSession(779 → 336 lines) is now a thin facade composingall four.
MediaPlayerView(634 → 238 lines) owns no playback state — readsand displays
session.engine. Addsinit(playing: URL)/init(playing: [URL])for genuinely drop-in single-file use.Why
Fixes #60 by construction: nothing outside
PlaybackEnginewritesisPlaying. Fixes #62 by not adding a fourth responsibility to thegod object while fixing #60 — splitting now instead.
Concrete payoff beyond tidiness:
requestPlaybackOnNextLoad()/takePlaybackIntent()/takePendingRestoredSeek()are deleted.Their own doc comments explained they existed only because "the
session doesn't own the player" — once
NowPlayingStoreandPlaybackEngineare composed together, that's no longer true, sothere's nothing left to park.
Naming: a
Storeis durable data independent of any player instanceand persists itself; the
Engineis live machinery that exists onlywhile something is loaded and persists nothing.
PlaybackEngine, notPlayer— that name's taken by the existingAVPlayerViewControllerwrapper.
Behavior changes (intentional)
rather than no-opping — the no-op only existed to avoid stranding a
parked intent that no longer exists.
play/unload rather than the view's
onAppear/onDisappear—batteries no longer depend on a specific View staying alive.
Deliberately not done here
PlaybackEnginewith its delegate is trivially testable outside a View), but that's
its own PR.
MediaPlayerViewstill draws the title/creator block, sonightlydoesn't regress before the pinned-header UI lands.
@Observablemix moved into theengine unchanged.
Verification
No Xcode in my environment. Verified by grepping for every removed
symbol across the tree and tracing every call site by hand — not by
building. A real build is still the first thing this needs.
Journal:
LLM transparency/Claude Fable 5.1 • Issues #60 & #62 journal.mdFixes #60
Fixes #62
Depends on #77