Merge pull request #73 from hotwired/navigator-ready-fixes

Fix crash/timing issues when the `Activity` is recreated

navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivity.kt

@@ -2,6 +2,7 @@ package dev.hotwire.navigation.activities
 
 import android.os.Bundle
 import androidx.appcompat.app.AppCompatActivity
+import dev.hotwire.navigation.navigator.Navigator
 import dev.hotwire.navigation.navigator.NavigatorConfiguration
 
 /**
@@ -11,8 +12,18 @@ abstract class HotwireActivity : AppCompatActivity() {
     lateinit var delegate: HotwireActivityDelegate
         private set
 
+    /**
+     * Provide a list of navigator configurations for the Activity. Configurations
+     * for all navigator instances available throughout the app should be provided here.
+     */
     abstract fun navigatorConfigurations(): List<NavigatorConfiguration>
 
+    /**
+     * Called when a navigator has been initialized and is ready for navigation. The
+     * root destination for the navigator has already been created.
+     */
+    open fun onNavigatorReady(navigator: Navigator) {}
+
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
         delegate = HotwireActivityDelegate(this)

navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivityDelegate.kt

@@ -2,7 +2,7 @@ package dev.hotwire.navigation.activities
 
 import androidx.activity.OnBackPressedCallback
 import androidx.annotation.IdRes
-import androidx.navigation.NavController
+import dev.hotwire.navigation.logging.logEvent
 import dev.hotwire.navigation.navigator.Navigator
 import dev.hotwire.navigation.navigator.NavigatorConfiguration
 import dev.hotwire.navigation.navigator.NavigatorHost
@@ -25,10 +25,6 @@ class HotwireActivityDelegate(val activity: HotwireActivity) {
     }
 
     private var currentNavigatorHostId = activity.navigatorConfigurations().first().navigatorHostId
-        set(value) {
-            field = value
-            updateOnBackPressedCallback(currentNavigatorHost.navController)
-        }
 
     /**
      * Initializes the Activity with a BackPressedDispatcher that properly
@@ -48,10 +44,14 @@ class HotwireActivityDelegate(val activity: HotwireActivity) {
      * Returns null if the navigator is not ready for navigation.
      */
     val currentNavigator: Navigator?
