From 783d7f2ebc048a0903761209e9a9e08f80c05aca Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:11:20 +0000 Subject: [PATCH] library: address artist art review findings Cap the resolver at four covers, re-rank after deduplication so duplicate covers do not consume the grid budget, carry a thumbnail per fallback step, and advance to the grid when both the full image and its thumbnail fail. Negative-cache failed provider-image fetches per artist URL, clear materialized downloaded covers when all downloads are removed, and drop redundant plumbing. --- .../com/subtracks/data/db/LibraryDao.kt | 15 +++++++-- .../subtracks/data/repo/DownloadRepository.kt | 32 ++++++++++--------- .../com/subtracks/ui/components/ArtistArt.kt | 23 ++++++++----- .../com/subtracks/ui/components/Components.kt | 15 +++++++-- .../com/subtracks/ui/home/HomeScreen.kt | 3 +- .../ui/library/ArtistDetailScreen.kt | 8 +---- .../com/subtracks/data/db/LibraryDaoTest.kt | 21 ++++++++++++ .../data/repo/DownloadRepositoryTest.kt | 12 ++++++- .../com/subtracks/ui/AppScreenshotTest.kt | 13 +++++--- .../subtracks/ui/components/ArtistArtTest.kt | 12 +++++-- 10 files changed, 109 insertions(+), 45 deletions(-) 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 8135c8cf..853e6828 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -209,8 +209,10 @@ internal const val ARTIST_ART_UPDATE_SQL = "FROM credits JOIN albums ON albums.sourceId = :sourceId AND albums.id = credits.albumId " + "WHERE albums.coverArt IS NOT NULL), " + "unique_ranked AS (SELECT artistId, coverArt, MIN(coverRank) AS coverRank FROM ranked GROUP BY artistId, coverArt), " + + "deduped AS (SELECT artistId, coverArt, " + + "ROW_NUMBER() OVER (PARTITION BY artistId ORDER BY coverRank) AS coverRank FROM unique_ranked), " + "covers AS (SELECT artistId, group_concat(coverArt, char(31)) AS coverArts FROM (" + - "SELECT artistId, coverArt, coverRank FROM unique_ranked WHERE coverRank <= 4 ORDER BY artistId, coverRank" + + "SELECT artistId, coverArt, coverRank FROM deduped WHERE coverRank <= 4 ORDER BY artistId, coverRank" + ") GROUP BY artistId), " + "downloaded_albums AS (SELECT DISTINCT artistId, albumId FROM ($DOWNLOADED_COVER_ALBUM_ROWS)), " + "downloaded AS (SELECT dl.artistId AS artistId, albums.coverArt AS coverArt, " + @@ -219,8 +221,10 @@ internal const val ARTIST_ART_UPDATE_SQL = "FROM downloaded_albums dl JOIN albums ON albums.sourceId = :sourceId AND albums.id = dl.albumId " + "WHERE albums.coverArt IS NOT NULL), " + "unique_downloaded AS (SELECT artistId, coverArt, MIN(coverRank) AS coverRank FROM downloaded GROUP BY artistId, coverArt), " + + "downloaded_deduped AS (SELECT artistId, coverArt, " + + "ROW_NUMBER() OVER (PARTITION BY artistId ORDER BY coverRank) AS coverRank FROM unique_downloaded), " + "downloaded_covers AS (SELECT artistId, group_concat(coverArt, char(31)) AS coverArts FROM (" + - "SELECT artistId, coverArt, coverRank FROM unique_downloaded WHERE coverRank <= 4 ORDER BY artistId, coverRank" + + "SELECT artistId, coverArt, coverRank FROM downloaded_deduped WHERE coverRank <= 4 ORDER BY artistId, coverRank" + ") GROUP BY artistId) " + "UPDATE artists SET " + "coverArts = COALESCE((SELECT coverArts FROM covers WHERE covers.artistId = artists.id), ''), " + @@ -237,8 +241,10 @@ internal const val DOWNLOADED_ARTIST_ART_UPDATE_SQL = "FROM downloaded_albums dl JOIN albums ON albums.sourceId = :sourceId AND albums.id = dl.albumId " + "WHERE albums.coverArt IS NOT NULL), " + "unique_downloaded AS (SELECT artistId, coverArt, MIN(coverRank) AS coverRank FROM downloaded GROUP BY artistId, coverArt), " + + "downloaded_deduped AS (SELECT artistId, coverArt, " + + "ROW_NUMBER() OVER (PARTITION BY artistId ORDER BY coverRank) AS coverRank FROM unique_downloaded), " + "downloaded_covers AS (SELECT artistId, group_concat(coverArt, char(31)) AS coverArts FROM (" + - "SELECT artistId, coverArt, coverRank FROM unique_downloaded WHERE coverRank <= 4 ORDER BY artistId, coverRank" + + "SELECT artistId, coverArt, coverRank FROM downloaded_deduped WHERE coverRank <= 4 ORDER BY artistId, coverRank" + ") GROUP BY artistId) " + "UPDATE artists SET downloadedCoverArts = COALESCE((SELECT coverArts FROM downloaded_covers " + "WHERE downloaded_covers.artistId = artists.id), '') " + @@ -406,6 +412,9 @@ interface LibraryDao { artistIds: List, ) + @Query("UPDATE artists SET downloadedCoverArts = '' WHERE sourceId = :sourceId AND downloadedCoverArts != ''") + suspend fun clearDownloadedArtistArt(sourceId: Long) + @Transaction suspend fun recomputeAggregates(sourceId: Long) { recomputeAlbumPlayData(sourceId) 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 ec8c9715..deb915ea 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -68,6 +68,7 @@ class DownloadRepository( private val statesFlow = MutableStateFlow>(emptyMap()) private val attemptedBios = ConcurrentHashMap.newKeySet>() + private val failedArtistImages = ConcurrentHashMap.newKeySet() fun start() { if (started) return @@ -377,6 +378,7 @@ class DownloadRepository( .mapNotNull { it.engineId } .let(engine::cancel) db.downloadDao().deleteSource(sourceId) + db.libraryDao().clearDownloadedArtistArt(sourceId) dir(sourceId).deleteRecursively() } } @@ -422,13 +424,9 @@ class DownloadRepository( val albumArtist = album?.artistId?.let { db.libraryDao().artistOnce(sourceId, it) } listOfNotNull(song?.artistId, album?.artistId).forEach(artistIds::add) listOfNotNull(songArtist, albumArtist).forEach { storeArtistImage(sourceId, it) } - val artistFallbackCovers = - listOfNotNull(songArtist, albumArtist) - .filter { it.artistImageUrl.isNullOrBlank() } - .mapNotNull { it.coverArt } val coverArt = ( - listOfNotNull(album?.coverArt) + artistFallbackCovers + + listOfNotNull(album?.coverArt) + db.downloadDao().playlistCoversForSong(sourceId, songId) ).distinct() coverArt.forEach { cover -> @@ -445,17 +443,21 @@ class DownloadRepository( if (!imageUrl.isNullOrBlank()) { val cacheKey = artistImageKey(sourceId, artist.id) if (artworkStore.file(sourceId, cacheKey).exists()) return - val bytes = - try { - artworkFetcher.fetch(imageUrl) - } catch (e: CancellationException) { - throw e - } catch (_: Exception) { - null + val attemptKey = "$sourceId:${artist.id}:$imageUrl" + if (attemptKey !in failedArtistImages) { + val bytes = + try { + artworkFetcher.fetch(imageUrl) + } catch (e: CancellationException) { + throw e + } catch (_: Exception) { + null + } + if (bytes != null && bytes.isNotEmpty()) { + artworkStore.write(sourceId, cacheKey, bytes) + return } - if (bytes != null && bytes.isNotEmpty()) { - artworkStore.write(sourceId, cacheKey, bytes) - return + failedArtistImages += attemptKey } } artist.coverArt?.let { diff --git a/app/src/main/kotlin/com/subtracks/ui/components/ArtistArt.kt b/app/src/main/kotlin/com/subtracks/ui/components/ArtistArt.kt index a07104fd..5c8671b6 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/ArtistArt.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/ArtistArt.kt @@ -29,6 +29,7 @@ import kotlin.math.sin data class ArtistArt( val image: CoverArtRef? = null, + val thumbnail: CoverArtRef? = null, val covers: List = emptyList(), val fallback: ArtistArt? = null, ) { @@ -57,15 +58,22 @@ internal fun resolveArtistArt( val coverIds = if (offline) artist.downloadedCoverArts else artist.coverArts val grid = ArtistArt( - covers = coverIds.mapNotNull { coverArt(it, forIcon || coverIds.size > 1) }, + thumbnail = coverIds.singleOrNull()?.let { coverArt(it, true) }, + covers = coverIds.take(4).mapNotNull { coverArt(it, forIcon || coverIds.size > 1) }, ) val own = - artist.coverArt - ?.let { coverArt(it, forIcon) } - ?.let { ArtistArt(image = it, fallback = grid) } + artist.coverArt?.let { coverId -> + coverArt(coverId, forIcon)?.let { image -> + ArtistArt( + image = image, + thumbnail = if (forIcon) image else coverArt(coverId, true), + fallback = grid, + ) + } + } return artist.artistImageUrl ?.let { artistImage(artist.id, it) } - ?.let { ArtistArt(image = it, fallback = own ?: grid) } + ?.let { ArtistArt(image = it, thumbnail = it, fallback = own ?: grid) } ?: own ?: grid } @@ -104,7 +112,6 @@ fun ArtistIcon( fun ArtistHeroArt( art: ArtistArt, name: String, - thumbnail: CoverArtRef?, modifier: Modifier = Modifier, ) { var shown by remember(art) { mutableStateOf(art) } @@ -116,7 +123,7 @@ fun ArtistHeroArt( CoverArt( ref = shown.image, name = name, - thumbnailRef = thumbnail, + thumbnailRef = shown.thumbnail, showPlaceholder = false, modifier = modifier, onError = showFallback, @@ -127,7 +134,7 @@ fun ArtistHeroArt( ArtistArtGrid( covers = shown.covers, name = name, - singleThumbnail = thumbnail, + singleThumbnail = shown.thumbnail, modifier = modifier, ) } diff --git a/app/src/main/kotlin/com/subtracks/ui/components/Components.kt b/app/src/main/kotlin/com/subtracks/ui/components/Components.kt index 3261bb4e..1cd69bcc 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/Components.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/Components.kt @@ -93,6 +93,7 @@ fun CoverArt( val hasThumbnailFallback = thumbnailRef != null && thumbnailRef != ref var failed by remember(refKey, thumbnailKey) { mutableStateOf(false) } var thumbnailLoaded by remember(refKey, thumbnailKey) { mutableStateOf(false) } + var thumbnailFailed by remember(refKey, thumbnailKey) { mutableStateOf(false) } var thumbnailRatio by remember(refKey, thumbnailKey) { mutableStateOf(thumbnailKey?.let(ArtworkRatioCache::get)) } val letterScheme = if (showPlaceholder || (failed && !thumbnailLoaded)) { @@ -117,6 +118,10 @@ fun CoverArt( thumbnailLoaded = thumbnailLoaded, onThumbnailLoaded = { thumbnailLoaded = true }, onThumbnailRatio = { thumbnailRatio = it }, + onThumbnailError = { + thumbnailFailed = true + if (failed) onError?.invoke() + }, contentScale = ContentScale.Crop, colorFilter = colorFilter, placeholderContentColor = letterScheme?.onPrimaryContainer, @@ -135,7 +140,7 @@ fun CoverArt( }, onError = { failed = true - if (!hasThumbnailFallback) onError?.invoke() + if (!hasThumbnailFallback || thumbnailFailed) onError?.invoke() }, modifier = Modifier.fillMaxSize(), ) @@ -167,7 +172,7 @@ fun CoverArt( }, onError = { failed = true - if (!hasThumbnailFallback) onError?.invoke() + if (!hasThumbnailFallback || thumbnailFailed) onError?.invoke() }, contentScale = ContentScale.Fit, filterQuality = FilterQuality.High, @@ -187,6 +192,10 @@ fun CoverArt( thumbnailLoaded = thumbnailLoaded, onThumbnailLoaded = { thumbnailLoaded = true }, onThumbnailRatio = { thumbnailRatio = it }, + onThumbnailError = { + thumbnailFailed = true + if (failed) onError?.invoke() + }, contentScale = ContentScale.Fit, colorFilter = colorFilter, placeholderContentColor = letterScheme?.onPrimaryContainer, @@ -216,6 +225,7 @@ private fun BoxScope.CoverArtContent( thumbnailLoaded: Boolean, onThumbnailLoaded: () -> Unit, onThumbnailRatio: (Float) -> Unit, + onThumbnailError: () -> Unit, contentScale: ContentScale, colorFilter: ColorFilter?, placeholderContentColor: Color?, @@ -254,6 +264,7 @@ private fun BoxScope.CoverArtContent( ArtworkRatioCache.put(thumbnailRef.cacheKey, it) } }, + onError = { onThumbnailError() }, modifier = Modifier.fillMaxSize(), ) } diff --git a/app/src/main/kotlin/com/subtracks/ui/home/HomeScreen.kt b/app/src/main/kotlin/com/subtracks/ui/home/HomeScreen.kt index 22fdad84..4bf1e610 100644 --- a/app/src/main/kotlin/com/subtracks/ui/home/HomeScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/home/HomeScreen.kt @@ -320,7 +320,7 @@ fun OfflineHomeScreen( ) { homeTitle() downloadedAlbumRow(albums, coverArt, onAlbumClick, onLongClick) { onMore(OfflineListKind.Albums) } - downloadedArtistRow(artists, coverArt, artistArt, onArtistClick, onLongClick) { onMore(OfflineListKind.Artists) } + downloadedArtistRow(artists, artistArt, onArtistClick, onLongClick) { onMore(OfflineListKind.Artists) } downloadedPlaylistRow(playlists, coverArt, onPlaylistClick, onLongClick) { onMore(OfflineListKind.Playlists) } songList( key = "downloaded-songs", @@ -372,7 +372,6 @@ private fun LazyListScope.downloadedAlbumRow( private fun LazyListScope.downloadedArtistRow( artists: LazyPagingItems, - coverArt: (String?, Boolean) -> CoverArtRef?, artistArt: (Artist) -> ArtistArt, onArtistClick: (Artist) -> Unit, onLongClick: (MenuTarget) -> Unit, diff --git a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt index 1f4699a3..4c3e9522 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt @@ -286,7 +286,6 @@ fun ArtistDetailScreen( bioExpanded: Boolean = false, art: CoverArtRef?, artThumbnail: CoverArtRef? = null, - artCovers: List = emptyList(), heroArt: ArtistArt? = null, artwork: ArtworkColors? = null, coverArt: (String?, Boolean) -> CoverArtRef?, @@ -406,13 +405,8 @@ fun ArtistDetailScreen( ) { Box(Modifier.fillMaxWidth()) { ArtistHeroArt( - art = - heroArt ?: ArtistArt( - image = art.takeIf { artCovers.isEmpty() }, - covers = artCovers, - ), + art = heroArt ?: ArtistArt(image = art, thumbnail = artThumbnail), name = displayName, - thumbnail = artThumbnail, modifier = Modifier.fillMaxWidth().height(ART_HEIGHT), ) if (nameScrimAlpha > 0f) { diff --git a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt index 710d82be..cea503b9 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt @@ -682,6 +682,27 @@ class LibraryDaoTest { assertEquals(listOf("art-2", "art-1"), dao.artistOnce(sourceId, "ar-1")!!.downloadedCoverArts) } + @Test + fun artistArtDeduplicatesCoversBeforeApplyingTheFourCoverBudget() = + runTest { + val sourceId = source() + val dao = db.libraryDao() + dao.upsertArtists(listOf(artist(sourceId, "ar-1", "Artist", albumCount = 6))) + dao.upsertAlbums( + listOf("A", "A", "O", "B", "C", "D").mapIndexed { index, cover -> + album(sourceId, "al-$index", "Album $index", year = 2021L - index, starred = null) + .copy(artistId = "ar-1", coverArt = cover) + }, + ) + + dao.recomputeArtistArt(sourceId) + + assertEquals( + listOf("A", "O", "B", "C"), + dao.artistOnce(sourceId, "ar-1")!!.coverArts, + ) + } + @Test fun artistArtMaterializationUsesTheCreditIndexes() = runTest { 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 2a1bd98f..ffb7a453 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -346,7 +346,10 @@ class DownloadRepositoryTest { runTest { seedLibrary() writeFile(1, "s1", "audio") - runBlocking { repository.download(1, "s1") } + runBlocking { + repository.download(1, "s1") + db.libraryDao().upsertArtists(listOf(artist(ARTIST_ART).copy(downloadedCoverArts = listOf(ALBUM_ART)))) + } val id = engine.requests.single().first runBlocking { repository.removeSource(1) } @@ -354,6 +357,13 @@ class DownloadRepositoryTest { assertTrue(engine.cancelled.contains(id)) assertFalse(file(1, "s1").exists()) assertTrue(allRows().isEmpty()) + assertTrue( + db + .libraryDao() + .artistOnce(1, "ar1")!! + .downloadedCoverArts + .isEmpty(), + ) } @Test diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index ce455e68..1d501cb2 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt @@ -334,11 +334,14 @@ class AppScreenshotTest { ), art = CoverArtRef("art-al-kid-a", "test:art-al-kid-a"), artThumbnail = CoverArtRef("art-al-kid-a", "test:art-al-kid-a"), - artCovers = - Fixtures.albums - .take(3) - .mapNotNull { it.coverArt } - .map { CoverArtRef(it, "test:$it") }, + heroArt = + ArtistArt( + covers = + Fixtures.albums + .take(3) + .mapNotNull { it.coverArt } + .map { CoverArtRef(it, "test:$it") }, + ), artwork = artworkColorsFromSeed(Color.rgb(120, 80, 200)), coverArt = { id, _ -> id?.let { CoverArtRef(it, "test:$it") } }, onBack = {}, diff --git a/app/src/test/kotlin/com/subtracks/ui/components/ArtistArtTest.kt b/app/src/test/kotlin/com/subtracks/ui/components/ArtistArtTest.kt index d12a4f91..613e818c 100644 --- a/app/src/test/kotlin/com/subtracks/ui/components/ArtistArtTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/components/ArtistArtTest.kt @@ -76,8 +76,16 @@ class ArtistArtTest { } val artistImage: (String, String?) -> CoverArtRef? = { _, _ -> null } - resolveArtistArt(artist(coverArts = listOf("a")), coverArt, artistImage, offline = false, forIcon = false) - assertEquals(listOf(false), thumbnails) + val single = + resolveArtistArt( + artist(coverArts = listOf("a")), + coverArt, + artistImage, + offline = false, + forIcon = false, + ) + assertEquals(listOf(true, false), thumbnails) + assertEquals("a", single.thumbnail?.url) thumbnails.clear() resolveArtistArt(artist(coverArts = listOf("a", "b")), coverArt, artistImage, offline = false, forIcon = false)