Interop with UINavigationController user instantiated popping

[ambientlight]

Motivation:
ReSwiftRouter should provide the optional mechanism that assists in triggering of route pop corresponding actions to the store when:

1. User hits the back button on the navigation bar
2. User swipes over the left edge of the screen

API Proposal:
Users needs to subclass the ReSwiftInit.NavigationController and override popRoute() methods where they specify the dispatch logic to their store. Like:

class RootNavigationController: ReSwiftRouter.NavigationController {
    override func popRoute() {
        let currentRoute = mainStore.state.navigationState.route
        
        guard currentRoute.startIndex != currentRoute.endIndex else {
            return
        }
        
        mainStore <- SetRouteAction([RouteElementIdentifier](currentRoute[currentRoute.startIndex ..< currentRoute.endIndex.advanced(by: -1)]))
    }
}

Implementation details:
When user hits the back button popViewController() can be canceled if navigationBar(:shouldPop item:) is implemented. In such case popRoute() will be called that should result in SetRouteAction dispatched. Our routable can then subsequently call the actual popViewController() as desired.

However, when swiping popViewContoller() call cannot be canceled. When swipe is recognized, popViewController() will be called before popRoute() callback. In order to avoid the redundant popViewController() from our routable, popViewController() semantics was changed to ignore all popViewController() calls when swipe is still in progress. This semantic change is reasonable since nothing should be calling popViewController() while user is swiping anyway. Also it allows us having our routable transparent of the swiping state. It can instead (if needed) check if pop was canceled since the instance of PopWasCancelled dummy viewController would be returned. Like:

let popped = mapViewController.navigationController?.popViewController(animated: animated, completion: completionHandler)
if popped is PopWasIgnored {
       //additional logic on cancelation during swipe
}

Additionally the utility navigationController's push/pop with completion were provided.
The sample hypothetical routable can use it like:

public func popRouteSegment(_ routeElementIdentifier: RouteElementIdentifier, animated: Bool, completionHandler: @escaping ReSwiftRouter.RoutingCompletionHandler) {
        
    guard routeElementIdentifier == Routes.editFeature else {
         completionHandler()
         return
    }
        
    _ = mapViewController.navigationController?.popViewController(animated: animated, completion: completionHandler)
}

[codecov-io]

Codecov Report

Merging #74 into master will decrease coverage by 15.3%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master      #74       +/-   ##
===========================================
- Coverage   80.28%   64.98%   -15.31%     
===========================================
  Files           5        6        +1     
  Lines         208      257       +49     
===========================================
  Hits          167      167               
- Misses         41       90       +49

Impacted Files	Coverage Δ
ReSwiftRouter/NavigationController.swift	0% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6216d2...d93be78. Read the comment docs.

[ambientlight]

@Ben-G: what do you think about it?

[Ben-G]

Hey! Sorry I was out for a long weekend. Will try to look at this tomorrow!

…

On Jul 3, 2017, at 23:09, Taras Vozniuk ***@***.***> wrote: @Ben-G: what do you think about it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[Ben-G]

Hi @ambientlight!

Thanks a lot for the PR, it's great that you're trying to solve this problem!
I have a few thoughts:

1. Do you happen to have a project that uses this component already? It would be a lot easier to review when seeing the component in action. If you don't have that, I can try to integrate it into https://github.com/ReSwift/GitHubBrowserExample
2. Since this component has no dependencies on ReSwiftRouter/ReSwift and since it's highly dependent on UIKit, I think it would make sense to offer it as part of a separate micro-library instead of as part of ReSwiftRouter. ReSwiftRouter is intended to be unaware of the UI framework that is being used. However, if we can demonstrate that your component works well, it would make sense to link to it from this repo

Once again, thx for the PR 🎉

[ambientlight]

@Ben-G:

Do you happen to have a project that uses this component already? It would be a lot easier to review when seeing the component in action. If you don't have that, I can try to integrate it into https://github.com/ReSwift/GitHubBrowserExample

Let me make a sample project for it.

Since this component has no dependencies on ReSwiftRouter/ReSwift and since it's highly dependent on UIKit, I think it would make sense to offer it as part of a separate micro-library instead of as part of ReSwiftRouter.

Makes sense. Should we continue this conversation here or close and inside #17?

[Ben-G]

@ambientlight Happy to keep the conversation here, until we've decided where the API should live!

Thanks a lot for looking into a sample project. And to clarify: how did you come up with this solution? Because you saw the open issue here or because you solved the issue in your own project?

[ambientlight]

@Ben-G
I am using it in my app. Originally my router would override the back button on a pushed viewController, with a navigationItem selector associated with pop action.

I didn't like it this way, I thought it is possible to do it transparently - incorporate pop action inside navigationController, so I went to see how you guys do it, and since this wasn't yet resolved, I basically propose such implementation here.

My point to provide it within ReSwift-Router:
this is such a common problem that all users of ReSwift-Router will face, the solution to it should be available out-of-the box in ReSwift-Router without any extra dependency like ReSwift-Router-NavigationController.

[ambientlight]

@Ben-G:
I have updated the project you referenced. Please refer to ambientlight/GitHubBrowserExample@57403b4

Brief set of changes:

1. Removed SetRouteAction([mainViewRoute]) dispatches from BookmarkViewController and RepositoryDetailViewController since now it is handled with our navigationController.
2. Subclassed our navigationController and provided generic pop action:

