Add extension function to retrieve a Flow of NavBackStackEntries #89
Conversation
dlam
requested review from
ianhanniballake and
jbw0033
and removed request for
ianhanniballake
| .withIndex() | ||
| .onEach { (index, backStackEntry) -> | ||
| val expectedDestination = index 1 | ||
| assertEquals(expectedDestination, backStackEntry.destination.id) |
There was a problem hiding this comment.
Please use Truth asserts (note, we already have an androidTest dependency on Truth) over Junit asserts here:
assertThat(backStackEntry.destination.idt).isEqualTo(expectedDestination)
Or preferably use the assertWithMessage() version that includes a more detailed message that is printed when the assert fails.
| @Suppress("EXPERIMENTAL_API_USAGE") | ||
| public fun NavController.getBackStackEntryFlow(): Flow<NavBackStackEntry> = callbackFlow { | ||
| val listener = NavController.OnDestinationChangedListener { controller, _, _ -> | ||
| controller.currentBackStackEntry?.let(::sendBlocking) |
There was a problem hiding this comment.
would it be better to use second argument instead? given that it is non null it will make things as easy as sendBlocking(destination)
another corner case question: sendBlocking can theoretically block on the main thread, when there is slow consumer and a lot of actions here. Though it seems like fairly theoretical concern
There was a problem hiding this comment.
Would be possible, but then it would be a Flow<NavDestination> instead of a Flow<NavBackStackEntry>, which is not what we want I guess (as NavBackStackEntry is the class that gives access to things like ViewModelStore and the SavedStateRegistry). But maybe it would make sense to have both, a backStackEntryFlow and a destinationFlow?
Valid point, I also wasn't sure what's best to use here. The other option would be to use offer, which could lead to lost items it the consumer is slow.
There was a problem hiding this comment.
We are specifically looking for the NavBackStackEntry here, not NavDestination.
| * it changes. If there is no active [NavBackStackEntry], no item will be emitted. | ||
| */ | ||
| @Suppress("EXPERIMENTAL_API_USAGE") | ||
| public fun NavController.getBackStackEntryFlow(): Flow<NavBackStackEntry> = callbackFlow { |
There was a problem hiding this comment.
I'd probably suggest to shape it as extension property:
public val NavController.backStackEntryFlow : ....
| * Creates and returns a [Flow] that will emit the currently active [NavBackStackEntry] whenever | ||
| * it changes. If there is no active [NavBackStackEntry], no item will be emitted. | ||
| */ | ||
| @Suppress("EXPERIMENTAL_API_USAGE") |
There was a problem hiding this comment.
I think it should be marked as experimental api. We won't be able to release it as stable until callbackFlow will be stable (androidx gives binary compatibility guarantees that could be broken by callbackFlow)
There was a problem hiding this comment.
Just to make sure I understand correctly - since it is part of the public API @ExperimentalCoroutinesApi is required. If it were an internal function, @Suppress would be fine (as it does not cause binary incompatible changes), correct?
| @Suppress("EXPERIMENTAL_API_USAGE") | ||
| public fun NavController.getBackStackEntryFlow(): Flow<NavBackStackEntry> = callbackFlow { | ||
| @ExperimentalCoroutinesApi | ||
| public val NavController.backStackEntryFlow: Flow<NavBackStackEntry> get() = callbackFlow { |
There was a problem hiding this comment.
Could we change this to NavController.currentBackStackEntryFlow to be consistent with NavController.currentBackStackEntry API?
There was a problem hiding this comment.
Good point, done 👍
| @Suppress("EXPERIMENTAL_API_USAGE") | ||
| public fun NavController.getBackStackEntryFlow(): Flow<NavBackStackEntry> = callbackFlow { | ||
| val listener = NavController.OnDestinationChangedListener { controller, _, _ -> | ||
| controller.currentBackStackEntry?.let(::sendBlocking) |
There was a problem hiding this comment.
We are specifically looking for the NavBackStackEntry here, not NavDestination.
|
CI is red, but looks unrelated - was this a build flake? Can I retrigger somehow? |
|
Hey the build was just fixed! Sorry it took a few days to get it landed - could you try rebasing your change on top of androidx-master-dev and see if the tests pass now? |
simonschiller
force-pushed
the
feature/back-stack-entry-flow
branch
from
eca0101
to
a04c781
Compare
|
Rebasing fixed the issue 👍 |
|
Thanks again for implementing this! It'll become available in Navigation 2.4.0-alpha01. |
Proposed Changes
NavBackStackEntryas aFlow.Testing
Test: Added a test for the new function
Issues Fixed
Fixes: 163947280