[Feature] Nested backstacks
mheras opened this issue · 29 comments
Hi!
Is there any way to push a [child] controller from within another child controller?
Let's say I'm displaying a child controller (named A) in a container view (named V), that it is just a part of the whole screen.
What I want is to push another controller from A and make its view be contained in V.
From what I understand, if we push a controller using the router, that would replace the view of the parent controller, and that's not what I want to achieve.
Thank you in advance!
EDIT
My use case is:
- I want a tabbed controller so I'm using
ControllerPagerAdapter. - I want its child controllers to have their own backstacks so I can navigate within that "section" and keep the tabbar always visible.
This is currently unsupported. I'm not sure I really want to support nested backstacks, due to the greatly increased complexity of dealing with such things, both from a library maintainer's point of view and an application developer's point of view.
That being said, your use case doesn't seem unreasonable, so it would be nice to come up with a workable solution. One thing I'm not totally sure about though - would this even work with a ViewPager in the first place? Can you replace one of its pages and expect it to continue working correctly?
Im also facing a similar use case: i.e. an E-mail app on a tablet
displaying the inbox (list of mails). When I click on a mail item from
inbox, I would like to show the details to that email. It would be nice if
I could add a child controller to the parents layout dynamically to switch
to a master-detal view with changehandler runing animations. I e.
LayoutTransition or TransitionManager (api19)
Eric Kuck notifications@github.com schrieb am Sa., 23. Apr. 2016, 00:54:
This is currently unsupported. I'm not sure I really want to support
nested backstacks, due to the greatly increased complexity of dealing with
such things, both from a library maintainer's point of view and an
application developer's point of view.That being said, your use case doesn't seem unreasonable, so it would be
nice to come up with a workable solution. One thing I'm not totally sure
about though - would this even work with a ViewPager in the first place?
Can you replace one of its pages and expect it to continue working
correctly?—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#27 (comment)
@sockeqwe, I think your use case is a bit different from @mheras's. You're looking for master/detail view, while he's looking for having one backstack per page in a ViewPager. My original thought was that master/detail view applications would use a single Activity with multiple routers. So instead of
--------------------------------------------
| Parent Controller |
| -------------------------------------- |
| | Child | Child | |
| | Controller | Controller | |
| | 1 | 2 | |
| -------------------------------------- |
--------------------------------------------
you'd have
--------------------------------------------
| Activity |
| -------------------------------------- |
| | Router | Router | |
| | 1 | 2 | |
| | or | | |
| | ViewGroup | | |
| -------------------------------------- |
--------------------------------------------
This would allow independent backstacks without getting into confusing child/parent or nested backstack situations. The downside is that animations would have to happen at the ViewGroup level rather than within a Router.
That being said, it seems like there might be interest in allowing infinitely nested controllers, each with routers and backstacks for their host viewgroup. I hadn't really anticipated this being a use case, but the more I've thought about it, the more I think it probably wouldn't be terribly difficult to develop and test. My hesitation on it is whether or not it's something that would benefit developers rather than get them caught doing things in a way they should probably be avoiding in the first place.
That being said, it seems like there might be interest in allowing infinitely nested controllers, each with routers and backstacks for their host viewgroup. I hadn't really anticipated this being a use case, but the more I've thought about it, the more I think it probably wouldn't be terribly difficult to develop and test. My hesitation on it is whether or not it's something that would benefit developers rather than get them caught doing things in a way they should probably be avoiding in the first place.
This is how our entire app is structured (not using Conductor) and it works fabulously.
you are right, it's different, but I may also wanted to add navigation to the detail container (which I haven't mentioned earlier, sorry). I haven't thought about multiple routers yet, will play around a little bit with that approach.
I tried something like this:
--------------------------------------------
| Parent Controller |
| ------------------------------------- |
| | Master | Detail | |
| | Controller | Controller | |
| | 1 | 2 | |
| -------------------------------------- |
--------------------------------------------
But also tried something like this:
--------------------------------------------
| Master Controller ( full screen) |
| |
--------------------------------------------
Then add DetailController as child controller of MasterController
--------------------------------------------
| Master Controller (split screen) |
| ---------------- |
| | Detail | |
| | Controller | |
| ---------------- |
--------------------------------------------
IMHO infinitely nested controllers would be nice to have, but only if implementable in an easy and maintainable way. Keep Conductor focused on doing a few important things right, instead of having 1000 features. (I know that my super fancy Master-Detail animation stuff is not the common use case)
@EricKuck @sockeqwe @JakeWharton
Actually, what I'm trying to do is an app with tabbed main content.
Recently, Google changed the Material Design Guide and it now allows a Bottom navigation.
They say that:
- ...the bottom navigation bar should be kept in the screen on top of anything else, as the app main navigation widget. This is the main reason I opened this issue. In this video from Google you can see how each tab have their own backstack.
- ...horizontal scrolling (swipe) should not be allowed to move through the "pages", as we usually did before with the
PageAdapter. I realized about this yesterday, so I won't be using theControllerPageAdapteranymore and I will be swapping the pages right away (with a nice fade animation, as they also say in the guide).
Of course I can achieve this by adding the bottom navigation bar (and the Toolbar) directly in the Activity and then attaching the Router with the main container, but I would love to move all the views and logic into the Controllers, leaving the Activity with just the code that sets Conductor up.
What do you think?
Thanks!
P.S.
@sockeqwe
Keep Conductor focused on doing a few important things right, instead of having 1000 features. (I know that my super fancy Master-Detail animation stuff is not the common use case)
I think this is a common navigation feature, even more after Google announced the bottom navigation, and would be great if Conductor supported it.
Everyone's made good points. While I've never had a use for this feature, it seems that other people do have very valid uses for it. I played around with the concept of nesting routers and it seems like it'll work pretty well. I don't anticipate it add much, if any, complexity to the library.
One thing we'd have to figure out now is what to do with the existing getRouter() method. You'd think this would return the router for the viewgroup that the controller is hosted in, but currently that's not the case, and it would be a breaking change to make it work that way. Should we deprecate that method and add getLocalRouter(), getParentRouter(), and maybe a getRootRouter() method? That seems really messy and confusing, but I'm not sure what other options there are.
Given that nested backtacks seem to be a needed feature, I don't see why getParentRouter() and getRootRouter are messy or confusing. However, I wouldn't deprecate getRouter() and would return the local router in that case, but I don't know if that would break any compatibility for those who are already using the lib for longer than I am.
That's something I've always wondered... How do you manage stuff like NavigationViews/Toolbar ?
I don't like the idea of adding it to the Activity (as @sockeqwe mentioned, totally agree) and keeping all this code in it, creating something like a ActionBarOwner class and keeping it on top of a Dagger2 graph and down-inject it to it's children. Is there a better approach to this, I mean, having a "root" controller (for keeping the NavigationView and Toolbar for example) and just change the [ViewGroup] content ?
Thanks !
but currently that's not the case, and it would be a breaking change to make it work that way.
I think until now there are not too many users hat were effected by breaking changes, so it shouldn't be a big deal to do breaking changes right now. Increasing version to 2.0.0 could be an option to highlight breaking changes even better (maybe do some 2.0.0-beta as retrofit did)
add getLocalRouter(), getParentRouter(), and maybe a getRootRouter()
It would like to keep this stupid simple. Does a nested child controller really needs access to the parents router? It depends. I guess the child controller mainly needs a router to pop himself or add another child controller (a child controller of a child controller). However, if he wants to navigate to another "screen" than you might need access to root router or parent router (maybe via target controller? but that might requires to write some boilerplate code and limits reusability especially with deeply nested child controllers.).
So I would like to keep this simple but actually I don't know how to simplify getLocalRouter(), getParentRouter(), and maybe a getRootRouter() even more. I guess @JakeWharton has more experience and maybe a solution to share from his (not conductor based) app?
Btw. I also think that getRouter() could become getLocalRouter() as @mheras has suggested, but I think some people will run into this breaking changes thing without noticing it because they don't have compile time errors when updating from 1.x to 2.0 (I have assumed that 2.0 is the version with the discussed changes). But again, I think the number of affected users of such breaking changes is low and imho getRouter() sounds simpler then getLocalRouter().
Been working on adding child routers to every controller. I've hit a few points that I'm not sure how to address though and was hoping people could chime in. The first question that I'd like to hear people's input on is: Should multiple routers (backstacks) be allowed per parent?
If so, what should happen if the parent has several child routers, all with a few controllers pushed onto them and the user presses the back button? I guess the parent would have to keep track of a backstack for all child routers and remember which one should be popped, regardless of which containing ViewGroup it's in? Seems very awkward and hard to manage.
If not, I guess there would be the standard addChildController() method and a new addChildRouter() method? This doesn't seem so great either.
Hmmm, you are right... That's another possible use case, but with a weird backstack interaction.
I guess there is no alternative to do what you said:
I guess the parent would have to keep track of a backstack for all child routers and remember which one should be popped, regardless of which containing ViewGroup it's in? Seems very awkward and hard to manage.
This issue happens whenever you have sibling controllers, each of them with their own router... I cannot come up with something elegant at this point 😞, but I still think that the library should support it.
For me this feature is crucial to develop complex and modern apps.
But I wouldn't worry about multiple routers per parent. Parent controller could implement its own onBackPressed method and decide there which router should handle this event, just like we do now in Activity.onBackPressed: router.handleBack.
This should be flexible enough for most of the use cases I can imagine and I have done it in my previous projects and it worked well.
@sdrygalski Agreed, I've been trying to use a Toolbar and a NavigationView inside a RootController but currently I need to put all the code inside the Activity and use interface callbacks, but using a Controller would be much easier !
@leonardo2204 and sometimes you can't put Toolbar inside MainActivity. Eg when you make something like SplashController without toolbars etc (controller with just some animations and welcome messages). From SplashController/WizzardController you make transition to MainController that contains toolbar. You are forced to nest some controllers inside MainController (to exclude toolbar, just like in demo), otherwise toolbar will weirdly animate when changing from MainController to something else...
@sdrygalski @leonardo2204 can you guys show code on how you're using toolbar/nav view with conductor...this modern approach is well new...code would have clarify my confusion. thank you!
@sirvon just look at the demo in the conductor repo (use Android Studio to clone entire Conductor repo and you will be able to run "demo" module and investigate source inside IDE)
The first question that I'd like to hear people's input on is: Should multiple routers (backstacks) be allowed per parent?
What would be the other questions you want to share with us?
I'm looking forward to having support for this feature.
This branch has WIP code for this feature. You can download the current state's snapshot with the 2.0.0-SNAPSHOT dependency if you want to start playing with it. I've also added a bottom navigation sample to the demo app so you can check it out in action.
This is going to be released as v2, so some methods have been removed or added. There isn't much documentation yet, but going through the demo app should give you hints as to what's changed.
I'm not totally pleased with where it's at right now, so feedback welcome!
@EricKuck this is looking very good
Is it possible to do the same thing with the Toolbar?
I would like to move all UI stuff from the Activity to the parent most controller.
I played with the current snapshot, and it works pretty well so far! Some feedback:
- The Material-Design-Bottom-Nav use case should probably be handled with only one container
ViewGroup. Looking at the Google apps with a bottom nav, bottom tabs don’t have their own backstack, but instead open sub-screens in what would be the parent router in Conductor, i.e. filling the whole screen, overlaying the bottom nav. This also allows for animations viaRouterTransitionbetween tabs instead of having to handle that manually. setHandlesBackseems like state spread out too much. My horizon might be limited, but feels like having one ‘active’ child router per parent router is the most common use case (e.g. bottom nav or master-detail) and should be treated as default without manual configuration, with the option to override. Generally, an API that prevents accidental inconsistencies (forgetting tosetHandlesBack(false)on old controllers) would be better.
Also, not sure how it is supposed to behave, but if I comment outgetChildRouter(oldContainer, null).setHandlesBack(false);in the demo app, back is still only handled by one child router (and things seem to break a little).- After two rotations, the child router stops working (both in my own single container setup and the demo app).
(Point 3
bump? :(
Sorry for all the time away. We've had some crazy deadlines and I just didn't have time to look at this much.
Thanks for the suggestions @hannesstruss.
- You're right that the bottom nav isn't really implemented correctly in the demo. I'm trying to figure out what a demo for nested routers, each with their own independent backstacks, could be. I think I'm going to switch to having a few children all visible at the same time just for the sake of demoing it.
- This is a good suggestion. How would you recommend being able to override this only having one child handle back events?
- Point 3 has been fixed.
@EricKuck welcome back :)
What if we use Controller#handleBack for this?
- If the controller has no child routers, pressing back will pop from the default router
- If the controller has one child router, pressing back will pop from the child router. If the child router's backstack is empty, it'll pop from the parent router, if that is empty from the grandparent, and so on.
- It the controller has more than one child router,
handleBackMUST be overridden. The back press can be delegated to a child router of the controller's choice, or ignored, or whatever. If the controller does not overridehandleBack, an exception will be thrown.
I agree with points 1 and 2 that you made. Those two situations are going to be by far the primary use cases and I think that's the right way to handle both scenarios.
Regarding point 3, I guess I'm not a huge fan of forcing someone to override a method when there's no way to add a compile-time check to making sure they've done this. A lot of the issues I've had with Fragments is that they throw unexpected exceptions and cause crashes in weird corner cases, and would like to avoid taking Conductor down this road.
I think that by default, the back button should pop whatever the most recent controller was, regardless of which router it was pushed to. What I'm not sure of is how to allow the developer to override this behavior if they need to. From what I can tell there's really only two options:
- Allow the developer to prevent certain child router(s) from handle back presses. (How it's currently implemented on the feature branch)
- Allow the developer to set a single "active" child router, which is the only one that would handle presses. (Your initial suggestion)
Both options have pros and cons, depending on the situation they're being used in. I don't have a strong preference either way. If we went with option 2 we'd probably have to allow the developer to set active to null, which would cause it to revert to the original behavior of letting any children handle presses.
You're right, defaulting to pop the most recently pushed controller in point 3 is better than throwing, and also more convenient as it's probably the most commonly wanted behavior.
Using Controller#handleBack to override that behavior still makes sense to me though – to me it feels like back press behavior should be defined in a callback, not via stateful configuration that has to be managed and can get inconsistent over time.
handleBack exists and can be used to alter backstack behavior already. It's also an analogue to overriding Activity#onBackPressed which people are already used to. I wouldn't add an additional lever to the framework API, but make this the one true point people go to for backstacking.
You're right, good call! I'll just go with onBackPressed overriding.
This is now available in the v2 release candidate. Updated docs can be found at https://github.com/bluelinelabs/Conductor/blob/v2/README.md. Feel free to suggest any changes before before we officially go to v2!