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

Show file tree
Hide file tree
Changes from 3 commits
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
4 changes: 4 additions & 0 deletions navigation/navigation-compose/api/current.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ package androidx.navigation.compose {
method @androidx.navigation.NavDestinationDsl public static androidx.navigation.compose.NamedNavArgument navArgument(String name, kotlin.jvm.functions.Function1<? super androidx.navigation.NavArgumentBuilder,kotlin.Unit> builder);
}

public final class NavBackStackEntryProviderKt {
method @androidx.compose.runtime.Composable public static void provideToCompositionLocals(androidx.navigation.NavBackStackEntry, androidx.compose.runtime.saveable.SaveableStateHolder saveableStateHolder, kotlin.jvm.functions.Function0<kotlin.Unit> content);
}

public final class NavGraphBuilderKt {
method public static void composable(androidx.navigation.NavGraphBuilder, String route, optional java.util.List<androidx.navigation.compose.NamedNavArgument> arguments, optional java.util.List<androidx.navigation.NavDeepLink> deepLinks, kotlin.jvm.functions.Function1<? super androidx.navigation.NavBackStackEntry,kotlin.Unit> content);
method public static void navigation(androidx.navigation.NavGraphBuilder, String startDestination, String route, kotlin.jvm.functions.Function1<? super androidx.navigation.NavGraphBuilder,kotlin.Unit> builder);
Expand Down
4 changes: 4 additions & 0 deletions navigation/navigation-compose/api/public_plus_experimental_current.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ package androidx.navigation.compose {
method @androidx.navigation.NavDestinationDsl public static androidx.navigation.compose.NamedNavArgument navArgument(String name, kotlin.jvm.functions.Function1<? super androidx.navigation.NavArgumentBuilder,kotlin.Unit> builder);
}

public final class NavBackStackEntryProviderKt {
method @androidx.compose.runtime.Composable public static void provideToCompositionLocals(androidx.navigation.NavBackStackEntry, androidx.compose.runtime.saveable.SaveableStateHolder saveableStateHolder, kotlin.jvm.functions.Function0<kotlin.Unit> content);
}

public final class NavGraphBuilderKt {
method public static void composable(androidx.navigation.NavGraphBuilder, String route, optional java.util.List<androidx.navigation.compose.NamedNavArgument> arguments, optional java.util.List<androidx.navigation.NavDeepLink> deepLinks, kotlin.jvm.functions.Function1<? super androidx.navigation.NavBackStackEntry,kotlin.Unit> content);
method public static void navigation(androidx.navigation.NavGraphBuilder, String startDestination, String route, kotlin.jvm.functions.Function1<? super androidx.navigation.NavGraphBuilder,kotlin.Unit> builder);
Expand Down
4 changes: 4 additions & 0 deletions navigation/navigation-compose/api/restricted_current.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ package androidx.navigation.compose {
method @androidx.navigation.NavDestinationDsl public static androidx.navigation.compose.NamedNavArgument navArgument(String name, kotlin.jvm.functions.Function1<? super androidx.navigation.NavArgumentBuilder,kotlin.Unit> builder);
}

public final class NavBackStackEntryProviderKt {
method @androidx.compose.runtime.Composable public static void provideToCompositionLocals(androidx.navigation.NavBackStackEntry, androidx.compose.runtime.saveable.SaveableStateHolder saveableStateHolder, kotlin.jvm.functions.Function0<kotlin.Unit> content);
}

public final class NavGraphBuilderKt {
method public static void composable(androidx.navigation.NavGraphBuilder, String route, optional java.util.List<androidx.navigation.compose.NamedNavArgument> arguments, optional java.util.List<androidx.navigation.NavDeepLink> deepLinks, kotlin.jvm.functions.Function1<? super androidx.navigation.NavBackStackEntry,kotlin.Unit> content);
method public static void navigation(androidx.navigation.NavGraphBuilder, String startDestination, String route, kotlin.jvm.functions.Function1<? super androidx.navigation.NavGraphBuilder,kotlin.Unit> builder);
Expand Down
151 changes: 151 additions & 0 deletions ...compose/src/androidTest/java/androidx/navigation/compose/NavBackStackEntryProviderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
/*
* Copyright 2021 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.navigation.compose

import androidx.compose.runtime.saveable.rememberSaveable
import androidx.compose.runtime.saveable.rememberSaveableStateHolder
import androidx.compose.ui.platform.LocalLifecycleOwner
import androidx.compose.ui.platform.LocalSavedStateRegistryOwner
import androidx.compose.ui.test.junit4.StateRestorationTester
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.ViewModelStoreOwner
import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner
import androidx.navigation.testing.TestNavigatorState
import androidx.savedstate.SavedStateRegistryOwner
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.testutils.TestNavigator
import com.google.common.truth.Truth.assertThat
import com.google.common.truth.Truth.assertWithMessage
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith

@RunWith(AndroidJUnit4::class)
class NavBackStackEntryProviderTest {

@get:Rule
val composeTestRule = createComposeRule()

@Test
fun testViewModelStoreOwnerProvided() {
val testNavigator = TestNavigator()
val testNavigatorState = TestNavigatorState()
testNavigator.onAttach(testNavigatorState)
val backStackEntry = testNavigatorState.createBackStackEntry(
testNavigator.createDestination(),
null
)
testNavigator.navigate(listOf(backStackEntry), null, null)

var viewModelStoreOwner: ViewModelStoreOwner? = null

composeTestRule.setContent {
val saveableStateHolder = rememberSaveableStateHolder()
backStackEntry.provideToCompositionLocals(saveableStateHolder) {
viewModelStoreOwner = LocalViewModelStoreOwner.current
}
}

assertWithMessage("ViewModelStoreOwner is provided by $backStackEntry")
.that(viewModelStoreOwner).isEqualTo(backStackEntry)
}

@Test
fun testLifecycleOwnerProvided() {
val testNavigator = TestNavigator()
val testNavigatorState = TestNavigatorState()
testNavigator.onAttach(testNavigatorState)
val backStackEntry = testNavigatorState.createBackStackEntry(
testNavigator.createDestination(),
null
)
testNavigator.navigate(listOf(backStackEntry), null, null)

var lifecycleOwner: LifecycleOwner? = null

composeTestRule.setContent {
val saveableStateHolder = rememberSaveableStateHolder()
backStackEntry.provideToCompositionLocals(saveableStateHolder) {
lifecycleOwner = LocalLifecycleOwner.current
}
}

assertWithMessage("LifecycleOwner is provided by $backStackEntry")
.that(lifecycleOwner).isEqualTo(backStackEntry)
}

@Test
fun testLocalSavedStateRegistryOwnerProvided() {
val testNavigator = TestNavigator()
val testNavigatorState = TestNavigatorState()
testNavigator.onAttach(testNavigatorState)
val backStackEntry = testNavigatorState.createBackStackEntry(
testNavigator.createDestination(),
null
)
testNavigator.navigate(listOf(backStackEntry), null, null)

var localSavedStateRegistryOwner: SavedStateRegistryOwner? = null

composeTestRule.setContent {
val saveableStateHolder = rememberSaveableStateHolder()
backStackEntry.provideToCompositionLocals(saveableStateHolder) {
localSavedStateRegistryOwner = LocalSavedStateRegistryOwner.current
}
}

assertWithMessage("LocalSavedStateRegistryOwner is provided by $backStackEntry")
.that(localSavedStateRegistryOwner).isEqualTo(backStackEntry)
}

@Test
fun testSaveableValueInContentIsSaved() {
val testNavigator = TestNavigator()
val testNavigatorState = TestNavigatorState()
testNavigator.onAttach(testNavigatorState)
val backStackEntry = testNavigatorState.createBackStackEntry(
testNavigator.createDestination(),
null
)
testNavigator.navigate(listOf(backStackEntry), null, null)

val restorationTester = StateRestorationTester(composeTestRule)
var array: IntArray? = null

restorationTester.setContent {
val saveableStateHolder = rememberSaveableStateHolder()
backStackEntry.provideToCompositionLocals(saveableStateHolder) {
array = rememberSaveable {
intArrayOf(0)
}
}
}

assertThat(array).isEqualTo(intArrayOf(0))

composeTestRule.runOnUiThread {
array!![0] = 1
// we null it to ensure recomposition happened
array = null
}

restorationTester.emulateSavedInstanceStateRestore()

assertThat(array).isEqualTo(intArrayOf(1))
}
}
82 changes: 82 additions & 0 deletions ...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
@@ -0,0 +1,82 @@
/*
* Copyright 2021 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package androidx.navigation.compose

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
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner
import androidx.lifecycle.viewmodel.compose.viewModel
import androidx.navigation.NavBackStackEntry
import java.util.UUID

/**
* Provides [this] [NavBackStackEntry] as [LocalViewModelStoreOwner], [LocalLifecycleOwner] and
* [LocalSavedStateRegistryOwner] to the [content] and saves the [content]'s saveable states with
* the given [saveableStateHolder].
*
* @param saveableStateHolder The [SaveableStateHolder] that holds the saved states
* @param content The content [Composable]
*/
@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 = rememberSaveableStateHolder(),
content: @Composable () -> Unit
) {
CompositionLocalProvider(
LocalViewModelStoreOwner provides this,
LocalLifecycleOwner provides this,
LocalSavedStateRegistryOwner provides this
) {
saveableStateHolder.SaveableStateProvider {
content()
}
}
}

@Composable
private fun SaveableStateHolder.SaveableStateProvider(content: @Composable () -> Unit) {
val viewModel = viewModel<BackStackEntryIdViewModel>()
viewModel.saveableStateHolder = this
SaveableStateProvider(viewModel.id, content)
}

internal class BackStackEntryIdViewModel(handle: SavedStateHandle) : ViewModel() {

private val IdKey = "SaveableStateHolder_BackStackEntryKey"

// we create our own id for each back stack entry to support multiple entries of the same
// destination. this id will be restored by SavedStateHandle
val id: UUID = handle.get<UUID>(IdKey) ?: UUID.randomUUID().also { handle.set(IdKey, it) }

var saveableStateHolder: SaveableStateHolder? = null

// onCleared will be called on the entries removed from the back stack. here we notify
// RestorableStateHolder that we shouldn't save the state for this id, so when we open this
// destination again the state will not be restored.
override fun onCleared() {
super.onCleared()
saveableStateHolder?.removeState(id)
}
}
43 changes: 2 additions & 41 deletions navigation/navigation-compose/src/main/java/androidx/navigation/compose/NavHost.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,21 @@ package androidx.navigation.compose
import androidx.activity.compose.LocalOnBackPressedDispatcherOwner
import androidx.compose.foundation.layout.Box
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.DisposableEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.runtime.saveable.SaveableStateHolder
import androidx.compose.runtime.saveable.rememberSaveableStateHolder
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.LocalLifecycleOwner
import androidx.compose.ui.platform.LocalSavedStateRegistryOwner
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner
import androidx.lifecycle.viewmodel.compose.viewModel
import androidx.navigation.NavDestination
import androidx.navigation.NavGraph
import androidx.navigation.NavGraphBuilder
import androidx.navigation.NavHostController
import androidx.navigation.Navigator
import androidx.navigation.get
import java.util.UUID

/**
* Provides in place in the Compose hierarchy for self contained navigation to occur.
Expand Down Expand Up @@ -136,41 +129,9 @@ public fun NavHost(
// while in the scope of the composable, we provide the navBackStackEntry as the
// ViewModelStoreOwner and LifecycleOwner
Box(modifier, propagateMinConstraints = true) {
CompositionLocalProvider(
LocalViewModelStoreOwner provides backStackEntry,
LocalLifecycleOwner provides backStackEntry,
LocalSavedStateRegistryOwner provides backStackEntry
) {
saveableStateHolder.SaveableStateProvider {
destination.content(backStackEntry)
}
backStackEntry.provideToCompositionLocals(saveableStateHolder) {
destination.content(backStackEntry)
}
}
}
}

@Composable
private fun SaveableStateHolder.SaveableStateProvider(content: @Composable () -> Unit) {
val viewModel = viewModel<BackStackEntryIdViewModel>()
viewModel.saveableStateHolder = this
SaveableStateProvider(viewModel.id, content)
}

internal class BackStackEntryIdViewModel(handle: SavedStateHandle) : ViewModel() {

private val IdKey = "SaveableStateHolder_BackStackEntryKey"

// we create our own id for each back stack entry to support multiple entries of the same
// destination. this id will be restored by SavedStateHandle
val id: UUID = handle.get<UUID>(IdKey) ?: UUID.randomUUID().also { handle.set(IdKey, it) }

var saveableStateHolder: SaveableStateHolder? = null

// onCleared will be called on the entries removed from the back stack. here we notify
// RestorableStateHolder that we shouldn't save the state for this id, so when we open this
// destination again the state will not be restored.
override fun onCleared() {
super.onCleared()
saveableStateHolder?.removeState(id)
}
}