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 bd17ad0a..9cf59a88 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -204,8 +204,8 @@ private const val ARTIST_COVER_CREDITS = "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "WHERE song_artists.sourceId = :sourceId AND albums.coverArt IS NOT NULL" -internal const val ARTIST_ART_UPDATE_SQL = - "WITH credits AS (SELECT artistId, albumId, " + +private const val ARTIST_COVER_RANKS = + "credits AS (SELECT artistId, albumId, " + "MIN(CASE creditType WHEN 'album' THEN 0 ELSE 1 END) AS creditRank " + "FROM ($ARTIST_COVER_CREDITS) GROUP BY artistId, albumId), " + "ranked AS (SELECT credits.artistId AS artistId, albums.coverArt AS coverArt, " + @@ -215,7 +215,10 @@ internal const val ARTIST_ART_UPDATE_SQL = "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), " + + "ROW_NUMBER() OVER (PARTITION BY artistId ORDER BY coverRank) AS coverRank FROM unique_ranked)" + +internal const val ARTIST_ART_UPDATE_SQL = + "WITH $ARTIST_COVER_RANKS, " + "covers AS (SELECT artistId, group_concat(coverArt, char(31)) AS coverArts FROM (" + "SELECT artistId, coverArt, coverRank FROM deduped WHERE coverRank <= 4 ORDER BY artistId, coverRank" + ") GROUP BY artistId), " + @@ -241,11 +244,8 @@ internal const val ARTIST_ART_UPDATE_SQL = "(artists.coverArts != desired.coverArts OR artists.downloadedCoverArts != desired.downloadedCoverArts)" internal const val ARTIST_COVER_ARTS_SQL = - "SELECT DISTINCT albums.coverArt FROM ($ARTIST_COVER_CREDITS) credits " + - "JOIN albums ON albums.sourceId = :sourceId AND albums.id = credits.albumId " + - "WHERE credits.artistId = :artistId AND albums.coverArt IS NOT NULL " + - "ORDER BY CASE credits.creditType WHEN 'album' THEN 0 ELSE 1 END, albums.year DESC, " + - "albums.sortName COLLATE NOCASE, albums.id LIMIT 4" + "WITH $ARTIST_COVER_RANKS " + + "SELECT coverArt FROM deduped WHERE artistId = :artistId AND coverRank <= 4 ORDER BY coverRank" internal const val DOWNLOADED_ARTIST_ART_UPDATE_SQL = "WITH downloaded_albums AS (SELECT DISTINCT artistId, albumId FROM ($DOWNLOADED_COVER_ALBUM_ROWS) " + @@ -433,13 +433,7 @@ interface LibraryDao { suspend fun recomputeArtistArt(sourceId: Long) @Query(ARTIST_COVER_ARTS_SQL) - suspend fun artistCoverArts( - sourceId: Long, - artistId: String, - ): List - - @Query(ARTIST_COVER_ARTS_SQL) - fun artistCoverArtsFlow( + fun artistCoverArts( sourceId: Long, artistId: String, ): Flow> diff --git a/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt index f752c60f..635d6fcd 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/LibraryRepository.kt @@ -445,7 +445,7 @@ class LibraryRepository( fun artistCoverArts( sourceId: Long, artistId: String, - ): Flow> = db.libraryDao().artistCoverArtsFlow(sourceId, artistId) + ): Flow> = db.libraryDao().artistCoverArts(sourceId, artistId) fun albumDiscs( sourceId: Long, diff --git a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailViewModel.kt b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailViewModel.kt index 5356d4ea..50628f25 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailViewModel.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailViewModel.kt @@ -145,50 +145,16 @@ class ArtistDetailViewModel( init { viewModelScope.launch { - combine( - artist, - sourceRepository.offline, - sourceRepository.activeSource, - sourceRepository.activeSource.filterNotNull().flatMapLatest { - libraryRepository.artistCoverArts(it, artistId) - }, - ) { loaded, offline, activeSource, covers -> ArtistArtInput(loaded, offline, activeSource, covers) } - .collectLatest { input -> - if (input.sourceId == null) return@collectLatest - val resolved = - if (input.covers.isEmpty()) { - input.artist - } else { - when { - input.artist == null -> { - Artist( - sourceId = input.sourceId, - id = artistId, - name = "", - albumCount = 0, - starred = null, - coverArts = input.covers, - downloadedCoverArts = if (input.offline) input.covers else emptyList(), - ) - } - - input.offline -> { - if (input.artist.downloadedCoverArts.isNullOrEmpty()) { - input.artist.copy(downloadedCoverArts = input.covers) - } else { - input.artist - } - } - - input.artist.coverArts == input.covers -> { - input.artist - } - - else -> { - input.artist.copy(coverArts = input.covers) - } - } - } + sourceRepository.activeSource + .filterNotNull() + .flatMapLatest { sourceId -> + combine( + libraryRepository.artist(sourceId, artistId), + libraryRepository.artistCoverArts(sourceId, artistId), + sourceRepository.offline, + ) { loaded, covers, offline -> ArtistArtInput(loaded, artistId, offline, sourceId, covers) } + }.collectLatest { input -> + val resolved = resolveArtistArtInput(input) if (resolved == null) { _art.value = null _artThumbnail.value = null @@ -216,9 +182,34 @@ class ArtistDetailViewModel( } } -private data class ArtistArtInput( +internal data class ArtistArtInput( val artist: Artist?, + val artistId: String, val offline: Boolean, - val sourceId: Long?, + val sourceId: Long, val covers: List, ) + +internal fun resolveArtistArtInput(input: ArtistArtInput): Artist? { + val artist = input.artist + if (input.covers.isEmpty()) return artist + if (artist == null) { + return Artist( + sourceId = input.sourceId, + id = input.artistId, + name = "", + albumCount = 0, + starred = null, + coverArts = input.covers, + downloadedCoverArts = if (input.offline) input.covers else emptyList(), + ) + } + return if (input.offline) { + artist.copy( + coverArts = input.covers, + downloadedCoverArts = artist.downloadedCoverArts.ifEmpty { input.covers }, + ) + } else { + artist.copy(coverArts = input.covers) + } +} 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 85321bdc..527ef321 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt @@ -656,7 +656,7 @@ class LibraryDaoTest { } @Test - fun coverArtFallbackFindsTrackCreditsWithoutAnArtistRow() = + fun coverArtFallbackFindsEveryCreditPath() = runTest { val sourceId = source() val dao = db.libraryDao() @@ -696,9 +696,44 @@ class LibraryDaoTest { assertNull(dao.artistOnce(sourceId, "ar-missing")) assertNull(dao.artistOnce(sourceId, "ar-featured")) assertNull(dao.artistOnce(sourceId, "ar-guest")) - assertEquals(listOf("track-art"), dao.artistCoverArts(sourceId, "ar-missing")) - assertEquals(listOf("featured-art"), dao.artistCoverArts(sourceId, "ar-featured")) - assertEquals(listOf("featured-art"), dao.artistCoverArts(sourceId, "ar-guest")) + assertEquals(listOf("track-art"), dao.artistCoverArts(sourceId, "ar-missing").first()) + assertEquals(listOf("featured-art", "track-art"), dao.artistCoverArts(sourceId, "ar-other").first()) + assertEquals(listOf("featured-art"), dao.artistCoverArts(sourceId, "ar-featured").first()) + assertEquals(listOf("featured-art"), dao.artistCoverArts(sourceId, "ar-guest").first()) + } + + @Test + fun artistCoverArtsRanksDeduplicatesCapsAndStaysInSource() = + runTest { + val sourceId = source() + val otherSourceId = source() + val dao = db.libraryDao() + dao.upsertArtists(listOf(artist(sourceId, "ar-rank", "Rank", albumCount = 0))) + dao.upsertAlbums( + listOf( + album(sourceId, "a-new", "New", year = 2020, starred = null) + .copy(artistId = "ar-rank", coverArt = "shared"), + album(sourceId, "a-mid", "Mid", year = 2015, starred = null) + .copy(artistId = "ar-rank", coverArt = "other"), + album(sourceId, "a-old", "Old", year = 2010, starred = null) + .copy(artistId = "ar-rank", coverArt = "shared"), + album(sourceId, "a-4", "Four", year = 2012, starred = null) + .copy(artistId = "ar-rank", coverArt = "c4"), + album(sourceId, "a-5", "Five", year = 2011, starred = null) + .copy(artistId = "ar-rank", coverArt = "c5"), + album(sourceId, "a-6", "Six", year = 2009, starred = null) + .copy(artistId = "ar-rank", coverArt = "c6"), + album(otherSourceId, "a-leak", "Leak", year = 2025, starred = null) + .copy(artistId = "ar-rank", coverArt = "leak"), + ), + ) + + val expected = listOf("shared", "other", "c4", "c5") + assertEquals(expected, dao.artistCoverArts(sourceId, "ar-rank").first()) + + dao.recomputeArtistArt(sourceId) + + assertEquals(expected, dao.artistOnce(sourceId, "ar-rank")!!.coverArts) } @Test diff --git a/app/src/test/kotlin/com/subtracks/ui/library/ArtistDetailArtTest.kt b/app/src/test/kotlin/com/subtracks/ui/library/ArtistDetailArtTest.kt new file mode 100644 index 00000000..5d4eeba8 --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/ui/library/ArtistDetailArtTest.kt @@ -0,0 +1,105 @@ +package com.subtracks.ui.library + +import com.subtracks.data.model.Artist +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +class ArtistDetailArtTest { + @Test + fun aCreditedArtistWithoutARowUsesTheCreditCovers() { + val resolved = + resolveArtistArtInput( + ArtistArtInput( + artist = null, + artistId = "ar1", + offline = false, + sourceId = 7, + covers = listOf("a", "b"), + ), + ) + + assertEquals(7L, resolved?.sourceId) + assertEquals("ar1", resolved?.id) + assertEquals(listOf("a", "b"), resolved?.coverArts) + } + + @Test + fun anOfflineArtistKeepsItsDownloadedCovers() { + val resolved = + resolveArtistArtInput( + ArtistArtInput( + artist = artist(coverArts = listOf("online"), downloadedCoverArts = listOf("downloaded")), + artistId = "ar1", + offline = true, + sourceId = 7, + covers = listOf("online", "other"), + ), + ) + + assertEquals(listOf("online", "other"), resolved?.coverArts) + assertEquals(listOf("downloaded"), resolved?.downloadedCoverArts) + } + + @Test + fun anOfflineArtistWithoutDownloadedCoversFallsBackToTheCreditCovers() { + val resolved = + resolveArtistArtInput( + ArtistArtInput( + artist = artist(downloadedCoverArts = emptyList()), + artistId = "ar1", + offline = true, + sourceId = 7, + covers = listOf("a", "b"), + ), + ) + + assertEquals(listOf("a", "b"), resolved?.downloadedCoverArts) + } + + @Test + fun anArtistWithMaterializedCoversIsKeptWhenTheCreditQueryIsEmpty() { + val original = artist(coverArts = listOf("materialized")) + + val resolved = + resolveArtistArtInput( + ArtistArtInput( + artist = original, + artistId = "ar1", + offline = false, + sourceId = 7, + covers = emptyList(), + ), + ) + + assertEquals(original, resolved) + } + + @Test + fun noArtistAndNoCoversClearsTheArt() { + assertNull( + resolveArtistArtInput( + ArtistArtInput( + artist = null, + artistId = "ar1", + offline = false, + sourceId = 7, + covers = emptyList(), + ), + ), + ) + } + + private fun artist( + coverArts: List = emptyList(), + downloadedCoverArts: List = emptyList(), + ) = Artist( + sourceId = 7, + id = "ar1", + name = "Artist", + albumCount = 1, + starred = null, + coverArts = coverArts, + downloadedCoverArts = downloadedCoverArts, + ) +}