queue: add fine-grained updates

Add fine-grained updates to the queue view. Should do the following: 1. Make queue updates faster (no diff calculation) 2. Resolve some bugs regarding duplicate queue items. 3. Finally complete the new list framework. Resolves #350 Resolves #335 Resolves #380
2023-03-16 19:37:03 -06:00 · 2023-03-16 19:37:03 -06:00 · 59818471d6
commit 59818471d6
parent bde4035654
10 changed files with 88 additions and 113 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -10,6 +10,7 @@ deletion
 #### What's Improved
 - Accept `REPLAYGAIN_*` adjustment information on OPUS files alongside
 `R128_*` adjustments.
+- List updates are now consistent across the app

 ## 3.0.3

--- a/app/src/main/java/org/oxycblt/auxio/list/adapter/FlexibleListAdapter.kt
+++ b/app/src/main/java/org/oxycblt/auxio/list/adapter/FlexibleListAdapter.kt
@ -23,6 +23,7 @@ import androidx.recyclerview.widget.*
 import androidx.recyclerview.widget.DiffUtil
 import androidx.recyclerview.widget.RecyclerView
 import java.util.concurrent.Executor
+import org.oxycblt.auxio.util.logD

 /**
 * A variant of ListDiffer with more flexible updates.
@ -52,7 +53,10 @@ abstract class FlexibleListAdapter<T, VH : RecyclerView.ViewHolder>(
        newData: List<T>,
        instructions: UpdateInstructions?,
        callback: (() -> Unit)? = null
-    ) = differ.update(newData, instructions, callback)
+    ) =
+        differ.update(newData, instructions, callback).also {
+            logD("Update delivered: $instructions" + "")
+        }
 }

 /**
@ -72,6 +76,14 @@ sealed class UpdateInstructions {
     */
    data class Replace(val from: Int) : UpdateInstructions()

+    /**
+     * Add a new set of items.
+     *
+     * @param at The position at which to add.
+     * @param size The amount of items to add.
+     */
+    data class Add(val at: Int, val size: Int) : UpdateInstructions()
+
    /**
     * Move one item to another location.
     *
@ -116,10 +128,6 @@ private class FlexibleListDiffer<T>(
    fun update(newList: List<T>, instructions: UpdateInstructions?, callback: (() -> Unit)?) {
        // incrementing generation means any currently-running diffs are discarded when they finish
        val runGeneration = ++maxScheduledGeneration
-        if (currentList == newList) {
-            callback?.invoke()
-            return
-        }
        when (instructions) {
            is UpdateInstructions.Replace -> {
                updateCallback.onRemoved(instructions.from, currentList.size - instructions.from)
@ -130,6 +138,11 @@ private class FlexibleListDiffer<T>(
                }
                callback?.invoke()
            }
+            is UpdateInstructions.Add -> {
+                currentList = newList
+                updateCallback.onInserted(instructions.at, instructions.size)
+                callback?.invoke()
+            }
            is UpdateInstructions.Move -> {
                currentList = newList
                updateCallback.onMoved(instructions.from, instructions.to)
--- a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt
@ -114,9 +114,9 @@ constructor(
        _song.value = queue.currentSong
    }

-    override fun onQueueChanged(queue: Queue, change: Queue.ChangeResult) {
+    override fun onQueueChanged(queue: Queue, change: Queue.Change) {
        // Other types of queue changes preserve the current song.
-        if (change == Queue.ChangeResult.SONG) {
+        if (change.type == Queue.Change.Type.SONG) {
            _song.value = queue.currentSong
        }
    }
--- a/app/src/main/java/org/oxycblt/auxio/playback/queue/Queue.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/queue/Queue.kt
@ -19,6 +19,7 @@ package org.oxycblt.auxio.playback.queue

 import kotlin.random.Random
 import kotlin.random.nextInt
+import org.oxycblt.auxio.list.adapter.UpdateInstructions
 import org.oxycblt.auxio.music.Music
 import org.oxycblt.auxio.music.Song

@ -51,19 +52,25 @@ interface Queue {
    fun resolve(): List<Song>

    /**
-     * Represents the possible changes that can occur during certain queue mutation events. The
-     * precise meanings of these differ somewhat depending on the type of mutation done.
+     * Represents the possible changes that can occur during certain queue mutation events.
+     *
+     * @param type The [Type] of the change to the internal queue state.
+     * @param instructions The update done to the resolved queue list.
     */
