address review round 3: keep the search bar across recomposition, fix tab landing, page the queried songs queue

2026-11-10 15:11:58 +01:00 · 2026-10-04 00:29:56 +00:00
parent 605bd1a6c6
commit f5948812bb
4 changed files with 245 additions and 30 deletions
@@ -35,6 +35,13 @@ internal const val SONGS_IDS_SELECT =
        "AND (:starredFilter = 0 OR (:starredFilter = 1 AND songs.starred IS NOT NULL) OR " +
        "(:starredFilter = 2 AND songs.starred IS NULL)) "

+internal const val SONGS_LIST_SELECT =
+    "SELECT songs.*, albums.coverArt AS coverArt FROM songs " +
+        "LEFT JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " +
+        "WHERE songs.sourceId = :sourceId " +
+        "AND (:starredFilter = 0 OR (:starredFilter = 1 AND songs.starred IS NOT NULL) OR " +
+        "(:starredFilter = 2 AND songs.starred IS NULL)) "
+
@Dao
@DaoReturnTypeConverters(PagingSourceDaoReturnTypeConverter::class)
 interface LibraryDao {
@@ -357,7 +364,7 @@ interface LibraryDao {
            "AND (:search = '' OR instr(lower(songs.title), lower(:search)) > 0 " +
            "OR instr(lower(songs.artist), lower(:search)) > 0 OR instr(lower(songs.album), lower(:search)) > 0) " +
            "ORDER BY albums.albumArtist COLLATE NOCASE, songs.album COLLATE NOCASE, songs.disc, songs.track, " +
-            "songs.title COLLATE NOCASE, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id",
+            "songs.title COLLATE NOCASE, songs.id",
    )
    fun songs(
        sourceId: Long,
@@ -622,4 +629,104 @@ interface LibraryDao {
        sourceId: Long,
        starredFilter: Int,
    ): List<String>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY albums.albumArtist COLLATE NOCASE, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.title COLLATE NOCASE, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByAlbum(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY albums.albumArtist COLLATE NOCASE DESC, songs.album COLLATE NOCASE DESC, songs.disc DESC, songs.track DESC, songs.title COLLATE NOCASE DESC, songs.id DESC LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByAlbumReversed(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.title COLLATE NOCASE, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByTitle(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.title COLLATE NOCASE DESC, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByTitleReversed(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.artist COLLATE NOCASE, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByArtist(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.artist COLLATE NOCASE DESC, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByArtistReversed(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.starred IS NULL, songs.starred DESC, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByStarred(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.starred IS NULL, songs.starred ASC, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByStarredReversed(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.created DESC, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByAdded(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
+
+    @Query(
+        "${SONGS_LIST_SELECT}ORDER BY songs.created ASC, songs.album COLLATE NOCASE, songs.disc, songs.track, songs.id LIMIT :limit OFFSET :offset",
+    )
+    suspend fun songsPageByAddedReversed(
+        sourceId: Long,
+        starredFilter: Int,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem>
 }
@@ -42,7 +42,17 @@ private val Context.preferences: DataStore<Preferences> by preferencesDataStore(
 class UserPreferences(
    private val store: DataStore<Preferences>,
 ) {
-    fun listQuery(tab: LibraryListTab): Flow<ListQuery> = store.data.map { prefs -> decode(prefs[listQueryKey(tab)], tab.defaultSort) }
+    fun listQuery(tab: LibraryListTab): Flow<ListQuery> =
+        store.data.map { prefs -> decode(prefs[listQueryKey(tab)] ?: legacyQuery(prefs, tab), tab.defaultSort) }
+
+    private fun legacyQuery(
+        prefs: Preferences,
+        tab: LibraryListTab,
+    ): String? {
+        if (tab != LibraryListTab.Albums) return null
+        val sort = prefs[ALBUM_SORT]?.let { if (it == "RecentlyAdded") "Added" else it } ?: return null
+        return "$sort|0|0"
+    }

    suspend fun setListQuery(
        tab: LibraryListTab,
@@ -83,6 +93,7 @@ class UserPreferences(
    }

    private companion object {
+        val ALBUM_SORT = stringPreferencesKey("album_sort")
        val MAX_BITRATE = intPreferencesKey("max_bitrate")
        val STREAM_FORMAT = stringPreferencesKey("stream_format")
    }
@@ -4,6 +4,7 @@ import androidx.room3.deferredTransaction
 import androidx.room3.immediateTransaction
 import androidx.room3.useReaderConnection
 import androidx.room3.useWriterConnection
+import com.subtracks.data.db.LibraryDao
 import com.subtracks.data.db.SubtracksDatabase
 import com.subtracks.data.model.PlaybackCursor
 import com.subtracks.data.model.QueueEntry
@@ -173,6 +174,56 @@ class QueueRepository(
        }
    }

+    private suspend fun LibraryDao.songsPage(
+        sourceId: Long,
+        query: QueueSongQuery,
+        limit: Int,
+        offset: Long,
+    ): List<SongListItem> {
+        val starred = query.starred.ordinal
+        return when (query.sort) {
+            SongSort.Album -> {
+                if (query.descending) {
+                    songsPageByAlbumReversed(sourceId, starred, limit, offset)
+                } else {
+                    songsPageByAlbum(sourceId, starred, limit, offset)
+                }
+            }
+
+            SongSort.Title -> {
+                if (query.descending) {
+                    songsPageByTitleReversed(sourceId, starred, limit, offset)
+                } else {
+                    songsPageByTitle(sourceId, starred, limit, offset)
+                }
+            }
+
+            SongSort.Artist -> {
+                if (query.descending) {
+                    songsPageByArtistReversed(sourceId, starred, limit, offset)
+                } else {
+                    songsPageByArtist(sourceId, starred, limit, offset)
+                }
+            }
+
+            SongSort.Starred -> {
+                if (query.descending) {
+                    songsPageByStarredReversed(sourceId, starred, limit, offset)
+                } else {
+                    songsPageByStarred(sourceId, starred, limit, offset)
+                }
+            }
+
+            SongSort.Added -> {
+                if (query.descending) {
+                    songsPageByAddedReversed(sourceId, starred, limit, offset)
+                } else {
+                    songsPageByAdded(sourceId, starred, limit, offset)
+                }
+            }
+        }
+    }
+
    suspend fun replace(
        entries: List<QueueEntry>,
        shuffleOrder: LongArray? = null,
@@ -247,23 +298,29 @@ class QueueRepository(
        snapshot: QueueSnapshot,
        position: Long,
    ): SongListItem? {
-        val flat = snapshot.flatPosition(position) ?: return null
-        val id = flatIds(snapshot).getOrNull(flat.toInt()) ?: return null
-        val sourceId =
-            snapshot.entries
-                .firstOrNull()
-                ?.entry
-                ?.sourceId ?: return null
-        return dao.songsByIds(sourceId, listOf(id)).firstOrNull()
+        if (snapshot.shuffled) {
+            val sourceId =
+                snapshot.entries
+                    .firstOrNull()
+                    ?.entry
+                    ?.sourceId ?: return null
+            val flat = snapshot.flatPosition(position) ?: return null
+            val id = flatIds(snapshot).getOrNull(flat.toInt()) ?: return null
+            return dao.songsByIds(sourceId, listOf(id)).firstOrNull()
+        }
+        val (entry, offset) = snapshot.locate(position) ?: return null
+        return rows(entry, entry.offset + offset, 1).firstOrNull()
    }

    suspend fun flatIndexOf(
        snapshot: QueueSnapshot,
        songId: String,
    ): Long? {
-        if (!snapshot.shuffled) return null
-        val index = flatIds(snapshot).indexOf(songId)
-        return if (index >= 0) index.toLong() else null
+        val order = snapshot.shuffleOrder ?: return null
+        val flat = flatIds(snapshot).indexOf(songId)
+        if (flat < 0) return null
+        val position = order.indexOf(flat.toLong())
+        return if (position >= 0) position.toLong() else null
    }

    suspend fun range(
@@ -271,22 +328,37 @@ class QueueRepository(
        first: Long,
        last: Long,
    ): List<QueueWindowItem> {
-        if (snapshot.size == 0L) return emptyList()
-        val ids = flatIds(snapshot)
-        val requested = mutableListOf<Pair<Long, String>>()
-        for (position in first..last) {
-            val flat = snapshot.flatPosition(position) ?: continue
-            val id = ids.getOrNull(flat.toInt()) ?: continue
-            requested += position to id
+        if (snapshot.shuffled) {
+            val sourceId =
+                snapshot.entries
+                    .firstOrNull()
+                    ?.entry
+                    ?.sourceId ?: return emptyList()
+            val ids = flatIds(snapshot)
+            val requested = mutableListOf<Pair<Long, String>>()
+            for (position in first..last) {
+                val flat = snapshot.flatPosition(position) ?: continue
+                val id = ids.getOrNull(flat.toInt()) ?: continue
+                requested += position to id
+            }
+            if (requested.isEmpty()) return emptyList()
+            val byId = dao.songsByIds(sourceId, requested.map { it.second }.distinct()).associateBy { it.song.id }
+            return requested.mapNotNull { (position, id) -> byId[id]?.let { QueueWindowItem(position, it) } }
        }
-        if (requested.isEmpty()) return emptyList()
-        val sourceId =
-            snapshot.entries
-                .firstOrNull()
-                ?.entry
-                ?.sourceId ?: return emptyList()
-        val byId = dao.songsByIds(sourceId, requested.map { it.second }.distinct()).associateBy { it.song.id }
-        return requested.mapNotNull { (position, id) -> byId[id]?.let { QueueWindowItem(position, it) } }
+        val items = mutableListOf<QueueWindowItem>()
+        var start = 0L
+        for (resolved in snapshot.entries) {
+            val end = start + resolved.length - 1
+            val entryStart = start
+            start += resolved.length
+            if (resolved.length == 0L || end < first || entryStart > last) continue
+            val from = maxOf(first, entryStart)
+            val to = minOf(last, end)
+            val offset = resolved.entry.offset + (from - entryStart)
+            rows(resolved.entry, offset, (to - from + 1).toInt())
+                .forEachIndexed { index, item -> items += QueueWindowItem(from + index, item) }
+        }
+        return items
    }

    suspend fun window(
@@ -386,6 +458,31 @@ class QueueRepository(
        return ids
    }

+    private suspend fun rows(
+        entry: QueueEntry,
+        offset: Long,
+        limit: Int,
+    ): List<SongListItem> =
+        when (entry.kind) {
+            QueueKind.Playlist -> {
+                dao.playlistSongs(entry.sourceId, entry.refId, offset, limit)
+            }
+
+            QueueKind.Album -> {
+                dao.albumSongs(entry.sourceId, entry.refId, offset, limit)
+            }
+
+            QueueKind.Songs -> {
+                songsQuery(entry.refId)?.let { query ->
+                    db.libraryDao().songsPage(entry.sourceId, query, limit, offset)
+                } ?: dao.songs(entry.sourceId, offset, limit)
+            }
+
+            QueueKind.Song -> {
+                dao.song(entry.sourceId, entry.refId, offset, limit)
+            }
+        }
+
    private suspend fun QueueEntry.resolvedLength(): Long {
        val total =
            when (kind) {
@@ -269,7 +269,7 @@ fun LibraryScreen(
    val searchFocus = remember { FocusRequester() }

    LaunchedEffect(pagerState) {
-        snapshotFlow { pagerState.currentPage }.collect { page ->
+        snapshotFlow { pagerState.settledPage }.collect { page ->
            onTabSelected(LibraryTab.entries[page])
        }
    }
@@ -278,7 +278,7 @@ fun LibraryScreen(
        if (pagerState.currentPage != selectedTab.ordinal) {
            pagerState.animateScrollToPage(selectedTab.ordinal)
        }
-        searchActive = false
+        searchActive = search.isNotEmpty()
    }

    val density = LocalDensity.current