Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix janks and memory leaks in CupertinoPageTransition and CupertinoFullscreenDialogTransition #146999

Merged
merged 5 commits into from
May 6, 2024
Merged

Fix janks and memory leaks in CupertinoPageTransition and CupertinoFullscreenDialogTransition #146999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
939d8db
fix: Avoid janks when push is interrupted
ValentinVignal Apr 29, 2024
a2758a1
test: Update the tests
ValentinVignal Apr 29, 2024
72a5113
refactor: Use pumpAndSettle to finish animations
ValentinVignal Apr 30, 2024
8f3c106
test: Add back missing test
ValentinVignal May 5, 2024
67f3775
Merge branch 'master' into flutter/fix-memory-leaks-in-curpertino-pag…
polina-c May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/flutter/lib/src/cupertino/route.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,22 +435,25 @@ class _CupertinoPageTransitionState extends State<CupertinoPageTransition> {
super.didUpdateWidget(oldWidget);
if (oldWidget.primaryRouteAnimation != widget.primaryRouteAnimation
|| oldWidget.secondaryRouteAnimation != widget.secondaryRouteAnimation
|| oldWidget.child != widget.child || oldWidget.linearTransition != widget.linearTransition) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to the memory leak? If not, maybe this should be a separate change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well now this PR is not about memory leaks anymore since #147133 got merged in between. Maybe I should rename this PR. Would "Fix jank in cupertino page transition" sounds nice to you ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a great idea. Can you update the PR description to what issue this is fixing now as well? Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the title and the description of the PR. Tell me if something is unclear

|| oldWidget.linearTransition != widget.linearTransition) {
_disposeCurve();
_setupAnimation();
}
}

@override
void dispose() {
super.dispose();
_disposeCurve();
super.dispose();
}

void _disposeCurve() {
_primaryPositionCurve?.dispose();
_secondaryPositionCurve?.dispose();
_primaryShadowCurve?.dispose();
_primaryPositionCurve = null;
_secondaryPositionCurve = null;
_primaryShadowCurve = null;
}

void _setupAnimation() {
Expand Down Expand Up @@ -557,7 +560,6 @@ class _CupertinoFullscreenDialogTransitionState extends State<CupertinoFullscree
super.didUpdateWidget(oldWidget);
if (oldWidget.primaryRouteAnimation != widget.primaryRouteAnimation ||
oldWidget.secondaryRouteAnimation != widget.secondaryRouteAnimation ||
oldWidget.child != widget.child ||
oldWidget.linearTransition != widget.linearTransition) {
_disposeCurve();
_setupAnimation();
Expand Down
199 changes: 193 additions & 6 deletions packages/flutter/test/cupertino/route_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,10 @@ void main() {
);
});

testWidgets('Fullscreen route animates correct transform values over time', (WidgetTester tester) async {
testWidgets('Fullscreen route animates correct transform values over time',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await tester.pumpWidget(
CupertinoApp(
home: Builder(
Expand Down Expand Up @@ -712,11 +715,27 @@ void main() {
await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dy, moreOrLessEquals(7.4, epsilon: 0.1));

await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dy, moreOrLessEquals(3, epsilon: 0.1));

await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dy, moreOrLessEquals(0, epsilon: 0.1));

// Give time to the animation to finish and update its status to
// AnimationState.completed, so the reverse curved can be used in the next
// step.
await tester.pumpAndSettle(const Duration(milliseconds: 1));

// Exit animation
await tester.tap(find.text('Close'));
await tester.pump();

await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dy, moreOrLessEquals(156.3, epsilon: 0.1));

await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dy, moreOrLessEquals(308.1, epsilon: 0.1));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dy, moreOrLessEquals(411.03, epsilon: 0.1));

Expand Down Expand Up @@ -818,25 +837,173 @@ void main() {
await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-263.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-265.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-266.0, epsilon: 1.0));

// Give time to the animation to finish and update its status to
// AnimationState.completed, so the reverse curved can be used in the next
// step.
await tester.pumpAndSettle(const Duration(milliseconds: 1));

// Exit animation
await tester.tap(find.text('Close'));
await tester.pump();

await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-197.0, epsilon: 1.0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change from -83 to -197 looks alerting to me.
The test passes at flutter master with -83. And it stops passing with -83 after your change. That means your change is not just improvement in memory management, it modified functionality, that should not happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I was trying to "fix" those changes in the tests. However, with my changes the behavior changes and I will have to update those values.

Before, a new CurvedAnimation was recreated at every build (in the constructor of the stateless widget CupertinoPageTransition). Now, it is reusing the same one. This leads to a change in the functionality (which I believe is correct).

As a reminder, here are some useful values:

