Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
+256 −41
Closed

[Navigation] Introduce NavBackStackEntry#provideToCompositionLocals #175

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
be27bc8
Introduce new NavBackStackEntry#provideToCompositionLocals extension …
jossiwolf May 5, 2021
be4bf48
Provide a default value for NavBackStackEntry#provideToCompositionLoc…
jossiwolf May 6, 2021
3755c15
Use TestNavigatorState to set up NavBackStackEntry instead of the ugl…
jossiwolf May 6, 2021
6dc8b2c
Extract back stack entry creation into test-private method
jossiwolf May 6, 2021
2d31918
Add testNonSaveableValueInContentIsNotSaved to save a non-saveable va…
jossiwolf May 6, 2021
f57fd49
Update API after providing default param
jossiwolf May 6, 2021
8c2a3d7
Rename provideToCompositionLocals to CompositionLocalProvider to adhe…
jossiwolf May 6, 2021
a8100a0
Rename CompositionLocalProvider to LocalOwnersProvider and make its s…
jossiwolf May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Provide a default value for NavBackStackEntry#provideToCompositionLoc… 
…als' `saveableStateHolder`

Change-Id: I0525a524e8373d9d3d4f7bb11a3c71046a74f64a
  • Loading branch information
@jossiwolf
jossiwolf committed May 6, 2021
commit be4bf48a9339068b74021487502bb47c76e79654
3 changes: 2 additions & 1 deletion ...navigation-compose/src/main/java/androidx/navigation/compose/NavBackStackEntryProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import android.annotation.SuppressLint
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.saveable.SaveableStateHolder
import androidx.compose.runtime.saveable.rememberSaveableStateHolder
import androidx.compose.ui.platform.LocalLifecycleOwner
import androidx.compose.ui.platform.LocalSavedStateRegistryOwner
import androidx.lifecycle.SavedStateHandle
Expand All @@ -41,7 +42,7 @@ import java.util.UUID
@SuppressLint("ComposableNaming")
@Composable
public fun NavBackStackEntry.provideToCompositionLocals(
Copy link
Contributor

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()

Copy link
Contributor Author

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

Copy link
Contributor Author

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😂)

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Member

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).

Copy link
Contributor

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?

Copy link
Member

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.

saveableStateHolder: SaveableStateHolder,
saveableStateHolder: SaveableStateHolder = rememberSaveableStateHolder(),
content: @Composable () -> Unit
) {
CompositionLocalProvider(
Expand Down