Fix UINavigationControllerDelegate's forwarding

[tgrapperon]

#307 changed the behavior of the UINavigationControllderDelegate's forwarding: all the methods of this protocol are optional but the only cases where we'd want to return true to respondsToSelector are:

• When the base delegate is not nil and responds to the selector ¹
• When the method is navigationController(_:didShow:animated:) which is the only one where we perform some effective business logic.

Otherwise, the current implementation forces base to properly implement all delegate methods (which is not a requirement when using a standard UINavigationController). For example, this breaks interactive pop if base is only used to track willShow, as UIKit will otherwise assume that the delegate implements animationControllerFor:from:to: and returns nil, which iOS infers as having no interactive dismissal (the documentation is inexact and nil means no animationController, not "use defaults", at least from Swift)

As a bonus, it allows to simplify the implementation.

Footnotes

1. Note that is this not 100% correct, as we're not forwarding any method, but in that occurrence, we only expect UINavigationControllerDelegate calls to be made. I'm not sure we want to start tweaking the perform methods. ↩

[acosmicflamingo]

Aw man, that's a darn shame. Hm, I wonder if this would be a good compromise:

diff --git a/Sources/UIKitNavigation/Navigation/NavigationStackController.swift b/Sources/UIKitNavigation/Navigation/Nav
igationStackController.swift
index 11468d70..a3bfe0f1 100644
--- a/Sources/UIKitNavigation/Navigation/NavigationStackController.swift
+++ b/Sources/UIKitNavigation/Navigation/NavigationStackController.swift
@@ -196,8 +196,25 @@
       weak var base: (any UINavigationControllerDelegate)?
 
       override func responds(to aSelector: Selector!) -> Bool {
-        (MainActor._assumeIsolated { base?.responds(to: aSelector) } ?? false)
-          || aSelector == #selector(navigationController(_:didShow:animated:))
+        aSelector == #selector(navigationController(_:didShow:animated:))
+        || MainActor._assumeIsolated {
+          let interactiveSelectors = [
+            #selector(navigationController(_:willShow:animated:)),
+            #selector(navigationController(_:interactionControllerFor:)),
+            #selector(navigationController(_:animationControllerFor:from:to:))
+          ]
+          let isInteractive =
+            if let base = base as? NavigationStackInteractiveDelegate,
+               base.supportsInteractivePop,
+               interactiveSelectors.contains(aSelector)
+            {
+              true
+            } else {
+              false
+            }
+          return isInteractive
+          || base?.responds(to: aSelector) ?? false
+        }
       }
 
       func navigationController(
@@ -314,6 +331,13 @@
     }
   }
 
