Make Cupertino page transitions match native behaviours by xster · Pull Request #9138 · flutter/flutter

xster · 2017-03-31T23:55:58Z

Diffbases against #9059. View the diff only via https://github.com/xster/flutter/compare/mountainview...xster:cupertino-transition?diff=split&name=cupertino-transition

Makes the standard Cupertino transition come in from the right while parallaxing the screen behind the incoming one
Refactor away the custom Cupertino transition curves. Get rid of the math since only the person that wrote it can understand it :). Use composition instead
Add a fullscreen dialog route mode where the screen comes in from the bottom in Cupertino. Can't be used in named routes yet
Fixed the fullscreen dialog gallery to use the new mode
Moved material tests from widget to material. Wrote a bunch of more tests
Appbar parallax blending effect will be in a next PR

…ge for modal

…k to its parent

# Conflicts: # packages/flutter/lib/src/cupertino/page.dart

# Conflicts: # packages/flutter/test/material/page_test.dart

# Conflicts: # packages/flutter/lib/src/material/page.dart

Hixie · 2017-03-31T23:59:32Z

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

  }
 }

+/// An animation of [double]s that tracks the sum of two given animations.


if we're going to just provide this class, we should change the example at CompoundAnimation to be an example we don't actually provide :-)

looks like you don't need this class after all so you can remove it and get away without changing the example. :-)

ya, reverted. Thanks

xster · 2017-04-01T00:02:47Z

cc @Hixie @abarth @cbracken
(compare with diffbase only via https://github.com/xster/flutter/compare/mountainview...xster:cupertino-transition?diff=split&name=cupertino-transition)

Hixie · 2017-04-01T00:03:26Z

packages/flutter/lib/src/cupertino/page.dart

+    @required Animation<double> outgoingRouteAnimation,
+    @required this.child,
+  })
+      : incomingPositionAnimation = _kRightMiddleTween.animate(


nit: the : should be one space to the right of the ) on the same line

Did something. Hope it's right

Hixie · 2017-04-01T00:08:28Z

packages/flutter/lib/src/cupertino/page.dart

