fix initial routes do not run secondary animation when pops

[chunhtai]

Description

Whenever a new route pushed on top of another route, it will chains the secondary animation of bottom route with the primary animation of top route. However, there is a bug in the method that chain them together. In the method, we try to do train hopping if the two animations does not have the same values. It is possible that the two animations are animating with different directions and the hopping will never happen, and that introduce this bug.

here are the steps, assume initial route is routeA/routeB
1, routeA pushed with pri-animation=1.0, sec-animation=0.0
2, routeB pushed with pri-animation=1.0, and it try to do train hopping on sec-animation of routeA which never happen. The sec-animation of routeA stay at 0.0
3, when routeB pop, the routeA didpopnext will try to do the train hopping again with sec-animation of routeA = 0.0 and pri-animation of routeB =1.0 in reverse order. This is wrong because the sec-animation of routeA should be 1.0 at this moment and animate with pri-animation of routeB.

Related Issues

Closes #46000

Tests

I added the following tests:
"secondary animation is triggered when pop initial route"

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: Replace with a link to your migration guide

[chunhtai]

@goderbauer you wrote this test. However, something is unclear what the expected behavior should be.
The last pop trigger a train hopping where pri-animation of top most route = 0.16 reverse and sec-animation of the route below =0.67 forward. They will never meet and sec-animation just hang there until everything is disposed. This is what it looks like in real world

[chunhtai]

This is what it looks like after this pr

[goderbauer]

These animations both look broken.

[goderbauer]

How do we know that the two trains will never cross in the future?

[goderbauer]

The code below also doesn't seem to check that the trains will not cross in the future.

[chunhtai]

The check is to detect whether they will cross with the current animations.

[goderbauer]

The test below only looks at the current value and the direction of the animation. How's that enough to determine if they will cross? AFIK, the animation values doesn't have to run monotonically from 0.0 to 1.0. It could bounce up and down in between?

[goderbauer]

No need to have the else branches here. I would write this as:

if (conditionA)
  return ...;
if (conditionB)
  return ...;
return ...;

[goderbauer]

These animations both look broken.

[goderbauer]

nit: remove extra blank line

[goderbauer]

I wonder if we just need to change this condition to jump directly when the values are the same and to also just jump directly when the nextTrain animation is completed (meaning the next route appeared on top of this route without animation).

[chunhtai]

This will only fix the initial route issue. what about the test below?

[goderbauer]

Oh, right. So somehow we need to guarantee that we will jump even when the trains haven't crossed at the end of the animation.

[chunhtai]

@goderbauer This is blocking the work of navigator page api. When you re-arrange the routes and try to update the secondary animation it is really easy to hit this case where the train hopping never finishes because the animation of next route is already completed

[goderbauer]

LGTM

[goderbauer]

How do you know at this point that there's an existing train hopping in progress?

[chunhtai]

whoever calls this method will need to make sure to set this private field to null as to indicate there is no more train hopping in progress. I think the comment in line 253 is not clear enough, will update

[goderbauer]

use _updateSecondaryAnimation to make it clear that you mean this method?

[goderbauer]

nit: end with .

[goderbauer]

it should be -> it has been

[goderbauer]

nit: start with a capital letter ("Secondary").

[goderbauer]

Could we just set _trainHoppingListenerRemover to null at the very beginning of this method after we copied it into previousTrainHoppingListenerRemover?

[ldelafuente]

Hi @chunhtai , do you have a planned date to release this in the stable branch?
I tried patching routes.dart on the latest stable release with the insertions you made on this PR, but secondary animations still don't run for initial routes. Do these changes have dependencies on other files maybe?

I'm using nested navigators and the initial route doesn't pop even if I explicitly set initialRoute to false

[chunhtai]

@ldelafuente This patch does not depend on other file, can you provide a runnable example that reproduce the problem you are having? I don't know when the next stable release will be yet