+  @MainActor
+  public protocol NavigationStackInteractiveDelegate: UINavigationControllerDelegate {
+    // Marks that the delegate can provide interactive pop behavior
+    var supportsInteractivePop: Bool { get }
+  }
+
+
   extension UIViewController {
     @available(iOS, deprecated: 17, renamed: "traitCollection.push")
     @available(macOS, deprecated: 14, renamed: "traitCollection.push")

I've been trying all day to try and get my code to work with your changes, but have been unsuccessful.

[acosmicflamingo]

Oh no, base isn't the subclassed NavigationStackController, so that won't work. :(

[tgrapperon]

I don't think it needs any compromise. The proposed change should make it work as expected: if you implement a method with your delegate, it is spotted and called by UIKit, otherwise, it's no-op, like if we wouldn't be proxying through PathDelegate. It should work in your case and mine, and I believe setting a delegate with no method being implemented should result in the same as not setting the delegate at all (which is not currently the case). Something is maybe off somewhere, but I think the change makes sense. Your delegates methods should be spotted and hit with the first check, provided your delegate was also working with a standard UINavigationController. If you put breakpoints, are they hit?

[acosmicflamingo]

I am getting the willShow called in my own delegate as expected. However, it's the following functions that are getting ignored (the breakpoint is never touched from the first line of either of those functions):

public func navigationController(
    _ navigationController: UINavigationController,
    animationControllerFor operation: UINavigationController.Operation,
    from fromVC: UIViewController,
    to toVC: UIViewController
  ) -> UIViewControllerAnimatedTransitioning? {
  }
  
   public func navigationController(
    _ navigationController: UINavigationController,
    interactionControllerFor animationController: UIViewControllerAnimatedTransitioning
  ) -> UIViewControllerInteractiveTransitioning? {
  }

[mbrandonw]

@tgrapperon What bad behavior are you seeing with the changes?

This change essentially undos everything done in #307, which I agree what was in pre-#307 does seem correct to me. However, both @acosmicflamingo and @takehilo seemed to observe that the changes fixed some bad behavior they were seeing.

[acosmicflamingo]

It could also be that both @takehilo and I used the same approach in setting up interactive transitions that was problematic from the get-go and this PR exposes the issue in our codebases. I'm in favor of reverting the changes; just now at a loss as to how to get my code to work with it.

Edit: if I am going to use my UIPercentDrivenInteractiveTransition subclass properly instead of some hackery I'm doing in my end, then willShow is never called.

[tgrapperon]

I happen to have a delegate that was tracking willShow, and the changes from #307 disabled interactive dismissal in my NavigationStackController. I also agree that that PR effectively reverts to pre #307. I worked from the current state and didn't check what we had before.

I just confirmed that a UINavigationController with a delegate that implements no methods doesn't break interactive dismissal, and returning nil with a animationControllerFor method disables it. In other words, I don't know if it's possible to have system interactive dismissal with #307, which I also just confirmed to be disabled in the Case Studies.

My feeling is that returning that we respond to a selector when the base delegate doesn't is not correct. From UIKit's point of view, #307 makes the delegate to implement all the methods from the protocol, and unfortunately, that's not no-op (maybe it would be using Obj-C, but it isn't from Swift).

[tgrapperon]

I'm not an expert with Swift/Obj-C interop because it's been a long time, and it also evolved a lot, but it's possible that some delegate methods should be annotated with @objc or something to be picked by repondsToSelector.

[acosmicflamingo]

Wait a minute...oh my goodness I wonder if it has to do with not having @objc in the functions; I've had issues like that in the past when subclassing delegates like that. I will try it when I'm in front of a keyboard.

[acosmicflamingo]

I now understand what's happening. When the app is first launched and the NavigationStackController is created, the PathDelegate's responds(to aSelector: Selector!) function is called many times, going through all the different selectors that are proposed to be removed, but base is nil; the delegate hasn't been setup yet:

aSelector: Optional(navigationController:willShowViewController:animated:)
(lldb) po base
nil
aSelector: Optional(navigationController:didShowViewController:animated:)
(lldb) po base
nil
aSelector: Optional(navigationController:animatorForOperation:fromViewController:toViewController:)
(lldb) po base
nil

aSelector: Optional(navigationController:animationControllerForOperation:fromViewController:toViewController:)
(lldb) po base
nil

aSelector: Optional(navigationController:interactionControllerUsingAnimator:)
(lldb) po base
nil
aSelector: Optional(navigationController:interactionControllerForAnimationController:)
(lldb) po base
nil

aSelector: Optional(_navigationControllerShouldCrossFadeNavigationBar:)
(lldb) po base
nil
aSelector: Optional(_navigationControllerShouldCrossFadeBottomBars:)
(lldb) po base
nil

aSelector: Optional(_navigationControllerShouldUseBuiltinInteractionController:)
(lldb) po base
nil

aSelector: Optional(navigationControllerSupportedInterfaceOrientations:)
(lldb) po base
nil
aSelector: Optional(navigationControllerPreferredInterfaceOrientationForPresentation:)
(lldb) po base
nil

aSelector: Optional(_navigationControllerLayoutTransitioningClass:)
(lldb) po base
nil

aSelector: Optional(navigationController:willShowViewController:animated:)
(lldb) po base
some

It's only after all the functions have been called do we eventually see that base is set, which happens when my NavigationStackController's subclass sets it (delegate = myAlreadyInitializedNavigationControllerDelegate). By the time our view controllers are being pushed and popped into the stack, it's already too late and something was missed during the configuration that would help us out here. @tgrapperon and @mbrandonw 's suspicions were correct; base does return true when selectors like navigationController:interactionControllerForAnimationController: are passed to the responds function. I'll try and figure out how to fix this.

[tgrapperon]

I have no concerns closing that PR if we revert, but I wonder if it's worth saving the test @acosmicflamingo made in #307. In that case, I just rearranged so my implementation matches what we had pre-#307, and merging it would effectively soft-revert and keep the test (provided it's not failing). Otherwise I can close and we can consider reverting #307.

[acosmicflamingo]

I think it'd be helpful. Plus, I have found a fix that is compatible with this PR. Looks like we don't have to do anything complicated like re-create PathDelegate whenever base is updated. The selector function isn't even called until this line in viewDidLoad():

open class NavigationStackController: UINavigationController {
  /* */
  open override func viewDidLoad() {
    super.viewDidLoad()

    // Base should be configured by the time super.delegate is called for 
    // delegates of derived classes to work
    super.delegate = pathDelegate

    if #available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) {
      traitOverrides.push = UIPushAction { [weak self] value in
        self?._push(value: value)
      }
    }
  }
}

