From 6773fb2d567ff80892a21cd62045fafea29ae15f Mon Sep 17 00:00:00 2001 From: Jay Ohms Date: Wed, 27 Nov 2024 06:34:32 -0500 Subject: [PATCH 1/2] Fix lifecycle crash/timing issues when the Activity is recreated (such as during config changes) --- .../navigation/activities/HotwireActivity.kt | 11 ++++ .../activities/HotwireActivityDelegate.kt | 53 ++++++++++++------- .../destinations/HotwireDestination.kt | 4 +- .../fragments/HotwireBottomSheetFragment.kt | 5 +- .../navigation/fragments/HotwireFragment.kt | 5 +- .../fragments/HotwireFragmentDelegate.kt | 6 ++- .../fragments/HotwireWebFragmentDelegate.kt | 2 +- .../hotwire/navigation/navigator/Navigator.kt | 6 +-- .../navigator/NavigatorArguments.kt | 26 +++++++++ .../navigator/NavigatorGraphBuilder.kt | 7 ++- .../navigation/navigator/NavigatorHost.kt | 21 ++++++-- .../navigation/navigator/NavigatorRule.kt | 7 ++- .../navigation/util/NavigationExtensions.kt | 3 +- 13 files changed, 116 insertions(+), 40 deletions(-) create mode 100644 navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorArguments.kt diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivity.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivity.kt index 3735c4e..b0804e7 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivity.kt +++ b/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 + /** + * 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) diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivityDelegate.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivityDelegate.kt index 1240cc1..e237a27 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/activities/HotwireActivityDelegate.kt +++ b/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) } diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/destinations/HotwireDestination.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/destinations/HotwireDestination.kt index ff593bf..d512130 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/destinations/HotwireDestination.kt +++ b/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") } diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireBottomSheetFragment.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireBottomSheetFragment.kt index 85c5db8..5b0aa36 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireBottomSheetFragment.kt +++ b/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) } diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragment.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragment.kt index d7e394a..f38ffc7 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragment.kt +++ b/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) } diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragmentDelegate.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragmentDelegate.kt index fbd0e01..1468b24 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireFragmentDelegate.kt +++ b/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( diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt index 7a8a0a8..e128a54 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt +++ b/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() diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/Navigator.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/Navigator.kt index daa7f66..0f65180 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/Navigator.kt +++ b/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) { val attributes = params.toMutableList().apply { diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorArguments.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorArguments.kt new file mode 100644 index 0000000..2b3b42b --- /dev/null +++ b/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 + } \ No newline at end of file diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorGraphBuilder.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorGraphBuilder.kt index f505966..7c9a848 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorGraphBuilder.kt +++ b/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 diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt index 782f8b1..56de95f 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorHost.kt +++ b/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() diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorRule.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorRule.kt index 07aca06..86dcd13 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/navigator/NavigatorRule.kt +++ b/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) } } diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/util/NavigationExtensions.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/util/NavigationExtensions.kt index d81511e..4624dd9 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/util/NavigationExtensions.kt +++ b/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, From 87280b4f77ce040376ef1857cfb83a8c4ad29143 Mon Sep 17 00:00:00 2001 From: Jay Ohms Date: Wed, 27 Nov 2024 06:39:06 -0500 Subject: [PATCH 2/2] Fix tests --- .../navigation/navigator/NavigatorRuleTest.kt | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorRuleTest.kt b/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorRuleTest.kt index f89c936..40ac9e9 100644 --- a/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorRuleTest.kt +++ b/navigation-fragments/src/test/kotlin/dev/hotwire/navigation/navigator/NavigatorRuleTest.kt @@ -52,6 +52,7 @@ class NavigatorRuleTest { private val webModalUri = Uri.parse("hotwire://fragment/web/modal") private val webHomeUri = Uri.parse("hotwire://fragment/web/home") + private val navigatorName = "test" private val extras = null private val navOptions = navOptions { anim { @@ -387,12 +388,19 @@ class NavigatorRuleTest { bundle: Bundle? = null ): NavigatorRule { return NavigatorRule( - location, visitOptions, bundle, navOptions, extras, pathConfiguration, controller + location = location, + visitOptions = visitOptions, + bundle = bundle, + navOptions = navOptions, + extras = extras, + pathConfiguration = pathConfiguration, + navigatorName = navigatorName, + controller = controller ) } private fun locationArgs(location: String): Bundle { - return bundleOf("location" to location) + return bundleOf(ARG_LOCATION to location) } private fun buildControllerWithGraph(): TestNavHostController { @@ -421,7 +429,8 @@ class NavigatorRuleTest { navigator = provider.getNavigator("test"), id = webHomeDestinationId ).apply { - argument("location") { defaultValue = homeUrl } + argument(ARG_LOCATION) { defaultValue = homeUrl } + argument(ARG_NAVIGATOR_NAME) { defaultValue = navigatorName } deepLink(webHomeUri.toString()) } )