-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Navigation] Introduce NavBackStackEntry#provideToCompositionLocals #175
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(btw, AS keeps telling me that it should indeed start with a lowercase letter😂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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#testSaveableValueInContentIsSaved
is 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