Fixes issues in Router.pushController, Router.popController & Router.setBackstack where pushChangeHandler().removesFromViewOnPush() was not respected

[chc-unwire]

Motivation: I encountered these issues while migrating existing code to androidX Navigation, and thus creating a ControllerNavigator along with others to support coexisting FragmentDialogs/Controllers with the hope of being able to just plug in Fragments (if the need ever arises) or hopefully Compose when its more mature and time permits.

Issue: During this pursuit, I started leveraging Router.setBackstack for all changes to the Router.backstack, and in my test app I had a flow that looked like this: AboutController -> DialogController (with pushChangeHandler().removesFromViewOnPush() = false) -> DetailsController

When pressing back while on DetailsController, the DialogController would re-appear, but without AboutController being visible underneath. During debugging I found that AboutController was not properly detached, even though its view was removed during the transition from DialogController -> DetailsController.

Another issue that would manifest is if we take the following backstack: AboutController -> DialogController (with pushChangeHandler().removesFromViewOnPush() = false) -> DetailsAController -> DetailsBController When pressing back while on DetailsBController, the transitions would not be as expected, since it would first transition from(DetailsBController) to(null), and then from(null) to(DetailsAController), rather than the expected from(DetailsBController) to(DetailsAController).

While I suppose not popping a dialog when navigating away from it, is atypical behaviour, I see no reason why it shouldn't be supported. A concrete use case in the app I'm working on is showing a dialog to select Time and then the option to open another dialog to select Date.

The Fix: I added three test cases to RouterTests which proved the issue prior to my fix, during which I discovered that pushController shared the same issue as setBackstack, and that popController would not properly attach all views. I also updated the demo app to have let the DialogController navigate to another Controller, to trigger some of this behaviour and verify transitions looked as expected. I realise that this code may not be something you'd want in the repo, but I thought I'd leave it in a separate commit, for your testing convenience - and for ease of removal if need be.

Inspired by how replaceTopController worked, I used that, along with ensuring that getVisibleTransactions would no longer incorrectly assume that any transaction prior to a removesFromViewOnPush=false transaction, is visible.

Perhaps there are other cases which I didn't think to trigger, so hopefully whoever reviews this has more intimate knowledge of the inner Router workings, and can have a think as to whether there are other cases, or whether it would be better to change performControllerChange to handle some of these fixes too. The issues seem related to #608, since both are about removesFromViewOnPush when manipulating the backstack, but I also noticed that ViewLeakTests.testViewRemovedIfLayeredNotRemovesFromViewOnPush was making the assumption that the Controller which comes before the one with removesFromViewOnPush=false remains attached even when pushing a new Controller with removesFromViewOnPush=true on top - which to me was the whole issue.

Thanks for many years of Conductor-awesomeness, I've truly been happy using it, just as I am happy to be able to contribute meaningfully (as you hopefully agree with! :) ) /Christian

Some brief video recordings of two issues before and after my fix:

https://user-images.githubusercontent.com/62065253/153869081-c0481500-b986-4305-8a13-aa187c8b0c5a.mp4

https://user-images.githubusercontent.com/62065253/153869095-e52bf9ca-0990-4f24-a548-194a98fc332c.mp4

https://user-images.githubusercontent.com/62065253/153869101-cd7aa9de-7b69-4b25-bcad-5cb928fe36f8.mp4

https://user-images.githubusercontent.com/62065253/153869099-3642f40b-153b-47cf-9b11-e4bab9032202.mp4

[EricKuck]

I'm really sorry it took so long to respond to this. This is a very high-quality PR and I really appreciate the effort.

Overall it looks like a good improvement and fixes a real issue. Unfortunately there are a few cases in which it doesn't work well. For instance, try having the dialog controller push to another dialog controller - before your fix, it transitions to and from a different dialog nicely. After the fix, things get visually confusing.

I know it's been quite a while since you originally opened the PR. Are you up for working on a fix for this other case?

Again, I'm sorry it took so long to respond, and thanks for the great PR.

[chris6647]

I'm absolutely up for looking into this case as well. Appreciate you going through it and testing it well enough to find that issue! As mentioned in the PR this would let me remove some hacky things from the ControllerNavigator in the androidX Navigation world, which I'm working on. Honestly I'd kinda given up on getting feedback, so I really appreciate the kind words - I did strive towards aligning with the existing code base and making the PR as frictionless as possible :) I'm the same person as the PR opener, but for company reasons they wanted me to open the PR using one with the company name, Unwire, in it. I'll get around to this as soon as possible, and hopefully the response-time will be quicker this time around :D

[chris6647]

Testing your description of using pushChangeHandler(FadeChangeHandler(false)) looked fine to me, but using pushChangeHandler(HorizontalChangeHandler(false)), I see some oddities. Is this the case you are referring to?

If so, then I'm unsure what the desired behaviour ought to be. If multiple different screens in a row, chooses to not pop any of the preceding views, but their animations are of the sort that seemingly transition views out of bounds, then what should happen? The next screen' pushChangehandler says to not pop the view, but it was transitioned out, so it is no longer visible. That means the definition of the animation, and the forceRemoveViewOnPush contradicts each other. I'd say the definition of the animation ought to win, and ignore the forceRemoveViewOnPush flag, in which case the HorizontalChangeHandler wins and the fromView gets removed anyway. I.e. the HorizontalChangeHandler should not support forceRemoveViewOnPush at all. Nor should the VerticalChangeHandler or SimpleSwapChangeHandler. Only the FadeChangeHandler really makes sense to me, to allow a fade transition to occur on top of something, without removing that something.

But if you really have several screens in a row, which are supposed to overlay each other, then the only sensible way for me would be if they each occupy different parts of the screen and thus are visible and interactable at the same time, then you need to take special care with custom ChangeHandlers anyway. I even had such a scenario in production, with a HorizontalChangeHandler and I had to extend it to make a sort of HorizontalOnTopChangeHandler which would ignore the to all together, and thus not transition it out of bounds.

Looking forward to hearing your thoughts.