From 0c2e85134bb247cfa608ead79fd19a5788fa5baa Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 18 Oct 2026 05:26:59 +0000 Subject: [PATCH] library: fix sort-name resolution and migration rebuild Collect every credited artist id when resolving album and song sort keys so the singular id and plural refs can both be looked up, and add regression tests for the compilation and stale-sort-name cases. Drop the FTS tables before the backfill so the old triggers do not index rows that are about to be discarded, and rely on createSearchIndex's rebuild instead of rebuilding a second time. Treat a whitespace-only artist sortName as absent, and use the sort keys in the downloads tree too. --- .../com/subtracks/data/db/DownloadDao.kt | 5 +- .../com/subtracks/data/db/Migrations.kt | 16 +---- .../com/subtracks/data/model/Download.kt | 3 + .../data/source/subsonic/SubsonicXml.kt | 2 +- .../com/subtracks/data/sync/SyncService.kt | 10 ++- .../ui/downloads/DownloadsViewModel.kt | 10 ++- .../com/subtracks/data/db/LibraryDaoTest.kt | 30 +++++--- .../subtracks/data/sync/SyncServiceTest.kt | 71 +++++++++++++++++++ 8 files changed, 118 insertions(+), 29 deletions(-) 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 ebc30c9a..7b475ce4 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt @@ -166,7 +166,10 @@ 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, COALESCE(ar.name, al.albumArtist) AS artistName, d.status AS status, " + - "d.bytes AS bytes, d.total AS total " + + "d.bytes AS bytes, d.total AS total, " + + "COALESCE(NULLIF(ar.sortName, ''), NULLIF(al.albumArtistSort, ''), ar.name, al.albumArtist, '') AS artistSort, " + + "COALESCE(NULLIF(al.sortName, ''), al.name, '') AS albumSort, " + + "COALESCE(NULLIF(s.sortName, ''), s.title, '') AS songSort " + "FROM song_downloads d " + "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 " + diff --git a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt index b580593a..7e062476 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt @@ -461,6 +461,9 @@ val MIGRATION_30_31 = connection.execSQL("ALTER TABLE `albums` ADD COLUMN `albumArtistSort` TEXT NOT NULL DEFAULT '' COLLATE NOCASE") connection.execSQL("ALTER TABLE `songs` ADD COLUMN `sortName` TEXT NOT NULL DEFAULT '' COLLATE NOCASE") connection.execSQL("ALTER TABLE `songs` ADD COLUMN `artistSort` TEXT NOT NULL DEFAULT '' COLLATE NOCASE") + dropSearchIndex(connection, "artist_search") + dropSearchIndex(connection, "album_search") + dropSearchIndex(connection, "song_search") connection.execSQL("UPDATE `artists` SET `sortName` = `name`") connection.execSQL("UPDATE `albums` SET `sortName` = `name`") connection.execSQL("UPDATE `songs` SET `sortName` = `title`") @@ -488,15 +491,9 @@ val MIGRATION_30_31 = ALBUM_INDEXES.forEach { connection.execSQL("DROP INDEX IF EXISTS `$it`") } ARTIST_INDEX_SQL.forEach { connection.execSQL(it) } ALBUM_INDEX_SQL.forEach { connection.execSQL(it) } - dropSearchIndex(connection, "artist_search") - dropSearchIndex(connection, "album_search") - dropSearchIndex(connection, "song_search") createSearchIndex(connection, "artist_search", "artists", listOf("name", "sortName")) createSearchIndex(connection, "album_search", "albums", listOf("name", "albumArtist", "sortName", "albumArtistSort")) createSearchIndex(connection, "song_search", "songs", listOf("title", "artist", "sortName", "artistSort")) - rebuildSearchIndex(connection, "artist_search") - rebuildSearchIndex(connection, "album_search") - rebuildSearchIndex(connection, "song_search") } } @@ -554,13 +551,6 @@ private suspend fun dropSearchIndex( connection.execSQL("DROP TABLE IF EXISTS `$table`") } -private suspend fun rebuildSearchIndex( - connection: SQLiteConnection, - table: String, -) { - connection.execSQL("INSERT INTO `$table`(`$table`) VALUES('rebuild')") -} - // Mirrors the FTS5 table and content-sync triggers Room generates for the @Fts5 entities. private suspend fun createSearchIndex( connection: SQLiteConnection, 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 52d44b26..d4412882 100644 --- a/app/src/main/kotlin/com/subtracks/data/model/Download.kt +++ b/app/src/main/kotlin/com/subtracks/data/model/Download.kt @@ -94,6 +94,9 @@ data class DownloadedSong( val size: Long = 0, val bytes: Long = 0, val total: Long = 0, + val artistSort: String = artistName.orEmpty(), + val albumSort: String = albumName.orEmpty(), + val songSort: String = title, ) { val progress: Float? get() = total.takeIf { it > 0 }?.let { (bytes.toFloat() / it).coerceIn(0f, 1f) } 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 45640f65..6d24454d 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 @@ -32,7 +32,7 @@ object SubsonicXml { albumCount = attrs.longAttr("albumCount") ?: 0L, starred = attrs.dateAttr("starred"), coverArt = attrs.attr("coverArt").ifEmpty { null }, - sortName = attrs.attr("sortName").ifEmpty { attrs.attr("name") }, + sortName = attrs.attr("sortName").takeIf { it.isNotBlank() } ?: attrs.attr("name"), ) internal fun albumDraft( diff --git a/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt b/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt index 78041b47..8a47b817 100644 --- a/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt +++ b/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt @@ -190,7 +190,10 @@ class SyncService( val sortNames = artistSortNames( batch.first().sourceId, - batch.mapNotNull { it.artistRefs.firstOrNull()?.id }.toSet(), + batch + .flatMap { listOfNotNull(it.artistId) + it.artistRefs.map { ref -> ref.id } } + .filter { it.isNotBlank() } + .toSet(), ) return batch.map { song -> val primary = song.artistId ?: song.artists.firstOrNull()?.id @@ -207,7 +210,10 @@ class SyncService( val sortNames = artistSortNames( batch.first().sourceId, - batch.mapNotNull { it.artistId ?: it.artists.firstOrNull()?.id }.toSet(), + batch + .flatMap { listOfNotNull(it.artistId) + it.artistRefs.map { ref -> ref.id } } + .filter { it.isNotBlank() } + .toSet(), ) return batch.map { album -> val primary = album.artistId ?: album.artists.firstOrNull()?.id diff --git a/app/src/main/kotlin/com/subtracks/ui/downloads/DownloadsViewModel.kt b/app/src/main/kotlin/com/subtracks/ui/downloads/DownloadsViewModel.kt index 1b368271..f823f266 100644 --- a/app/src/main/kotlin/com/subtracks/ui/downloads/DownloadsViewModel.kt +++ b/app/src/main/kotlin/com/subtracks/ui/downloads/DownloadsViewModel.kt @@ -31,6 +31,7 @@ data class DownloadArtistNode( val name: String, val bytes: Long, val albums: List, + val sortName: String = name, ) data class DownloadAlbumNode( @@ -38,6 +39,7 @@ data class DownloadAlbumNode( val name: String, val bytes: Long, val songs: List, + val sortName: String = name, ) @OptIn(ExperimentalCoroutinesApi::class) @@ -105,6 +107,7 @@ internal fun buildTree( id = artistKey.first, name = artistKey.second.ifBlank { unknownArtist }, bytes = artistSongs.sumOf { it.size }, + sortName = artistSongs.first().artistSort.ifBlank { artistKey.second }, albums = artistSongs .groupBy { it.albumId.orEmpty() to it.albumName.orEmpty() } @@ -113,9 +116,10 @@ internal fun buildTree( id = albumKey.first, name = albumKey.second.ifBlank { unknownAlbum }, bytes = albumSongs.sumOf { it.size }, - songs = albumSongs.sortedBy { it.title.lowercase() }, + sortName = albumSongs.first().albumSort.ifBlank { albumKey.second }, + songs = albumSongs.sortedBy { it.songSort.lowercase() }, ) - }.sortedBy { it.name.lowercase() }, + }.sortedBy { it.sortName.lowercase() }, ) - }.sortedBy { it.name.lowercase() }, + }.sortedBy { it.sortName.lowercase() }, ) 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 8270bfe2..3522d910 100644 --- a/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/db/LibraryDaoTest.kt @@ -985,31 +985,43 @@ class LibraryDaoTest { val dao = db.libraryDao() dao.upsertArtists( listOf( - artist(sourceId, "ar-1", "宇多田ヒカル", albumCount = 1).copy(sortName = "Utada Hikaru"), - artist(sourceId, "ar-2", "あいみょん", albumCount = 1).copy(sortName = "Aimyon"), + artist(sourceId, "ar-1", "あいみょん", albumCount = 1).copy(sortName = "Zzz"), + artist(sourceId, "ar-2", "宇多田ヒカル", albumCount = 1).copy(sortName = "Aaa"), ), ) dao.upsertAlbums( listOf( - album(sourceId, "al-1", "First Love", year = null, starred = null) - .copy(sortName = "First Love", albumArtistSort = "Utada Hikaru"), - album(sourceId, "al-2", "Zutto", year = null, starred = null) - .copy(sortName = "Zutto", albumArtistSort = "Aimyon"), + album(sourceId, "al-1", "Zzz", year = null, starred = null) + .copy(sortName = "Aaa", albumArtistSort = "Zzz"), + album(sourceId, "al-2", "Aaa", year = null, starred = null) + .copy(sortName = "Zzz", albumArtistSort = "Aaa"), ), ) assertEquals( - listOf("あいみょん", "宇多田ヒカル"), + listOf("宇多田ヒカル", "あいみょん"), dao.artistsByName(sourceId, 0, "").page().map { it.name }, ) assertEquals( - listOf("First Love", "Zutto"), + listOf("あいみょん", "宇多田ヒカル"), + dao.artistsByNameReversed(sourceId, 0, "").page().map { it.name }, + ) + assertEquals( + listOf("Zzz", "Aaa"), dao.albumsByName(sourceId, 0, "").page().map { it.name }, ) assertEquals( - listOf("Zutto", "First Love"), + listOf("Aaa", "Zzz"), + dao.albumsByNameReversed(sourceId, 0, "").page().map { it.name }, + ) + assertEquals( + listOf("Aaa", "Zzz"), dao.albumsByArtist(sourceId, 0, "").page().map { it.name }, ) + assertEquals( + listOf("Zzz", "Aaa"), + dao.albumsByArtistReversed(sourceId, 0, "").page().map { it.name }, + ) } @Test 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 63ac82ed..cbe28871 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncServiceTest.kt @@ -993,6 +993,77 @@ class SyncServiceTest { assertEquals("Utada Hikaru", db.libraryDao().songOnce(1, "s1")!!.artistSort) } + @Test + fun songArtistSortUsesTheSingularArtistOverThePluralList() = + runTest { + insertSource() + val source = + FakeMusicSource( + artists = + listOf( + artist("a1", "A").copy(sortName = "Alpha"), + artist("a2", "B").copy(sortName = "Beta"), + ), + albums = listOf(album("al1")), + songs = + listOf( + song("s1").copy( + artistId = "a2", + artists = listOf(ArtistRef("a1", "A"), ArtistRef("a2", "B")), + ), + ), + ) + + SyncService(db, source).sync() + + assertEquals("Beta", db.libraryDao().songOnce(1, "s1")!!.artistSort) + } + + @Test + fun albumArtistSortFallsBackToAPluralRefWhenTheSingularArtistIsMissing() = + runTest { + insertSource() + val source = + FakeMusicSource( + artists = listOf(artist("a1", "A").copy(sortName = "Alpha")), + albums = + listOf( + album("al1").copy( + artistId = "missing", + albumArtist = "A", + artists = listOf(ArtistRef("a1", "A")), + ), + ), + ) + + SyncService(db, source).sync() + + assertEquals("Alpha", db.libraryDao().albumOnce(1, "al1")!!.albumArtistSort) + } + + @Test + fun changingAnArtistSortNameUpdatesAlbumAndSongSortKeys() = + runTest { + insertSource() + val source = + FakeMusicSource( + artists = listOf(artist("a1", "A").copy(sortName = "Alpha")), + albums = listOf(album("al1")), + songs = listOf(song("s1")), + ) + + SyncService(db, source).sync() + assertEquals("Alpha", db.libraryDao().albumOnce(1, "al1")!!.albumArtistSort) + assertEquals("Alpha", db.libraryDao().songOnce(1, "s1")!!.artistSort) + + source.artists = listOf(artist("a1", "A").copy(sortName = "Zulu")) + + SyncService(db, source).sync() + + assertEquals("Zulu", db.libraryDao().albumOnce(1, "al1")!!.albumArtistSort) + assertEquals("Zulu", db.libraryDao().songOnce(1, "s1")!!.artistSort) + } + @Test fun everyAlbumArtistAndTrackArtistReachesTheSharedAlbum() = runTest {