ui: refine navigation listeners

Make sure that we don't drop selections or playlist edits when we
navigate to dialogs, this time achieved through a more general
navigation listener implementation than prior.

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

@@ -27,8 +27,6 @@ import androidx.core.view.ViewCompat
 import androidx.core.view.isInvisible
 import androidx.core.view.updatePadding
 import androidx.fragment.app.activityViewModels
-import androidx.navigation.NavController
-import androidx.navigation.NavDestination
 import androidx.navigation.findNavController
 import androidx.navigation.fragment.findNavController
 import com.google.android.material.R as MR
@@ -49,6 +47,7 @@ import org.oxycblt.auxio.playback.OpenPanel
 import org.oxycblt.auxio.playback.PlaybackBottomSheetBehavior
 import org.oxycblt.auxio.playback.PlaybackViewModel
 import org.oxycblt.auxio.playback.queue.QueueBottomSheetBehavior
+import org.oxycblt.auxio.ui.DialogAwareNavigationListener
 import org.oxycblt.auxio.ui.ViewBindingFragment
 import org.oxycblt.auxio.util.collectImmediately
 import org.oxycblt.auxio.util.context
@@ -67,9 +66,7 @@ import org.oxycblt.auxio.util.unlikelyToBeNull
  */
 @AndroidEntryPoint
 class MainFragment :
