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 d1ccec9f..ef5d943c 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt @@ -13,27 +13,31 @@ import kotlinx.coroutines.flow.Flow // A song is attributed to its album artist and to its credited track artist, so a guest artist's // downloads are visible even though they lead no albums of their own. -private const val ARTIST_DOWNLOAD_ROWS = - "SELECT albums.artistId AS id, songs.id AS songId, song_downloads.status AS status " + +internal const val ARTIST_DOWNLOAD_ROWS = + "SELECT albums.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + + "song_downloads.downloadedAt AS downloadedAt " + "FROM songs " + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId AND albums.artistId IS NOT NULL " + "UNION ALL " + - "SELECT album_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status " + + "SELECT album_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + + "song_downloads.downloadedAt AS downloadedAt " + "FROM songs " + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "JOIN album_artists ON album_artists.sourceId = albums.sourceId AND album_artists.albumId = albums.id " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId " + "UNION ALL " + - "SELECT songs.artistId AS id, songs.id AS songId, song_downloads.status AS status " + + "SELECT songs.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + + "song_downloads.downloadedAt AS downloadedAt " + "FROM songs " + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId AND songs.artistId IS NOT NULL " + "UNION ALL " + - "SELECT song_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status " + + "SELECT song_artists.artistId AS id, songs.id AS songId, song_downloads.status AS status, " + + "song_downloads.downloadedAt AS downloadedAt " + "FROM songs " + "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "JOIN song_artists ON song_artists.sourceId = songs.sourceId AND song_artists.songId = songs.id " + @@ -128,7 +132,7 @@ interface DownloadDao { @Query( "SELECT d.sourceId AS sourceId, d.songId AS songId, s.title AS title, s.albumId AS albumId, al.name AS albumName, " + - "al.artistId AS artistId, ar.name AS artistName, d.status AS status, " + + "al.artistId AS artistId, COALESCE(ar.name, al.albumArtist) AS artistName, d.status AS status, " + "d.bytes AS bytes, d.total AS total " + "FROM song_downloads d " + "JOIN songs s ON s.sourceId = d.sourceId AND s.id = d.songId " + 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 9ac53dfc..72a14d4d 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/LibraryDao.kt @@ -73,6 +73,10 @@ internal const val ALBUM_MATCHES_ARTIST = "(albums.artistId = :artistId OR albums.id IN (SELECT albumId FROM album_artists " + "WHERE sourceId = albums.sourceId AND artistId = :artistId))" +private const val ARTIST_ALBUM_MATCH = + "(a.artistId = artists.id OR a.id IN (SELECT albumId FROM album_artists " + + "WHERE sourceId = artists.sourceId AND artistId = artists.id))" + internal const val ARTISTS_FILTER = "FROM artists WHERE sourceId = :sourceId " + "AND (:starredFilter = 0 OR (:starredFilter = 1 AND starred IS NOT NULL) OR (:starredFilter = 2 AND starred IS NULL)) " + @@ -201,14 +205,14 @@ interface LibraryDao { @Query( "UPDATE artists SET " + "playCount = COALESCE((SELECT SUM(a.playCount) FROM albums a INDEXED BY index_albums_sourceId_artistId " + - "WHERE a.sourceId = artists.sourceId AND a.artistId = artists.id), 0), " + + "WHERE a.sourceId = artists.sourceId AND $ARTIST_ALBUM_MATCH), 0), " + "played = (SELECT MAX(a.played) FROM albums a INDEXED BY index_albums_sourceId_artistId " + - "WHERE a.sourceId = artists.sourceId AND a.artistId = artists.id) " + + "WHERE a.sourceId = artists.sourceId AND $ARTIST_ALBUM_MATCH) " + "WHERE sourceId = :sourceId AND (" + "playCount IS NOT COALESCE((SELECT SUM(a.playCount) FROM albums a INDEXED BY index_albums_sourceId_artistId " + - "WHERE a.sourceId = artists.sourceId AND a.artistId = artists.id), 0) " + + "WHERE a.sourceId = artists.sourceId AND $ARTIST_ALBUM_MATCH), 0) " + "OR played IS NOT (SELECT MAX(a.played) FROM albums a INDEXED BY index_albums_sourceId_artistId " + - "WHERE a.sourceId = artists.sourceId AND a.artistId = artists.id))", + "WHERE a.sourceId = artists.sourceId AND $ARTIST_ALBUM_MATCH))", ) suspend fun recomputeArtistPlayData(sourceId: Long) @@ -240,7 +244,9 @@ interface LibraryDao { @Query( "UPDATE artists SET playCount = playCount + 1, played = MAX(COALESCE(played, :at), :at) " + - "WHERE sourceId = :sourceId AND id = (SELECT artistId FROM albums WHERE sourceId = :sourceId AND id = :albumId)", + "WHERE sourceId = :sourceId AND (" + + "id = (SELECT artistId FROM albums WHERE sourceId = :sourceId AND id = :albumId) " + + "OR id IN (SELECT artistId FROM album_artists WHERE sourceId = :sourceId AND albumId = :albumId))", ) suspend fun bumpArtistPlay( sourceId: Long, @@ -825,20 +831,8 @@ interface LibraryDao { @Query( "SELECT artists.* FROM artists " + - "JOIN (" + - "SELECT albums.artistId AS artistId, d.downloadedAt AS downloadedAt " + - "FROM songs " + - "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + - "JOIN song_downloads d ON d.sourceId = songs.sourceId AND d.songId = songs.id AND d.status = 'Completed' " + - "WHERE songs.sourceId = :sourceId AND albums.artistId IS NOT NULL " + - "UNION ALL " + - "SELECT songs.artistId AS artistId, d.downloadedAt AS downloadedAt " + - "FROM songs " + - "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + - "JOIN song_downloads d ON d.sourceId = songs.sourceId AND d.songId = songs.id AND d.status = 'Completed' " + - "WHERE songs.sourceId = :sourceId AND songs.artistId IS NOT NULL" + - ") dl ON dl.artistId = artists.id " + - "WHERE artists.sourceId = :sourceId " + + "JOIN ($ARTIST_DOWNLOAD_ROWS) dl ON dl.id = artists.id " + + "WHERE artists.sourceId = :sourceId AND dl.status = 'Completed' " + "GROUP BY artists.sourceId, artists.id " + "ORDER BY MAX(dl.downloadedAt) DESC, artists.name COLLATE NOCASE, artists.id", ) diff --git a/app/src/main/kotlin/com/subtracks/data/model/Models.kt b/app/src/main/kotlin/com/subtracks/data/model/Models.kt index 415faafe..6d51fd5e 100644 --- a/app/src/main/kotlin/com/subtracks/data/model/Models.kt +++ b/app/src/main/kotlin/com/subtracks/data/model/Models.kt @@ -555,21 +555,33 @@ class ArtistRefsConverter { } object ArtistRefsCodec { - fun encode(refs: List): String = refs.joinToString(RECORD) { "${it.id}$FIELD${it.name}" } + fun encode(refs: List): String = + buildString { + refs.forEach { ref -> + append(ref.id.length).append(':').append(ref.id) + append(ref.name.length).append(':').append(ref.name) + } + } - fun decode(value: String): List = - value - .takeIf { it.isNotEmpty() } - ?.split(RECORD) - ?.map { entry -> - val separator = entry.indexOf(FIELD) - if (separator < 0) { - ArtistRef(entry, "") - } else { - ArtistRef(entry.substring(0, separator), entry.substring(separator + 1)) - } - }.orEmpty() - - private const val FIELD = "\u001F" - private const val RECORD = "\u001E" + fun decode(value: String): List { + val refs = mutableListOf() + var index = 0 + while (index < value.length) { + val idLengthEnd = value.indexOf(':', index) + if (idLengthEnd < 0) return refs + val idLength = value.substring(index, idLengthEnd).toIntOrNull() ?: return refs + val idStart = idLengthEnd + 1 + val idEnd = idStart + idLength + if (idLength < 0 || idEnd > value.length) return refs + val nameLengthEnd = value.indexOf(':', idEnd) + if (nameLengthEnd < 0) return refs + val nameLength = value.substring(idEnd, nameLengthEnd).toIntOrNull() ?: return refs + val nameStart = nameLengthEnd + 1 + val nameEnd = nameStart + nameLength + if (nameLength < 0 || nameEnd > value.length) return refs + refs += ArtistRef(value.substring(idStart, idEnd), value.substring(nameStart, nameEnd)) + index = nameEnd + } + return refs + } } diff --git a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicXml.kt b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicXml.kt index 40c744b6..0c469f91 100644 --- a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicXml.kt +++ b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicXml.kt @@ -91,6 +91,7 @@ object SubsonicXml { "artists" -> if (level == 1) entity.addArtists(attrs) "albumArtists" -> entity.clearNestedArtists() "artist" -> if (level == 2) entity.addNestedArtist(attrs) + else -> if (level == 1) entity.clearNestedArtists() } } @@ -104,6 +105,7 @@ object SubsonicXml { "discTitles" -> if (level == 1) entity.addDiscTitle(attrs) "artists" -> if (level == 1) entity.addArtists(attrs) "artist" -> if (level == 2) entity.addNestedArtist(attrs) + else -> if (level == 1) entity.clearArtists() } } @@ -197,6 +199,10 @@ internal class AlbumDraft( if (artistContainer) attrs.artistRef()?.let { artists += it } } + fun clearArtists() { + artistContainer = false + } + fun toAlbum(): Album = Album( sourceId = sourceId, diff --git a/app/src/main/kotlin/com/subtracks/ui/components/ArtistChooserSheet.kt b/app/src/main/kotlin/com/subtracks/ui/components/ArtistChooserSheet.kt index 4e8453f7..0bbae3cd 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/ArtistChooserSheet.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/ArtistChooserSheet.kt @@ -1,9 +1,14 @@ package com.subtracks.ui.components import androidx.compose.foundation.clickable +import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.heightIn import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.verticalScroll import androidx.compose.material.icons.Icons import androidx.compose.material.icons.rounded.Person import androidx.compose.material3.ExperimentalMaterial3Api @@ -49,19 +54,27 @@ fun ArtistChooserSheet( style = MaterialTheme.typography.titleMedium, modifier = Modifier.padding(start = 16.dp, top = 16.dp, end = 16.dp, bottom = 8.dp), ) - artists.forEach { artist -> - ListItem( - headlineContent = { Text(artist.name.ifBlank { stringResource(R.string.resources_filter_artist) }) }, - leadingContent = { Icon(Icons.Rounded.Person, contentDescription = null) }, - colors = ListItemDefaults.colors(containerColor = Color.Transparent), - modifier = - Modifier.clickable { - scope.launch { - sheetState.hide() - onArtistClick(artist) - } - }, - ) + Column( + modifier = + Modifier + .fillMaxWidth() + .heightIn(max = 400.dp) + .verticalScroll(rememberScrollState()), + ) { + artists.forEach { artist -> + ListItem( + headlineContent = { Text(artist.name.ifBlank { stringResource(R.string.resources_filter_artist) }) }, + leadingContent = { Icon(Icons.Rounded.Person, contentDescription = null) }, + colors = ListItemDefaults.colors(containerColor = Color.Transparent), + modifier = + Modifier.clickable { + scope.launch { + sheetState.hide() + onArtistClick(artist) + } + }, + ) + } } Spacer(Modifier.height(16.dp)) } 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 ec196937..c8d643ea 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt @@ -140,6 +140,11 @@ private const val THEME_TRANSITION_MS = 100 private const val SCRIM_FADE_MS = 180 internal const val ARTIST_NAME_SCRIM_TAG = "artistNameScrim" +internal fun isAlbumArtist( + albums: List, + artistId: String, +): Boolean = albums.any { album -> album.artistId == artistId || album.artistRefs.any { it.id == artistId } } + @Composable fun ArtistDetailRoute( artistId: String, @@ -187,7 +192,7 @@ fun ArtistDetailRoute( ArtistDetailScreen( artist = artist, artistName = artistName.orEmpty(), - isAlbumArtist = albums.any { album -> album.artistRefs.any { it.id == artistId } }, + isAlbumArtist = isAlbumArtist(albums, artistId), albums = albums, art = art, artThumbnail = artThumbnail, diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt index 2e9db6bb..f71e054e 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt @@ -205,6 +205,7 @@ fun NowPlayingRoute( val download = songId?.let { downloads[it] } val scope = rememberCoroutineScope() var showArtistChooser by remember { mutableStateOf(false) } + LaunchedEffect(songId) { showArtistChooser = false } val song by produceState(null, sourceId, songId) { if (sourceId != null && songId != null) { diff --git a/app/src/test/kotlin/com/subtracks/data/model/ArtistRefsCodecTest.kt b/app/src/test/kotlin/com/subtracks/data/model/ArtistRefsCodecTest.kt new file mode 100644 index 00000000..0ed2c07c --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/data/model/ArtistRefsCodecTest.kt @@ -0,0 +1,25 @@ +package com.subtracks.data.model + +import org.junit.Assert.assertEquals +import org.junit.Test + +class ArtistRefsCodecTest { + @Test + fun roundTripsNamesWithTheOldSeparatorsAndColons() { + val refs = + listOf( + ArtistRef("ar1", "Name\u001Ewith\u001Fseparators"), + ArtistRef("ar:2", "1:Colon Artist"), + ArtistRef("", ""), + ) + + assertEquals(refs, ArtistRefsCodec.decode(ArtistRefsCodec.encode(refs))) + } + + @Test + fun decodesEmptyAndMalformedValuesAsEmpty() { + assertEquals(emptyList(), ArtistRefsCodec.decode("")) + assertEquals(emptyList(), ArtistRefsCodec.decode("not a ref")) + assertEquals(emptyList(), ArtistRefsCodec.decode("9:short4:name")) + } +} diff --git a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicXmlTest.kt b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicXmlTest.kt index 1beb53c0..c2140bba 100644 --- a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicXmlTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicXmlTest.kt @@ -191,6 +191,27 @@ class SubsonicXmlTest { ) } + @Test + fun ignoresNestedArtistsOutsideTheArtistsContainer() { + val songs = + songs( + """ + + + + + + + + + + + """.trimIndent(), + ) + + assertEquals(listOf(ArtistRef("ar1", "A")), songs.single().artists) + } + @Test fun mapsNestedSongArtistRefs() { val songs = @@ -205,6 +226,9 @@ class SubsonicXmlTest { + + + """.trimIndent(), diff --git a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt index ee06e1e0..03b1bc7d 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt @@ -1045,6 +1045,44 @@ class SyncServiceTest { assertEquals(listOf("A", "B"), filtered.map { it.name }) assertEquals(listOf(1L, 1L), filtered.map { it.albumCount }) assertEquals(ListDownloadStatus(total = 1, downloaded = 1), db.downloadDao().artistStatus(1, "a2").first()) + assertEquals( + setOf("A", "B"), + db + .libraryDao() + .downloadedArtists(1) + .allRows() + .map { it.name } + .toSet(), + ) + } + + @Test + fun playsOnASharedAlbumReachEveryAlbumArtist() = + runTest { + insertSource() + val source = + FakeMusicSource( + artists = listOf(artist("a1", "A"), artist("a2", "B")), + albums = + listOf( + album("al1").copy( + artistId = "a1", + artists = listOf(ArtistRef("a1", "A"), ArtistRef("a2", "B")), + ), + ), + songs = listOf(song("s1").copy(albumId = "al1", playCount = 3, played = 100)), + ) + + SyncService(db, source).sync() + + assertEquals(3L, db.libraryDao().artistOnce(1, "a2")!!.playCount) + assertEquals(100L, db.libraryDao().artistOnce(1, "a2")!!.played) + + db.libraryDao().recordPlay(1, "s1", 500) + + assertEquals(4L, db.libraryDao().artistOnce(1, "a1")!!.playCount) + assertEquals(4L, db.libraryDao().artistOnce(1, "a2")!!.playCount) + assertEquals(500L, db.libraryDao().artistOnce(1, "a2")!!.played) } private suspend fun totalChanges(): Long = diff --git a/app/src/test/kotlin/com/subtracks/ui/components/ArtistChooserSheetTest.kt b/app/src/test/kotlin/com/subtracks/ui/components/ArtistChooserSheetTest.kt index 68786571..f85a9e68 100644 --- a/app/src/test/kotlin/com/subtracks/ui/components/ArtistChooserSheetTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/components/ArtistChooserSheetTest.kt @@ -5,6 +5,7 @@ import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.performScrollTo import androidx.test.ext.junit.runners.AndroidJUnit4 import com.subtracks.data.model.ArtistRef import com.subtracks.ui.theme.SubtracksTheme @@ -43,4 +44,27 @@ class ArtistChooserSheetTest { assertEquals(ArtistRef("ar2", "B"), opened) } + + @Test + fun longListsScrollToTheLastArtist() { + var opened: ArtistRef? = null + val artists = (1..20).map { ArtistRef("ar$it", "Artist $it") } + composeRule.setContent { + SubtracksTheme { + ArtistChooserSheet( + artists = artists, + onArtistClick = { opened = it }, + onDismiss = {}, + ) + } + } + + composeRule.onNodeWithText("Artist 20").performScrollTo().assertIsDisplayed() + + composeRule.mainClock.autoAdvance = false + composeRule.onNodeWithText("Artist 20").performClick() + composeRule.mainClock.advanceTimeBy(1_000) + + assertEquals(ArtistRef("ar20", "Artist 20"), opened) + } } diff --git a/app/src/test/kotlin/com/subtracks/ui/library/AlbumArtistLabelTest.kt b/app/src/test/kotlin/com/subtracks/ui/library/AlbumArtistLabelTest.kt new file mode 100644 index 00000000..4cbb9591 --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/ui/library/AlbumArtistLabelTest.kt @@ -0,0 +1,40 @@ +package com.subtracks.ui.library + +import com.subtracks.data.model.Album +import com.subtracks.data.model.ArtistRef +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class AlbumArtistLabelTest { + @Test + fun creditsBothTheSingularIdAndEveryPluralRef() { + val album = + album( + artistId = "a1", + artists = listOf(ArtistRef("a2", "B")), + ) + + assertTrue(isAlbumArtist(listOf(album), "a1")) + assertTrue(isAlbumArtist(listOf(album), "a2")) + assertFalse(isAlbumArtist(listOf(album), "a3")) + } + + private fun album( + artistId: String?, + artists: List, + ) = Album( + sourceId = 1, + id = "al1", + artistId = artistId, + name = "Album", + albumArtist = "Various Artists", + created = 0, + coverArt = null, + genre = null, + year = null, + starred = null, + songCount = 1, + artists = artists, + ) +}