google / horologist

Horologist is a group of libraries that aim to supplement Wear OS developers with features that are commonly required by developers but not yet available.
https://google.github.io/horologist/
Apache License 2.0
575 stars 95 forks source link

Using Horologist wrapper for SwipeDismissableNavHost: Cannot find startDestination kotlin.String from NavGraph. #2352

Closed SamLeatherdale closed 3 months ago

SamLeatherdale commented 3 months ago

If I use com.google.android.horologist.compose.nav.SwipeDismissableNavHost from Horologist (v0.6.17), instead of androidx.wear.compose.navigation.SwipeDismissableNavHost, then I get the following error:

SwipeDismissableNavHost(
    navController = navHostController,
    startDestination = "home",
) {
    composable(route = "home") {
        // component
    }
}
IllegalStateException: Cannot find startDestination kotlin.String from NavGraph. Ensure the starting NavDestination was added with route from KClass.

I take it that the Horologist wrapper is now not necessary? In that case should it be deprecated?

However, from doing a bit of debugging, it seems to be caused by a mismatch between the ID generation used by the creation of the NavDestination of the route, and then the lookup of the route using the startDestination.

The ID for the NavDestination is originally created using:

val internalRoute = createRoute(route)
id = internalRoute.hashCode()

But then, in NavGraph, a node is looked up using the following ID:

val id = serializer.generateHashCode()
val startDest = findNode(id)

The Kotlin string serializer and the built in object hashCode produce different values for the same string, which can be demonstrated with the following simple Kotlin script:

val string = "abc"
println(string.hashCode())

@OptIn(InternalSerializationApi::class)
println(string::class.serializer().generateHashCode())

Output

96354
366142910

This is why it is unable to find the NavDestination referenced by the route.

However I'm not sure why this works fine if we use the AndroidX version of SwipeDismissableNavHost directly.

yschimke commented 3 months ago

That wrapper is intended as a TypeSafe navigation wrapper. https://developer.android.com/guide/navigation/design/type-safety

If you want to continue to use String routes, then you don't need to use this component. Apologies, I'll add the missing documentation and guide.

yschimke commented 3 months ago

Apologies for the confusion, would these docs clear it up?

https://github.com/google/horologist/pull/2356

SamLeatherdale commented 3 months ago

Hi, thanks for getting back to me about this.

So just to clarify, this type safe navigation wrapper doesn't work for Strings, because String::class.serializer().generateHashCode() will always result in the same value for any value of String?

Turns out I have actually been using a similar pattern to the type safe navigation already using data objects to build my routes, however I still fall back to using string routes.

sealed class MainActivityRoute(val route: String) {
    data object Main : MainActivityRoute("main")
    data object TripList : MainActivityRoute("tripList")
    data object TripDetails : MainActivityRoute("tripDetails") {
        fun buildRoute(tripId: String): String {
            return "$route/$tripId"
        }
    }
}

However, I'm not quite sure how to use these objects directly with the Horologist wrapper (com.google.android.horologist.compose.nav.SwipeDismissableNavHost). If I try like this, I get the following error No type arguments expected for fun NavGraphBuilder.composable:

com.google.android.horologist.compose.nav.SwipeDismissableNavHost(
    navController = navHostController,
    startDestination = MainActivityRoute.Main,
) {
    composable<MainActivityRoute.Main> {
       ScreenScaffold {}
    }
}

My androidx.wear.compose:compose-navigation version is 1.4.0-beta03

SamLeatherdale commented 3 months ago

Ahh, I figured it out, all I had to do was import the type safe helper from Horologist:

import com.google.android.horologist.compose.nav.composable
SamLeatherdale commented 3 months ago

@yschimke I've now been migrating all of my NavGraphs over to using Type Safe navigation, however I've run into a bit of a blocker.

Would you be able to shed any light on this question? https://stackoverflow.com/questions/78489838/unable-to-get-route-object-from-currentbackstackentry-in-compose-navigation-outs

How can we use currentBackStackEntry?.toRoute<ParentRoute>() to deserialize an unknown subclass of ParentRoute? Having to resort to reflection using the class name is a nasty hack.

yschimke commented 3 months ago

I think from these it's unsupported

This seems clearest https://issuetracker.google.com/issues/343827261

https://issuetracker.google.com/issues/353880546 https://issuetracker.google.com/issues/346834135

You are probably fighting the design of navigation, even if you and I might not agree with it.

yschimke commented 3 months ago

If you are ok with indirection, I suspect you can put it in a wrapper type that forces the polymorphic serialisation to work

SamLeatherdale commented 3 months ago

I would be happy to give that a try, anything is better than my current solution based on using the class name at runtime, also requiring me to add the class name to ProGuard.

Any suggestions on how to get started with the wrapper class? I'm not quite sure what to be looking for, something with a custom Serialisation implementation?

yschimke commented 3 months ago

Nope, I tried it but it seems tough without control of a custom serializer module. Sorry