-
Notifications
You must be signed in to change notification settings - Fork 952
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
Closed
jossiwolf
wants to merge
8
commits into
androidx:androidx-main
from
jossiwolf:nav-backstackentry-composable-wrapper
Closed
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
be27bc8
Introduce new NavBackStackEntry#provideToCompositionLocals extension …
jossiwolf be4bf48
Provide a default value for NavBackStackEntry#provideToCompositionLoc…
jossiwolf 3755c15
Use TestNavigatorState to set up NavBackStackEntry instead of the ugl…
jossiwolf 6dc8b2c
Extract back stack entry creation into test-private method
jossiwolf 2d31918
Add testNonSaveableValueInContentIsNotSaved to save a non-saveable va…
jossiwolf f57fd49
Update API after providing default param
jossiwolf 8c2a3d7
Rename provideToCompositionLocals to CompositionLocalProvider to adhe…
jossiwolf a8100a0
Rename CompositionLocalProvider to LocalOwnersProvider and make its s…
jossiwolf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
163 changes: 163 additions & 0 deletions
163
...compose/src/androidTest/java/androidx/navigation/compose/NavBackStackEntryProviderTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| /* | ||
| * 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.remember | ||
| 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.NavBackStackEntry | ||
| import androidx.navigation.testing.TestNavigatorState | ||
| import androidx.savedstate.SavedStateRegistryOwner | ||
| import androidx.test.ext.junit.runners.AndroidJUnit4 | ||
| import androidx.test.filters.LargeTest | ||
| 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 | ||
|
|
||
| @LargeTest | ||
| @RunWith(AndroidJUnit4::class) | ||
| class NavBackStackEntryProviderTest { | ||
|
|
||
| @get:Rule | ||
| val composeTestRule = createComposeRule() | ||
|
|
||
| @Test | ||
| fun testViewModelStoreOwnerProvided() { | ||
| val backStackEntry = createBackStackEntry() | ||
| 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 backStackEntry = createBackStackEntry() | ||
| 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 backStackEntry = createBackStackEntry() | ||
| 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 backStackEntry = createBackStackEntry() | ||
| 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)) | ||
| } | ||
|
|
||
| @Test | ||
| fun testNonSaveableValueInContentIsNotSaved() { | ||
| val backStackEntry = createBackStackEntry() | ||
| val restorationTester = StateRestorationTester(composeTestRule) | ||
| var nonSaveable: IntArray? = null | ||
| val initialValue = intArrayOf(10) | ||
|
|
||
| restorationTester.setContent { | ||
| val saveableStateHolder = rememberSaveableStateHolder() | ||
| backStackEntry.provideToCompositionLocals(saveableStateHolder) { | ||
| nonSaveable = remember { initialValue } | ||
| } | ||
| } | ||
|
|
||
| assertThat(nonSaveable).isEqualTo(initialValue) | ||
|
|
||
| composeTestRule.runOnUiThread { | ||
| nonSaveable!![0] = 1 | ||
| // we null it to ensure recomposition happened | ||
| nonSaveable = null | ||
| } | ||
|
|
||
| restorationTester.emulateSavedInstanceStateRestore() | ||
|
|
||
| assertThat(nonSaveable).isEqualTo(initialValue) | ||
| } | ||
|
|
||
| private fun createBackStackEntry(): NavBackStackEntry { | ||
| val testNavigator = TestNavigator() | ||
| val testNavigatorState = TestNavigatorState() | ||
| testNavigator.onAttach(testNavigatorState) | ||
| val backStackEntry = testNavigatorState.createBackStackEntry( | ||
| testNavigator.createDestination(), | ||
| null | ||
| ) | ||
| // We navigate to move the NavBackStackEntry to the correct state | ||
| testNavigator.navigate(listOf(backStackEntry), null, null) | ||
| return backStackEntry | ||
| } | ||
| } |
82 changes: 82 additions & 0 deletions
82
...navigation-compose/src/main/java/androidx/navigation/compose/NavBackStackEntryProvider.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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( | ||
| saveableStateHolder: SaveableStateHolder = rememberSaveableStateHolder(), | ||
| content: @Composable () -> Unit | ||
| ) { | ||
| CompositionLocalProvider( | ||
| LocalViewModelStoreOwner provides this, | ||
| LocalLifecycleOwner provides this, | ||
| LocalSavedStateRegistryOwner provides this | ||
| ) { | ||
| saveableStateHolder.SaveableStateProvider { | ||
jossiwolf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LocalOwnersProviderisn't that descriptive to me, what do you think ofNavBackStackEntry. CompositionLocalProvider? (What Ian originally suggested, just pascal case) @ianhanniballakeThere 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 ofCompositionLocalProvider(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.NavControllerwas built in a such a way that it knows nothing about Fragments, Compose, Views, Dialogs, etc.It is the individual
Navigatorinstances that are added to theNavControllerthat provide that functionality -ComposeNavigatoris 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.