Curves.easeInToLinear.transform(0.72); // 0.3149519026279449
- (800 / 3) * Curves.easeInToLinear.transform(0.72);  // -83.98717403411865
Curves.linearToEaseOut.transform(0.72); // 0.979008914809674
- (800 / 3) * Curves.linearToEaseOut.transform(0.72); // -261.0690439492464

And the secondary animation is

_secondPositionCurvedAnimation = CurvedAnimation(
  parent: widget.secondaryRouteAnimation,
  curve: Curves.linearToEaseOut,
  reverseCurve: Curves.easeInToLinear,
);

Before, the test was animating the push of the page from 0ms to 400ms (the entire duration of the push animation is 500ms) and then popping. So the push animation was not completed when being reversed. During the forward animation, the curve used is Curves.linearToEaseOut (which is correct).
During the pop/reverse animation, because the CurvedAnimation is recreated, it was using the reversedAnimation: Curves.easeInToLinear which led to this -83 value.

But this shouldn't happen, if the animation is not complete, it should use its forward curve during the reverse animation. I guess this is to prevent jumps and janks.

This is what those lines are doing:

flutter/packages/flutter/lib/src/animation/animations.dart

Lines 427 to 432 in 89a4ffa

void _updateCurveDirection(AnimationStatus status) {
_curveDirection = switch (status) {
AnimationStatus.dismissed || AnimationStatus.completed => null,
AnimationStatus.forward || AnimationStatus.reverse => _curveDirection ?? status,
};
}

flutter/packages/flutter/lib/src/animation/animations.dart

Lines 434 to 436 in 89a4ffa

bool get _useForwardCurve {
return reverseCurve == null || (_curveDirection ?? parent.status) != AnimationStatus.reverse;
}

flutter/packages/flutter/lib/src/animation/animations.dart

Lines 450 to 457 in 89a4ffa

double get value {
final Curve? activeCurve = _useForwardCurve ? curve : reverseCurve;
final double t = parent.value;
if (activeCurve == null) {
return t;
}
if (t == 0.0 || t == 1.0) {

Now that the CurvedAnimation is stored in the state and reused, it keeps using the forward curve and doesn't "jump" to the reversedCurve, which I believe fixes a jank/jump that currently exists on the master channel.

For the test, I decided the modify it so the push animation finishes and the pop uses the reversedCurved by changing the durations from 40ms to 50ms. But I can revert those duration changes and make the pop use the forward curve instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than changing an existing test, can you add a new one? This feel like a potential regression.

Copy link
Contributor

@MitchellGoodwin MitchellGoodwin Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on leaving this test alone as much as possible. It does look like logically the animation wouldn't finish by the time the original test tries to go back, so the jump makes sense. I'd rather the test was left as it was to check for regression, even with the jump, and probably renamed. A new test that correctly waits for the animation to finish, then goes back would be preferable.

Does the original test still fail, and how?

Copy link
Contributor Author

@ValentinVignal ValentinVignal Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those were the minimum changes I found to "let this test alone as much as possible".

What was happening in this test:

  1. Push using the forward animation (500ms)
  2. The push is interrupted with a pop at 400ms
  3. The pop uses the reversed animation (but should not because it leads to a discontinuity).

After my changes without any modification on the test:

  1. Push using the forward animation (500ms)
  2. The push is interrupted with a pop at 400ms
  3. The pop uses the forward animation for the reverse animation (there is no discontinuity)
  4. The test fails at the line
    await tester.pump(const Duration(milliseconds: 40));
    expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-83.0, epsilon: 1.0));

Expected: -83
Actual value: -261

(Comment for reference: https://github.com/flutter/flutter/pull/146999/files#r1575569345)

To make this test pass, I have 2 options:

a. I let the push being interrupted and I need to change the expected values to match the use of the forward curve instead of the reverse curved.
b. I let the push animation complete with more pumps so the reverse animation for the pop uses the reversed curve and I don't need to change the values in the expects.

I have implemented both options. The one linked to the comment is a.. And a bit below, there is the group('Interrupted push', () { ... }); that implements b.

Is that what you are expecting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MitchellGoodwin , does this help?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation. That all makes sense and the two tests LGTM. Thank you for adjusting the old on and adding a new one.


await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-129.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-83.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 360));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-0.0, epsilon: 1.0));
}