-        get() = if (currentNavigatorHost.isReady()) {
-            currentNavigatorHost.navigator
-        } else {
-            null
+        get() {
+            val navigator = navigatorHosts[currentNavigatorHostId]
+
+            return if (navigator?.isReady() == true) {
+                navigator.navigator
+            } else {
+                null
+            }
         }
 
 
@@ -61,20 +61,38 @@ class HotwireActivityDelegate(val activity: HotwireActivity) {
      *  you must update this whenever the current navigator changes.
      */
     fun setCurrentNavigator(configuration: NavigatorConfiguration) {
+        logEvent("navigatorSetAsCurrent", listOf("navigator" to configuration.name))
         currentNavigatorHostId = configuration.navigatorHostId
+
+        val navigatorHost = navigatorHosts[currentNavigatorHostId]
+        if (navigatorHost != null) {
+            updateOnBackPressedCallback(navigatorHost)
+        }
     }
 
     internal fun registerNavigatorHost(host: NavigatorHost) {
+        logEvent("navigatorRegistered", listOf("navigator" to host.navigator.configuration.name))
+
         if (navigatorHosts[host.id] == null) {
             navigatorHosts[host.id] = host
-            listenToDestinationChanges(host.navController)
+            listenToDestinationChanges(host)
+
+            if (currentNavigatorHostId == host.id) {
+                updateOnBackPressedCallback(host)
+            }
         }
     }
 
     internal fun unregisterNavigatorHost(host: NavigatorHost) {
+        logEvent("navigatorUnregistered", listOf("navigator" to host.navigator.configuration.name))
         navigatorHosts.remove(host.id)
     }
 
+    internal fun onNavigatorHostReady(host: NavigatorHost) {
+        logEvent("navigatorReady", listOf("navigator" to host.navigator.configuration.name))
+        activity.onNavigatorReady(host.navigator)
+    }
+
     /**
      * Finds the navigator host associated with the provided resource ID.
      *
@@ -101,18 +119,15 @@ class HotwireActivityDelegate(val activity: HotwireActivity) {
         navigatorHosts.forEach { it.value.navigator.reset() }
     }
 
-    private fun listenToDestinationChanges(navController: NavController) {
-        navController.addOnDestinationChangedListener { controller, _, _ ->
-            updateOnBackPressedCallback(controller)
+    private fun listenToDestinationChanges(host: NavigatorHost) {
+        host.navController.addOnDestinationChangedListener { controller, _, _ ->
+            updateOnBackPressedCallback(host)
         }
     }
 
-    private fun updateOnBackPressedCallback(navController: NavController) {
-        if (navController == currentNavigatorHost.navController) {
-            onBackPressedCallback.isEnabled = navController.previousBackStackEntry != null
+    private fun updateOnBackPressedCallback(host: NavigatorHost) {
+        if (host.id == currentNavigatorHostId) {
+            onBackPressedCallback.isEnabled = host.navController.previousBackStackEntry != null
         }
     }
-
-    private val currentNavigatorHost: NavigatorHost
-        get() = navigatorHost(currentNavigatorHostId)
 }

navigation-fragments/src/main/java/dev/hotwire/navigation/destinations/HotwireDestination.kt

@@ -19,6 +19,7 @@ import dev.hotwire.navigation.config.HotwireNavigation
 import dev.hotwire.navigation.fragments.HotwireFragmentDelegate
 import dev.hotwire.navigation.fragments.HotwireFragmentViewModel
 import dev.hotwire.navigation.navigator.Navigator
+import dev.hotwire.navigation.navigator.location
 import dev.hotwire.navigation.routing.Router
 
 /**
@@ -177,7 +178,4 @@ interface HotwireDestination : BridgeDestination {
     override fun bridgeWebViewIsReady(): Boolean {
         return navigator.session.isReady
     }
-
-    private val Bundle.location
-        get() = getString("location")
 }

navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireBottomSheetFragment.kt

@@ -21,12 +21,13 @@ import dev.hotwire.navigation.navigator.NavigatorHost
  */
 abstract class HotwireBottomSheetFragment : BottomSheetDialogFragment(),
     HotwireDestination, HotwireDialogDestination {
-    override lateinit var navigator: Navigator
     internal lateinit var delegate: HotwireFragmentDelegate
 
+    override val navigator: Navigator
+        get() = (parentFragment as NavigatorHost).navigator
+
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
-        navigator = (parentFragment as NavigatorHost).navigator
         delegate = HotwireFragmentDelegate(this)
     }
 

navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragment.kt

@@ -22,12 +22,13 @@ import dev.hotwire.navigation.session.SessionModalResult
  * For web fragments, refer to [HotwireWebFragment].
  */
 abstract class HotwireFragment : Fragment(), HotwireDestination {
-    override lateinit var navigator: Navigator
     internal lateinit var delegate: HotwireFragmentDelegate
 
+    override val navigator: Navigator
+        get() = (parentFragment as NavigatorHost).navigator
+
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
-        navigator = (parentFragment as NavigatorHost).navigator
         delegate = HotwireFragmentDelegate(this)
     }
 

navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragmentDelegate.kt

@@ -2,6 +2,7 @@ package dev.hotwire.navigation.fragments
 
 import dev.hotwire.navigation.logging.logEvent
 import dev.hotwire.navigation.destinations.HotwireDestination
+import dev.hotwire.navigation.navigator.navigatorName
 import dev.hotwire.navigation.session.SessionModalResult
 import dev.hotwire.navigation.session.SessionViewModel
 import dev.hotwire.navigation.util.displayBackButton
@@ -15,10 +16,11 @@ import dev.hotwire.navigation.util.displayBackButtonAsCloseIcon
 class HotwireFragmentDelegate(private val navDestination: HotwireDestination) {
     private val fragment = navDestination.fragment
     private val location = navDestination.location
-    private val navigator = navDestination.navigator
+    private val navigatorName = requireNotNull(fragment.arguments?.navigatorName)
+    private val navigator get() = navDestination.navigator
 
     internal val sessionViewModel = SessionViewModel.get(
-        sessionName = navigator.configuration.name,
+        sessionName = navigatorName,
         activity = fragment.requireActivity()
     )
     internal val fragmentViewModel = HotwireFragmentViewModel.get(

navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt

@@ -45,7 +45,7 @@ internal class HotwireWebFragmentDelegate(
     private var screenshotOrientation = 0
     private var screenshotZoomed = false
     private var currentlyZoomed = false
-    private val navigator = navDestination.navigator
+    private val navigator get() = navDestination.navigator
     private val session get() = navigator.session
     private val turboView get() = callback.hotwireView
     private val viewTreeLifecycleOwner get() = turboView?.findViewTreeLifecycleOwner()

navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/Navigator.kt

@@ -121,6 +121,7 @@ class Navigator(
             navOptions = navOptions(location, options.action),
             extras = extras,
             pathConfiguration = Hotwire.config.pathConfiguration,
+            navigatorName = configuration.name,
             controller = currentControllerForLocation(location)
         )
 
@@ -391,10 +392,7 @@ class Navigator(
     }
 
     private val NavBackStackEntry?.isModalContext: Boolean
-        get() {
-            val context = this?.arguments?.getSerializable("presentation-context")
-            return context as? PresentationContext == PresentationContext.MODAL
-        }
+        get() = this?.arguments?.presentationContext == PresentationContext.MODAL
 
     private fun logEvent(event: String, vararg params: Pair<String, Any>) {
         val attributes = params.toMutableList().apply {

navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorArguments.kt

@@ -0,0 +1,26 @@
+package dev.hotwire.navigation.navigator
+
+import android.annotation.SuppressLint
+import android.os.Bundle
+import dev.hotwire.core.turbo.nav.PresentationContext
+import dev.hotwire.navigation.logging.logError
+import kotlin.text.uppercase
+
+internal const val ARG_LOCATION = "location"
+internal const val ARG_NAVIGATOR_NAME = "navigator-name"
+internal const val ARG_PRESENTATION_CONTEXT = "presentation-context"
+
+internal val Bundle.location
+    get() = getString(ARG_LOCATION)
+
+internal val Bundle.navigatorName
+    get() = getString(ARG_NAVIGATOR_NAME)
+
+internal val Bundle.presentationContext
+    @SuppressLint("DefaultLocale") get() = try {
+        val value = getString(ARG_PRESENTATION_CONTEXT) ?: "default"
+        PresentationContext.valueOf(value.uppercase())
+    } catch (e: IllegalArgumentException) {
+        logError("unknownPresentationContext", e)
+        PresentationContext.DEFAULT
+    }

navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorGraphBuilder.kt

@@ -22,6 +22,7 @@ import kotlin.reflect.KClass
 import kotlin.reflect.full.isSubclassOf
 
 internal class NavigatorGraphBuilder(
+    private val navigatorName: String,
     private val startLocation: String,
     private val navController: NavController,
     private val pathConfiguration: PathConfiguration
@@ -69,10 +70,14 @@ internal class NavigatorGraphBuilder(
                 }
             }
 
-            argument("location") {
+            argument(ARG_LOCATION) {
                 defaultValue = startLocation
             }
 
+            argument(ARG_NAVIGATOR_NAME) {
+                defaultValue = navigatorName
+            }
+
             // Use a random value to represent a unique instance of the graph, so the
             // graph is unique every time. This lets it be reset/recreated on-demand from
             // `NavigatorHost.reset()`. Replacing an existing nav graph with

navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt

@@ -1,13 +1,17 @@
 package dev.hotwire.navigation.navigator
 
 import android.os.Bundle
+import android.view.View
+import androidx.fragment.app.Fragment
+import androidx.fragment.app.FragmentManager
+import androidx.fragment.app.FragmentOnAttachListener
 import androidx.navigation.fragment.NavHostFragment
 import androidx.navigation.fragment.findNavController
 import dev.hotwire.core.config.Hotwire
 import dev.hotwire.navigation.activities.HotwireActivity
 import dev.hotwire.navigation.config.HotwireNavigation
 
-open class NavigatorHost : NavHostFragment() {
+open class NavigatorHost : NavHostFragment(), FragmentOnAttachListener {
     internal lateinit var activity: HotwireActivity
     lateinit var navigator: Navigator
         private set
@@ -16,15 +20,25 @@ open class NavigatorHost : NavHostFragment() {
         super.onCreate(savedInstanceState)
 
         activity = requireActivity() as HotwireActivity
-        activity.delegate.registerNavigatorHost(this)
         navigator = Navigator(this, configuration)
+        childFragmentManager.addFragmentOnAttachListener(this)
 
         initControllerGraph()
     }
 
+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        super.onViewCreated(view, savedInstanceState)
+        activity.delegate.registerNavigatorHost(this)
+    }
+
+    override fun onAttachFragment(fragmentManager: FragmentManager, fragment: Fragment) {
+        activity.delegate.onNavigatorHostReady(this)
+        childFragmentManager.removeFragmentOnAttachListener(this)
+    }
+
     override fun onDestroy() {
-        super.onDestroy()
         activity.delegate.unregisterNavigatorHost(this)
+        super.onDestroy()
     }
 
     /**
@@ -39,6 +53,7 @@ open class NavigatorHost : NavHostFragment() {
     internal fun initControllerGraph() {
         navController.apply {
             graph = NavigatorGraphBuilder(
+                navigatorName = configuration.name,
                 startLocation = configuration.startLocation,
                 pathConfiguration = Hotwire.config.pathConfiguration,
                 navController = findNavController()

navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorRule.kt

@@ -33,6 +33,7 @@ internal class NavigatorRule(
     navOptions: NavOptions,
     extras: FragmentNavigator.Extras?,
     pathConfiguration: PathConfiguration,
+    navigatorName: String,
     val controller: NavController
 ) {
     val defaultUri = HotwireDestinationDeepLink.from(HotwireNavigation.defaultFragmentDestination).uri.toUri()
@@ -45,6 +46,7 @@ internal class NavigatorRule(
     val isAtStartDestination = controller.previousBackStackEntry == null
 
     // New destination
+    val newNavigatorName = navigatorName
     val newLocation = location
     val newProperties = pathConfiguration.properties(newLocation)
     val newPresentationContext = newProperties.context
@@ -156,8 +158,9 @@ internal class NavigatorRule(
     private fun Bundle?.withNavArguments(): Bundle {
         val bundle = this ?: bundleOf()
         return bundle.apply {
-            putString("location", newLocation)
-            putSerializable("presentation-context", newPresentationContext)
+            putString(ARG_LOCATION, newLocation)
+            putString(ARG_NAVIGATOR_NAME, newNavigatorName)
+            putString(ARG_PRESENTATION_CONTEXT, newPresentationContext.name)
         }
     }
 

navigation-fragments/src/main/java/dev/hotwire/navigation/util/NavigationExtensions.kt

@@ -9,6 +9,7 @@ import androidx.appcompat.widget.Toolbar
 import androidx.core.content.ContextCompat
 import androidx.navigation.NavBackStackEntry
 import dev.hotwire.navigation.R
+import dev.hotwire.navigation.navigator.location
 
 fun Toolbar.displayBackButton() {
     navigationIcon = ContextCompat.getDrawable(context, R.drawable.ic_back)
@@ -19,7 +20,7 @@ fun Toolbar.displayBackButtonAsCloseIcon() {
 }
 
 internal val NavBackStackEntry?.location: String?
-    get() = this?.arguments?.getString("location")
+    get() = this?.arguments?.location
 
 internal fun Context.colorFromThemeAttr(
     @AttrRes attrColor: Int,