From cc2561f20fcee692d45d342aa95f70ec26afeee1 Mon Sep 17 00:00:00 2001 From: OxygenCobalt Date: Tue, 7 Jun 2022 20:00:10 -0600 Subject: [PATCH] ui: rework primitive backing data into sync Rework PrimitiveBackingData into SyncBackingData. Google says that you should not use DiffUtil on the main thread, as it is far too slow. In practice, the time it takes to diff is negligable, even more so when costly diffs are filled in with a replace call instead. Not only that, but switching to a synced list differ fixees the horrible nightmare that is the queue adapter. Generally a win-win for most use-cases in Auxio. --- .../auxio/home/list/AlbumListFragment.kt | 6 +- .../auxio/home/list/ArtistListFragment.kt | 6 +- .../auxio/home/list/GenreListFragment.kt | 6 +- .../auxio/home/list/SongListFragment.kt | 6 +- .../auxio/music/excluded/ExcludedAdapter.kt | 34 ++++- .../auxio/music/excluded/ExcludedDialog.kt | 2 +- .../auxio/playback/PlaybackViewModel.kt | 16 +- .../auxio/playback/queue/QueueAdapter.kt | 70 +-------- .../auxio/playback/queue/QueueDragCallback.kt | 13 +- .../org/oxycblt/auxio/ui/RecyclerFramework.kt | 140 ++++++++++++++---- 10 files changed, 164 insertions(+), 135 deletions(-) diff --git a/app/src/main/java/org/oxycblt/auxio/home/list/AlbumListFragment.kt b/app/src/main/java/org/oxycblt/auxio/home/list/AlbumListFragment.kt index 2249c1bee..bc2893784 100644 --- a/app/src/main/java/org/oxycblt/auxio/home/list/AlbumListFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/home/list/AlbumListFragment.kt @@ -28,8 +28,8 @@ import org.oxycblt.auxio.ui.DisplayMode import org.oxycblt.auxio.ui.Item import org.oxycblt.auxio.ui.MenuItemListener import org.oxycblt.auxio.ui.MonoAdapter -import org.oxycblt.auxio.ui.PrimitiveBackingData import org.oxycblt.auxio.ui.Sort +import org.oxycblt.auxio.ui.SyncBackingData import org.oxycblt.auxio.ui.newMenu import org.oxycblt.auxio.util.formatDuration import org.oxycblt.auxio.util.launch @@ -50,7 +50,7 @@ class AlbumListFragment : HomeListFragment() { adapter = homeAdapter } - launch { homeModel.albums.collect(homeAdapter.data::submitList) } + launch { homeModel.albums.collect(homeAdapter.data::replaceList) } } override fun getPopup(pos: Int): String? { @@ -89,7 +89,7 @@ class AlbumListFragment : HomeListFragment() { class AlbumAdapter(listener: MenuItemListener) : MonoAdapter(listener) { - override val data = PrimitiveBackingData(this) + override val data = SyncBackingData(this, AlbumViewHolder.DIFFER) override val creator = AlbumViewHolder.CREATOR } } diff --git a/app/src/main/java/org/oxycblt/auxio/home/list/ArtistListFragment.kt b/app/src/main/java/org/oxycblt/auxio/home/list/ArtistListFragment.kt index d306b64a3..034bea7ff 100644 --- a/app/src/main/java/org/oxycblt/auxio/home/list/ArtistListFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/home/list/ArtistListFragment.kt @@ -28,8 +28,8 @@ import org.oxycblt.auxio.ui.DisplayMode import org.oxycblt.auxio.ui.Item import org.oxycblt.auxio.ui.MenuItemListener import org.oxycblt.auxio.ui.MonoAdapter -import org.oxycblt.auxio.ui.PrimitiveBackingData import org.oxycblt.auxio.ui.Sort +import org.oxycblt.auxio.ui.SyncBackingData import org.oxycblt.auxio.ui.newMenu import org.oxycblt.auxio.util.formatDuration import org.oxycblt.auxio.util.launch @@ -50,7 +50,7 @@ class ArtistListFragment : HomeListFragment() { adapter = homeAdapter } - launch { homeModel.artists.collect(homeAdapter.data::submitList) } + launch { homeModel.artists.collect(homeAdapter.data::replaceList) } } override fun getPopup(pos: Int): String? { @@ -83,7 +83,7 @@ class ArtistListFragment : HomeListFragment() { class ArtistAdapter(listener: MenuItemListener) : MonoAdapter(listener) { - override val data = PrimitiveBackingData(this) + override val data = SyncBackingData(this, ArtistViewHolder.DIFFER) override val creator = ArtistViewHolder.CREATOR } } diff --git a/app/src/main/java/org/oxycblt/auxio/home/list/GenreListFragment.kt b/app/src/main/java/org/oxycblt/auxio/home/list/GenreListFragment.kt index 12e439e3a..37f280e3a 100644 --- a/app/src/main/java/org/oxycblt/auxio/home/list/GenreListFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/home/list/GenreListFragment.kt @@ -28,8 +28,8 @@ import org.oxycblt.auxio.ui.GenreViewHolder import org.oxycblt.auxio.ui.Item import org.oxycblt.auxio.ui.MenuItemListener import org.oxycblt.auxio.ui.MonoAdapter -import org.oxycblt.auxio.ui.PrimitiveBackingData import org.oxycblt.auxio.ui.Sort +import org.oxycblt.auxio.ui.SyncBackingData import org.oxycblt.auxio.ui.newMenu import org.oxycblt.auxio.util.formatDuration import org.oxycblt.auxio.util.launch @@ -50,7 +50,7 @@ class GenreListFragment : HomeListFragment() { adapter = homeAdapter } - launch { homeModel.genres.collect(homeAdapter.data::submitList) } + launch { homeModel.genres.collect(homeAdapter.data::replaceList) } } override fun getPopup(pos: Int): String? { @@ -83,7 +83,7 @@ class GenreListFragment : HomeListFragment() { class GenreAdapter(listener: MenuItemListener) : MonoAdapter(listener) { - override val data = PrimitiveBackingData(this) + override val data = SyncBackingData(this, GenreViewHolder.DIFFER) override val creator = GenreViewHolder.CREATOR } } diff --git a/app/src/main/java/org/oxycblt/auxio/home/list/SongListFragment.kt b/app/src/main/java/org/oxycblt/auxio/home/list/SongListFragment.kt index 2d358446c..089a71030 100644 --- a/app/src/main/java/org/oxycblt/auxio/home/list/SongListFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/home/list/SongListFragment.kt @@ -26,9 +26,9 @@ import org.oxycblt.auxio.ui.DisplayMode import org.oxycblt.auxio.ui.Item import org.oxycblt.auxio.ui.MenuItemListener import org.oxycblt.auxio.ui.MonoAdapter -import org.oxycblt.auxio.ui.PrimitiveBackingData import org.oxycblt.auxio.ui.SongViewHolder import org.oxycblt.auxio.ui.Sort +import org.oxycblt.auxio.ui.SyncBackingData import org.oxycblt.auxio.ui.newMenu import org.oxycblt.auxio.util.formatDuration import org.oxycblt.auxio.util.launch @@ -49,7 +49,7 @@ class SongListFragment : HomeListFragment() { adapter = homeAdapter } - launch { homeModel.songs.collect(homeAdapter.data::submitList) } + launch { homeModel.songs.collect(homeAdapter.data::replaceList) } } override fun getPopup(pos: Int): String? { @@ -90,7 +90,7 @@ class SongListFragment : HomeListFragment() { inner class SongsAdapter(listener: MenuItemListener) : MonoAdapter(listener) { - override val data = PrimitiveBackingData(this) + override val data = SyncBackingData(this, SongViewHolder.DIFFER) override val creator = SongViewHolder.CREATOR } } diff --git a/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedAdapter.kt b/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedAdapter.kt index 9b9580ed5..9e84d14e8 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedAdapter.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedAdapter.kt @@ -19,9 +19,9 @@ package org.oxycblt.auxio.music.excluded import android.content.Context import org.oxycblt.auxio.databinding.ItemExcludedDirBinding +import org.oxycblt.auxio.ui.BackingData import org.oxycblt.auxio.ui.BindingViewHolder import org.oxycblt.auxio.ui.MonoAdapter -import org.oxycblt.auxio.ui.PrimitiveBackingData import org.oxycblt.auxio.util.inflater import org.oxycblt.auxio.util.textSafe @@ -31,12 +31,42 @@ import org.oxycblt.auxio.util.textSafe */ class ExcludedAdapter(listener: Listener) : MonoAdapter(listener) { - override val data = PrimitiveBackingData(this) + override val data = ExcludedBackingData(this) override val creator = ExcludedViewHolder.CREATOR interface Listener { fun onRemoveDirectory(dir: ExcludedDirectory) } + + class ExcludedBackingData(private val adapter: ExcludedAdapter) : + BackingData() { + private val _currentList = mutableListOf() + val currentList: List = _currentList + + override fun getItemCount(): Int = _currentList.size + override fun getItem(position: Int): ExcludedDirectory = _currentList[position] + + fun add(dir: ExcludedDirectory) { + if (_currentList.contains(dir)) { + return + } + + _currentList.add(dir) + adapter.notifyItemInserted(_currentList.lastIndex) + } + + fun addAll(dirs: List) { + val oldLastIndex = dirs.lastIndex + _currentList.addAll(dirs) + adapter.notifyItemRangeInserted(oldLastIndex, dirs.size) + } + + fun remove(dir: ExcludedDirectory) { + val idx = _currentList.indexOf(dir) + _currentList.removeAt(idx) + adapter.notifyItemRemoved(idx) + } + } } /** The viewholder for [ExcludedAdapter]. Not intended for use in other adapters. */ diff --git a/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedDialog.kt b/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedDialog.kt index c9a543230..111945623 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedDialog.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/excluded/ExcludedDialog.kt @@ -97,7 +97,7 @@ class ExcludedDialog : ?.mapNotNull(ExcludedDirectory::fromString) ?: settingsManager.excludedDirs - excludedAdapter.data.submitList(dirs) + excludedAdapter.data.addAll(dirs) requireBinding().excludedEmpty.isVisible = dirs.isEmpty() } 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 3cb83aaa7..b2a3e8d70 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt @@ -192,28 +192,20 @@ class PlaybackViewModel : ViewModel(), PlaybackStateManager.Callback, MusicStore playbackManager.prev() } - /** - * Remove a queue item using it's recyclerview adapter index. If the indices are valid, [apply] - * is called just before the change is committed so that the adapter can be updated. - */ - fun removeQueueDataItem(adapterIndex: Int, apply: () -> Unit) { + /** Remove a queue item using it's recyclerview adapter index. */ + fun removeQueueDataItem(adapterIndex: Int) { val index = adapterIndex + (playbackManager.queue.size - unlikelyToBeNull(_nextUp.value).size) if (index in playbackManager.queue.indices) { - apply() playbackManager.removeQueueItem(index) } } - /** - * Move queue items using their recyclerview adapter indices. If the indices are valid, [apply] - * is called just before the change is committed so that the adapter can be updated. - */ - fun moveQueueDataItems(adapterFrom: Int, adapterTo: Int, apply: () -> Unit): Boolean { + /** Move queue items using their recyclerview adapter indices. */ + fun moveQueueDataItems(adapterFrom: Int, adapterTo: Int): Boolean { val delta = (playbackManager.queue.size - unlikelyToBeNull(_nextUp.value).size) val from = adapterFrom + delta val to = adapterTo + delta if (from in playbackManager.queue.indices && to in playbackManager.queue.indices) { - apply() playbackManager.moveQueueItem(from, to) return true } 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 3f1d8738d..bd0705dc0 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 @@ -23,17 +23,15 @@ import android.graphics.drawable.ColorDrawable import android.view.MotionEvent import android.view.View import androidx.core.view.isInvisible -import androidx.recyclerview.widget.AsyncListDiffer -import androidx.recyclerview.widget.DiffUtil import androidx.recyclerview.widget.RecyclerView import com.google.android.material.shape.MaterialShapeDrawable import org.oxycblt.auxio.IntegerTable import org.oxycblt.auxio.databinding.ItemQueueSongBinding import org.oxycblt.auxio.music.Song -import org.oxycblt.auxio.ui.BackingData import org.oxycblt.auxio.ui.BindingViewHolder import org.oxycblt.auxio.ui.MonoAdapter import org.oxycblt.auxio.ui.SongViewHolder +import org.oxycblt.auxio.ui.SyncBackingData import org.oxycblt.auxio.util.context import org.oxycblt.auxio.util.disableDropShadowCompat import org.oxycblt.auxio.util.inflater @@ -42,7 +40,7 @@ import org.oxycblt.auxio.util.textSafe class QueueAdapter(listener: QueueItemListener) : MonoAdapter(listener) { - override val data = HybridBackingData(this, QueueSongViewHolder.DIFFER) + override val data = SyncBackingData(this, QueueSongViewHolder.DIFFER) override val creator = QueueSongViewHolder.CREATOR } @@ -107,67 +105,3 @@ private constructor( val DIFFER = SongViewHolder.DIFFER } } - -/** - * A list-backed [BackingData] that can be modified with both adapter primitives and - * [AsyncListDiffer]. This is incredibly dangerous and can probably crash the app if you look at it - * the wrong way, so please don't use it outside of the queue module. - */ -class HybridBackingData( - private val adapter: RecyclerView.Adapter<*>, - diffCallback: DiffUtil.ItemCallback -) : BackingData() { - private var _currentList = mutableListOf() - val currentList: List - get() = _currentList - - private val differ = AsyncListDiffer(adapter, diffCallback) - - override fun getItem(position: Int): T = _currentList[position] - override fun getItemCount(): Int = _currentList.size - - fun submitList(newData: List, onDone: () -> Unit = {}) { - if (newData != _currentList) { - _currentList = newData.toMutableList() - differ.submitList(newData, onDone) - } - } - - fun moveItems(from: Int, to: Int) { - _currentList.add(to, _currentList.removeAt(from)) - differ.rewriteListUnsafe(_currentList) - adapter.notifyItemMoved(from, to) - } - - fun removeItem(at: Int) { - _currentList.removeAt(at) - differ.rewriteListUnsafe(_currentList) - adapter.notifyItemRemoved(at) - } - - /** - * Rewrites the AsyncListDiffer's internal list, cancelling any diffs that are currently in - * progress. I cannot describe in words how dangerous this is, but it's also the only thing I - * can do to marry the adapter primitives with DiffUtil. - */ - private fun AsyncListDiffer.rewriteListUnsafe(newList: List) { - differMaxGenerationsField.set(this, (differMaxGenerationsField.get(this) as Int) + 1) - differListField.set(this, newList.toMutableList()) - differImmutableListField.set(this, newList) - } - - companion object { - private val differListField = - AsyncListDiffer::class.java.getDeclaredField("mList").apply { isAccessible = true } - - private val differImmutableListField = - AsyncListDiffer::class.java.getDeclaredField("mReadOnlyList").apply { - isAccessible = true - } - - private val differMaxGenerationsField = - AsyncListDiffer::class.java.getDeclaredField("mMaxScheduledGeneration").apply { - isAccessible = true - } - } -} 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 4267ed25c..aeb9f6bef 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 @@ -148,17 +148,12 @@ class QueueDragCallback( recyclerView: RecyclerView, viewHolder: RecyclerView.ViewHolder, target: RecyclerView.ViewHolder - ): Boolean { - val from = viewHolder.bindingAdapterPosition - val to = target.bindingAdapterPosition - - return playbackModel.moveQueueDataItems(from, to) { queueAdapter.data.moveItems(from, to) } - } + ): Boolean = + playbackModel.moveQueueDataItems( + viewHolder.bindingAdapterPosition, target.bindingAdapterPosition) override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) { - playbackModel.removeQueueDataItem(viewHolder.bindingAdapterPosition) { - queueAdapter.data.removeItem(viewHolder.bindingAdapterPosition) - } + playbackModel.removeQueueDataItem(viewHolder.bindingAdapterPosition) } override fun isLongPressDragEnabled(): Boolean = false diff --git a/app/src/main/java/org/oxycblt/auxio/ui/RecyclerFramework.kt b/app/src/main/java/org/oxycblt/auxio/ui/RecyclerFramework.kt index fbfe17c4c..5f1e4a43f 100644 --- a/app/src/main/java/org/oxycblt/auxio/ui/RecyclerFramework.kt +++ b/app/src/main/java/org/oxycblt/auxio/ui/RecyclerFramework.kt @@ -20,8 +20,8 @@ package org.oxycblt.auxio.ui import android.content.Context import android.view.View import android.view.ViewGroup -import android.widget.Adapter import androidx.annotation.StringRes +import androidx.recyclerview.widget.AdapterListUpdateCallback import androidx.recyclerview.widget.AsyncListDiffer import androidx.recyclerview.widget.DiffUtil import androidx.recyclerview.widget.RecyclerView @@ -183,50 +183,128 @@ abstract class BackingData { } /** - * A list-backed [BackingData] that is modified using adapter primitives. Useful in cases where - * [AsyncBackingData] is not preferable due to bugs involving diffing. + * A list-backed [BackingData] that is modified synchronously. This is generally the recommended + * option for most adapters. * @author OxygenCobalt */ -class PrimitiveBackingData(private val adapter: RecyclerView.Adapter<*>) : BackingData() { - private var _currentList = mutableListOf() - +class SyncBackingData(adapter: RecyclerView.Adapter<*>, diffCallback: DiffUtil.ItemCallback) : + BackingData() { + private var differ = SyncListDiffer(adapter, diffCallback) /** The current list backing this adapter. */ val currentList: List - get() = _currentList + get() = differ.currentList - override fun getItem(position: Int): T = _currentList[position] - override fun getItemCount(): Int = _currentList.size + override fun getItem(position: Int): T = differ.currentList[position] + override fun getItemCount(): Int = differ.currentList.size + + /** Submit a list normally, doing a diff synchronously. */ + fun submitList(newList: List) { + differ.currentList = newList + } /** - * Update the list with a [newList]. This does a basic animation that removes all items and - * replaces them with the new ones. + * Replace this list with a new list. This is useful for very large list diffs that would + * generally be too chaotic and slow to provide a good UX. */ - @Suppress("NotifyDatasetChanged") - fun submitList(newList: List) { - if (_currentList.isNotEmpty()) { - val oldListSize = _currentList.size - _currentList.clear() - adapter.notifyItemRangeRemoved(0, oldListSize) + fun replaceList(newList: List) { + if (newList == differ.currentList) { + return } - _currentList = newList.toMutableList() - adapter.notifyItemRangeInserted(0, _currentList.size) + differ.currentList = emptyList() + differ.currentList = newList } +} - /** Add an item to the list. */ - fun add(item: T) { - _currentList.add(item) - adapter.notifyItemInserted(_currentList.lastIndex) - } +/** + * Like [AsyncListDiffer], but synchronous. This may seem like it would be inefficient, but in + * practice Auxio's lists tend to be small enough to the point where this does not matter, + * and situations that would be inefficient are ruled out with [SyncBackingData.replaceList]. + */ +private class SyncListDiffer( + adapter: RecyclerView.Adapter<*>, + private val diffCallback: DiffUtil.ItemCallback +) { + private val updateCallback = AdapterListUpdateCallback(adapter) - /** Remove an item from the list. */ - fun remove(item: T) { - val idx = _currentList.indexOf(item) - if (idx != -1) { - _currentList.removeAt(idx) - adapter.notifyItemRemoved(idx) + private var _currentList: List = emptyList() + var currentList: List + get() = _currentList + set(newList) { + if (newList === _currentList || newList.isEmpty() && _currentList.isEmpty()) { + return + } + + if (newList.isEmpty()) { + val oldListSize = _currentList.size + _currentList = emptyList() + updateCallback.onRemoved(0, oldListSize) + return + } + + if (_currentList.isEmpty()) { + _currentList = newList + updateCallback.onInserted(0, newList.size) + return + } + + val oldList = _currentList + val result = + DiffUtil.calculateDiff( + object : DiffUtil.Callback() { + override fun getOldListSize(): Int { + return oldList.size + } + + override fun getNewListSize(): Int { + return newList.size + } + + override fun areItemsTheSame( + oldItemPosition: Int, + newItemPosition: Int + ): Boolean { + val oldItem: T? = oldList[oldItemPosition] + val newItem: T? = newList[newItemPosition] + return if (oldItem != null && newItem != null) { + diffCallback.areItemsTheSame(oldItem, newItem) + } else { + oldItem == null && newItem == null + } + } + + override fun areContentsTheSame( + oldItemPosition: Int, + newItemPosition: Int + ): Boolean { + val oldItem: T? = oldList[oldItemPosition] + val newItem: T? = newList[newItemPosition] + return if (oldItem != null && newItem != null) { + diffCallback.areContentsTheSame(oldItem, newItem) + } else if (oldItem == null && newItem == null) { + true + } else { + throw AssertionError() + } + } + + override fun getChangePayload( + oldItemPosition: Int, + newItemPosition: Int + ): Any? { + val oldItem: T? = oldList[oldItemPosition] + val newItem: T? = newList[newItemPosition] + return if (oldItem != null && newItem != null) { + diffCallback.getChangePayload(oldItem, newItem) + } else { + throw AssertionError() + } + } + }) + + _currentList = newList + result.dispatchUpdatesTo(updateCallback) } - } } /**