diff --git a/CHANGELOG.md b/CHANGELOG.md index cc96f1176..e7f73b305 100644 --- 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 diff --git a/app/src/main/java/org/oxycblt/auxio/list/adapter/FlexibleListAdapter.kt b/app/src/main/java/org/oxycblt/auxio/list/adapter/FlexibleListAdapter.kt index 73d8de99d..8f6491845 100644 --- 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( newData: List, 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( fun update(newList: List, 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( } 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) diff --git a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt index 9e0af6490..c89726c87 100644 --- 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 } } diff --git a/app/src/main/java/org/oxycblt/auxio/playback/queue/Queue.kt b/app/src/main/java/org/oxycblt/auxio/playback/queue/Queue.kt index 9f7d462bd..3a32e444d 100644 --- 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 /** - * 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() @Volatile private var orderedMapping = mutableListOf() @@ -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): Queue.ChangeResult { - if (orderedMapping.isEmpty()) { - // No playback, start playing these songs. - start(songs[0], songs, false) - return Queue.ChangeResult.SONG - } - + fun playNext(songs: List): 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): Queue.ChangeResult { - if (orderedMapping.isEmpty()) { - // No playback, start playing these songs. - start(songs[0], songs, false) - return Queue.ChangeResult.SONG - } - + fun addToQueue(songs: List): 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)) } /** diff --git a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueAdapter.kt b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueAdapter.kt index ade55e065..fe317ca68 100644 --- 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) : position: Int, payload: List ) { + logD("$position ${getItem(position).rawName}") if (payload.isEmpty()) { viewHolder.bind(getItem(position), listener) } diff --git a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueDragCallback.kt b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueDragCallback.kt index 7a54c14e2..51b87f305 100644 --- 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) diff --git a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueFragment.kt b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueFragment.kt index 427c5244d..3244201a6 100644 --- 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(), 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) { diff --git a/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueViewModel.kt b/app/src/main/java/org/oxycblt/auxio/playback/queue/QueueViewModel.kt index 377381305..a950545a3 100644 --- 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 } diff --git a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt index 30362bb9c..c6e952eb4 100644 --- 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) { - 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) { - 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) } diff --git a/app/src/main/java/org/oxycblt/auxio/playback/system/MediaSessionComponent.kt b/app/src/main/java/org/oxycblt/auxio/playback/system/MediaSessionComponent.kt index d3a0fcb69..1ad377b39 100644 --- 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) } }