library: cover every credited artist in plays, downloads and menus

Credit all album artists in play aggregation and recorded plays, include the relation rows in the downloaded artist list, and fall back to the album artist display name in download groups. Also keep the chooser scrollable, close it when the track changes, keep the singular id in the album-artist label, stop a nested artists container from swallowing later artist elements, and make the refs encoding length-prefixed so tag values cannot corrupt it.
2026-11-11 07:29:14 +01:00 · 2026-10-18 02:15:13 +00:00
parent daf305f680
commit 9971128078
12 changed files with 241 additions and 55 deletions
@@ -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 " +
@@ -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",
    )
@@ -555,21 +555,33 @@ class ArtistRefsConverter {
 }

 object ArtistRefsCodec {
-    fun encode(refs: List<ArtistRef>): String = refs.joinToString(RECORD) { "${it.id}$FIELD${it.name}" }
+    fun encode(refs: List<ArtistRef>): 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<ArtistRef> =
-        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<ArtistRef> {
+        val refs = mutableListOf<ArtistRef>()
+        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
+    }
 }
@@ -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,
@@ -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))
    }
@@ -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<Album>,
+    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,
@@ -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<Song?>(null, sourceId, songId) {
            if (sourceId != null && songId != null) {
@@ -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<ArtistRef>(), ArtistRefsCodec.decode(""))
+        assertEquals(emptyList<ArtistRef>(), ArtistRefsCodec.decode("not a ref"))
+        assertEquals(emptyList<ArtistRef>(), ArtistRefsCodec.decode("9:short4:name"))
+    }
+}
@@ -191,6 +191,27 @@ class SubsonicXmlTest {
        )
    }

+    @Test
+    fun ignoresNestedArtistsOutsideTheArtistsContainer() {
+        val songs =
+            songs(
+                """
+                <subsonic-response status="ok">
+                  <song id="s1" title="Collab" albumId="al1">
+                    <artists>
+                      <artist id="ar1" name="A"/>
+                    </artists>
+                    <contributors>
+                      <artist id="ar9" role="composer"/>
+                    </contributors>
+                  </song>
+                </subsonic-response>
+                """.trimIndent(),
+            )
+
+        assertEquals(listOf(ArtistRef("ar1", "A")), songs.single().artists)
+    }
+
    @Test
    fun mapsNestedSongArtistRefs() {
        val songs =
@@ -205,6 +226,9 @@ class SubsonicXmlTest {
                    <albumArtists>
                      <artist id="ar3" name="Album Artist"/>
                    </albumArtists>
+                    <contributors>
+                      <artist id="ar4" role="composer"/>
+                    </contributors>
                  </song>
                </subsonic-response>
                """.trimIndent(),
@@ -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 =
@@ -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)
+    }
 }
@@ -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<ArtistRef>,
+    ) = 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,
+    )
+}