QuickBirdEng / XCoordinator

🎌 Powerful navigation library for iOS based on the coordinator pattern
MIT License
2.27k stars 179 forks source link

Dismiss several coordinators, pop and iOS 13 modal #160

Closed Robuske closed 4 years ago

Robuske commented 4 years ago

Hello, we are starting to use this lib, but we found some problems that we are not sure how to solve, or if we are doing something wrong.

0 - Returning to a coordinator

Lets say A has 3 routes:

How should I return from one of them to A? Should A have other routes like dismissB and popC? Or even more generic ones, just dismiss and pop? I could have initial do a popToRoot and a dismissToRoot like the example does for the deeplink, but that seems pretty wrong. What if I wanted to go from B to C through A? In the sense that I don't want to keep B in the hierarchy.

1 - Dismiss several coordinators

Lets say our "logout" button is several coordinators in (initial coordinator > logged coordinator > menu coordinator). What is the best way to exit the entire chain? Should we be injecting the parent coordinator and have logout routes through the entire chain? That seems like it would be coupling the coordinators too strongly.

2 - Pop and iOS 13 modal

If I push another coordinator, does it deallocate when the user pop its screen? Even if is trough the swipe from the left instead of the back button? The example project does not handle the iOS 13 modal correctly, if it is dismissed using the new system the coordinator stays in memory, any ideia on how to avoid that?

3 - Extra

This is less important, but I'm not sure what is the purpose of the chain function, seems like a type constrained deeplink.

pauljohanneskraft commented 4 years ago

Hello @Robuske , I'm glad to hear you like XCoordinator!

0 - Returning to a coordinator

This is highly dependent on your use case. Individual implementations of coordinators are normally used in only one context (i.e. they are either pushed or presented) and can therefore dismiss/pop themselves. When that does not work, you can either inject a parent router (e.g. in the form of an UnownedRouter) or use other techniques (notifications, reactive streams, closures, delegate protocols, etc) to inform the parent that it should remove that child from the view hierarchy - here a dismissB or popC route would make sense.

Btw: Please try to use .dismiss over .dismissToRoot, since it might otherwise not result in the intended behavior.

1 - Dismiss several coordinators

Again - getting the information across to the parent can be done using many different techniques (notifications, reactive streams, closures, delegate protocols, etc). To dismiss all viewControllers currently being presented on top of your rootViewController, you can use Transition.dismissAll() as defined in an extension on our example app.

2 - Pop and iOS 13 modal

The first issue should be solved with the most recent update to XCoordinator, since it will check for unnecessarily held children whenever the navigationController shows a new viewController, i.e. at the end of any push/pop/set operation, even the ones using the interactivePopGestureRecognizer. Regarding modally presented viewControllers/coordinators using the default/pageSheet modalPresentationStyle: They are removed after another route has been triggered (i.e. some time later), but unfortunately we were not yet successful to find a solution which would not involve swizzling the viewDidDisappear method. We would be glad for some help on this issue, may it be a rough idea/sketch or a full implementation.

3 - Extra

It is, in a way, a type-safe variant of deepLink or you can also see it as a shorter way to write the following:

case .caseA:
    return .multiple(
        prepareTransition(for: .caseB),
        prepareTransition(for: .caseC),
        prepareTransition(for: .caseD)
    )

which can be written as

case .caseA:
    return chain(routes: [.caseB, .caseC, .caseD])

You can think of it more like syntactic sugar than an actual feature.

slxl commented 4 years ago

@pauljohanneskraft just to not create a separate ticket for that: in the example project, AppCoordinator has 3 routes: .home, .login and .newsDetail. When you show .home after .login latter one stays in memory which might be a problem for larger projects. What might be the better options there? (sorry just started to check the lib, but anyway it looks really promising, thanks for it!)

pauljohanneskraft commented 4 years ago

@slxl just so I fully understand your issue: The coordinator being presented is being removed after the next route has been triggered, right? So, it keeps the reference a bit too long, but not forever. Is that correct?

Similar to my response here, you can use different techniques to get a piece of code being executed whenever the dismissal has been started, which might be your best bet at the moment to ensure instant removal of that coordinator from the coordinator hierarchy... Of course, you can also use a custom UIAdaptivePresentationControllerDelegate. In case, you are using RxSwift, you can simply use

childCoordinator.rx.dismissal.take(1)
     .subscribe(onNext: { [weak self] in self?.removeChildrenIfNeeded() })
     .disposed(by: disposeBag)

to get informed about a childCoordinator being dismissed inside the parent coordinator.

We are still gathering information and discussing different solutions to possibly solve this problem internally once and for all, so in case you find a better-suited solution that we can use, feel free to propose it here.

slxl commented 4 years ago

@pauljohanneskraft not really, in demo project both LoginViewController and LoginViewModelImpl stays in memory after Home coordinator appears on the screen. So the question is if it intended behavior or not?

pauljohanneskraft commented 4 years ago

@slxl ah, that would be a completely unrelated issue...

Yes, it is intended, that the LoginViewController (and therefore all of its references as well, such as its viewModel) stays in memory in this case. I unfortunately could not find a suitable existing issue/response that would guide you to a solution, but you will simply have to make sure to use a different transition to switch between login and home coordinators/viewControllers. At the moment, we use Transition.present and Transition.dismiss, which is presenting the home coordinator onto the LoginViewController (that's why it is still in memory, since it stays in the view hierarchy as the presenting view controller). Alternatively, you might be able to, e.g., reset the rootViewController of your UIWindow to the home coordinator's rootViewController (there are possibly endless other solutions, but this would be one), which you will then also have to do when going back to the login screen as well.

slxl commented 4 years ago

@pauljohanneskraft Ok, will try. Thank you!

txaiwieser commented 4 years ago

The new iOS 13 modal style is a problem for us too. Because you can exit at any time from a modal flow (even if is a Navigation flow). It would be great to find a solution for this, maybe using the delegate, just like it was solved for back button and swipe right on navigationcontroller?