Fix onDismiss not called when view controller is explicitly dismissed

[acosmicflamingo]

Looks like present(item:content:) function in UIKit does not successfully call onDismiss as one would expect. I'm not seeing a print statement when I do this:

// Using item parameter
present(item: $model.destination.alert, id: \.self, onDismiss: {
  print("Yay!!!")
}) { message in
  let alert = UIAlertController(
    title: "This is an alert",
    message: message,
    preferredStyle: .alert
  )
  alert.addAction(UIAlertAction(title: "OK", style: .default))
  return alert
}

// Using isPresented parameter
present(isPresented: UIBinding($model.destination.alert), onDismiss: {
  print("Yay!!!")
}) { //message in
  let alert = UIAlertController(
    title: "This is an alert",
    message: "LOL",
    preferredStyle: .alert
  )
  alert.addAction(UIAlertAction(title: "OK", style: .default))
  return alert
}

Going through breakpoints, it appears that controller is always nil: https://github.com/pointfreeco/swift-navigation/blob/main/Sources/UIKitNavigation/Navigation/Presentation.swift#L377. I find it strange because I the debugger does at some point set the dict with a value: https://github.com/pointfreeco/swift-navigation/blob/main/Sources/UIKitNavigation/Navigation/Presentation.swift#L365. Dismissal was working properly, and I have a better understanding of how caching works after spending a few minutes reading the code, so it didn't look like the problem resided there.

The next thing that stood out to me was the weakified controller property in Presented, and that the dictionary's controller properties were being immediately deallocated. Removing weak will certainly fix this, and I explicitly deallocate it in the deinit to mimic the weak behavior.

[mbrandonw]

Hi @acosmicflamingo, thanks for looking into this! However, it seems to have broken the test testPushViewController_ManualPop. I don't have time to look into why right now, but that is something that would need to be taken care of.

It would also be good to get some test coverage on this change to prove that it behaves the way you say it does.

[acosmicflamingo]

Oh okay! Sure, I can investigate and the test suggestion is a great one :)

[acosmicflamingo]

@mbrandonw I added a test that checks whether vc.isPresented is false during explicit dismissal. It works as expected when dismissal occurs using UITraitCollection functionality (in testPresents_TraitDismissal function), but it looks like calling vc.presentedViewController?.dismiss(animated: false) does not actually set vc.isPresented to false as one would expect.

[acosmicflamingo]

@mbrandonw revisiting this, I noticed locally that the test that was crashing also did so even without these changes. Something tells me that when the CI ran months ago and failed in testPushViewController_ManualPop, that it was also failing in the main branch.

Running this again, I see that the tests are all now passing. Perhaps the perfect solution was the one that was with us the whole time ;)

[acosmicflamingo]

I GOT IT! Something puzzled me how if there truly was an issue going on, you'd think the presentation tests would catch this. However, main currently is green.

I believe the tests are missing a specific use-case of present(item:id:) function, where item is of the UIBinding type and is an Identifiable type (and #291 gave me this idea). When I added a test for this, the test failed as it should have.

[acosmicflamingo]

I'm going to create a separate PR that adds the test to demonstrate the issue, and will then create a PR for that branch to show that it fixes the problem.

[acosmicflamingo]

NOOOOOOOO! The most basic test I wrote to check will only sometimes fail at the very last line :(

@MainActor
func testPresents_Dismissal() async throws {
  let vc = BasicViewController()
  try await setUp(controller: vc)

  await assertEventuallyNil(vc.presentedViewController)

  withUITransaction(\.uiKit.disablesAnimations, true) {
    vc.model.isPresented = true
  }
  await assertEventuallyNotNil(vc.presentedViewController)

  withUITransaction(\.uiKit.disablesAnimations, true) {
    vc.presentedViewController?.dismiss(animated: false)
  }
  await assertEventuallyNil(vc.presentedViewController)
  await assertEventuallyEqual(vc.model.isPresented, false)
  await assertEventuallyEqual(vc.isPresenting, false)
}

[acosmicflamingo]

Going to close this in favor of this PR: #303