testWidgets('CupertinoPageRoute has parallax when non fullscreenDialog route is pushed on top', (WidgetTester tester) async {
testWidgets('CupertinoPageRoute has parallax when non fullscreenDialog route is pushed on top',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await testParallax(tester, fromFullscreenDialog: false);
});

testWidgets('FullscreenDialog CupertinoPageRoute has parallax when non fullscreenDialog route is pushed on top', (WidgetTester tester) async {
testWidgets('FullscreenDialog CupertinoPageRoute has parallax when non fullscreenDialog route is pushed on top',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await testParallax(tester, fromFullscreenDialog: true);
});

group('Interrupted push', () {
Future<void> testParallax(WidgetTester tester, {required bool fromFullscreenDialog}) async {
await tester.pumpWidget(
CupertinoApp(
onGenerateRoute: (RouteSettings settings) => CupertinoPageRoute<void>(
fullscreenDialog: fromFullscreenDialog,
settings: settings,
builder: (BuildContext context) {
return Column(
children: <Widget>[
const Placeholder(),
CupertinoButton(
child: const Text('Button'),
onPressed: () {
Navigator.push<void>(context, CupertinoPageRoute<void>(
builder: (BuildContext context) {
return CupertinoButton(
child: const Text('Close'),
onPressed: () {
Navigator.pop<void>(context);
},
);
},
));
},
),
],
);
},
),
),
);

// Enter animation.
await tester.tap(find.text('Button'));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(0.0, epsilon: 0.1));
await tester.pump();

// The push animation duration is 500ms. We let it run for 400ms before
// interrupting and popping it.

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-55.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-111.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-161.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-200.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-226.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-242.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-251.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-257.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-261.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-263.0, epsilon: 1.0));

// Exit animation
await tester.tap(find.text('Close'));
await tester.pump();

// When the push animation is interrupted, the forward curved is used for
// the reversed animation to avoid discontinuities.


await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-261.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-257.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-251.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-242.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-226.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-200.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-161.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-111.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(-55.0, epsilon: 1.0));

await tester.pump(const Duration(milliseconds: 40));
expect(tester.getTopLeft(find.byType(Placeholder)).dx, moreOrLessEquals(0.0, epsilon: 1.0));
}

testWidgets('CupertinoPageRoute has parallax when non fullscreenDialog route is pushed on top and gets popped before the end of the animation',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await testParallax(tester, fromFullscreenDialog: false);
});

testWidgets('FullscreenDialog CupertinoPageRoute has parallax when non fullscreenDialog route is pushed on top and gets popped before the end of the animation',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await testParallax(tester, fromFullscreenDialog: true);
});
});

Future<void> testNoParallax(WidgetTester tester, {required bool fromFullscreenDialog}) async{
await tester.pumpWidget(
CupertinoApp(
Expand Down Expand Up @@ -918,15 +1085,24 @@ void main() {
expect(tester.getTopLeft(find.byType(Placeholder)).dx, 0.0);
}

testWidgets('CupertinoPageRoute has no parallax when fullscreenDialog route is pushed on top', (WidgetTester tester) async {
testWidgets('CupertinoPageRoute has no parallax when fullscreenDialog route is pushed on top',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await testNoParallax(tester, fromFullscreenDialog: false);
});

testWidgets('FullscreenDialog CupertinoPageRoute has no parallax when fullscreenDialog route is pushed on top', (WidgetTester tester) async {
testWidgets('FullscreenDialog CupertinoPageRoute has no parallax when fullscreenDialog route is pushed on top',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await testNoParallax(tester, fromFullscreenDialog: true);
});

testWidgets('Animated push/pop is not linear', (WidgetTester tester) async {
testWidgets('Animated push/pop is not linear',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
await tester.pumpWidget(
const CupertinoApp(
home: Text('1'),
Expand All @@ -944,6 +1120,11 @@ void main() {
tester.state<NavigatorState>(find.byType(Navigator)).push(route2);
// The whole transition is 500ms based on CupertinoPageRoute.transitionDuration.
// Break it up into small chunks.
//
// The screen width is 800.
// The top left corner of the text 1 will go from 0 to -800 / 3 = - 266.67.
// The top left corner of the text 2 will go from 800 to 0.


await tester.pump();
await tester.pump(const Duration(milliseconds: 50));
Expand All @@ -961,8 +1142,14 @@ void main() {

// Finish the rest of the animation
await tester.pump(const Duration(milliseconds: 350));
// Give time to the animation to finish and update its status to
// AnimationState.completed, so the reverse curved can be used in the next
// step.
await tester.pumpAndSettle(const Duration(milliseconds: 1));

tester.state<NavigatorState>(find.byType(Navigator)).pop();
// The top left corner of the text 1 will go from -800 / 3 = - 266.67 to 0.
// The top left corner of the text 2 will go from 0 to 800.
await tester.pump();
await tester.pump(const Duration(milliseconds: 50));
expect(tester.getTopLeft(find.text('1')).dx, moreOrLessEquals(-197, epsilon: 1));
Expand Down