Fix janks and memory leaks in `CupertinoPageTransition` and `CupertinoFullscreenDialogTransition`

ValentinVignal commented 4 weeks ago

Initially, part of https://github.com/flutter/flutter/issues/141198 to fix leak memories in CupertinoPageTransition. But it was overlapping with https://github.com/flutter/flutter/pull/147133 which got merged first. During my work, I noticed the CupertinoPageTransition and CupertinoFullscreenDialogTransition's animations states were lost when a push was interrupted with a pop. When that is the case, it should use the forward curve for the reverse animation to avoid discontinuities. This PR fixes it

Pre-launch Checklist

[x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
[x] I read the Tree Hygiene wiki page, which explains my responsibilities.
[x] I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
[x] I signed the CLA.
[x] I listed at least one issue that this PR fixes in the description above.
[x] I updated/added relevant documentation (doc comments with ///).
[x] I added new tests to check the change I am making, or this PR is test-exempt.
[x] I followed the breaking change policy and added Data Driven Fixes where supported.
[x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ValentinVignal commented 4 weeks ago

cc @polina-c

polina-c commented 3 weeks ago

Looks puzzling. Maybe usage after disposal?

ValentinVignal commented 3 weeks ago

@polina-c I managed to "fix" the tests in route_test.dart, I tried to explain them, tell me if something is obscure

ValentinVignal commented 3 weeks ago

@polina-c This was the same as https://github.com/flutter/flutter/pull/147133 but I believe there are some flows in the PR that this one fixes. I think we can keep it open

polina-c commented 3 weeks ago

@Piinks , Valentin is suggesting functional fix in the animation, as side effect of the leak fix. Can you help to review it or suggest someone who has enough expertise?

polina-c commented 3 weeks ago

@polina-c This was the same as #147133 but I believe there are some flows in the PR that this one fixes. I think we can keep it open

Nope, if the change is good, we want to push it forward. You believe it is good, right? It just will take more time to explain it to others :)

ValentinVignal commented 3 weeks ago

You believe it is good, right?

I've at least started to convince myself it is good haha 😅 It for sure needs a confirmation from someone with expertise

ValentinVignal commented 2 weeks ago

@ValentinVignal it looks like there are a lot of changes here unrelated to fixing the leak, some of the changes are identical code, just moved around. I can't actually tell what has changed to fix the leak. Can you revert those unrelated changes?

Sorry about that, it's because we had 2 PRs with conflicts. I can try to start everything again

ValentinVignal commented 2 weeks ago

@Piinks @polina-c The changes should have been cleaned now

polina-c commented 1 week ago

@MitchellGoodwin , may I get your formal approval to kick off google testing?

auto-submit[bot] commented 1 week ago

auto label is removed for flutter/flutter/146999, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

polina-c commented 1 week ago

Google testing is marked as red, but the tests on CL actually passed. Marking as infra failure.

flutter / flutter

Fix janks and memory leaks in `CupertinoPageTransition` and `CupertinoFullscreenDialogTransition` #146999

Pre-launch Checklist