Returning a result up several navigation destinations causes extra dismiss animations
mgod opened this issue · 12 comments
I'm building a flow to let a user configure something over several screens. At the end of the process, if it's successful, I'd like to return the result back to the fragment that launched the first screen of the flow. So, something like this:
Main fragment -> Fragment 1 -> Fragment 2 -> Fragment 3
And when Fragment 3 returns a result, close fragments 1, 2 and 3 and have Main fragment handle the result.
I can do this manually by having Fragments 1 and 2 do something like:
val result = registerForResult<Value> {
navigationHandle.closeWithResult(it)
}
Which is fine except that due to how the lifecycles interact with results, this means that on completing Fragment 3, you see it disappear followed by a flash of Fragment 2 then Fragment 1 disappearing. Is there a way to do this? It seems like result channel has enough infrastructure to be able to pass the result all the way back with minimal changes, but I can't see a clean way to dismiss all the fragments.
As a work-around, it is straightforward to start the flow in an Activity so you get:
Main fragment -> Flow activity -> Fragment 1 -> Fragment 2 -> Fragment 3
and have Fragment 3 grab the Flow activity navigation handle to return the result, but I'd prefer to avoid this if possible.
I'm happy to spend some time putting a PR together for this if it seems like a reasonable change and there's not a way to do this currently.
Hi! This is a great issue, thanks for raising it. Flows, and this kind of navigation, are something I've thought about before. Unfortunately, I didn't come to any super clear conclusions. When I've implemented these flows myself, I have usually wrapped them as a nested flow, inside a Fragment or Activity as you've mentioned. You may be able to use an override on the NavigationController to override the closing animations for particular screens (if you're using the 1.3 beta), but this might need some functionality added to be able to peek pending results.
I would absolutely welcome a contribution in the form of a PR if you've got some time to do something like that, but perhaps it's best if we discussed the approach a bit before you dive into building something out. Compose support is front-of-mind for me at the moment, so I want to make sure anything that we'd build out will work for that when I add @NavigationDestination support to @Composable functions.
There are a few ways I can think of off the top of my head, perhaps adding something like a "delegatedResultChannel" or "flowResultChannel" that would automatically pass back results and skip past animations as results were received. The best solution would be to completely skip the other Fragments and immediately return the result to the original caller, but I don't have any good ideas on how to get that working nicely to also support Activities.
What sort of approach were you thinking of?
I'd be inclined to add a new extension method forwardNavigationResult similar to registerForNavigationResult. The forward version of the function would still register a result channel like normal, but it would use a channel that consumed the result immediately instead of waiting for the destination to resume. I'd need to keep track of how many navigation destinations forwarded results and then determine what kind of navigation stack manipulation is needed to get back to the starting destination. I can't see exactly how to hook this into the result processor yet, so this might not be a reasonable approach.
I think maybe the simplest way to do this is to not try to generalize it for now and see how it works. Maybe for a first pass we only add the forwardNavigationResult for Fragments? I think I can see how to safely bypass the normal fragment navigation to close several in a row without animations. I also can't see how to make this work for activities easily, though with a careful use of the normal activity result forwarding mechanism, I think it would be possible to build an activity or mixed activity/fragment version on top of Enro to skip the animations.
I think we have the same idea about how to go about making the change. It would prefer not to add the forwardNavigationResult to Fragments only. I'd like it to be supported by all navigation destinations, but I'm less concerned if the animations displayed are a little wonky for Activities specifically.
I think the first place to start is to add a naive implementation, where the "forwardNavigationResult" doesn't do anything fancy with the backstack, but rather when the result is received it just closes in the same way as your example, but we can add some additional handling of this to prevent animations being displayed (so it appears to be instant when Fragments are involved). This would be similar to what happens when you open multiple navigation keys at once - all the screens are opened, but with no animations being displayed. Obviously, this isn't the ideal solution, but I think once we have the basics of that in place as an API, we'll be able to add some tests to capture the behaviour we're after, and then can iterate on the internals of how it works.
How does that sound?
Also, I'm not sure if you've looked at the 1.3.0 changes that I have recently made, but there has been some re-packaging of things, and results are now part of enro-core.
@mgod I've added a branch called "forward-navigation-result". This is really just a quick POC, but would be keen to get your feedback.
In the example app, you'll be able to "Request Flow" or "Request Flow Activity" to kick off a "Flow" type navigation with forwarded results. This works reasonably well if the flow is entirely launched in Fragments, or is kicked off with an Activity, but runs into trouble if you're jumping between Activity and Fragment destinations throughout the flow.
Let me know what you think.
That looks like what I had in mind. I like that the forwarding is set up on the launch side of the navigation as this means the destination doesn't need to care if it's forwarding a result or not. I can see how it's tricky to figure out how to skip over intermediate screens, especially if there are several activities in the process. Maybe instead of a custom executor and skipping steps, instead there's an animation that restores the previous fragment/activity as a transparent screen until you reach the start of the flow and use the regular animation there?
This would resolve my core issue, though I'll be updating to 1.3.0 this week and it might be that I can work around this problem for my situation without any library changes give the changes in lifecycle/lateinit handling there. This does seem like a common enough pattern to be worth supporting specifically, but maybe just by documenting how to set it up.
I did have one issue with what I can see in 1.3.0 - I'm using the parentInstruction in some cases where I have a child fragment embedded in a parent fragment to make sure navigation returns to the parent fragment instead of the child. I can't see how to do this in 1.3.0, though I'm also not sure if I'm setting up embedded fragments in a good Enro compatible way. At the moment, I've been doing this to make this work ok:
fun <T: NavigationKey, F: Fragment> Fragment.embedChildFragment(child: F, container: Int, key: T): F {
val instruction = NavigationInstruction.Open(
navigationDirection = NavigationDirection.REPLACE,
navigationKey = key,
parentInstruction = arguments?.readOpenInstruction<NavigationKey>()
)
child.addOpenInstruction(instruction)
childFragmentManager.beginTransaction()
.replace(container, child)
.commit()
return child
}
This feels like maybe I'm not approaching this correctly, but seemed like the spot I might get stuck in the update.
Thanks for this library, by the way! It's made a lot of navigation tasks much simpler than anything else I've used.
Part of the reason that I made those API changes in 1.3.0 was so that things like parentInstruction (which were really only for internal use in Enro) weren't on the public API.
I'm interested in your use case here - You want the child fragment to "return to the parent fragment". This should happen anyway, without the need to set the parentInstruction on the fragment. A standard "Replace" instruction, sent from the "parent" fragment should ensure that the fragment will close back to the parent fragment. In fact, if you have ParentFragment, and ChildFragmentOne and ChildFragmentTwo, a replace instruction from ChildFragmentOne that opens ChildFragmentTwo should cause ChildFragmentTwo to close back to ParentFragment (because ChildFragmentOne would have opened to the ParentFragment, so this instruction is passed on).
Edit: actually answer the correct question
What I have is a case where I have ParentFragment that contains ChildFragmentOne where say the ParentFragment is a toolbar and bottom bar, and ChildFragmentOne is the content in between the two bars. ChildFragmentTwo is a full screen page that should cover all views for both ParentFragment and ChildFragmentOne, so I can't replace ChildFragmentOne. I want the back/close behavior on ChildFragmentTwo to go back to the ParentFragment with the embedded ChildFragmentOne. Ideally I'd like to be able to navigate from ChildFragmentOne to ChildFragmentTwo without needing to worry about how ChildFragmentOne is being displayed.
I'll see if I can put an example together.
Sorry for the delay, here's an example showing the navigation I'm struggling with: https://github.com/splitwise/Enro/tree/embedded-fragment-demo
Here you can see the navigation I'm having to use to get the navigation stack to behave as I want from an embedded fragment.
Here is how I would like to navigate in the EmbeddedFragment, but you can see that if you navigate froward from the embedded buttons, when you go back, it goes back to the wrong location.
This is all something I can work around, but I'd love to be able to make some call at the embed site here that allows me to use navigation normally in the EmbeddedFragment and have the system know that the parent fragment is actually the fragment that should be used in the navigation stack. The one issue I have with my current work around is that in order to launch navigation destinations that return results, I have to coordinate a lot between the child and parent to get the result to be handled appropriately. For normal navigation, it's fairly easy to work around, though the Embedded fragment has to be opinionated about how it's parent behaves.
Hi, sorry for the delay on my part. I've had a look at this issue, and I believe the best place for a fix is with the 2.0.0 release, which will add some changes to container handling. I have added an example on a branch named "2.0.0-embedded-fragments-example".
Please have a look at that branch, and see if that solves the problem for you in the way that you'd like.
The embedded fragment example works great. I was able to remove almost all of my custom embedded fragment handling code by using the 2.0.0 code for this.
I'm glad to hear that it worked well for you.
I keep creeping the scope for the 2.0.0 release, so I'll take this as a bit of incentive to finish it up and actually cut a release candidate in the next week or so 😅
@mgod apologies for taking so long to reply to this. 2.0.0 was released a little while ago (in fact, 2.1.1 is the current release).
The 2.0.0 releases have this functionality, using deliverResultFromPresent and deliverResultFromPush. This allows you to delegate results from one key to another, without having duplicate animations as the destinations are closed. There are some examples in the example app here. The examples use Composables as the destinations, but deliverResultFromX works exactly the same with Fragments. In the example, we build up a Sentence by selecting an adverb, adjective and noun from three screens, each of which have a NavigationKey.WithResult<Sentence>, and pass their selected data down to the next screen's NavigationKey. On reaching the end, the whole stack is collapsed and the result is delivered directly to the screen that opened the original key.