From 11cc5e9ab83964dacd3d796bd5a7a05109dc3e32 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 27 Sep 2026 08:04:45 +0000 Subject: [PATCH] download a song's artwork with it and use it before the network Second block of #65. - ArtworkStore keeps the album's and the artist's cover art, thumbnail and full, in downloads//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. --- .../com/subtracks/data/db/DownloadDao.kt | 10 ++ .../com/subtracks/data/db/LibraryDao.kt | 12 ++ .../subtracks/data/download/ArtworkStore.kt | 81 ++++++++++ .../com/subtracks/data/model/CoverArtRef.kt | 6 + .../com/subtracks/data/model/Download.kt | 6 + .../subtracks/data/repo/DownloadRepository.kt | 93 +++++++++-- .../subtracks/data/repo/SourceRepository.kt | 18 ++- .../main/kotlin/com/subtracks/di/AppModule.kt | 14 +- .../data/download/ArtworkStoreTest.kt | 65 ++++++++ .../data/repo/DownloadRepositoryTest.kt | 153 +++++++++++++++++- .../data/repo/LibraryRepositoryTest.kt | 4 +- .../data/repo/SourceRepositoryTest.kt | 43 ++++- .../subtracks/data/sync/SyncManagerTest.kt | 4 +- .../playback/PlaybackControllerTest.kt | 13 +- .../ui/playback/QueueViewModelTest.kt | 14 +- 15 files changed, 508 insertions(+), 28 deletions(-) create mode 100644 app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt create mode 100644 app/src/test/kotlin/com/subtracks/data/download/ArtworkStoreTest.kt diff --git a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt index 66da7913..ba762d5c 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt @@ -3,6 +3,7 @@ package com.subtracks.data.db import androidx.room3.Dao import androidx.room3.Query import androidx.room3.Upsert +import com.subtracks.data.model.DownloadArtwork import com.subtracks.data.model.SongDownload import kotlinx.coroutines.flow.Flow @@ -14,6 +15,15 @@ interface DownloadDao { @Query("SELECT * FROM song_downloads") suspend fun all(): List + @Query( + "SELECT d.sourceId AS sourceId, al.coverArt AS albumCoverArt, ar.coverArt AS artistCoverArt " + + "FROM song_downloads d " + + "LEFT JOIN songs s ON s.sourceId = d.sourceId AND s.id = d.songId " + + "LEFT JOIN albums al ON al.sourceId = s.sourceId AND al.id = s.albumId " + + "LEFT JOIN artists ar ON ar.sourceId = s.sourceId AND ar.id = s.artistId", + ) + suspend fun artwork(): List + @Query("SELECT * FROM song_downloads WHERE sourceId = :sourceId AND songId = :songId") suspend fun find( sourceId: Long, diff --git a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt index 83916dd3..ade304dc 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -357,6 +357,18 @@ interface LibraryDao { albumId: String, ): Flow + @Query("SELECT * FROM albums WHERE sourceId = :sourceId AND id = :albumId") + suspend fun albumOnce( + sourceId: Long, + albumId: String, + ): Album? + + @Query("SELECT * FROM artists WHERE sourceId = :sourceId AND id = :artistId") + suspend fun artistOnce( + sourceId: Long, + artistId: String, + ): Artist? + @Query("SELECT * FROM artists WHERE sourceId = :sourceId AND id = :artistId") fun artist( sourceId: Long, diff --git a/app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt b/app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt new file mode 100644 index 00000000..495b52fb --- /dev/null +++ b/app/src/main/kotlin/com/subtracks/data/download/ArtworkStore.kt @@ -0,0 +1,81 @@ +package com.subtracks.data.download + +import android.net.Uri +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext +import okhttp3.OkHttpClient +import okhttp3.Request +import java.io.File +import java.io.IOException +import java.util.Base64 + +/** + * Cover art downloaded alongside the media, stored per source under `downloads//art`. + * + * Cover art ids come from the server, so keys are base64url encoded rather than used as file + * names: a key can contain separators or spaces and must never reach outside the art directory. + */ +class ArtworkStore( + private val downloadsDir: File, +) { + fun file( + sourceId: Long, + cacheKey: String, + ): File = artDir(sourceId).resolve(fileName(cacheKey)) + + fun uri( + sourceId: Long, + cacheKey: String, + ): String? = file(sourceId, cacheKey).takeIf { it.exists() }?.let { Uri.fromFile(it).toString() } + + fun write( + sourceId: Long, + cacheKey: String, + bytes: ByteArray, + ): Boolean { + val target = file(sourceId, cacheKey) + return runCatching { + target.parentFile?.mkdirs() + val partial = File(target.parentFile, target.name + PARTIAL_SUFFIX) + partial.writeBytes(bytes) + partial.renameTo(target) + }.getOrDefault(false) + } + + fun sweep( + sourceId: Long, + keep: Set, + ) { + val keepNames = keep.mapTo(HashSet(), ::fileName) + artDir(sourceId).listFiles().orEmpty().forEach { file -> + if (file.isFile && file.name !in keepNames) file.delete() + } + } + + private fun artDir(sourceId: Long): File = downloadsDir.resolve(sourceId.toString()).resolve(ART_DIR) + + private fun fileName(cacheKey: String): String = + Base64.getUrlEncoder().withoutPadding().encodeToString(cacheKey.toByteArray(Charsets.UTF_8)) + + private companion object { + const val ART_DIR = "art" + const val PARTIAL_SUFFIX = ".part" + } +} + +fun interface ArtworkFetcher { + suspend fun fetch(url: String): ByteArray +} + +class OkHttpArtworkFetcher( + private val http: OkHttpClient, +) : ArtworkFetcher { + override suspend fun fetch(url: String): ByteArray = + withContext(Dispatchers.IO) { + val request = Request.Builder().url(url).build() + http.newCall(request).execute().use { response -> + if (!response.isSuccessful) throw IOException("Artwork request failed with ${response.code}") + response.body?.bytes() ?: throw IOException("Artwork response had no body") + } + } +} diff --git a/app/src/main/kotlin/com/subtracks/data/model/CoverArtRef.kt b/app/src/main/kotlin/com/subtracks/data/model/CoverArtRef.kt index 34c52e3f..39c2e19a 100644 --- a/app/src/main/kotlin/com/subtracks/data/model/CoverArtRef.kt +++ b/app/src/main/kotlin/com/subtracks/data/model/CoverArtRef.kt @@ -4,3 +4,9 @@ data class CoverArtRef( val url: String, val cacheKey: String, ) + +fun coverArtKey( + sourceId: Long, + coverArt: String, + thumbnail: Boolean, +): String = "$sourceId:$coverArt:$thumbnail" diff --git a/app/src/main/kotlin/com/subtracks/data/model/Download.kt b/app/src/main/kotlin/com/subtracks/data/model/Download.kt index d9cb3eba..69e503a5 100644 --- a/app/src/main/kotlin/com/subtracks/data/model/Download.kt +++ b/app/src/main/kotlin/com/subtracks/data/model/Download.kt @@ -40,6 +40,12 @@ data class SongDownload( get() = total.takeIf { it > 0 }?.let { (bytes.toFloat() / it).coerceIn(0f, 1f) } } +data class DownloadArtwork( + val sourceId: Long, + val albumCoverArt: String?, + val artistCoverArt: String?, +) + class DownloadStatusConverter { @ColumnTypeConverter fun fromDownloadStatus(status: DownloadStatus): String = status.name diff --git a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt index 6e794b2a..c3f1fdcd 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -2,12 +2,15 @@ package com.subtracks.data.repo import android.net.Uri import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkFetcher +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.download.DownloadEngine import com.subtracks.data.download.EngineDownload import com.subtracks.data.download.EngineRequest import com.subtracks.data.download.EngineStatus import com.subtracks.data.model.DownloadStatus import com.subtracks.data.model.SongDownload +import com.subtracks.data.model.coverArtKey import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope @@ -27,6 +30,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull import java.io.File class DownloadRepository( @@ -34,6 +38,8 @@ class DownloadRepository( private val sourceRepository: SourceRepository, private val engine: DownloadEngine, private val downloadsDir: File, + private val artworkStore: ArtworkStore, + private val artworkFetcher: ArtworkFetcher, private val showMessage: (String) -> Unit = {}, private val dispatcher: CoroutineDispatcher = Dispatchers.IO, ) { @@ -84,19 +90,23 @@ class DownloadRepository( sourceId: Long, songId: String, ) = withContext(dispatcher) { + if (sourceRepository.activeSourceIdOnce() != sourceId) { + showMessage(NOT_ACTIVE) + return@withContext + } + if (alreadyDownloadingOrDownloaded(sourceId, songId)) return@withContext + if (sourceRepository.downloadUri(sourceId, songId) == null) { + showMessage(NO_ADDRESS) + return@withContext + } + // Artwork runs in our process and is fetched while the app is in the foreground; the + // media itself is handed to the platform, which keeps going without us. + runCatching { withTimeoutOrNull(ART_TIMEOUT_MS) { fetchArtwork(sourceId, songId) } } mutex.withLock { - if (sourceRepository.activeSourceIdOnce() != sourceId) { - showMessage("Can't download from a source that isn't active") - return@withLock - } + if (alreadyDownloadingOrDownloaded(sourceId, songId)) return@withLock val existing = db.downloadDao().find(sourceId, songId) - if (existing != null && existing.status != DownloadStatus.Failed) return@withLock existing?.engineId?.let(engine::cancel) - val url = sourceRepository.downloadUri(sourceId, songId) - if (url == null) { - showMessage(NO_ADDRESS) - return@withLock - } + val url = sourceRepository.downloadUri(sourceId, songId) ?: return@withLock enqueue(sourceId, songId, url) } } @@ -110,6 +120,7 @@ class DownloadRepository( row.engineId?.let(engine::cancel) file(sourceId, songId).delete() db.downloadDao().delete(sourceId, songId) + sweep(db.downloadDao().all()) } } @@ -144,6 +155,57 @@ class DownloadRepository( .flatMapLatest { sourceId -> if (sourceId == null) flowOf(emptyList()) else db.downloadDao().downloads(sourceId) } .map { rows -> rows.associateBy { it.songId } } + private suspend fun alreadyDownloadingOrDownloaded( + sourceId: Long, + songId: String, + ): Boolean = + db + .downloadDao() + .find(sourceId, songId) + ?.status + ?.let { it != DownloadStatus.Failed } == true + + private suspend fun fetchArtwork( + sourceId: Long, + songId: String, + ) { + val song = db.libraryDao().songOnce(sourceId, songId) + val coverArt = + listOfNotNull( + song?.albumId?.let { db.libraryDao().albumOnce(sourceId, it)?.coverArt }, + song?.artistId?.let { db.libraryDao().artistOnce(sourceId, it)?.coverArt }, + ).distinct() + coverArt.forEach { cover -> + storeArtwork(sourceId, cover, thumbnail = false) + storeArtwork(sourceId, cover, thumbnail = true) + } + } + + private suspend fun storeArtwork( + sourceId: Long, + coverArt: String, + thumbnail: Boolean, + ) { + val cacheKey = coverArtKey(sourceId, coverArt, thumbnail) + if (artworkStore.file(sourceId, cacheKey).exists()) return + val url = sourceRepository.networkCoverArt(sourceId, coverArt, thumbnail) ?: return + val bytes = runCatching { artworkFetcher.fetch(url) }.getOrNull() ?: return + if (bytes.isEmpty()) return + artworkStore.write(sourceId, cacheKey, bytes) + } + + private suspend fun neededArtwork(): Map> = + db + .downloadDao() + .artwork() + .groupBy { it.sourceId } + .mapValues { (sourceId, rows) -> + rows + .flatMap { row -> listOfNotNull(row.albumCoverArt, row.artistCoverArt) } + .flatMap { cover -> listOf(false, true).map { thumbnail -> coverArtKey(sourceId, cover, thumbnail) } } + .toSet() + } + private suspend fun enqueue( sourceId: Long, songId: String, @@ -234,13 +296,14 @@ class DownloadRepository( } } - private fun sweep(rows: List) { - val known = rows.map { it.sourceId to it.songId }.toHashSet() - // Artwork (a later block) lives in subdirectories and is swept separately. + private suspend fun sweep(rows: List) { + val knownSongs = rows.map { it.sourceId to it.songId }.toHashSet() + val knownArt = neededArtwork() downloadsDir.listFiles().orEmpty().filter { it.isDirectory }.forEach { sourceDir -> val sourceId = sourceDir.name.toLongOrNull() ?: return@forEach + artworkStore.sweep(sourceId, knownArt[sourceId].orEmpty()) sourceDir.listFiles().orEmpty().filter { it.isFile }.forEach { file -> - if ((sourceId to file.name) !in known) file.delete() + if ((sourceId to file.name) !in knownSongs) file.delete() } } } @@ -267,5 +330,7 @@ class DownloadRepository( const val MISSING_FILE = "The downloaded file is missing" const val NO_ADDRESS = "Can't download: the server address is unavailable" const val STOPPED = "The download stopped unexpectedly" + const val NOT_ACTIVE = "Can't download from a source that isn't active" + const val ART_TIMEOUT_MS = 10_000L } } diff --git a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt index 81b5c432..863f5ea9 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt @@ -1,10 +1,12 @@ package com.subtracks.data.repo import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.model.CoverArtRef import com.subtracks.data.model.Source import com.subtracks.data.model.SubsonicConfig import com.subtracks.data.model.SubsonicSource +import com.subtracks.data.model.coverArtKey import com.subtracks.data.net.NetworkMode import com.subtracks.data.prefs.StreamQuality import com.subtracks.data.prefs.UserPreferences @@ -31,6 +33,7 @@ class SourceRepository( private val db: SubtracksDatabase, private val http: OkHttpClient, private val prefs: UserPreferences, + private val artworkStore: ArtworkStore, networkMode: Flow = flowOf(NetworkMode.Wifi), private val showMessage: (String) -> Unit = {}, ) { @@ -81,11 +84,20 @@ class SourceRepository( coverArt: String?, thumbnail: Boolean = false, ): CoverArtRef? { - val source = active ?: return null - val url = source.coverArtUri(coverArt, thumbnail)?.toString() ?: return null - return CoverArtRef(url = url, cacheKey = "${source.id}:$coverArt:$thumbnail") + val sourceId = activeSourceId ?: return null + if (coverArt == null) return null + val cacheKey = coverArtKey(sourceId, coverArt, thumbnail) + artworkStore.uri(sourceId, cacheKey)?.let { return CoverArtRef(url = it, cacheKey = cacheKey) } + val url = active?.coverArtUri(coverArt, thumbnail)?.toString() ?: return null + return CoverArtRef(url = url, cacheKey = cacheKey) } + fun networkCoverArt( + sourceId: Long, + coverArt: String, + thumbnail: Boolean, + ): String? = if (activeSourceId == sourceId) active?.coverArtUri(coverArt, thumbnail)?.toString() else null + fun streamUri( songId: String, durationMs: Long?, diff --git a/app/src/main/kotlin/com/subtracks/di/AppModule.kt b/app/src/main/kotlin/com/subtracks/di/AppModule.kt index 67d49c5e..9e3959c3 100644 --- a/app/src/main/kotlin/com/subtracks/di/AppModule.kt +++ b/app/src/main/kotlin/com/subtracks/di/AppModule.kt @@ -6,7 +6,10 @@ import android.os.Handler import android.os.Looper import android.widget.Toast import com.subtracks.data.db.createAndroidDatabase +import com.subtracks.data.download.ArtworkFetcher +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.download.DownloadEngine +import com.subtracks.data.download.OkHttpArtworkFetcher import com.subtracks.data.download.SystemDownloadEngine import com.subtracks.data.net.networkMode import com.subtracks.data.prefs.createUserPreferences @@ -44,18 +47,22 @@ fun appModule( single { createAndroidDatabase(context) } single { http } single { createUserPreferences(context) } - single { SourceRepository(get(), get(), get(), networkMode = networkMode(context.applicationContext), showMessage = toast) } + single { SourceRepository(get(), get(), get(), get(), networkMode = networkMode(context.applicationContext), showMessage = toast) } single { LibraryRepository(get(), get(), toast) } single { ArtworkSeedRepository(get()) } single { SyncManager(get(), get(), get()) } single { QueueRepository(get()) } + single { ArtworkStore(downloadsRoot(context)) } + single { OkHttpArtworkFetcher(get()) } single { SystemDownloadEngine(context.applicationContext) } single { DownloadRepository( db = get(), sourceRepository = get(), engine = get(), - downloadsDir = File(context.getExternalFilesDir(Environment.DIRECTORY_MUSIC) ?: context.filesDir, "downloads"), + downloadsDir = downloadsRoot(context), + artworkStore = get(), + artworkFetcher = get(), showMessage = toast, ).also { it.start() } } @@ -70,3 +77,6 @@ fun appModule( viewModel { params -> PlaylistDetailViewModel(get(), get(), get(), get(), params.get()) } viewModel { QueueViewModel(get(), get()) } } + +private fun downloadsRoot(context: Context): File = + File(context.getExternalFilesDir(Environment.DIRECTORY_MUSIC) ?: context.filesDir, "downloads") diff --git a/app/src/test/kotlin/com/subtracks/data/download/ArtworkStoreTest.kt b/app/src/test/kotlin/com/subtracks/data/download/ArtworkStoreTest.kt new file mode 100644 index 00000000..73800e54 --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/data/download/ArtworkStoreTest.kt @@ -0,0 +1,65 @@ +package com.subtracks.data.download + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import java.io.File + +@RunWith(AndroidJUnit4::class) +class ArtworkStoreTest { + private val context = ApplicationProvider.getApplicationContext() + private val root = File(context.cacheDir, "art-${System.nanoTime()}") + private val store = ArtworkStore(root) + + @Test + fun storedBytesComeBackThroughTheFile() { + store.write(1, "1:art-1:false", byteArrayOf(1, 2, 3)) + + assertArrayEquals(byteArrayOf(1, 2, 3), store.file(1, "1:art-1:false").readBytes()) + assertTrue(store.uri(1, "1:art-1:false")!!.startsWith("file:")) + } + + @Test + fun anUnstoredKeyHasNoUri() { + assertNull(store.uri(1, "1:missing:false")) + } + + @Test + fun aServerSuppliedCoverArtIdCannotEscapeTheArtDirectory() { + val hostile = "1:x/../../../../escape:false" + + store.write(1, hostile, byteArrayOf(1)) + + assertTrue(store.file(1, hostile).canonicalPath.startsWith(root.canonicalPath + File.separator)) + } + + @Test + fun sweepKeepsWhatIsStillNeededAndDropsTheRest() { + store.write(1, "1:keep:false", byteArrayOf(1)) + store.write(1, "1:drop:false", byteArrayOf(1)) + store.write(2, "2:other:false", byteArrayOf(1)) + + store.sweep(1, setOf("1:keep:false")) + + assertTrue(store.file(1, "1:keep:false").exists()) + assertFalse(store.file(1, "1:drop:false").exists()) + assertTrue(store.file(2, "2:other:false").exists()) + } + + @Test + fun anUnwritableArtDirectoryIsReportedRatherThanThrown() { + File(root, "1/art").apply { + parentFile?.mkdirs() + writeText("not a directory") + } + + assertFalse(store.write(1, "1:art-1:false", byteArrayOf(1))) + assertFalse(store.file(1, "1:art-1:false").exists()) + } +} diff --git a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt index 3e9dbb80..5026c135 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -7,12 +7,17 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkFetcher +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.download.FakeDownloadEngine +import com.subtracks.data.model.Album +import com.subtracks.data.model.Artist import com.subtracks.data.model.DownloadStatus import com.subtracks.data.model.Song import com.subtracks.data.model.SongDownload import com.subtracks.data.model.Source import com.subtracks.data.model.SubsonicSource +import com.subtracks.data.model.coverArtKey import com.subtracks.data.prefs.fakeUserPreferences import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest @@ -21,12 +26,14 @@ import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import java.io.File +import java.io.IOException import java.util.concurrent.CopyOnWriteArrayList @RunWith(AndroidJUnit4::class) @@ -35,8 +42,17 @@ class DownloadRepositoryTest { private lateinit var sources: SourceRepository private lateinit var engine: FakeDownloadEngine private lateinit var repository: DownloadRepository + private lateinit var artwork: ArtworkStore private lateinit var dir: File private val messages = CopyOnWriteArrayList() + private val requestedArt = CopyOnWriteArrayList() + private var failArtworkFetch = false + private val fetcher = + ArtworkFetcher { url -> + requestedArt += url + if (failArtworkFetch) throw IOException("boom") + byteArrayOf(1, 2, 3) + } @Before fun setUp() { @@ -46,10 +62,20 @@ class DownloadRepositoryTest { .inMemoryDatabaseBuilder(context, SubtracksDatabase::class.java) .setDriver(BundledSQLiteDriver()) .build() - sources = SourceRepository(db, OkHttpClient(), fakeUserPreferences()) + artwork = ArtworkStore(File(context.cacheDir, "art-${System.nanoTime()}")) + sources = SourceRepository(db, OkHttpClient(), fakeUserPreferences(), artwork) engine = FakeDownloadEngine() dir = File(context.cacheDir, "downloads-${System.nanoTime()}") - repository = DownloadRepository(db, sources, engine, dir, showMessage = { messages += it }) + repository = + DownloadRepository( + db, + sources, + engine, + dir, + artworkStore = artwork, + artworkFetcher = fetcher, + showMessage = { messages += it }, + ) } @After @@ -270,6 +296,108 @@ class DownloadRepositoryTest { assertFalse(file(1, "orphan").exists()) } + @Test + fun startingADownloadAlsoStoresTheAlbumAndArtistArtwork() = + runTest { + seedLibrary() + + runBlocking { repository.download(1, "s1") } + + assertEquals(4, requestedArt.size) + listOf(ALBUM_ART, ARTIST_ART).forEach { cover -> + listOf(false, true).forEach { thumbnail -> + val key = coverArtKey(1, cover, thumbnail) + assertNotNull("expected stored art for $key", artwork.uri(1, key)) + } + } + } + + @Test + fun aSongWithoutArtworkStillDownloads() = + runTest { + seedLibrary() + runBlocking { + db.libraryDao().upsertAlbums(listOf(album(coverArt = null))) + db.libraryDao().upsertArtists(listOf(artist(coverArt = null))) + } + + runBlocking { repository.download(1, "s1") } + + assertTrue(requestedArt.isEmpty()) + assertEquals(1, engine.requests.size) + } + + @Test + fun aFailedArtworkFetchDoesNotStopTheDownload() = + runTest { + seedLibrary() + failArtworkFetch = true + + runBlocking { repository.download(1, "s1") } + + assertEquals(1, engine.requests.size) + assertEquals(DownloadStatus.Queued, row(1, "s1")?.status) + assertNull(artwork.uri(1, coverArtKey(1, ALBUM_ART, false))) + } + + @Test + fun artworkThatIsAlreadyStoredIsNotFetchedAgain() = + runTest { + seedLibrary() + listOf(ALBUM_ART, ARTIST_ART).forEach { cover -> + listOf(false, true).forEach { thumbnail -> + artwork.write(1, coverArtKey(1, cover, thumbnail), byteArrayOf(9)) + } + } + + runBlocking { repository.download(1, "s1") } + + assertTrue(requestedArt.isEmpty()) + assertEquals(1, engine.requests.size) + } + + @Test + fun removingTheLastDownloadOfAnAlbumRemovesItsArtwork() = + runTest { + seedLibrary() + runBlocking { repository.download(1, "s1") } + + runBlocking { repository.remove(1, "s1") } + + assertNull(artwork.uri(1, coverArtKey(1, ALBUM_ART, false))) + assertNull(artwork.uri(1, coverArtKey(1, ARTIST_ART, false))) + } + + @Test + fun artworkAnotherDownloadStillNeedsIsKept() = + runTest { + seedLibrary() + runBlocking { repository.download(1, "s1") } + runBlocking { repository.download(1, "s2") } + + runBlocking { repository.remove(1, "s1") } + + assertNotNull(artwork.uri(1, coverArtKey(1, ALBUM_ART, false))) + } + + private fun album(coverArt: String?) = + Album( + sourceId = 1, + id = "al1", + artistId = "ar1", + name = "Album", + albumArtist = "Artist", + created = 0, + coverArt = coverArt, + genre = null, + year = 2000, + starred = null, + songCount = 2, + ) + + private fun artist(coverArt: String?) = + Artist(sourceId = 1, id = "ar1", name = "Artist", albumCount = 1, starred = null, coverArt = coverArt) + private fun seedLibrary() { runBlocking { db.sourcesDao().upsertSource(Source(1, "source", "http://localhost:4533/", isActive = true, createdAt = 0)) @@ -290,8 +418,24 @@ class DownloadRepositoryTest { starred = null, genre = null, ), + Song( + sourceId = 1, + id = "s2", + albumId = "al1", + artistId = "ar1", + title = "Song 2", + album = "Album", + artist = "Artist", + duration = 100, + track = 2, + disc = 1, + starred = null, + genre = null, + ), ), ) + db.libraryDao().upsertAlbums(listOf(album(ALBUM_ART))) + db.libraryDao().upsertArtists(listOf(artist(ARTIST_ART))) } await { sources.downloadUri(1, "s1") != null } } @@ -323,4 +467,9 @@ class DownloadRepositoryTest { while (!predicate() && System.nanoTime() < deadline) Thread.sleep(10) assertTrue("Timed out waiting for the expected download state", predicate()) } + + private companion object { + const val ALBUM_ART = "art-al1" + const val ARTIST_ART = "art-ar1" + } } diff --git a/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt index 5cbca415..24e38dd8 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/LibraryRepositoryTest.kt @@ -6,6 +6,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.model.Song import com.subtracks.data.prefs.fakeUserPreferences import com.subtracks.data.source.StarType @@ -25,6 +26,7 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import java.io.File import java.util.concurrent.TimeUnit @RunWith(AndroidJUnit4::class) @@ -42,7 +44,7 @@ class LibraryRepositoryTest { .inMemoryDatabaseBuilder(context, SubtracksDatabase::class.java) .setDriver(BundledSQLiteDriver()) .build() - sourceRepository = SourceRepository(db, OkHttpClient(), fakeUserPreferences()) + sourceRepository = SourceRepository(db, OkHttpClient(), fakeUserPreferences(), ArtworkStore(File(context.cacheDir, "art"))) repository = LibraryRepository(db, sourceRepository, showMessage = { messages += it }) } diff --git a/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt index db67244e..e6d22d10 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/SourceRepositoryTest.kt @@ -6,6 +6,9 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkStore +import com.subtracks.data.model.CoverArtRef +import com.subtracks.data.model.coverArtKey import com.subtracks.data.prefs.fakeUserPreferences import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first @@ -22,11 +25,13 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import java.io.File @RunWith(AndroidJUnit4::class) class SourceRepositoryTest { private lateinit var db: SubtracksDatabase private lateinit var repository: SourceRepository + private lateinit var artwork: ArtworkStore private val messages = ArrayList() @Before @@ -37,7 +42,8 @@ class SourceRepositoryTest { .inMemoryDatabaseBuilder(context, SubtracksDatabase::class.java) .setDriver(BundledSQLiteDriver()) .build() - repository = SourceRepository(db, OkHttpClient(), fakeUserPreferences(), showMessage = { messages += it }) + artwork = ArtworkStore(File(context.cacheDir, "art-${System.nanoTime()}")) + repository = SourceRepository(db, OkHttpClient(), fakeUserPreferences(), artwork, showMessage = { messages += it }) } @After @@ -95,6 +101,41 @@ class SourceRepositoryTest { } } + @Test + fun downloadedArtworkIsPreferredOverTheServer() = + runBlocking { + repository.addSource("server", "http://a.example/", "u", "p", true) + val sourceId = awaitActiveSourceId() + artwork.write(sourceId, coverArtKey(sourceId, "art-1", false), byteArrayOf(1, 2, 3)) + + val ref = awaitCoverArt("art-1") + + assertTrue("expected the stored file, was ${ref.url}", ref.url.startsWith("file:")) + assertEquals(coverArtKey(sourceId, "art-1", false), ref.cacheKey) + } + + @Test + fun coverArtFallsBackToTheServerWithoutDownloadedArtwork() = + runBlocking { + repository.addSource("server", "http://a.example/", "u", "p", true) + + val ref = awaitCoverArt("art-1") + + assertTrue("expected the server url, was ${ref.url}", ref.url.startsWith("http")) + } + + private suspend fun awaitActiveSourceId(): Long = withTimeout(5_000) { repository.activeSourceId().first { it != null }!! } + + private suspend fun awaitCoverArt(coverArt: String): CoverArtRef = + withTimeout(5_000) { + var ref = repository.coverArt(coverArt) + while (ref == null) { + delay(10) + ref = repository.coverArt(coverArt) + } + ref + } + private suspend fun withServer(block: suspend (MockWebServer) -> Unit) { val server = MockWebServer() server.start() diff --git a/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt b/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt index b3e199a9..f58710cb 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt @@ -6,6 +6,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.prefs.fakeUserPreferences import com.subtracks.data.repo.QueueRepository import com.subtracks.data.repo.SourceRepository @@ -19,6 +20,7 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import java.io.File @RunWith(AndroidJUnit4::class) class SyncManagerTest { @@ -35,7 +37,7 @@ class SyncManagerTest { .inMemoryDatabaseBuilder(context, SubtracksDatabase::class.java) .setDriver(BundledSQLiteDriver()) .build() - sourceRepository = SourceRepository(db, OkHttpClient(), fakeUserPreferences()) + sourceRepository = SourceRepository(db, OkHttpClient(), fakeUserPreferences(), ArtworkStore(File(context.cacheDir, "art"))) queueRepository = QueueRepository(db) manager = SyncManager(db, sourceRepository, queueRepository) } diff --git a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt index f3cb3ae2..b71001dd 100644 --- a/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt +++ b/app/src/test/kotlin/com/subtracks/playback/PlaybackControllerTest.kt @@ -7,6 +7,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.download.FakeDownloadEngine import com.subtracks.data.model.Album import com.subtracks.data.model.QueueEntry @@ -64,9 +65,17 @@ class PlaybackControllerTest { .build() prefs = fakeUserPreferences() networkMode = MutableStateFlow(NetworkMode.Wifi) - sources = SourceRepository(db, OkHttpClient(), prefs, networkMode = networkMode) + sources = SourceRepository(db, OkHttpClient(), prefs, ArtworkStore(File(context.cacheDir, "art")), networkMode = networkMode) queues = QueueRepository(db) - downloads = DownloadRepository(db, sources, FakeDownloadEngine(), File(context.cacheDir, "downloads")) + downloads = + DownloadRepository( + db, + sources, + FakeDownloadEngine(), + File(context.cacheDir, "downloads"), + artworkStore = ArtworkStore(File(context.cacheDir, "art")), + artworkFetcher = { ByteArray(0) }, + ) handle = FakePlayerHandle() controller = PlaybackController( diff --git a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt index abc2b358..1a88da11 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/QueueViewModelTest.kt @@ -6,6 +6,7 @@ import androidx.sqlite.driver.bundled.BundledSQLiteDriver import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.data.db.SubtracksDatabase +import com.subtracks.data.download.ArtworkStore import com.subtracks.data.download.FakeDownloadEngine import com.subtracks.data.model.QueueKind import com.subtracks.data.model.Song @@ -55,9 +56,18 @@ class QueueViewModelTest { .inMemoryDatabaseBuilder(context, SubtracksDatabase::class.java) .setDriver(BundledSQLiteDriver()) .build() - val sources = SourceRepository(db, OkHttpClient(), fakeUserPreferences()) + val sources = SourceRepository(db, OkHttpClient(), fakeUserPreferences(), ArtworkStore(File(context.cacheDir, "art"))) queues = QueueRepository(db) - val downloads = DownloadRepository(db, sources, FakeDownloadEngine(), File(context.cacheDir, "downloads")) + val artwork = ArtworkStore(File(context.cacheDir, "art")) + val downloads = + DownloadRepository( + db, + sources, + FakeDownloadEngine(), + File(context.cacheDir, "downloads"), + artworkStore = artwork, + artworkFetcher = { ByteArray(0) }, + ) controller = PlaybackController(sources, queues, FakePlayerConnection(FakePlayerHandle()), downloads, dispatcher = dispatcher) viewModel = QueueViewModel(queues, controller) }