[Navigation] Introduce NavBackStackEntry#provideToCompositionLocals #175
Conversation
aa92ff4
to
669c145
Compare
...on/navigation-compose/src/main/java/androidx/navigation/compose/NavBackStackEntryProvider.kt
Show resolved
Hide resolved
...on-compose/src/androidTest/java/androidx/navigation/compose/NavBackStackEntryProviderTest.kt
Outdated
Show resolved
Hide resolved
…that provides the NavBackStackEntry to the composition locals needed for all Compose-related navigators. Test: NavBackStackEntryProviderTest Fixes: b/187229439 Change-Id: If6af74ec67fbedef2cf5d623e9fc44ab6bf76e39
…als' `saveableStateHolder` Change-Id: I0525a524e8373d9d3d4f7bb11a3c71046a74f64a
…y createActiveBackStackEntry hack Change-Id: I8b7101292e107dded8426a7e38f100451720ac95
3c1bb7b
to
3755c15
Compare
...on-compose/src/androidTest/java/androidx/navigation/compose/NavBackStackEntryProviderTest.kt
Outdated
Show resolved
Hide resolved
Change-Id: I8b8c3c03b190e083928d013d82495bc98a9c5a6f
…lue isn't saved and restored Change-Id: Ib31cc443174d18912e55ac64e354f6721c4b1bb1
|
It looks like the |
Change-Id: I0738fe193a587d057ac88e0766ccf09556681efd
|
Oh yeah, sorry for that! |
...on/navigation-compose/src/main/java/androidx/navigation/compose/NavBackStackEntryProvider.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| @SuppressLint("ComposableNaming") | ||
| @Composable | ||
| public fun NavBackStackEntry.provideToCompositionLocals( |
There was a problem hiding this comment.
this function name doesn't follow our api guidelines:
the name of the composable function which returns unit should start with a capital letter and be a noun. even if it is an extension/member function on other class.
similarly to how we have SaveableStateHolder.SaveableStateProvider()
I don't know what is the best name, maybe Ian would have some ideas. something like:
NavBackStackEntry.LocalOwnersProvider()
There was a problem hiding this comment.
Naming is hard :D LocalOwnersProvider isn't that descriptive to me, what do you think of NavBackStackEntry. CompositionLocalProvider? (What Ian originally suggested, just pascal case) @ianhanniballake
There was a problem hiding this comment.
(btw, AS keeps telling me that it should indeed start with a lowercase letter😂)
There was a problem hiding this comment.
Yeah, I personally liked the scoping of making this an extension on NavBackStackEntry (vs the more obvious top level function of CompositionLocalProvider(NavBackStackEntry, SaveableStateHolder, @Composable () -> Unit)) and yet, the capital lettering feels very odd when it comes to using it with extension functions.
There was a problem hiding this comment.
Yeah, I agree - Pascal case extensions are a little weird. I can live with both options but camel case feels more natural to me
There was a problem hiding this comment.
I agree that on extension function it looks more strange, but the current guidelines are not having exceptions for extension/member functions.
with LocalOwners I tried to describe what composition locals would be provided, as with just CompositionLocalProvider name it doesn't explain that. but maybe what exactly we provide is an implementation detail, so ok to me
There was a problem hiding this comment.
This method would be something that every custom Navigator would use (that's why this needs to be public API).
There was a problem hiding this comment.
not blocking this pull-request but just out of my curiosity: what does it mean to have a custom Navigation? if I write my own navigation implementation how will I construct NavBackStackEntry objects?
There was a problem hiding this comment.
No, a custom Navigator, not custom Navigation. NavController was built in a such a way that it knows nothing about Fragments, Compose, Views, Dialogs, etc.
It is the individual Navigator instances that are added to the NavController that provide that functionality - ComposeNavigator is just a first party implementation of the public API surface, just like the implementation in #173 is a Navigator specifically designed for modal bottom sheets. They can be mixed and matched and NavController is the one who multiplexes them correctly and controls the creation of NavBackStackEntry instances for each Navigator.
…re with Compose API guidelines Change-Id: I2bd33a90236033933168411e70c6fbdb77e0abfe
…aveableStateHolder parameter non-optional Also improved the docs to make clear that the SaveableStateHolder should be hoisted. Change-Id: Ie918a80042ba3192cb04114ed9aa8fed0a0e2beb
91a06b1
to
a8100a0
Compare
…that provides the NavBackStackEntry to the composition locals needed for all Compose-related navigators.
Relnote: Introduced a new NavBackStackEntry#provideToCompositionLocals that provides the NavBackStackEntry to the relevant composition locals.
Testing
Test: NavBackStackEntryProviderTest
NavBackStackEntryProviderTest#testSaveableValueInContentIsSavedis pretty much just copied from RememberSaveableTest and as always, I'll be very happy about suggestions on improving the tests and additional tests!Issues Fixed
Fixes: b/187229439