detail: fix highlighting issues

Fix two major highlighting bugs based around the janky and stupid way
I would handle highlighting previously.

Previously, I would index the views of a RecyclerView in order to
highlight viewholders. In retrospect this was a pretty bad idea,
as viewholders could be in a weird limbo state where they are bound,
but not accessible. I mean, it's in the name. It's a Recycling View.

Fortunately, google actually knew what they were doing and provided
a way to mutate viewholders at runtime using notifyItemChanged.
And the API actually makes sense! Wow! Migrate all detail adapters
to a system that uses notifyItemChanged instead of the terrible
pre-existing system.

app/build.gradle

@@ -56,6 +56,7 @@ dependencies {
     implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version"
     implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version"
     implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
+    implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.5.2"
 
     // --- SUPPORT ---
 

app/src/main/java/org/oxycblt/auxio/MainActivity.kt

@@ -44,8 +44,6 @@ import org.oxycblt.auxio.util.systemBarInsetsCompat
  *
  * TODO: Custom language support
  *
- * TODO: Fix how selection works in the RecyclerViews (doing it poorly right now)
- *
  * TODO: Rework padding ethos
  *
  * @author OxygenCobalt

app/src/main/java/org/oxycblt/auxio/detail/AlbumDetailFragment.kt

@@ -25,7 +25,6 @@ import androidx.core.view.children
 import androidx.navigation.fragment.findNavController
 import androidx.navigation.fragment.navArgs
 import androidx.recyclerview.widget.LinearSmoothScroller
-import kotlinx.coroutines.launch
 import org.oxycblt.auxio.R
 import org.oxycblt.auxio.databinding.FragmentDetailBinding
 import org.oxycblt.auxio.detail.recycler.AlbumDetailAdapter
@@ -33,6 +32,7 @@ import org.oxycblt.auxio.detail.recycler.SortHeader
 import org.oxycblt.auxio.music.Album
 import org.oxycblt.auxio.music.Artist
 import org.oxycblt.auxio.music.Music
+import org.oxycblt.auxio.music.MusicParent
 import org.oxycblt.auxio.music.Song
 import org.oxycblt.auxio.playback.state.PlaybackMode
 import org.oxycblt.auxio.ui.Header
@@ -40,6 +40,7 @@ import org.oxycblt.auxio.ui.Item
 import org.oxycblt.auxio.ui.newMenu
 import org.oxycblt.auxio.util.applySpans
 import org.oxycblt.auxio.util.canScroll
+import org.oxycblt.auxio.util.collectWith
 import org.oxycblt.auxio.util.launch
 import org.oxycblt.auxio.util.logD
 import org.oxycblt.auxio.util.logW
@@ -70,7 +71,7 @@ class AlbumDetailFragment : DetailFragment(), AlbumDetailAdapter.Listener {
 
         launch { detailModel.albumData.collect(detailAdapter.data::submitList) }
         launch { navModel.exploreNavigationItem.collect(::handleNavigation) }
-        launch { playbackModel.song.collect(::updateSong) }
+        launch { playbackModel.song.collectWith(playbackModel.parent, ::updatePlayback) }
     }
 
     override fun onMenuItemClick(item: MenuItem): Boolean {
@@ -188,22 +189,24 @@ class AlbumDetailFragment : DetailFragment(), AlbumDetailAdapter.Listener {
         }
     }
 
-    /** Updates the queue actions when a song is present or not */
+    private fun updatePlayback(song: Song?, parent: MusicParent?) {
-    private fun updateSong(song: Song?) {
         val binding = requireBinding()
 
         for (item in binding.detailToolbar.menu.children) {
+            // If there is no playback going in, any queue additions will be wiped as soon as
+            // something is played. Disable these actions when playback is going on so that
+            // it isn't possible to add anything during that time.
             if (item.itemId == R.id.action_play_next || item.itemId == R.id.action_queue_add) {
                 item.isEnabled = song != null
             }
         }
 
-        if (playbackModel.parent.value is Album &&
+        if (parent is Album && parent.id == unlikelyToBeNull(detailModel.currentAlbum.value).id) {
-            playbackModel.parent.value?.id == unlikelyToBeNull(detailModel.currentAlbum.value).id) {
+            logD("update $song")
-            detailAdapter.highlightSong(song, binding.detailRecycler)
+            detailAdapter.highlightSong(song)
         } else {
             // Clear the ViewHolders if the mode isn't ALL_SONGS
-            detailAdapter.highlightSong(null, binding.detailRecycler)
+            detailAdapter.highlightSong(null)
         }
     }
 

app/src/main/java/org/oxycblt/auxio/detail/ArtistDetailFragment.kt

@@ -38,6 +38,7 @@ import org.oxycblt.auxio.ui.Header
 import org.oxycblt.auxio.ui.Item
 import org.oxycblt.auxio.ui.newMenu
 import org.oxycblt.auxio.util.applySpans
+import org.oxycblt.auxio.util.collectWith
 import org.oxycblt.auxio.util.launch
 import org.oxycblt.auxio.util.logD
 import org.oxycblt.auxio.util.logW
@@ -68,8 +69,7 @@ class ArtistDetailFragment : DetailFragment(), DetailAdapter.Listener {
 
         launch { detailModel.artistData.collect(detailAdapter.data::submitList) }
         launch { navModel.exploreNavigationItem.collect(::handleNavigation) }
-        launch { playbackModel.song.collect(::updateSong) }
+        launch { playbackModel.song.collectWith(playbackModel.parent, ::updatePlayback) }
-        launch { playbackModel.parent.collect(::updateParent) }
     }
 
     override fun onMenuItemClick(item: MenuItem): Boolean = false
@@ -135,23 +135,22 @@ class ArtistDetailFragment : DetailFragment(), DetailAdapter.Listener {
         }
     }
 
-    private fun updateSong(song: Song?) {
+    private fun updatePlayback(song: Song?, parent: MusicParent?) {
         val binding = requireBinding()
-        if (playbackModel.parent.value is Artist &&
-            playbackModel.parent.value?.id == detailModel.currentArtist.value?.id) {
-            detailAdapter.highlightSong(song, binding.detailRecycler)
-        } else {
-            // Clear the ViewHolders if the mode isn't ALL_SONGS
-            detailAdapter.highlightSong(null, binding.detailRecycler)
-        }
-    }
 
-    private fun updateParent(parent: MusicParent?) {
+        if (parent is Artist && parent.id == unlikelyToBeNull(detailModel.currentArtist.value).id) {
-        val binding = requireBinding()
+            detailAdapter.highlightSong(song)
-        if (parent is Album?) {
-            detailAdapter.highlightAlbum(parent, binding.detailRecycler)
         } else {
-            detailAdapter.highlightAlbum(null, binding.detailRecycler)
+            // Clear the ViewHolders if the given song is not part of this artist.
+            detailAdapter.highlightSong(null)
+        }
+
+        if (parent is Album &&
+            parent.artist.id == unlikelyToBeNull(detailModel.currentArtist.value).id) {
+            detailAdapter.highlightAlbum(parent)
+        } else {
+            // Clear out the album viewholder if the parent is not an album.
+            detailAdapter.highlightAlbum(null)
         }
     }
 }

app/src/main/java/org/oxycblt/auxio/detail/DetailAppBarLayout.kt

@@ -85,13 +85,11 @@ constructor(context: Context, attrs: AttributeSet? = null, @AttrRes defStyleAttr
 
     private fun findRecyclerView(): RecyclerView {
         val recycler = recycler
-
         if (recycler != null) {
             return recycler
         }
 
         val newRecycler = (parent as ViewGroup).findViewById<RecyclerView>(liftOnScrollTargetViewId)
-
         this.recycler = newRecycler
         return newRecycler
     }
@@ -124,10 +122,8 @@ constructor(context: Context, attrs: AttributeSet? = null, @AttrRes defStyleAttr
         this.titleAnimator =
             ValueAnimator.ofFloat(from, to).apply {
                 addUpdateListener { titleView?.alpha = it.animatedValue as Float }
-
                 duration =
                     resources.getInteger(R.integer.detail_app_bar_title_anim_duration).toLong()
-
                 start()
             }
     }

app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt

@@ -123,7 +123,7 @@ class DetailViewModel : ViewModel() {
 
         // To create a good user experience regarding disc numbers, we intersperse
         // items that show the disc number throughout the album's songs. In the case
-        // that the  album does not have disc numbers, we omit the header.
+        // that the  album does not have distinct disc numbers, we omit the header.
         val songs = albumSort.songs(album.songs)
         val byDisc = songs.groupBy { it.disc ?: 1 }
         if (byDisc.size > 1) {

app/src/main/java/org/oxycblt/auxio/detail/GenreDetailFragment.kt

@@ -22,7 +22,6 @@ import android.view.MenuItem
 import android.view.View
 import androidx.navigation.fragment.findNavController
 import androidx.navigation.fragment.navArgs
-import kotlinx.coroutines.launch
 import org.oxycblt.auxio.R
 import org.oxycblt.auxio.databinding.FragmentDetailBinding
 import org.oxycblt.auxio.detail.recycler.DetailAdapter
@@ -32,12 +31,14 @@ import org.oxycblt.auxio.music.Album
 import org.oxycblt.auxio.music.Artist
 import org.oxycblt.auxio.music.Genre
 import org.oxycblt.auxio.music.Music
+import org.oxycblt.auxio.music.MusicParent
 import org.oxycblt.auxio.music.Song
 import org.oxycblt.auxio.playback.state.PlaybackMode
 import org.oxycblt.auxio.ui.Header
 import org.oxycblt.auxio.ui.Item
 import org.oxycblt.auxio.ui.newMenu
 import org.oxycblt.auxio.util.applySpans
+import org.oxycblt.auxio.util.collectWith
 import org.oxycblt.auxio.util.launch
 import org.oxycblt.auxio.util.logD
 import org.oxycblt.auxio.util.unlikelyToBeNull
@@ -66,7 +67,7 @@ class GenreDetailFragment : DetailFragment(), DetailAdapter.Listener {
 
         launch { detailModel.genreData.collect(detailAdapter.data::submitList) }
         launch { navModel.exploreNavigationItem.collect(::handleNavigation) }
-        launch { playbackModel.song.collect(::updateSong) }
+        launch { playbackModel.song.collectWith(playbackModel.parent, ::updatePlayback) }
     }
 
     override fun onMenuItemClick(item: MenuItem): Boolean = false
@@ -124,14 +125,13 @@ class GenreDetailFragment : DetailFragment(), DetailAdapter.Listener {
         }
     }
 
-    private fun updateSong(song: Song?) {
+    private fun updatePlayback(song: Song?, parent: MusicParent?) {
         val binding = requireBinding()
-        if (playbackModel.parent.value is Genre &&
+        if (parent is Genre && parent.id == unlikelyToBeNull(detailModel.currentGenre.value).id) {
-            playbackModel.parent.value?.id == unlikelyToBeNull(detailModel.currentGenre.value).id) {
+            detailAdapter.highlightSong(song)
-            detailAdapter.highlightSong(song, binding.detailRecycler)
         } else {
             // Clear the ViewHolders if the mode isn't ALL_SONGS
-            detailAdapter.highlightSong(null, binding.detailRecycler)
+            detailAdapter.highlightSong(null)
         }
     }
 }

app/src/main/java/org/oxycblt/auxio/detail/recycler/AlbumDetailAdapter.kt

@@ -44,8 +44,8 @@ import org.oxycblt.auxio.util.textSafe
  */
 class AlbumDetailAdapter(listener: Listener) :
     DetailAdapter<AlbumDetailAdapter.Listener>(listener, DIFFER) {
-    private var highlightedSong: Song? = null
+    private var currentSong: Song? = null
-    private var highlightedViewHolder: Highlightable? = null
+    private var currentHighlightedSongPos: Int? = null
 
     override fun getCreatorFromItem(item: Item) =
         super.getCreatorFromItem(item)
@@ -76,25 +76,15 @@ class AlbumDetailAdapter(listener: Listener) :
     }
 
     override fun onHighlightViewHolder(viewHolder: Highlightable, item: Item) {
-        if (item is Song && item.id == highlightedSong?.id) {
+        viewHolder.setHighlighted(item.id == currentSong?.id)
-            // Reset the last ViewHolder before assigning the new, correct one to be highlighted
-            highlightedViewHolder?.setHighlighted(false)
-            highlightedViewHolder = viewHolder
-            viewHolder.setHighlighted(true)
-        } else {
-            viewHolder.setHighlighted(false)
-        }
     }
 
-    /**
+    /** Update the [song] that this adapter should highlight */
-     * Update the [song] that this adapter should highlight
+    fun highlightSong(song: Song?) {
-     * @param recycler The recyclerview the highlighting should act on.
+        if (song == currentSong) return
-     */
+        currentSong = song
-    fun highlightSong(song: Song?, recycler: RecyclerView) {
+        currentHighlightedSongPos?.let { notifyItemChanged(it, PAYLOAD_HIGHLIGHT_CHANGED) }
-        if (song == highlightedSong) return
+        currentHighlightedSongPos = highlightItem(song)
-        highlightedSong = song
-        highlightedViewHolder?.setHighlighted(false)
-        highlightedViewHolder = highlightItem(song, recycler)
     }
 
     companion object {

app/src/main/java/org/oxycblt/auxio/detail/recycler/ArtistDetailAdapter.kt

@@ -45,10 +45,10 @@ import org.oxycblt.auxio.util.textSafe
 class ArtistDetailAdapter(listener: Listener) :
     DetailAdapter<DetailAdapter.Listener>(listener, DIFFER) {
     private var currentAlbum: Album? = null
-    private var currentAlbumHolder: Highlightable? = null
+    private var currentHighlightedAlbumPos: Int? = null
 
     private var currentSong: Song? = null
-    private var currentSongHolder: Highlightable? = null
+    private var currentHighlightedSongPos: Int? = null
 
     override fun getCreatorFromItem(item: Item) =
         super.getCreatorFromItem(item)
@@ -79,40 +79,26 @@ class ArtistDetailAdapter(listener: Listener) :
     }
 
     override fun onHighlightViewHolder(viewHolder: Highlightable, item: Item) {
-        // If the item corresponds to a currently playing song/album then highlight it
+        viewHolder.setHighlighted(
-        if (item.id == currentAlbum?.id && item is Album) {
+            (item is Album && item.id == currentAlbum?.id) ||
-            currentAlbumHolder?.setHighlighted(false)
+                (item is Song && item.id == currentSong?.id)
-            currentAlbumHolder = viewHolder
+        )
-            viewHolder.setHighlighted(true)
-        } else if (item.id == currentSong?.id && item is Song) {
-            currentSongHolder?.setHighlighted(false)
-            currentSongHolder = viewHolder
-            viewHolder.setHighlighted(true)
-        } else {
-            viewHolder.setHighlighted(false)
-        }
     }
 
-    /**
+    /** Update the current [album] that this adapter should highlight */
-     * Update the current [album] that this adapter should highlight
+    fun highlightAlbum(album: Album?) {
-     * @param recycler The recyclerview the highlighting should act on.
-     */
-    fun highlightAlbum(album: Album?, recycler: RecyclerView) {
         if (album == currentAlbum) return
         currentAlbum = album
-        currentAlbumHolder?.setHighlighted(false)
+        currentHighlightedAlbumPos?.let { notifyItemChanged(it, PAYLOAD_HIGHLIGHT_CHANGED) }
-        currentAlbumHolder = highlightItem(album, recycler)
+        currentHighlightedAlbumPos = highlightItem(album)
     }
 
-    /**
+    /** Update the [song] that this adapter should highlight */
-     * Update the [song] that this adapter should highlight
+    fun highlightSong(song: Song?) {
-     * @param recycler The recyclerview the highlighting should act on.
-     */
-    fun highlightSong(song: Song?, recycler: RecyclerView) {
         if (song == currentSong) return
         currentSong = song
-        currentSongHolder?.setHighlighted(false)
+        currentHighlightedSongPos?.let { notifyItemChanged(it, PAYLOAD_HIGHLIGHT_CHANGED) }
-        currentSongHolder = highlightItem(song, recycler)
+        currentHighlightedSongPos = highlightItem(song)
     }
 
     companion object {

app/src/main/java/org/oxycblt/auxio/detail/recycler/DetailAdapter.kt

@@ -35,7 +35,6 @@ import org.oxycblt.auxio.ui.NewHeaderViewHolder
 import org.oxycblt.auxio.ui.SimpleItemCallback
 import org.oxycblt.auxio.util.context
 import org.oxycblt.auxio.util.inflater
-import org.oxycblt.auxio.util.logW
 import org.oxycblt.auxio.util.textSafe
 
 abstract class DetailAdapter<L : DetailAdapter.Listener>(
@@ -44,10 +43,7 @@ abstract class DetailAdapter<L : DetailAdapter.Listener>(
 ) : MultiAdapter<L>(listener) {
     abstract fun onHighlightViewHolder(viewHolder: Highlightable, item: Item)
 
-    protected inline fun <reified T : Item> highlightItem(
+    protected inline fun <reified T : Item> highlightItem(newItem: T?): Int? {
-        newItem: T?,
-        recycler: RecyclerView
-    ): Highlightable? {
         if (newItem == null) {
             return null
         }
@@ -55,19 +51,8 @@ abstract class DetailAdapter<L : DetailAdapter.Listener>(
         // Use existing data instead of having to re-sort it.
         // We also have to account for the album count when searching for the ViewHolder.
         val pos = data.currentList.indexOfFirst { item -> item.id == newItem.id && item is T }
-
+        notifyItemChanged(pos, PAYLOAD_HIGHLIGHT_CHANGED)
-        // Check if the ViewHolder for this song is visible, if it is then highlight it.
+        return pos
-        // If the ViewHolder is not visible, then the adapter should take care of it if
-        // it does become visible.
-        val viewHolder = recycler.findViewHolderForAdapterPosition(pos)
-
-        return if (viewHolder is Highlightable) {
-            viewHolder.setHighlighted(true)
-            viewHolder
-        } else {
-            logW("ViewHolder intended to highlight was not Highlightable")
-            null
-        }
     }
 
     @Suppress("LeakingThis") override val data = AsyncBackingData(this, diffCallback)
@@ -98,6 +83,13 @@ abstract class DetailAdapter<L : DetailAdapter.Listener>(
     }
 
     companion object {
+        // This payload value serves two purposes:
+        // 1. It disables animations from notifyItemChanged, which looks bad when highlighting
+        // values.
+        // 2. It instructs adapters to avoid re-binding information, and instead simply
+        // change the highlight state.
+        val PAYLOAD_HIGHLIGHT_CHANGED = Any()
+
         val DIFFER =
             object : SimpleItemCallback<Item>() {
                 override fun areItemsTheSame(oldItem: Item, newItem: Item): Boolean {

app/src/main/java/org/oxycblt/auxio/detail/recycler/GenreDetailAdapter.kt

@@ -43,7 +43,7 @@ import org.oxycblt.auxio.util.textSafe
 class GenreDetailAdapter(listener: Listener) :
     DetailAdapter<DetailAdapter.Listener>(listener, DIFFER) {
     private var currentSong: Song? = null
-    private var currentHolder: Highlightable? = null
+    private var currentHighlightedSongPos: Int? = null
 
     override fun getCreatorFromItem(item: Item) =
         super.getCreatorFromItem(item)
@@ -71,26 +71,15 @@ class GenreDetailAdapter(listener: Listener) :
     }
 
     override fun onHighlightViewHolder(viewHolder: Highlightable, item: Item) {
-        // If the item corresponds to a currently playing song/album then highlight it
+        viewHolder.setHighlighted(item.id == currentSong?.id)
-        if (item.id == currentSong?.id) {
-            // Reset the last ViewHolder before assigning the new, correct one to be highlighted
-            currentHolder?.setHighlighted(false)
-            currentHolder = viewHolder
-            viewHolder.setHighlighted(true)
-        } else {
-            viewHolder.setHighlighted(false)
-        }
     }
 
-    /**
+    /** Update the [song] that this adapter should highlight */
-     * Update the [song] that this adapter should highlight
+    fun highlightSong(song: Song?) {
-     * @param recycler The recyclerview the highlighting should act on.
-     */
-    fun highlightSong(song: Song?, recycler: RecyclerView) {
         if (song == currentSong) return
         currentSong = song
-        currentHolder?.setHighlighted(false)
+        currentHighlightedSongPos?.let { notifyItemChanged(it, PAYLOAD_HIGHLIGHT_CHANGED) }
-        currentHolder = highlightItem(song, recycler)
+        currentHighlightedSongPos = highlightItem(song)
     }
 
     companion object {

app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt

@@ -58,8 +58,7 @@ class PlaybackViewModel : ViewModel(), PlaybackStateManager.Callback, MusicStore
         get() = _song
     private val _parent = MutableStateFlow<MusicParent?>(null)
     /** The current model that is being played from, such as an [Album] or [Artist] */
-    val parent: StateFlow<MusicParent?>
+    val parent: StateFlow<MusicParent?> = _parent
-        get() = _parent
     private val _isPlaying = MutableStateFlow(false)
     val isPlaying: StateFlow<Boolean>
         get() = _isPlaying
@@ -298,8 +297,8 @@ class PlaybackViewModel : ViewModel(), PlaybackStateManager.Callback, MusicStore
     }
 
     override fun onNewPlayback(index: Int, queue: List<Song>, parent: MusicParent?) {
-        _parent.value = playbackManager.parent
         _song.value = playbackManager.song
+        _parent.value = playbackManager.parent
         _nextUp.value = queue.slice(index + 1 until queue.size)
     }
 

app/src/main/java/org/oxycblt/auxio/ui/RecyclerFramework.kt

@@ -53,6 +53,8 @@ private typealias AnyCreator = BindingViewHolder.Creator<out RecyclerView.ViewHo
  * An adapter for many viewholders tied to many types of data. Deriving this is more complicated
  * than [MonoAdapter], as less overrides can be provided "for free".
  * @author OxygenCobalt
+ * 
+ * TODO: Force impls to handle payload situations.
  */
 abstract class MultiAdapter<L>(private val listener: L) :
     RecyclerView.Adapter<RecyclerView.ViewHolder>() {

app/src/main/java/org/oxycblt/auxio/util/FrameworkUtil.kt

@@ -38,6 +38,9 @@ import androidx.recyclerview.widget.GridLayoutManager
 import androidx.recyclerview.widget.RecyclerView
 import androidx.viewbinding.ViewBinding
 import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.collect
+import kotlinx.coroutines.flow.combine
 import kotlinx.coroutines.launch
 import org.oxycblt.auxio.R
 
@@ -166,6 +169,11 @@ fun Fragment.launch(
     viewLifecycleOwner.lifecycleScope.launch { viewLifecycleOwner.repeatOnLifecycle(state, block) }
 }
 
+/** Combines the called flow with the given flow and then collects them both into [block]. */
+suspend fun <T1, T2> Flow<T1>.collectWith(other: Flow<T2>, block: suspend (T1, T2) -> Unit) {
+    combine(this, other) { a, b -> a to b }.collect { block(it.first, it.second) }
+}
+
 /**
  * Shortcut for querying all items in a database and running [block] with the cursor returned. Will
  * not run if the cursor is null.