The detail heroes ask for one entity's status at a time. The list surfaces
need every status on screen at once, so add one grouped flow per tab that
returns a lookup of entity id to counts, which the tiles and rows read the way
song rows read the per-song map.
ChoiceGroup emitted its header and option rows as siblings straight into the AlertDialog text slot, and they drew on top of each other, leaving only one radio visible with garbled labels. Wrap the group in its own Column so the rows stack, and cover it with a test that asserts the presets occupy separate rows.
Third review round:
- `promote` read every source's rows, so a single inactive source with waiting
rows could fill the window and starve the source in use: its songs stayed
queued with no engine id, for good, through every tick. It now scopes to the
active source, which is also the only one that can resolve a URL here.
- An artist with some songs downloaded offered only Download, because the more
menu rendered `action()` rather than the actions the state offers. It renders
the whole list now, so a partial artist gets Download and Delete.
- The hero's long press is only advertised when there is something to delete,
instead of announcing a Delete it would ignore, and it declares a button role
again after losing `IconButton`.
- The delete dialog uses the platform size formatter rather than a hand-rolled
one, which also fixes 999_999 reading as "1000.0 kB".
- The poll warms artwork through the same helper reconcile uses.
- Cancelling a big playlist did per-row work: a binder cancel, two queries and a
delete each, thousands of times, under the lock. The platform takes a list of
ids in one call, so the cancel is now one binder call, one delete per chunk of
rows and one sweep, which is both the several seconds and the stale button.
- Partial files are covered by the same sweep, which already deletes any file in
a source directory that no row names; a test now proves a stray .part goes with
the cancel.
- The hero's control was a plain clickable Box after the long-press work, so its
splash was square; it clips to a circle the way an IconButton does.
The hero's control now carries the whole rule: a tap does what the state offers
(Download, Cancel, Delete) and a long press asks to delete when there is anything
downloaded to delete, so a partially downloaded album can be emptied without
starting the rest first. Nothing downloaded means the long press does nothing.
The rule itself returns the actions rather than one of them, so a partially
downloaded list offers both Download and Delete; the tap takes the first. That
list is what the library tabs' context menus will read.
Only a single song deletes without asking; the artist's delete now opens the same
dialog as the album and playlist, naming the artist and the space it frees. The
artist's download action stays in its more-menu rather than getting a visible
control, deliberately, so bulk downloading a whole artist is a considered
action.
Two things a 3k-song playlist turned up, plus the hero's downloaded state.
- The platform runs about eight downloads at a time, but a bulk action handed it
every song at once. Three thousand pending requests meant a notification flood,
and a poll tick that rewrote every row's bytes/total, so the whole list churned
ten times a second. Now a bulk action only records the songs and hands over at
most IN_FLIGHT_LIMIT, oldest first, promoting the next as each finishes, so our
row state mirrors what is actually running and a cancel only has to reach what
is in flight.
- Cancelling was therefore unusable at that size. It now touches at most the
in-flight requests instead of thousands, and the rows wait behind it.
- A queued row with no engine id is waiting its turn, not lost, so reconcile no
longer re-enqueues the whole list on every pass; a row whose engine entry
vanished goes back to waiting and the next promote picks it up. A song that
cannot be handed over fails instead of being retried every tick.
- Requests follow the order the screen shows: artist songs resolve through the
same albums ordering the artist page uses, so they were the one list that did
not.
- The hero's fully-downloaded state shows the downloaded glyph rather than a bin,
and tapping it asks before deleting, naming the album or playlist and the space
it frees.
Second review round on the bulk downloads:
- The cancel-during-bulk fix had no test, so the invariant it exists for could
regress silently. The fake engine can now delay an enqueue, which holds the
race window open: the queue loop is kept busy, a cancel is fired mid-queue,
and the test asserts the whole list ends up cancelled. Reverting to per-song
locking fails it, which I checked.
- The per-song catch around the queue loop swallowed CancellationException, so
cancelling the caller did not stop the loop. It rethrows now.
- The artwork pass skips songs whose row a cancel has already taken.
- The lock-hold ceiling is recorded where the lock is taken: cancel latency and
the poll tick both scale with the list length, and a per-list cancel
generation would lift that if it ever bites.
- Tests: an empty list says so, one song failing to enqueue leaves the rest of
the list alone, the applyAction test covers Delete as well as Download and
Cancel, and the two playlist fixtures share a helper.
- Cancelling a list while its download loop was still enqueueing missed every
song queued after the cancel looked. The loop now holds the lock for the whole
queue, so a cancel waits for it to finish and then removes all of it. That
needed the enqueue helper split into a locked entry point and a body the loop
can call while already holding the lock — calling the locked one from inside
the lock deadlocked, which is what hung the tests.
- An empty resolved list now says so instead of doing nothing.
- One song failing to enqueue no longer aborts the rest of the batch.
- The artist actions resolve through the albums the artist page lists, matching
what is on screen, rather than through songs.artistId, which for a guest on a
compilation enqueued nothing.
- Bulk actions are one `applyAction` on the repository, so the three view models
and both screens stop repeating the same dispatch.
- Tests: the playlist and artist resolve paths, a Running row counted as
downloading, the action dispatch, and the action rule moved to a plain unit
test.
Third block of #65: bulk downloads and the list-level actions.
- The hero's download button on albums and playlists is real now. It starts the
songs the list is missing, cancels what is in flight, or deletes what is
already downloaded, chosen from the list's own status. The artist has no hero
button, so its bulk actions live in the context menu, which now carries the
same status.
- No new storage: a bulk job is the download rows for the list, and the status is
counted from song_downloads joined to the list's songs, so cancel and delete
stay well defined without anything to keep in sync.
- downloadAll fetches artwork for the whole list under one shared budget, once
per unique cover, and skips songs that are already downloaded so re-tapping
Download resumes rather than restarting.
- make the shuffle a filled pill matching the album/playlist controls
- wrap the artist name with a hanging indent: the first line may run the
full width over the button, while the last line stops short of it
- star/unstar from the context menu fills the icon, then dismisses
- drop the star badge from album art and song rows
- song rows keep a track-number column (natural order fallback), shrink
four-digit numbers, and gain right-hand download and duration columns
- the play indicator always sits in the left column, overlaid on cover art
with an inset primary outline when art is shown
- now playing is left aligned with a star toggle
- the media notification gets a star button backed by a session command
- Add a `SourceRepositoryTest` that sets the preference to 1 and proves the
active source fetches sequentially; removing the `maxConcurrentFetches`
wiring now fails it.
- `playlistSongsRespectsASmallerFetchBound` now holds the first `bound`
fetches and asserts no `(bound + 1)`th starts, instead of timing an
overlap; it fails when the semaphore ignores the bound.
- One `DEFAULT_FETCH_CONCURRENCY` (in `data.source`) backs both the source
default and the preference default, so they cannot drift.
- Clamp the stored preference in `syncConcurrency()` too, so a bad stored
value can't make `SubsonicSource` throw on construction.
The bound was only a constructor default with no production consumer. Add a
"Sync concurrency" preference (Settings -> Network, default 4) and pass it
through SourceRepository to SubsonicSource, so sync parallelism is tunable
without a rebuild. Both the app-scoped source and the on-demand
`activeMusicSource()` used by sync read the preference.
- UserPreferences.syncConcurrency()/setSyncConcurrency() (clamped >= 1)
- SourceRepository combines the preference into toMusicSource
- Settings row + dialog; SettingsViewModel state and setter
- Preferences test for default/round-trip/clamp
- The concurrency bound already existed; this parameter only exists so tests
can force the sequential path, and no production path passes it. Mark it
`@VisibleForTesting` so that is explicit rather than reading as a wired
setting.
- `playlistSongsUseTheSameRowsAtAnyFetchBound` could not fail (bound has no
effect on the rows) — delete it and its `normalized` helper.
- `playlistSongsRespectsASmallerFetchBound` relied on a 300ms overlap
window; use latches so the bound is proven deterministically (fails when
the semaphore ignores the bound).
- The NULL-aware backward album predicate is not sargable, so a backward seek
from a far cursor scans the whole album partition. Fall back to the offset
query when the anchor is more than twice the offset away; near-cursor
backward reads still use keyset.
- Cover the playlist keyset path: a DAO test for `playlistSongsBefore` and a
repository test scanning a playlist forward and backward. Both fail if the
playlist order key stops driving the seek.
`SongListItem` carried a nullable `position` that only playlist rows have,
so the album/song queries projected a fake `NULL AS position` just to map
the shared type — a leak of the playlist concept into unrelated reads.
Split it into `AlbumSongItem` (song, coverArt) and `PlaylistSongItem` (song,
coverArt, position), plus a `SongItem` interface so the queue stays generic
over kinds. The queue's `QueueWindowItem` now carries the optional
`playlistPosition` for the keyset cursor, so no projection fabricates it.
Playlist detail and its paging use `PlaylistSongItem`.
- `albumSongsBefore` used the null-intolerant row-value `<`, so an album
with NULL `disc`/`track` (untagged files) dropped exactly the rows a
backward window needs, returning the wrong track at a position. Use a
NULL-aware predicate (NULL sorts first, so it belongs to any `before`
set); `albumSongsFrom` is unaffected.
- The new `(sourceId, albumId, disc, track, id)` index has the old
`(sourceId, albumId)` index as a prefix, so drop the latter in the
migration and from the entity.
- Tests: mixed NULL disc/track album for the before-window and a full
backward scan; both fail on the previous commit.
- ArtworkStore's fetcher still did `response.body ?: throw` from before the body
size limit landed; OkHttp's `body` is not nullable, so the elvis was dead.
- DownloadRepository's `downloadStates` uses `flatMapLatest` without the opt-in
its callers annotate.
- HeroHeader's subtitle tap invoked `onSubtitleClick?.invoke()` inside the branch
where it is already known to be non-null.
- MediaItemTest used a safe call on `clippingConfiguration`, which never returns
null.
No behaviour change: a clean full rebuild now compiles without warnings.
#71 converted only two reads; the rest of `QueueViewModelTest` still read
`viewModel.rows` — a snapshot list mutated on the main dispatcher — from the
test thread, so `removeAndUndoKeepTheBoundedWindowConsistent` could map a torn
list (the removed position missing) under load. Route every test-thread read
through `rowsSnapshot()` and run the synchronous `reorder` mutations on the
dispatcher too.
- SourceRepository.coverArt and networkCoverArt read the source id and the
source object separately, so a source switch between the two could pair one
source's cache key with another source's URL; for the store that meant writing
the wrong server's artwork into the per-source file for good. Both now resolve
from a single active snapshot.
- ArtworkStore wrote through a fixed `<name>.part`, so two downloads of songs
from one album could interleave: one publishing the file while the other still
had the shared inode open, which leaves a torn image for Coil to decode. Each
write gets its own partial now.
- OkHttpArtworkFetcher read the whole body into memory whatever its size. It
refuses anything past 8 MB, checked by declared length and again while reading.
- Artwork whose library rows could not all be resolved is no longer swept:
pruning an album while its songs remain used to delete art a download still
held. A source with no download rows still sweeps everything.
- fetchArtworkSafely and storeArtwork no longer swallow CancellationException,
and the poll loop fetches a resumed row's artwork off the tick instead of
holding up the next one.
- Reconcile reads the download rows once rather than twice, and download() no
longer repeats the check enqueueIfAbsent makes under the lock.
- The artwork step no longer holds up the media: the song goes to the platform
first and the art is fetched afterwards, in the same foreground window. A
stalled artwork server can no longer delay a download, and a process death
during the fetch can no longer lose the download entirely.
- Resuming a queued download after a restart fetches its artwork now too; only a
fresh download did before.
- The artwork budget is a minute rather than ten seconds, which costs nothing but
a live coroutine now that nothing waits on it for the media.
- OkHttpArtworkFetcher derives a client with a per-call timeout. A blocking
OkHttp call is not interrupted by a coroutine timeout, so without it the outer
budget bounded our wait but not the socket.
Second block of #65.
- ArtworkStore keeps the album's and the artist's cover art, thumbnail and full,
in downloads/<sourceId>/art, keyed by the cache key the rest of the app already
uses for Coil and for palettes. Keys are base64url encoded because cover art ids
come from the server and must not reach outside the art directory.
- Downloading a song fetches that artwork first, in our process and while the app
is in the foreground, under a timeout, and hands the media to the platform
afterwards whether or not the artwork arrived. A song whose album or artist has
no art, or whose art fetch fails, still downloads.
- SourceRepository.coverArt resolves a stored file before the server, at the one
point every CoverArt, the notification bitmap loader and the palette prefetch go
through, so downloaded artwork works without the network.
- The sweep prunes art that no download references, so removing the last download
of an album drops its art while art another download still needs stays.
`QueueViewModelTest` read `viewModel.rows` (a `mutableStateListOf` mutated
on the main dispatcher) from the test thread, racing the view model: CI saw
`ConcurrentModificationException` in `removingWhileShuffledReconcilesTheRows`
and a torn read in `removingATrackUnderTheBlockReloadsRows`. Copy the rows
on the dispatcher via a `rowsSnapshot()` helper before asserting.
It measured pixel bounds to assert which line the indicator sits on and which
side of the artist name it lands. That is a presentational choice with exactly
one place in the code that makes it, so the test could only fail when someone
deliberately changed that line, and would break on unrelated layout changes.
The state-to-indicator tests and the accessibility descriptions stay.
The glyph and the progress ring sat in the title row after the star, spaced 8dp;
they now lead the subtitle row, 4dp from the artist name, which is where the
artist line has room for them. `theDownloadedIndicatorSitsOnTheArtistLineToItsLeft`
measures the unmerged bounds and fails if the indicator moves back up to the
title line or to the artist's right.
`single { SystemDownloadEngine(...) }` registers the concrete class, but
DownloadRepository asks for the DownloadEngine interface, so resolving
PlaybackController threw NoDefinitionFoundException and the app died on launch
with a Koin InstanceCreationException. It is now `single<DownloadEngine>`, the
way the other interface singletons in the module are declared.
Nothing in the suite ever started Koin — the repository tests build the
repository directly with a fake — which is why a launch-crashing binding got
through. AppModuleTest now resolves the playback graph from the real module, and
it failed with the same NoDefinitionFoundException before this fix.
Second review round on the downloads block:
- `loaded` was completed upstream of the state flow that publishes the map, so
`awaitLoaded()` could return while the map was still the initial empty one and
the first window after a cold start would stream a track that is already on
disk. The collector now assigns the map before completing, and the map is a
plain state flow instead of a lazy eager `stateIn` with a bare touch to start
it.
- A failed download now deletes its partial file, in both the engine-failure
and the recovery paths. A failed row used to protect its own partial bytes
from the sweep, and the UI offers no delete for it.
- A queued row is left queued when the download URL cannot be resolved yet,
instead of being failed. At launch the repository's reconcile can run before
the source repository has built its client, and failing there turned a
resumable download into a dead one.
- The poller only considers the active source's rows, so a queued row belonging
to another source no longer keeps it polling the database every second.
- `SourceRepository.downloadUri` takes the expected source id and returns null
unless it is the active one, so a download cannot be enqueued against the
previous server's URL during a source switch.
- Tests: a stream with no content length still gets its declared length through
the production data source factory (the http half of the chain, which the
first round left uncovered), and an engine failure is asserted to leave no
partial file. The engine's status mapping now runs as a plain unit test.
Review findings on the first downloads block:
- ExoPlayer could not open a downloaded file at all: the service's data source
factory was OkHttp only, so a file:// URI went to the HTTP stack and failed
with a malformed URL. Wrap it in DefaultDataSource so the file, asset and
content schemes are resolved before the stream chain, and cover it with a
test that reads a local file through the production factory.
- Reconcile no longer restarts a failed download, or a running one whose engine
entry vanished: only a queued row is started again, and the rest become failed
so the explicit Retry in the UI means something.
- song_downloads also references songs with CASCADE now, so a song pruned from
the mirror takes its download row with it and the sweep can reclaim the file.
- The mutating methods run on the injected dispatcher rather than the caller's,
which was the main thread for every call site, and the repository exposes one
shared state flow instead of a second map plus five subscriptions.
- Playback stops when the active source disappears, including deleting the last
source while a downloaded track is playing.
- Dropped the unused request mime type, the queued-at column nothing read, the
metered constructor parameter nobody varied, and a dead index guard.
- The running indicator is asserted positively, the engine's status and reason
mapping has a test, and the failure paths use the fake's failure and forget
helpers.
First block of #65: the state machine, the platform download engine, a
single-song download from the song context menu, the downloaded indicator in
song lists, and local-first playback.
- `song_downloads`, keyed `(sourceId, songId)` with CASCADE on the source, and
the 16 -> 17 migration.
- `DownloadEngine` over `android.app.DownloadManager`, with a fake for tests.
Files land in app-specific external storage at
`.../Music/downloads/<sourceId>/<songId>`, plus a `.nomedia` so media scanners
on pre-Android-10 devices leave them alone.
- `DownloadRepository` enqueues, mirrors progress by polling, cancels, deletes
and reconciles on start and on source change: a queued row whose engine entry
is gone is started again, a completed row whose file is gone becomes failed
rather than being resurrected, and files with no row are swept.
- Playback prefers the downloaded file: the media item build takes the local URI
when the download is complete and leaves it unclipped.
- Deleting a download reloads the player window at its position, so the item
falls back to streaming instead of failing mid-read.
- Deleting a source cancels its downloads and removes their files.
Downloads are the system's own notifications and stay off metered networks for
now. Artwork, bulk downloads and the management screen are later blocks.
`adjustAnchorOnRemove` moved the anchor to the canonical predecessor of the
removed track, which in a shuffled queue is an unrelated play position, so
removing the context anchor while playing a queued track teleported the
up-next block somewhere random (and out of view).
When the removed context track is the anchor, move the anchor to the track
before it in play order (or -1), so the block stays where it sits. This also
covers removing the playing context anchor, so the controller's separate
re-anchor is no longer needed and is removed.
The "play whatever now fills the removed track's slot" branch was written
for the old re-derive model: it mapped the removed canonical slot through
the new permutation, which now (with the fixed shuffle order) picks an
unrelated track. Removing the playing track should just resume at the same
slot, which holds whatever was next.
- Drop the special slot branch: `position == current` resumes at `current`
for shuffled and unshuffled alike. Without a block that is the next
shuffled track; with one, re-anchor the block from the removed track onto
the track now preceding that slot so the block takes the slot and plays
next (previously it was stranded behind a random track).
- Tests assert the immediate playing track, not just what is upcoming, which
is why they missed this. Both new tests fail on the previous commit.
- Cover advancing out of a block track and moving a block track while
shuffled (the last items from the QA review).
`removingThePlayingTrackWhileShuffledLeavesTheBlockReachable` looped 30
unseeded shuffles and only checked a weak invariant; it is now a single
deterministic case (fixed seed) that asserts the cursor resolves to the
playing track and the queued track still plays next. Add clearing the block
while shuffled, asserting the cursor stays on the playing track.
The shuffled-removal tests mostly removed the *current* track (a path that
was already correct) and asserted `state.item`, which `rebuildWindow` keeps
regardless of the computed cursor, so a wrong position went unnoticed. Add:
- deterministic shuffle setup (`seedShuffledQueue`, or replace + setShuffle
+ setCursor + connect) so shuffle tests assert exact behaviour instead of
looping over random seeds;
- controller tests that assert the queue position still resolves to the
playing track after removing a non-current track, with and without a block,
and that undo restores the shuffled order;
- a repository property test removing every position with a block present,
checking the play/flat bijection, the combined/contextPlay inverse, the
block span and that no track repeats;
- a `QueueRow.isPlaying` predicate (position AND song id) extracted and
unit-tested, and QueueViewModel tests for starting a shuffled queue at the
top and reconciling after a shuffled removal;
- Shuffle degenerate/out-of-range/MAX_SIZE coverage.
Removing a track re-derived the permutation from (seed, size), so every
remaining track moved to a new position and the queue visibly reshuffled
(in an album-heavy playlist, the removed row and its neighbours were
replaced by other tracks from the same album).
Keep the permutation fixed over the domain it was created with and subtract
removals from it instead: `ShuffleState` holds the seed, the domain size and
the removed domain slots, kept in memory only. `flatContext`/`playContext`
skip those slots so the surviving tracks keep their positions; the DB still
stores just the seed and domain, so the old per-track `shuffle_order` rows
stay gone. A new shuffle or a library change clears the removal list; on
restart the removals are gone, so a queue that was edited falls back to
unshuffled rather than guessing an order.
The previous commit assumed a removed/moved track only shifts play
positions by one, but a shuffled context removal re-derives the
permutation, so the current track's play position is a remap of its flat
index. `removeAt` now maps the current's flat through the updated order
(shuffled) or shifts it (unshuffled), and re-anchors the up-next block to
the track the cursor lands on. This fixes removing any track while
shuffled picking a random track and the anchor removal not advancing to
the block.
Restore "shuffle starts at the top": pick the seed so the requested start
track maps to play position 0 (`pinnedSeed`), instead of leaving the
current at its derived position. Applies to shuffle-play, tapping a track
while shuffled, toggling shuffle on, and restore.
The queue view's playing highlight now requires both the combined position
and the song id, so a transient position mismatch no longer flashes the
row above as playing.
The queue can hold the same song more than once (a song in both the context
and the up-next block, a duplicate album track, ...), so resolving the
playing entry by song id could match the wrong copy.
- `PlaybackController.move` and `removeAt` now derive the current track's
new position from the edit (block index / context play, read back through
the updated snapshot so a moved anchor is accounted for) instead of
`combinedIndexOf`/`combinedContextIndexOf`.
- the queue view's playing highlight compares the row position to the
playback position instead of the song id, so duplicates no longer both
light up.
Regression tests cover a move and a remove with a duplicate song in the
context; both fail without the change.
Still id-based, deliberately: `undo`'s canMirror guard (already position-
guarded), the shuffle-state-changed branch of `shiftWindowLocked` (rare,
needs the id to find the track after an arbitrary reorder) and the view's
LazyColumn keys (cosmetic only).
Removing the currently playing track resolved its id against the updated
queue afterwards; if the same song also exists in the context, the lookup
matched that copy and jumped the cursor to it instead of advancing. When the
removed position was the playing one, use the fallback (next in the block /
slot fill) rather than a by-id lookup.
Pre-existing on main, exposed while testing the shuffle rework. Regression
test fails without the fix.
- `QueueRepository.setShuffle(seed: Long?)` derives `shuffleEnabled` from the
seed, so disabled callers drop the meaningless sentinel `0L`.
- `playContext` returns `Long?` for symmetry with `flatContext`; `anchorPlay`
treats an out-of-range anchor as -1 instead of silently clamping to the last
track, and `combinedContextIndexOf` returns null.
Review found that removing the playing context track while shuffled with a
non-empty up-next block stranded the block behind the cursor: the
repository moves `upNextAnchor` to the preceding track, but the cursor's
re-derived slot position can land after the block, so the queued tracks
were skipped. Re-anchor the block to the track the cursor lands on, same
as `playAt`/`toggleShuffle`.
- regression test loops the sequence over fresh random seeds and asserts
the block stays reachable; fails without the fix ("stranded behind the
cursor").
- `removingTheCurrentTrackWhileShuffled...` now asserts the canonical slot
the current track occupies instead of recomputing the production position
formula (which only checked the implementation against itself).
- `QueueRepository.removeAt` returns Unit; every caller discarded the
snapshot and re-read it, so it no longer builds one.
Replaces the per-track `shuffle_order` table and the retained whole-order
arrays with `playback_cursor.shuffleSeed`/`shuffleSize` and a seekable
Feistel permutation of `[0, size)` (`Shuffle.kt`). Positions map through
`flatContext`/`playContext`; a snapshot is shuffled only while the stored
size matches the context, so a stale size is ignored. Queue migration
16->17 adds `shuffleSize` and drops `shuffle_order`.
Drops the "dragging a context row while shuffled reorders the canonical
queue" behaviour: it cannot be made WYSIWYG without materialising the
order. The context is playback-only while shuffled (the view already
disables context drags) and `QueueRepository.move` no-ops for a context
move while shuffled; manual ordering goes through the up-next block.
Removing a context track while shuffled re-derives the permutation at the
smaller size, keeping the same seed and the playhead on the current track,
or on whatever now fills the removed track's slot when it was playing.
Second review round on the downloads block:
- `loaded` was completed upstream of the state flow that publishes the map, so
`awaitLoaded()` could return while the map was still the initial empty one and
the first window after a cold start would stream a track that is already on
disk. The collector now assigns the map before completing, and the map is a
plain state flow instead of a lazy eager `stateIn` with a bare touch to start
it.
- A failed download now deletes its partial file, in both the engine-failure
and the recovery paths. A failed row used to protect its own partial bytes
from the sweep, and the UI offers no delete for it.
- A queued row is left queued when the download URL cannot be resolved yet,
instead of being failed. At launch the repository's reconcile can run before
the source repository has built its client, and failing there turned a
resumable download into a dead one.
- The poller only considers the active source's rows, so a queued row belonging
to another source no longer keeps it polling the database every second.
- `SourceRepository.downloadUri` takes the expected source id and returns null
unless it is the active one, so a download cannot be enqueued against the
previous server's URL during a source switch.
- Tests: a stream with no content length still gets its declared length through
the production data source factory (the http half of the chain, which the
first round left uncovered), and an engine failure is asserted to leave no
partial file. The engine's status mapping now runs as a plain unit test.
Review findings on the first downloads block:
- ExoPlayer could not open a downloaded file at all: the service's data source
factory was OkHttp only, so a file:// URI went to the HTTP stack and failed
with a malformed URL. Wrap it in DefaultDataSource so the file, asset and
content schemes are resolved before the stream chain, and cover it with a
test that reads a local file through the production factory.
- Reconcile no longer restarts a failed download, or a running one whose engine
entry vanished: only a queued row is started again, and the rest become failed
so the explicit Retry in the UI means something.
- song_downloads also references songs with CASCADE now, so a song pruned from
the mirror takes its download row with it and the sweep can reclaim the file.
- The mutating methods run on the injected dispatcher rather than the caller's,
which was the main thread for every call site, and the repository exposes one
shared state flow instead of a second map plus five subscriptions.
- Playback stops when the active source disappears, including deleting the last
source while a downloaded track is playing.
- Dropped the unused request mime type, the queued-at column nothing read, the
metered constructor parameter nobody varied, and a dead index guard.
- The running indicator is asserted positively, the engine's status and reason
mapping has a test, and the failure paths use the fake's failure and forget
helpers.
First block of #65: the state machine, the platform download engine, a
single-song download from the song context menu, the downloaded indicator in
song lists, and local-first playback.
- `song_downloads`, keyed `(sourceId, songId)` with CASCADE on the source, and
the 16 -> 17 migration.
- `DownloadEngine` over `android.app.DownloadManager`, with a fake for tests.
Files land in app-specific external storage at
`.../Music/downloads/<sourceId>/<songId>`, plus a `.nomedia` so media scanners
on pre-Android-10 devices leave them alone.
- `DownloadRepository` enqueues, mirrors progress by polling, cancels, deletes
and reconciles on start and on source change: a queued row whose engine entry
is gone is started again, a completed row whose file is gone becomes failed
rather than being resurrected, and files with no row are swept.
- Playback prefers the downloaded file: the media item build takes the local URI
when the download is complete and leaves it unclipped.
- Deleting a download reloads the player window at its position, so the item
falls back to streaming instead of failing mid-read.
- Deleting a source cancels its downloads and removes their files.
Downloads are the system's own notifications and stay off metered networks for
now. Artwork, bulk downloads and the management screen are later blocks.
Both targets were too greedy. They now hug the text instead of spanning the row, so a tap
beside the line does nothing, and the album detail target stops short of the controls
instead of reaching down to them.
Album detail: the line plus its 4dp lead-in and 8dp below (32dp), with the original
spacing kept, so the layout is exactly where it was. Now playing keeps the title+artist
block (58dp) but is no longer full width.
Both are shorter than the 48dp recommendation on purpose, to stay clear of the slider and
the play/shuffle buttons. Tests assert the measured geometry: no full-width target, a
12dp gap above the controls, and the now playing block staying at its own 58dp.
austinried