-    ViewBindingFragment<FragmentMainBinding>(),
+    ViewBindingFragment<FragmentMainBinding>(), ViewTreeObserver.OnPreDrawListener {
-    ViewTreeObserver.OnPreDrawListener,
-    NavController.OnDestinationChangedListener {
     private val detailModel: DetailViewModel by activityViewModels()
     private val homeModel: HomeViewModel by activityViewModels()
     private val listModel: ListViewModel by activityViewModels()
@@ -77,9 +74,9 @@ class MainFragment :
     private var sheetBackCallback: SheetBackPressedCallback? = null
     private var detailBackCallback: DetailBackPressedCallback? = null
     private var selectionBackCallback: SelectionBackPressedCallback? = null
+    private var selectionNavigationListener: DialogAwareNavigationListener? = null
     private var lastInsets: WindowInsets? = null
     private var elevationNormal = 0f
-    private var initialNavDestinationChange = true
 
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
@@ -111,6 +108,8 @@ class MainFragment :
         val selectionBackCallback =
             SelectionBackPressedCallback(listModel).also { selectionBackCallback = it }
 
+        selectionNavigationListener = DialogAwareNavigationListener(listModel::dropSelection)
+
         // --- UI SETUP ---
         val context = requireActivity()
 
@@ -162,8 +161,8 @@ class MainFragment :
         val binding = requireBinding()
         // Once we add the destination change callback, we will receive another initialization call,
         // so handle that by resetting the flag.
-        initialNavDestinationChange = false
+        requireNotNull(selectionNavigationListener) { "NavigationListener was not available" }
-        binding.exploreNavHost.findNavController().addOnDestinationChangedListener(this)
+            .attach(binding.exploreNavHost.findNavController())
         // Listener could still reasonably fire even if we clear the binding, attach/detach
         // our pre-draw listener our listener in onStart/onStop respectively.
         binding.playbackSheet.viewTreeObserver.addOnPreDrawListener(this@MainFragment)
@@ -184,7 +183,8 @@ class MainFragment :
     override fun onStop() {
         super.onStop()
         val binding = requireBinding()
-        binding.exploreNavHost.findNavController().removeOnDestinationChangedListener(this)
+        requireNotNull(selectionNavigationListener) { "NavigationListener was not available" }
+            .release(binding.exploreNavHost.findNavController())
         binding.playbackSheet.viewTreeObserver.removeOnPreDrawListener(this)
     }
 
@@ -193,6 +193,7 @@ class MainFragment :
         sheetBackCallback = null
         detailBackCallback = null
         selectionBackCallback = null
+        selectionNavigationListener = null
     }
 
     override fun onPreDraw(): Boolean {
@@ -286,25 +287,6 @@ class MainFragment :
         return true
     }
 
-    override fun onDestinationChanged(
-        controller: NavController,
-        destination: NavDestination,
-        arguments: Bundle?
-    ) {
-        // Drop the initial call by NavController that simply provides us with the current
-        // destination. This would cause the selection state to be lost every time the device
-        // rotates.
-        if (!initialNavDestinationChange) {
-            initialNavDestinationChange = true
-            return
-        }
-        if (destination.id != R.id.selection_menu_dialog) {
-            // Drop any pending playlist edits when navigating away. This could actually happen
-            // if the user is quick enough.
-            listModel.dropSelection()
-        }
-    }
-
     private fun handleShowOuter(outer: Outer?) {
         val directions =
             when (outer) {

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

@@ -20,9 +20,8 @@ package org.oxycblt.auxio.detail
 
 import android.os.Bundle
 import android.view.LayoutInflater
+import android.view.MenuItem
 import androidx.fragment.app.activityViewModels
-import androidx.navigation.NavController
-import androidx.navigation.NavDestination
 import androidx.navigation.fragment.findNavController
 import androidx.navigation.fragment.navArgs
 import androidx.recyclerview.widget.ConcatAdapter
@@ -51,6 +50,7 @@ import org.oxycblt.auxio.music.PlaylistDecision
 import org.oxycblt.auxio.music.Song
 import org.oxycblt.auxio.playback.PlaybackDecision
 import org.oxycblt.auxio.playback.PlaybackViewModel
+import org.oxycblt.auxio.ui.DialogAwareNavigationListener
 import org.oxycblt.auxio.util.collect
 import org.oxycblt.auxio.util.collectImmediately
 import org.oxycblt.auxio.util.logD
@@ -68,8 +68,7 @@ import org.oxycblt.auxio.util.unlikelyToBeNull
 class PlaylistDetailFragment :
     ListFragment<Song, FragmentDetailBinding>(),
     DetailHeaderAdapter.Listener,
-    PlaylistDetailListAdapter.Listener,
+    PlaylistDetailListAdapter.Listener {
-    NavController.OnDestinationChangedListener {
     private val detailModel: DetailViewModel by activityViewModels()
     override val listModel: ListViewModel by activityViewModels()
     override val musicModel: MusicViewModel by activityViewModels()
@@ -80,7 +79,7 @@ class PlaylistDetailFragment :
     private val playlistHeaderAdapter = PlaylistDetailHeaderAdapter(this)
     private val playlistListAdapter = PlaylistDetailListAdapter(this)
     private var touchHelper: ItemTouchHelper? = null
-    private var initialNavDestinationChange = false
+    private var editNavigationListener: DialogAwareNavigationListener? = null
 
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
@@ -98,6 +97,8 @@ class PlaylistDetailFragment :
     override fun onBindingCreated(binding: FragmentDetailBinding, savedInstanceState: Bundle?) {
         super.onBindingCreated(binding, savedInstanceState)
 
+        editNavigationListener = DialogAwareNavigationListener(detailModel::dropPlaylistEdit)
+
         // --- UI SETUP ---
         binding.detailNormalToolbar.apply {
             setNavigationOnClickListener { findNavController().navigateUp() }
@@ -147,17 +148,31 @@ class PlaylistDetailFragment :
         collect(playbackModel.playbackDecision.flow, ::handlePlaybackDecision)
     }
 
+    override fun onMenuItemClick(item: MenuItem): Boolean {
+        if (super.onMenuItemClick(item)) {
+            return true
+        }
+
+        if (item.itemId == R.id.action_save) {
+            detailModel.savePlaylistEdit()
+            return true
+        }
+
+        return false
+    }
+
     override fun onStart() {
         super.onStart()
         // Once we add the destination change callback, we will receive another initialization call,
         // so handle that by resetting the flag.
-        initialNavDestinationChange = false
+        requireNotNull(editNavigationListener) { "NavigationListener was not available" }
-        findNavController().addOnDestinationChangedListener(this)
+            .attach(findNavController())
     }
 
     override fun onStop() {
         super.onStop()
-        findNavController().removeOnDestinationChangedListener(this)
+        requireNotNull(editNavigationListener) { "NavigationListener was not available" }
+            .release(findNavController())
     }
 
     override fun onDestroyBinding(binding: FragmentDetailBinding) {
@@ -168,26 +183,7 @@ class PlaylistDetailFragment :
         // Avoid possible race conditions that could cause a bad replace instruction to be consumed
         // during list initialization and crash the app. Could happen if the user is fast enough.
         detailModel.playlistSongInstructions.consume()
-    }
+        editNavigationListener = null
-
-    override fun onDestinationChanged(
-        controller: NavController,
-        destination: NavDestination,
-        arguments: Bundle?
-    ) {
-        // Drop the initial call by NavController that simply provides us with the current
-        // destination. This would cause the selection state to be lost every time the device
-        // rotates.
-        if (!initialNavDestinationChange) {
-            initialNavDestinationChange = true
-            return
-        }
-        if (destination.id != R.id.playlist_detail_fragment &&
-            destination.id != R.id.playlist_song_sort_dialog) {
-            // Drop any pending playlist edits when navigating away. This could actually happen
-            // if the user is quick enough.
-            detailModel.dropPlaylistEdit()
-        }
     }
 
     override fun onRealClick(item: Song) {

app/src/main/java/org/oxycblt/auxio/list/ListViewModel.kt

@@ -114,10 +114,8 @@ constructor(private val listSettings: ListSettings, private val musicRepository:
      *
      * @return A list of [Song]s collated from each item selected.
      */
-    fun takeSelection(): List<Song> {
+    fun peekSelection() =
-        logD("Taking selection")
+        _selected.value.flatMap {
-        return _selected.value
-            .flatMap {
             when (it) {
                 is Song -> listOf(it)
                 is Album -> listSettings.albumSongSort.songs(it.songs)
@@ -126,7 +124,15 @@ constructor(private val listSettings: ListSettings, private val musicRepository:
                 is Playlist -> it.songs
             }
         }
-            .also { _selected.value = listOf() }
+
+    /**
+     * Clear the current selection and return it.
+     *
+     * @return A list of [Song]s collated from each item selected.
+     */
+    fun takeSelection(): List<Song> {
+        logD("Taking selection")
+        return peekSelection().also { _selected.value = listOf() }
     }
 
     /**

app/src/main/java/org/oxycblt/auxio/list/SelectionFragment.kt

@@ -49,7 +49,7 @@ abstract class SelectionFragment<VB : ViewBinding> :
             setNavigationOnClickListener { listModel.dropSelection() }
             setOnMenuItemClickListener(this@SelectionFragment)
             overrideOnOverflowMenuClick {
-                listModel.openMenu(R.menu.selection, listModel.takeSelection())
+                listModel.openMenu(R.menu.selection, listModel.peekSelection())
             }
         }
     }

app/src/main/java/org/oxycblt/auxio/list/menu/MenuDialogFragmentImpl.kt

@@ -79,10 +79,10 @@ class SongMenuDialogFragment : MenuDialogFragment<Menu.ForSong>() {
                 playbackModel.addToQueue(menu.song)
                 requireContext().showToast(R.string.lng_queue_added)
             }
+            R.id.action_playlist_add -> musicModel.addToPlaylist(menu.song)
             R.id.action_artist_details -> detailModel.showArtist(menu.song)
             R.id.action_album_details -> detailModel.showAlbum(menu.song.album)
             R.id.action_share -> requireContext().share(menu.song)
-            R.id.action_playlist_add -> musicModel.addToPlaylist(menu.song)
             R.id.action_detail -> detailModel.showSong(menu.song)
             else -> error("Unexpected menu item selected $item")
         }
@@ -352,6 +352,7 @@ class SelectionMenuDialogFragment : MenuDialogFragment<Menu.ForSelection>() {
     }
 
     override fun onClick(item: MenuItem, menu: Menu.ForSelection) {
+        listModel.dropSelection()
         when (item.itemId) {
             R.id.action_play -> playbackModel.play(menu.songs)
             R.id.action_shuffle -> playbackModel.shuffle(menu.songs)
@@ -363,8 +364,8 @@ class SelectionMenuDialogFragment : MenuDialogFragment<Menu.ForSelection>() {
                 playbackModel.addToQueue(menu.songs)
                 requireContext().showToast(R.string.lng_queue_added)
             }
-            R.id.action_share -> requireContext().share(menu.songs)
             R.id.action_playlist_add -> musicModel.addToPlaylist(menu.songs)
+            R.id.action_share -> requireContext().share(menu.songs)
             else -> error("Unexpected menu item selected $item")
         }
     }

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

@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2023 Auxio Project
+ * DialogAwareNavigationListener.kt is part of Auxio.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+ 
+package org.oxycblt.auxio.ui
+
+import android.os.Bundle
+import androidx.navigation.NavController
+import androidx.navigation.NavDestination
+
+/**
+ * A [NavController.OnDestinationChangedListener] that will call [callback] when moving between
+ * fragments only (not between dialogs or anything similar)
+ *
+ * @author Alexander Capehart (OxygenCobalt)
+ */
+class DialogAwareNavigationListener(private val callback: () -> Unit) :
+    NavController.OnDestinationChangedListener {
+    private var currentDestination: NavDestination? = null
+
+    /**
+     * Attach this instance to a [NavController]. This should be done in the onStart method of a
+     * Fragment.
+     *
+     * @param navController The [NavController] to add to.
+     */
+    fun attach(navController: NavController) {
+        currentDestination = null
+        navController.addOnDestinationChangedListener(this)
+    }
+
+    /**
+     * Remove this listener from it's [NavController]. This should be done in the onStop method of a
+     * Fragment.
+     *
+     * @param navController The [NavController] to remove from. Should be the same on used in
+     *   [attach].
+     */
+    fun release(navController: NavController) {
+        currentDestination = null
+        navController.removeOnDestinationChangedListener(this)
+    }
+
+    override fun onDestinationChanged(
+        controller: NavController,
+        destination: NavDestination,
+        arguments: Bundle?
+    ) {
+        // Drop the initial call by NavController that simply provides us with the current
+        // destination. This would cause the selection state to be lost every time the device
+        // rotates.
+        val lastDestination = currentDestination
+        currentDestination = destination
+        if (lastDestination == null) {
+            return
+        }
+
+        if (!lastDestination.isDialog() && !destination.isDialog()) {
+            callback()
+        }
+    }
+
+    /** This relies on special label naming used in-app. */
+    private fun NavDestination.isDialog() = label?.endsWith("dialog") == true
+}

app/src/main/res/navigation/inner.xml

@@ -7,7 +7,7 @@
     <fragment
         android:id="@+id/home_fragment"
         android:name="org.oxycblt.auxio.home.HomeFragment"
-        android:label="fragment_home"
+        android:label="home_fragment"
         tools:layout="@layout/fragment_home">
         <action
             android:id="@+id/search"
@@ -92,25 +92,25 @@
     <dialog
         android:id="@+id/album_sort_dialog"
         android:name="org.oxycblt.auxio.home.sort.AlbumSortDialog"
-        android:label="song_sort_dialog"
+        android:label="album_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <dialog
         android:id="@+id/artist_sort_dialog"
         android:name="org.oxycblt.auxio.home.sort.ArtistSortDialog"
-        android:label="song_sort_dialog"
+        android:label="artist_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <dialog
         android:id="@+id/genre_sort_dialog"
         android:name="org.oxycblt.auxio.home.sort.GenreSortDialog"
-        android:label="song_sort_dialog"
+        android:label="genre_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <dialog
         android:id="@+id/playlist_sort_dialog"
         android:name="org.oxycblt.auxio.home.sort.PlaylistSortDialog"
-        android:label="song_sort_dialog"
+        android:label="playlist_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <dialog
@@ -126,7 +126,7 @@
     <fragment
         android:id="@+id/search_fragment"
         android:name="org.oxycblt.auxio.search.SearchFragment"
-        android:label="SearchFragment"
+        android:label="search_fragment"
         tools:layout="@layout/fragment_search">
         <action
             android:id="@+id/show_song"
@@ -184,7 +184,7 @@
     <fragment
         android:id="@+id/album_detail_fragment"
         android:name="org.oxycblt.auxio.detail.AlbumDetailFragment"
-        android:label="AlbumDetailFragment"
+        android:label="album_detail_fragment"
         tools:layout="@layout/fragment_detail">
         <argument
             android:name="albumUid"
@@ -227,13 +227,13 @@
     <dialog
         android:id="@+id/album_song_sort_dialog"
         android:name="org.oxycblt.auxio.detail.sort.AlbumSongSortDialog"
-        android:label="AlbumSongSortDialog"
+        android:label="album_song_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <fragment
         android:id="@+id/artist_detail_fragment"
         android:name="org.oxycblt.auxio.detail.ArtistDetailFragment"
-        android:label="ArtistDetailFragment"
+        android:label="artist_detail_fragment"
         tools:layout="@layout/fragment_detail">
         <argument
             android:name="artistUid"
@@ -273,13 +273,13 @@
     <dialog
         android:id="@+id/artist_song_sort_dialog"
         android:name="org.oxycblt.auxio.detail.sort.ArtistSongSortDialog"
-        android:label="ArtistSongSortDialog"
+        android:label="artist_song_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <fragment
         android:id="@+id/genre_detail_fragment"
         android:name="org.oxycblt.auxio.detail.GenreDetailFragment"
-        android:label="GenreDetailFragment"
+        android:label="genre_detail_fragment"
         tools:layout="@layout/fragment_detail">
         <argument
             android:name="genreUid"
@@ -322,13 +322,13 @@
     <dialog
         android:id="@+id/genre_song_sort_dialog"
         android:name="org.oxycblt.auxio.detail.sort.GenreSongSortDialog"
-        android:label="GenreSongSortDialog"
+        android:label="genre_song_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <fragment
         android:id="@+id/playlist_detail_fragment"
         android:name="org.oxycblt.auxio.detail.PlaylistDetailFragment"
-        android:label="PlaylistDetailFragment"
+        android:label="playlist_detail_fragment"
         tools:layout="@layout/fragment_detail">
         <argument
             android:name="playlistUid"
@@ -374,7 +374,7 @@
     <dialog
         android:id="@+id/playlist_song_sort_dialog"
         android:name="org.oxycblt.auxio.detail.sort.PlaylistSongSortDialog"
-        android:label="PlaylistSongSortDialog"
+        android:label="playlist_song_sort_dialog"
         tools:layout="@layout/dialog_sort" />
 
     <dialog

app/src/main/res/navigation/outer.xml

@@ -8,7 +8,7 @@
     <fragment
         android:id="@+id/main_fragment"
         android:name="org.oxycblt.auxio.MainFragment"
-        android:label="fragment_main"
+        android:label="main_fragment"
         tools:layout="@layout/fragment_main">
         <action
             android:id="@+id/preferences"
@@ -20,7 +20,7 @@
     <fragment
         android:id="@+id/root_preferences_fragment"
         android:name="org.oxycblt.auxio.settings.RootPreferenceFragment"
-        android:label="fragment_settings">
+        android:label="settings_fragment">
         <action
             android:id="@+id/ui_preferences"
             app:destination="@id/ui_preferences_fragment" />
@@ -41,7 +41,7 @@
     <fragment
         android:id="@+id/ui_preferences_fragment"
         android:name="org.oxycblt.auxio.settings.categories.UIPreferenceFragment"
-        android:label="fragment_ui_preferences">
+        android:label="ui_preferences_fragment">
         <action
             android:id="@+id/accent_settings"
             app:destination="@id/accent_dialog" />
@@ -50,7 +50,7 @@
     <fragment
         android:id="@+id/personalize_preferences_fragment"
         android:name="org.oxycblt.auxio.settings.categories.PersonalizePreferenceFragment"
-        android:label="fragment_personalize_preferences">
+        android:label="personalize_preferences_fragment">
         <action
             android:id="@+id/tab_settings"
             app:destination="@id/tab_dialog" />
@@ -59,7 +59,7 @@
     <fragment
         android:id="@+id/music_preferences_fragment"
         android:name="org.oxycblt.auxio.settings.categories.MusicPreferenceFragment"
-        android:label="fragment_personalize_preferences">
+        android:label="personalize_preferences_fragment">
         <action
             android:id="@+id/separators_settings"
             app:destination="@id/separators_dialog" />
@@ -68,7 +68,7 @@
     <fragment
         android:id="@+id/audio_preferences_fragment"
         android:name="org.oxycblt.auxio.settings.categories.AudioPreferenceFragment"
-        android:label="fragment_personalize_preferences">
+        android:label="personalize_preferences_fragment">
         <action
             android:id="@+id/pre_amp_settings"
             app:destination="@id/pre_amp_dialog" />