class RootNavigationController: NavigationController {
    override func popRoute() {
        if let popRouteAction = popRouteAction(){
            store.dispatch(popRouteAction)
        }
    }
}

where popRouteAction is:

func popRouteAction() -> SetRouteAction? {
    let currentRoute = store.state.navigationState.route
    guard currentRoute.startIndex != currentRoute.endIndex else {
        return nil
    }
    
    return SetRouteAction([RouteElementIdentifier](currentRoute[currentRoute.startIndex ..< currentRoute.endIndex.advanced(by: -1)]))
}

3. Went to storyboard and replaced NavigationController class with our custom RootNavigationController

4. Added desired pop action into MainViewRoutable since now we need to actually perform perform pop corresponding to route change.

(self.viewController as? UINavigationController)?.popViewController(true, completion: completionHandler)

Please take a look and let me know if you have any questions.

[ambientlight]

@Ben-G What do you think? 🙂

[Ben-G]

Hey @ambientlight! Thanks a lot for putting this example together. It seems like you didn't add the new files to the Xcode project in the commit that you are pointing to? I have the source files in the repo, but they are not included in the project.

Overall I put more thought into this; I think this is a great starting point for a third-party library, but it is definitely outside of the scope of the core of ReSwift-Router which is and should remain independent of any UI library.

It would be really cool if you could build this as a standalone library (the code already has no real dependencies on ReSwift/ReSwift-Router), and I'd be more than happy to reference it.

I'd also be curious to see if there's a solution that doesn't require subclassing UINavigationController, but I probably won't have time to investigate that any time soon.

I will close this issue for now; but please let me know if you happen to build a separate library that I can reference in future.

Thanks a lot for the work on this 😃

(I'll also make sure to take some ideas from this approach if I update the GitHub sample project in future).

[ambientlight]

@Ben-G Will definitely do so. Let me get back to it and then I will probably open the issue for it once it is done.

Solution with UINavigationController subclass seems quite clean me. Are there any drawbacks?

Did I forget something? that's weird, will check.

Thanks a lot for the review and suggestions!

[ghost]

@ambientlight have you created a third-party library for this?

[ambientlight]

@hlineholm-flir: Thanks for reminding me, I will do my best to publish it soon (next week hopefully).
In the meantime you can just use this 100-lines navigation controller subclass.

1. Drop it in your project.
2. Create a subclass of this ReNavigationController with popRoute() override where you put action dispatch for your popRoute(). (like in the pull request description above)

import UIKit

// dummy view controller returned when popViewController is canceled
public final class PopWasIgnored: UIViewController {}

open class ReNavigationController: UINavigationController {
    
    fileprivate var isSwipping: Bool = false
    
    // indicates that backButton was pressed when set to false
    fileprivate var isPerformingPop: Bool = false
    
    override open func viewDidLoad() {
        super.viewDidLoad()
        self.interactivePopGestureRecognizer?.addTarget(self, action: #selector(NavigationController.handlePopSwipe))
        self.delegate = self
    }
    
    override open func popViewController(animated: Bool) -> UIViewController? {
        
        // when swipping we are discarding all subsequent popViewController calls
        guard !self.isSwipping else {
            return PopWasIgnored()
        }
        
        self.isPerformingPop = true
        return super.popViewController(animated: animated)
    }
    
    // will be called after popViewController call
    func handlePopSwipe(){
        self.isSwipping = true
    }
    
    /// should be overriden
    /// normally you should dispatch SetRouteAction here
    open func popRoute(){
        print("WARNING: \(#function) should to be overriden")
    }
}

extension NavigationController: UINavigationControllerDelegate {
    
    public func navigationController(_ navigationController: UINavigationController, didShow viewController: UIViewController, animated: Bool) {
        self.isSwipping = false
    }
}

extension NavigationController: UINavigationBarDelegate {
    
    // if overriden navigationController popViewController won't be called before this method
    // on back button press
    // isPerformingPop will be false here in this case
    public func navigationBar(_ navigationBar: UINavigationBar, shouldPop item: UINavigationItem) -> Bool {
        defer { isPerformingPop = false }
        
        // changeRoute is performed:
        // 1. back button was pressed
        // 2. pop swipe was triggerred
        if !isPerformingPop || self.isSwipping {
            self.popRoute()
        }
        
        // don't remove the navigationItem if navigationController
        // is not going to be popped
        return isPerformingPop
    }
}

extension UINavigationController {
    
    open func pushViewController(_ viewController: UIViewController, animated: Bool, completion: @escaping () -> Void) {
        
        self.pushViewController(viewController, animated: animated)
        
        guard animated, let coordinator = self.transitionCoordinator else {
            completion()
            return
        }
        
        coordinator.animate(alongsideTransition: nil) { _ in completion() }
    }
    
    open func popViewController(animated: Bool, completion: @escaping () -> Void) -> UIViewController? {
        
        let popped = self.popViewController(animated: animated)
        
        guard animated, let coordinator = self.transitionCoordinator else {
            completion()
            return popped
        }
        
        coordinator.animate(alongsideTransition: nil) { _ in completion() }
        return popped
    }
}

[ghost]

Ah! I went ahead and made RoutableUIKit, before reading your comment @ambientlight ...

[ambientlight]

@hlineholm-flir: that's great! I am not sure if RoutableUIKIt is a the most intuitive name for it, but let's now add cocoapods and carthage support to it!

[ghost]

Not sure either about the name there @ambientlight. I've created issues for both creating a CocoaPod and supporting Carthage. Think I can fix it during this week.