Stable DestinationsNavigator?

[Skeletonxf]

Hi, I'm wondering if it is possible to add a @Stable annotation to the DestinationsNavigator interface. After reading this article https://multithreaded.stitchfix.com/blog/2022/08/05/jetpack-compose-recomposition/ and reviewing the skipping rules https://developer.android.com/jetpack/compose/lifecycle#skipping I think the same issue as described in that article for view models applies to compose destinations.

Namely some code like

@Composable
fun FooScreen(navigator: DestinationsNavigator) {
  FooContents(onBack = { navigator.popBackStack() })
}
@Composable
fun FooContents(onBack: () -> Unit) { .. }

may miss out on potential skipping optimisations because the onBack lambda is capturing the DestinationsNavigator.

The Now In Android sample app also wraps the NavHostController in a @Stable class: https://github.com/android/nowinandroid/blob/main/app/src/main/java/com/google/samples/apps/nowinandroid/ui/NiaAppState.kt#L56

I think the contract should be possible to meet here as the DestinationsNavigator doesn't have any properties so it's just the equals contract that needs meeting.

A stable type must comply with the following contract:

• The result of equals for two instances will forever be the same for the same two instances.
• If a public property of the type changes, Composition will be notified.
• All public property types are also stable.

Compose considers a type stable only if it can prove it. For example, an interface is generally treated as not stable

[Nek-12]

Yeah I actually made my own wrapper for the destinations navigator in a multimodule project, and it's marked as stable and working perfectly fine.

So the workaround may be to wrap the navigator in your own interface.

[raamcosta]

Hi @Skeletonxf ! 👋

First of all, have you actually noticed unexpected recompositions? I cannot test myself atm, but I remember checking this and I had no unexpected recompositions 🤔

[Skeletonxf]

On the app I've actually been building no, we've not been measuring recompositions and/as there hasn't been any performance issues.

However, I can adapt the example in the article I shared to reproduce the 'unstable lambda' issue with DestinationsNavigator

class MainActivity : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContent {
            RecomposeTestTheme {
                // A surface container using the 'background' color from the theme
                Surface(
                    modifier = Modifier.fillMaxSize(),
                    color = MaterialTheme.colors.background
                ) {
                    DestinationsNavHost(navGraph = NavGraphs.root)
                }
            }
        }
    }
}

class RecompositionTestViewModel: ViewModel() {
    val state = MutableStateFlow(listOf("0", "1"))

    fun addName() {
        state.getAndUpdate { s -> s.plus(s.size.toString()) }
    }

    fun handleNameClick() {
        println("Clicked")
    }
}

@Composable
@Destination(start = true)
fun RecompositionTest(
    navigator: DestinationsNavigator
) {
    val viewModel = remember { RecompositionTestViewModel() }
    val state by viewModel.state.collectAsState()

    NameColumnWithButton(
        names = state,
        onButtonClick = viewModel::addName,
        onNameClick = viewModel::handleNameClick,
        onBackClick = { navigator.popBackStack() },
    )
}

@Composable
fun NameColumnWithButton(
    names: List<String>,
    onButtonClick: () -> Unit,
    onNameClick: () -> Unit,
    onBackClick: () -> Unit,
) {
    Column {
        names.forEach {
            CompositionTrackingName(name = it, onClick = onBackClick)
        }
        Button(onClick = onButtonClick) { Text("Add a Name") }
    }
}

@Composable
fun CompositionTrackingName(name: String, onClick: () -> Unit) {
    Log.e("*******COMPOSED", name)
    Text(name, modifier = Modifier.clickable(onClick = onClick))
}

Switching the onClick argument for CompositionTrackingName to the view model's method reference or a method reference for the DestinationsNavigator (navigator::popBackStack) ensures the expected log output where adding a new name only composes that new CompositionTrackingName. However, with no modifications the code in the snippet produces a log for every name in the list on each click of the button. ie clicking Add a name when there's 5 already logs 0 through 5.

[Nek-12]

The issue here is the List not being stable, and not the navigator itself, I think

[Skeletonxf]

The logging for compositions is for the CompositionTrackingName which doesn't take the List as an argument though.

[raamcosta]

But also doesn’t take the navigator, right?

[Nek-12]

@raamcosta It does, through an argument of an object that implements the Function1 interface.

@Skeletonxf But I also think that compose is not smart enough to understand that CompositionTrackingName's it argument which is part of the forEach block is not part of the List. I feel like we should inspect a compose compiler report instead of continuing this discussion.

[raamcosta]

@Skeletonxf thanks for the example. I got some time to try it out myself 🙂

Basically, it seems also if I use navigator::popBackStack, it fixes the issue. But if I annotate DestinationsNavigator with @Stable that also does the trick. Same thing for NavController and ViewModel. Since they're not annotated either, if I use method reference it avoids some compositions. This is very interesting thing to know, thanks a lot!

I don't see an issue with annotating DestinationsNavigator as Stable. It doesn't have any public fields, so the last two points are fine, and implementations don't override equals which means it defaults to reference comparison. So for two instances calling equals on them will always wield the same results for sure.

So you can count on it in the next release, probably some time during the weekend.

[ajhuntsman]

Thanks @raamcosta for making this enhancement! We're optimizing our Composables right now and 1.7.22-beta changed a ton of items in our Compose Compiler metrics report from "unstable" to "stable".