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.
- draw the play halo as an offset copy below API 28, where path
shadow layers are ignored, and skip the inner shadow there too
- add a repository toggleStar that reads and writes under the star lock,
so fast double taps on the notification cannot desync
- reset the star button when a track starts, so a failed source lookup
cannot leave the previous song's state showing
Three equaliser bars wave with staggered phases to draw the eye to the
playing track, keeping the soft shadow over cover art and flat bars in
the queue.
Keep the star and more pinned to the right of the row, top-align the row
so the album count sits close under the image, and let the centred
shuffle button's lower half pass just above the controls.
Move the shuffle out of the title block so the wrapped name keeps an even
bottom margin, and anchor the button centred on the image's bottom edge.
The title's last line still reserves room on the right, and the album
count row starts below the button's hanging half.
Render the wrapped artist name as two rows: the first line spans the full
width on its own row above, and the last line shares a row with the
shuffle button, so the text can never run under it.
- 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
- drop the passive star badge from the artists list
- add an album count, star, more and placeholder bio under the artist image
- move the more action down from the top bar into that row
- put a shuffle button beside the artist name, which can now wrap to two lines
- remove the now-unused StarredBadge component
- shadow the play indicator over album art so it reads against busy art
- drop the download indicator from the queue view
- enlarge the now playing star toggle
- leave a little more room between the detail controls and the song list
- opt in to the Media3 unstable API on the service and answer
unsupported custom commands with SessionError.ERROR_NOT_SUPPORTED
- add the star command on top of the trust-aware connection defaults
instead of granting untrusted controllers the full command set
- number tracks within their disc, not across the whole album
- leave the star button to the library flow so a failed write cannot
leave the notification flipped
- omit the leading cell entirely when a row has no art and no track
- 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.
austinried