-    enum class ChangeResult {
-        /** Only the mapping has changed. */
-        MAPPING,
-        /** The mapping has changed, and the index also changed to align with it. */
-        INDEX,
-        /**
-         * The current song has changed, possibly alongside the mapping and index depending on the
-         * context.
-         */
-        SONG
+    data class Change(val type: Type, val instructions: UpdateInstructions) {
+        enum class Type {
+            /** Only the mapping has changed. */
+            MAPPING,
+
+            /** The mapping has changed, and the index also changed to align with it. */
+            INDEX,
+
+            /**
+             * The current song has changed, possibly alongside the mapping and index depending on
+             * the context.
+             */
+            SONG
+        }
    }

    /**
@ -98,26 +105,6 @@ interface Queue {
    }
 }

-data class QueueChange(val internal: InternalChange, val externalChange: ExternalChange) {
-    enum class InternalChange {
-        /** Only the mapping has changed. */
-        MAPPING,
-        /** The mapping has changed, and the index also changed to align with it. */
-        INDEX,
-        /**
-         * The current song has changed, possibly alongside the mapping and index depending on the
-         * context.
-         */
-        SONG
-    }
-
-    sealed class ExternalChange {
-        data class Add(val at: Int, val amount: Int) : ExternalChange()
-        data class Remove(val at: Int) : ExternalChange()
-        data class Move(val from: Int, val to: Int) : ExternalChange()
-    }
-}
-
 class EditableQueue : Queue {
    @Volatile private var heap = mutableListOf<Song>()
    @Volatile private var orderedMapping = mutableListOf<Int>()
@ -213,17 +200,9 @@ class EditableQueue : Queue {
     * Add [Song]s to the top of the queue. Will start playback if nothing is playing.
     *
     * @param songs The [Song]s to add.
-     * @return [Queue.ChangeResult.MAPPING] if added to an existing queue, or
-     *   [Queue.ChangeResult.SONG] if there was no prior playback and these enqueued [Song]s start
-     *   new playback.
+     * @return A [Queue.Change] instance that reflects the changes made.
     */
-    fun playNext(songs: List<Song>): Queue.ChangeResult {
-        if (orderedMapping.isEmpty()) {
-            // No playback, start playing these songs.
-            start(songs[0], songs, false)
-            return Queue.ChangeResult.SONG
-        }
-
+    fun playNext(songs: List<Song>): Queue.Change {
        val heapIndices = songs.map(::addSongToHeap)
        if (shuffledMapping.isNotEmpty()) {
            // Add the new songs in front of the current index in the shuffled mapping and in front
@ -236,24 +215,17 @@ class EditableQueue : Queue {
            orderedMapping.addAll(index + 1, heapIndices)
        }
        check()
-        return Queue.ChangeResult.MAPPING
+        return Queue.Change(
+            Queue.Change.Type.MAPPING, UpdateInstructions.Add(index + 1, songs.size))
    }

    /**
     * Add [Song]s to the end of the queue. Will start playback if nothing is playing.
     *
     * @param songs The [Song]s to add.
-     * @return [Queue.ChangeResult.MAPPING] if added to an existing queue, or
-     *   [Queue.ChangeResult.SONG] if there was no prior playback and these enqueued [Song]s start
-     *   new playback.
+     * @return A [Queue.Change] instance that reflects the changes made.
     */
-    fun addToQueue(songs: List<Song>): Queue.ChangeResult {
-        if (orderedMapping.isEmpty()) {
-            // No playback, start playing these songs.
-            start(songs[0], songs, false)
-            return Queue.ChangeResult.SONG
-        }
-
+    fun addToQueue(songs: List<Song>): Queue.Change {
        val heapIndices = songs.map(::addSongToHeap)
        // Can simple append the new songs to the end of both mappings.
        orderedMapping.addAll(heapIndices)
@ -261,7 +233,8 @@ class EditableQueue : Queue {
            shuffledMapping.addAll(heapIndices)
        }
        check()
-        return Queue.ChangeResult.MAPPING
+        return Queue.Change(
+            Queue.Change.Type.MAPPING, UpdateInstructions.Add(index + 1, songs.size))
    }

    /**
@ -269,11 +242,9 @@ class EditableQueue : Queue {
     *
     * @param src The position of the [Song] to move.
     * @param dst The destination position of the [Song].
-     * @return [Queue.ChangeResult.MAPPING] if the move occurred after the current index,
-     *   [Queue.ChangeResult.INDEX] if the move occurred before or at the current index, requiring
-     *   it to be mutated.
+     * @return A [Queue.Change] instance that reflects the changes made.
     */
-    fun move(src: Int, dst: Int): Queue.ChangeResult {
+    fun move(src: Int, dst: Int): Queue.Change {
        if (shuffledMapping.isNotEmpty()) {
            // Move songs only in the shuffled mapping. There is no sane analogous form of
            // this for the ordered mapping.
@ -293,22 +264,20 @@ class EditableQueue : Queue {
            else -> {
                // Nothing to do.
                check()
-                return Queue.ChangeResult.MAPPING
+                return Queue.Change(Queue.Change.Type.MAPPING, UpdateInstructions.Move(src, dst))
            }
        }
        check()
-        return Queue.ChangeResult.INDEX
+        return Queue.Change(Queue.Change.Type.INDEX, UpdateInstructions.Move(src, dst))
    }

    /**
     * Remove a [Song] at the given position.
     *
     * @param at The position of the [Song] to remove.
-     * @return [Queue.ChangeResult.MAPPING] if the removed [Song] was after the current index,
-     *   [Queue.ChangeResult.INDEX] if the removed [Song] was before the current index, and
-     *   [Queue.ChangeResult.SONG] if the currently playing [Song] was removed.
+     * @return A [Queue.Change] instance that reflects the changes made.
     */
-    fun remove(at: Int): Queue.ChangeResult {
+    fun remove(at: Int): Queue.Change {
        if (shuffledMapping.isNotEmpty()) {
            // Remove the specified index in the shuffled mapping and the analogous song in the
            // ordered mapping.
@ -323,20 +292,20 @@ class EditableQueue : Queue {
        // of the player to be completely invalidated. It's generally easier to not remove the
        // song and retain player state consistency.

-        val result =
+        val type =
            when {
                // We just removed the currently playing song.
-                index == at -> Queue.ChangeResult.SONG
+                index == at -> Queue.Change.Type.SONG
                // Index was ahead of removed song, shift back to preserve consistency.
                index > at -> {
                    index -= 1
-                    Queue.ChangeResult.INDEX
+                    Queue.Change.Type.INDEX
                }
                // Nothing to do
-                else -> Queue.ChangeResult.MAPPING
+                else -> Queue.Change.Type.MAPPING
            }
        check()
-        return result
+        return Queue.Change(type, UpdateInstructions.Remove(at))
    }

    /**
--- a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueAdapter.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueAdapter.kt
@ -58,6 +58,7 @@ class QueueAdapter(private val listener: EditableListListener<Song>) :
        position: Int,
        payload: List<Any>
    ) {
+        logD("$position ${getItem(position).rawName}")
        if (payload.isEmpty()) {
            viewHolder.bind(getItem(position), listener)
        }
--- a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueDragCallback.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueDragCallback.kt
@ -127,9 +127,11 @@ class QueueDragCallback(private val playbackModel: QueueViewModel) : ItemTouchHe
        recyclerView: RecyclerView,
        viewHolder: RecyclerView.ViewHolder,
        target: RecyclerView.ViewHolder
-    ) =
-        playbackModel.moveQueueDataItems(
+    ): Boolean {
+        logD("${viewHolder.bindingAdapterPosition} ${target.bindingAdapterPosition}")
+        return playbackModel.moveQueueDataItems(
            viewHolder.bindingAdapterPosition, target.bindingAdapterPosition)
+    }

    override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) {
        playbackModel.removeQueueDataItem(viewHolder.bindingAdapterPosition)
--- a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueFragment.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueFragment.kt
@ -80,6 +80,9 @@ class QueueFragment : ViewBindingFragment<FragmentQueueBinding>(), EditableListL
        super.onDestroyBinding(binding)
        touchHelper = null
        binding.queueRecycler.adapter = null
+        // Avoid possible race conditions that could cause a bad instruction to be consumed
+        // during list initialization and crash the app. Could happen if the user is fast enough.
+        queueModel.queueInstructions.consume()
    }

    override fun onClick(item: Song, viewHolder: RecyclerView.ViewHolder) {
--- a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueViewModel.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueViewModel.kt
@ -63,12 +63,11 @@ class QueueViewModel @Inject constructor(private val playbackManager: PlaybackSt
        _index.value = queue.index
    }

-    override fun onQueueChanged(queue: Queue, change: Queue.ChangeResult) {
+    override fun onQueueChanged(queue: Queue, change: Queue.Change) {
        // Queue changed trivially due to item mo -> Diff queue, stay at current index.
-        // TODO: Terrible idea, need to manually deliver updates
-        _queueInstructions.put(UpdateInstructions.Diff)
+        _queueInstructions.put(change.instructions)
        _queue.value = queue.resolve()
-        if (change != Queue.ChangeResult.MAPPING) {
+        if (change.type != Queue.Change.Type.MAPPING) {
            // Index changed, make sure it remains updated without actually scrolling to it.
            _index.value = queue.index
        }
--- a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt
@ -244,12 +244,12 @@ interface PlaybackStateManager {
        fun onIndexMoved(queue: Queue) {}

        /**
-         * Called when the [Queue] changed in a manner outlined by the given [Queue.ChangeResult].
+         * Called when the [Queue] changed in a manner outlined by the given [Queue.Change].
         *
         * @param queue The new [Queue].
-         * @param change The type of [Queue.ChangeResult] that occurred.
+         * @param change The type of [Queue.Change] that occurred.
         */
-        fun onQueueChanged(queue: Queue, change: Queue.ChangeResult) {}
+        fun onQueueChanged(queue: Queue, change: Queue.Change) {}

        /**
         * Called when the [Queue] has changed in a non-trivial manner (such as re-shuffling), but
@ -423,31 +423,19 @@ class PlaybackStateManagerImpl @Inject constructor() : PlaybackStateManager {

    @Synchronized
    override fun playNext(songs: List<Song>) {
-        val internalPlayer = internalPlayer ?: return
-        when (queue.playNext(songs)) {
-            Queue.ChangeResult.MAPPING -> notifyQueueChanged(Queue.ChangeResult.MAPPING)
-            Queue.ChangeResult.SONG -> {
-                // Enqueueing actually started a new playback session from all songs.
-                parent = null
-                internalPlayer.loadSong(queue.currentSong, true)
-                notifyNewPlayback()
-            }
-            Queue.ChangeResult.INDEX -> error("Unreachable")
+        if (queue.currentSong == null) {
+            play(songs[0], null, songs, false)
+        } else {
+            notifyQueueChanged(queue.playNext(songs))
        }
    }

    @Synchronized
    override fun addToQueue(songs: List<Song>) {
-        val internalPlayer = internalPlayer ?: return
-        when (queue.addToQueue(songs)) {
-            Queue.ChangeResult.MAPPING -> notifyQueueChanged(Queue.ChangeResult.MAPPING)
-            Queue.ChangeResult.SONG -> {
-                // Enqueueing actually started a new playback session from all songs.
-                parent = null
-                internalPlayer.loadSong(queue.currentSong, true)
-                notifyNewPlayback()
-            }
-            Queue.ChangeResult.INDEX -> error("Unreachable")
+        if (queue.currentSong == null) {
+            play(songs[0], null, songs, false)
+        } else {
+            notifyQueueChanged(queue.addToQueue(songs))
        }
    }

@ -462,7 +450,7 @@ class PlaybackStateManagerImpl @Inject constructor() : PlaybackStateManager {
        val internalPlayer = internalPlayer ?: return
        logD("Removing item at $at")
        val change = queue.remove(at)
-        if (change == Queue.ChangeResult.SONG) {
+        if (change.type == Queue.Change.Type.SONG) {
            internalPlayer.loadSong(queue.currentSong, playerState.isPlaying)
        }
        notifyQueueChanged(change)
@ -568,7 +556,7 @@ class PlaybackStateManagerImpl @Inject constructor() : PlaybackStateManager {
        }
    }

-    private fun notifyQueueChanged(change: Queue.ChangeResult) {
+    private fun notifyQueueChanged(change: Queue.Change) {
        for (callback in listeners) {
            callback.onQueueChanged(queue, change)
        }
--- a/app/src/main/java/org/oxycblt/auxio/playback/system/MediaSessionComponent.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/system/MediaSessionComponent.kt
@ -118,16 +118,15 @@ constructor(
        invalidateSessionState()
    }

-    override fun onQueueChanged(queue: Queue, change: Queue.ChangeResult) {
+    override fun onQueueChanged(queue: Queue, change: Queue.Change) {
        updateQueue(queue)
-        when (change) {
+        when (change.type) {
            // Nothing special to do with mapping changes.
-            Queue.ChangeResult.MAPPING -> {}
+            Queue.Change.Type.MAPPING -> {}
            // Index changed, ensure playback state's index changes.
-            Queue.ChangeResult.INDEX -> invalidateSessionState()
+            Queue.Change.Type.INDEX -> invalidateSessionState()
            // Song changed, ensure metadata changes.
-            Queue.ChangeResult.SONG ->
-                updateMediaMetadata(queue.currentSong, playbackManager.parent)
+            Queue.Change.Type.SONG -> updateMediaMetadata(queue.currentSong, playbackManager.parent)
        }
    }