+  CupertinoPageTransition({
+    Key key,
+    // Linear route animation from 0.0 to 1.0 when this screen is being pushed.
+    @required Animation<double> incomingRouteAnimation,


i'm not sure incoming/outgoing really captures the semantics of these two animations, but i don't have a better suggestion. :-(

Let's figure it out in #9132 then I'll go rename everything.

Hixie · 2017-04-01T00:09:28Z

packages/flutter/lib/src/cupertino/page.dart

+          key: key,
+          // Trigger a rebuild whenever any animation route happens. The listenable's value is not
+          // used.
+          listenable: new AnimationSum(left: incomingRouteAnimation, right: outgoingRouteAnimation),


use a Listenable.merge instead:
https://docs.flutter.io/flutter/foundation/Listenable/Listenable.merge.html

yay! so much more readable! I was hoping there was something like this

Done. Thanks!

Hixie · 2017-04-01T00:11:05Z

packages/flutter/lib/src/cupertino/page.dart

+  // Fractional offset from fully on screen to 1/3 offscreen to the left.
+  static final FractionalOffsetTween _kMiddleLeftTween = new FractionalOffsetTween(
+    begin: FractionalOffset.topLeft,
+    end: const FractionalOffset(-0.33, 0.0),


you can do -1.0/3.0 to more precisely capture "one third"

Hixie · 2017-04-01T00:11:22Z

packages/flutter/lib/src/cupertino/page.dart

+        );
+
+  // Fractional offset from offscreen to the right to fully on screen.
+  static final FractionalOffsetTween _kRightMiddleTween = new FractionalOffsetTween(


can these be const?

Might not fully understand dart const constructors. Don't think one is defined for fractional offset tween though.

Hixie · 2017-04-01T00:15:10Z

packages/flutter/lib/src/cupertino/page.dart

+    ),
+  );
+
+  static final FractionalOffsetTween _kBottomUpTween = new FractionalOffsetTween(


Hixie · 2017-04-01T00:15:50Z

packages/flutter/lib/src/cupertino/page.dart

+    assert(controller != null);
+  }
+
+  AnimationController controller;


presumably this should be final?

Ah, done in diffbase PR

Hixie · 2017-04-01T00:16:31Z

packages/flutter/lib/src/material/page.dart

+    if (!(nextRoute is MaterialPageRoute<dynamic>))
+      return false;
+    final MaterialPageRoute<dynamic> nextMaterialPageRoute = nextRoute;
+    // Don't perform outgoing animation if the next route is a fullscreen dialog.


Though I had to implement new APIs on the page route for a sec until I remembered you mentioned it in #5690 (comment)

Hixie · 2017-04-01T00:17:33Z

packages/flutter/test/material/page_test.dart

    await tester.pump(const Duration(milliseconds: 1));

-    final Opacity widget2Opacity =
+    Opacity widget2Opacity =


nit: i don't think this newline really help readability since it makes this line not match the others

Ah, fits in one line now. Done.

Hixie · 2017-04-01T00:19:14Z

This is fantastic. LGTM.

…ing in dart

Hixie · 2017-04-01T04:18:40Z

packages/flutter/lib/src/material/page.dart

-    final MaterialPageRoute<dynamic> nextMaterialPageRoute = nextRoute;
    // Don't perform outgoing animation if the next route is a fullscreen dialog.
-    return !nextMaterialPageRoute.fullscreenDialog;
+    return nextRoute is MaterialPageRoute && !nextRoute.fullscreenDialog;


ah, neat. I can never figure out when this works and when it doesn't. :-(

Hixie · 2017-04-01T04:19:11Z

packages/flutter/lib/src/cupertino/page.dart

+        key: key,
+        // Trigger a rebuild whenever any of the 2 animation route happens.
+        listenable: new Listenable.merge(
+          <Listenable> [incomingRouteAnimation, outgoingRouteAnimation]


nit: no space between type and bracket

# Conflicts: # packages/flutter/lib/src/cupertino/page.dart

…ansition # Conflicts: # packages/flutter/lib/src/cupertino/page.dart # packages/flutter/lib/src/material/page.dart

Hixie · 2017-04-04T23:19:58Z

This made the flutter_gallery_ios__transition_perf average_frame_build_time_millis benchmark regress substantially.

Hixie · 2017-04-04T23:32:03Z

Also microbenchmarks_ios stock_build_iteration.

xster added 24 commits March 28, 2017 13:11

Moved stuff around yet

eeb8b50

Merge branch 'master' of github.com:flutter/flutter into mountainview

7751254

Fix depedencies

5ad0fe9

Add more dartdoc comments to packages

50c40cd

Remove Cupertino dependency on material

93359a5

Removed mountain_view package and added page transition test

034ae39

Fix analyze warnings

fcd997d

Remove commented code

ff9739b

Partial solution. Still need to stop the animation on the previous pa…

d1a5559

…ge for modal

Some review notes

8306afb

Move the cupertino back gesture controller’s lifecycle management bac…

f455980

…k to its parent

Merge branch 'mountainview' into cupertino-transition

93bd850

# Conflicts: # packages/flutter/lib/src/cupertino/page.dart

Merge branch 'master' of github.com:flutter/flutter into mountainview

b9af506

Merge branch 'mountainview' into cupertino-transition

681740a

Reviews

1849e44

Add background color

1239c56

Merge branch 'mountainview' into cupertino-transition

e220a4f

Directional curves, full screen transition

2f983ee

Merge branch 'master' of github.com:flutter/flutter into mountainview

9581bb4

# Conflicts: # packages/flutter/test/material/page_test.dart

Don’t perform the exit animation if the incoming page is a dialog

79b5307

It works!

35fd0d0

Test structures

ca4ff51

Merge branch 'mountainview' into cupertino-transition

1400fea

# Conflicts: # packages/flutter/lib/src/material/page.dart

Add a bunch of more tests and fix the gallery

9c3e1bc

googlebot added the cla: yes label Mar 31, 2017

Hixie reviewed Mar 31, 2017

View reviewed changes

One more comment

edd5499

Hixie reviewed Apr 1, 2017

View reviewed changes

xster added 4 commits March 31, 2017 17:25

Review notes

bb1c623

final controller

df12876

Use that sweet sweet is! keyword

843863c

Play golf, because I’m bitter that there’s no nullable as or someth…

f288e9b

…ing in dart

Hixie reviewed Apr 1, 2017

View reviewed changes

Remove a space

86b905f

xster mentioned this pull request Apr 3, 2017

Extract cupertino page transition out of material #9059

Merged

xster added 4 commits April 3, 2017 11:56

Review notes

2874a74

Merge branch 'mountainview' into cupertino-transition

389d523

# Conflicts: # packages/flutter/lib/src/cupertino/page.dart

Merge branch 'master' of github.com:flutter/flutter into cupertino-tr…

afcbda3

…ansition # Conflicts: # packages/flutter/lib/src/cupertino/page.dart # packages/flutter/lib/src/material/page.dart

Remove the last deprecated test

f06018b

xster merged commit 58ddde8 into flutter:master Apr 3, 2017

xster deleted the cupertino-transition branch April 3, 2017 19:44

xster mentioned this pull request Apr 4, 2017

Optimize the animation phase's performance #9205

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021

Conversation

xster commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Apr 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Apr 4, 2017

Uh oh!

Hixie commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xster commented Mar 31, 2017 •

edited

Loading