Allow a Coordinator to be requested to finish

[EBusch]

This is a proposal for an idea that gives parent coordinators a way of attempting to finish the children before they finish.

This is a proposal for an idea that adds a method on coordinators to request finishing, as well as parent coordinators to allow a delayed finish based on waiting for the child coordinators to finish.

[github-actions]

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 8a9b184.

♻️ This comment has been updated with latest results.

[mediabounds]

I think this is a worthwhile addition--for a long time, we've been hesitant to allow other classes "Finish" a coordinator because it is (was) hard to manipulate the navigation stack accordingly. While that is still somewhat true, we've been implementing one-off instances of this logic in many of our coordinators, so we should formalize it.

I have some suggestions to the approach:

1. Let's rename the method to RequestFinish and make it a method on ICoordinator
   • Consider whether it makes sense to make this method async
2. Define a new virtual protected method in Coordinator called HandleFinishRequested(ICoordinator, EventArgs)
   1. The ICoordinator should be the coordinator that was requested to finish
   2. The EventArgs should be the original arguments passed into RequestFinish
   3. The default implementation of this method should reset the navigation context if ICoordinator equals itself
   4. This method should also check whether managedPage is set -- if it is null, then Finish should be called
      • If there is no managedPage, then Finish would not automatically be called after reseting the navigation context
3. Coordinator should implement RequestFinish by simply calling HandleFinishRequested (i.e. HandleFinishRequested(this, args))
   1. Coordinator should declare it's implementation of RequestFinish as final
4. CoordinatorParent should override HandleFinishRequested to...
   1. Call HandleFinishRequested on all of it's children (the ICoordinator argument will be the parent)
   2. Store waitingToFinishEventArgs so that the parent coordinator can be finished with the original set of arguments
   3. Wait for all children to finish before the parent is finally Finished with the original event args.

A few key points to this approach:

• Defines a public method for all coordinators to be finished from outside the coordinator
• Clarifies in the name of the method that from outside the coordinator, a finish can merely be requested, but not guaranteed
• Implements a reasonable default for finishing a coordinator (reseting the navigation context)
• Ensures that when there are multiple coordinators being finished that there is enough context given to each child about the origin of the request (i.e. which was the coordinator being requested to finish)

Once this is done, this should replace instances where we created a KillCoordinator method in implementations.

[EBusch]

@mediabounds I mostly followed your notes here and it seems to be working well! I diverged a couple spots, mostly because I think there may have been a typo. I actually think in a lot of ways we'd be better served by getting more of these navigation methods to be asynchronous, which may also open us up for better animations, code flow, safety checks, etc.

[mediabounds]

@EBusch We're probably missing a call to the base implementation here. When you add that, you may have an issue with Finish getting called twice:

Coordinator.HandleNavigation may call it and so will CoordinatorParent.HandleChildFinish -- it is not currently clear to me the best way to address that.

[EBusch]

@mediabounds a few things:
-Curious as to your thoughts of dealing with the conflict here. Perhaps we should have a branch specifically for Forms? maybe legacy?
-I think you were right on the idea of moving away from booleans and I think in some ways I added some things here that aren't fully implemented in Float.Core itself but would make more sense to implement on the implementing-app side as the cases will be more unique.
-Some of the calls to the base implementation might be a bit different from what you were originally thinking but I think this is something that we often want to overwrite the behavior of the base class rather than supplementing it.

[mediabounds]

Most of the feedback from last time wasn't addressed--I reopened those conversations. Maybe not all the code got pushed?

As for the conflict--the latest version of Float.Core should be compatible with both Maui and Xamarin.Forms so I don't think a legacy branch will be needed.

[EBusch]

@mediabounds you were right, I missed pushing one of my commits. Whoops! Taking another spin through now to make sure I covered off on everything.

[EBusch]

Not quite sure how we can get this one building again. The errors all seem to be in unchecked files, but previous pull requests worked, which is strange. Interestingly, my local gets a different issue when trying to build this, that I'm missing maui-tizen. I've tried installing the workloads with no such luck. Very strange...

[mediabounds]

After SL-8129 has been tested and merged; the implementation here should be reviewed/updated to make sure it matches what was tested.