So as long as any subclasses configure their delegate before executing super.viewDidLoad(), everything will work perfectly.

[acosmicflamingo]

@tgrapperon @mbrandonw @stephencelis what do you all think about me merging this kind of change to this PR that adds a viewDidLoad(delegate: (any UINavigationControllerDelegate)?) function that allows users to not have to remember to configure their delegate before calling viewDidLoad()? Changes would be simple like this:

     open override func viewDidLoad() {
       super.viewDidLoad()
+      configureDelegate()
+      configureBindings()
+    }
+
+    open func viewDidLoad(delegate: (any UINavigationControllerDelegate)? = nil) {
+      configureDelegate(delegate: delegate)
+      super.viewDidLoad()
+      configureBindings()
+    }
 
+    private func configureDelegate(
+      delegate: (any UINavigationControllerDelegate)? = nil
+    ) {
+      pathDelegate.base = delegate
       super.delegate = pathDelegate
+    }
 
+    private func configureBindings() {
       if #available(iOS 17, macOS 14, tvOS 17, watchOS 10, *) {
         traitOverrides.push = UIPushAction { [weak self] value in
           self?._push(value: value)

This behavior also means that everything works as-is even when users do not have a delegate to pass to it.

[acosmicflamingo]

Looking further into my code, I realize that the solution is actually very, very simple: set your navigation controller's delegate before it's called viewDidLoad(). The reason using it in my init wasn't working was...drum roll the convenience initializer was never being called in the first place!

@tgrapperon make this change in your PR and the test will pass:

diff --git a/Examples/CaseStudiesTests/NavigationStackTests.swift b/Examples/CaseStudiesTests/NavigationStackTests.swift
index bcec16ff..e679e7a4 100644
--- a/Examples/CaseStudiesTests/NavigationStackTests.swift
+++ b/Examples/CaseStudiesTests/NavigationStackTests.swift
@@ -299,6 +299,13 @@ final class NavigationStackTests: XCTestCase {
     let nav = NavigationStackController(path: $path) {
       UIViewController()
     }
+    // Configure navigation delegate before it's viewDidLoad
+    // function has been called
+    let interaction = MockInteractiveTransition()
+    let delegate = MockNavigationControllerDelegate()
+    delegate.interactionController = interaction
+    nav.delegate = delegate
+
     nav.navigationDestination(for: Int.self) { number in
       ChildViewController(number: number)
     }
@@ -308,10 +315,7 @@ final class NavigationStackTests: XCTestCase {
     await assertEventuallyEqual(nav.viewControllers.count, 2)
     await assertEventuallyEqual(path, [1])
 
-    let interaction = MockInteractiveTransition()
-    let delegate = MockNavigationControllerDelegate()
-    delegate.interactionController = interaction
-    nav.delegate = delegate
+
 
     let interactionExpectation = expectation(
       description: "navigationController(_:interactionControllerFor:) called"

After I set the delegate in my subclass right after calling super.init, everything works:

public class MyNavigationController: StackNavigationController {
  /* */
  public required init(
    navigationBarClass: AnyClass? = nil,
    toolbarClass: AnyClass? = nil,
    path: UIBinding<UINavigationPath>,
    root: () -> UIViewController
  ) {
    super.init(
      navigationBarClass: navigationBarClass,
      toolbarClass: toolbarClass,
      path: path,
      root: root
    )
    self.delegate = navigationControllerDelegate
  }
}

[acosmicflamingo]

Alright, here's the PR that should make everyone happy :) #312

[tgrapperon]

Closing, as it will be handled in #312 likely