PageTransitionSwitcher #50

goderbauer · 2019-12-02T18:30:48Z

This is a variation of an AnimatedSwitcher [1], but it instead of using the same transition for enter and exit, two separate transitions can be specified, similar to how the enter and exit transitions of a PageRoute are defined.

The goal is that transitions written for transitioning PageRoutes in and out can be re-used to switch between two widgets. This can be useful in combination with a BottomNavigationBar to switch between widgets when another BottomNavigationBarItem is activated using a PageRoute-like transition.

[1] https://master-api.flutter.dev/flutter/widgets/AnimatedSwitcher-class.html

goderbauer · 2019-12-02T18:30:58Z

/cc @HansMuller @shihaohong

shihaohong

I'm holding off on the rest because I feel like moving further without being 100% sure of what "previous", "old" and "new" child may mean would make reviewing the rest of the code difficult.

packages/animations/lib/src/page_transition_switcher.dart

shihaohong

The logic and tests look good! Most of my comments are suggestions to clarify the language of the code since it can be confusing to wrap your head around if you're unfamiliar with the primary/secondaryAnimation jargon and expectations.

shihaohong · 2019-12-04T15:55:21Z

packages/animations/lib/src/page_transition_switcher.dart

+    @required this.secondaryController,
+    @required this.transition,
+    @required this.widgetChild,
+  })  : assert(primaryController != null),


nit:

Suggested change

}) : assert(primaryController != null),

}) : assert(primaryController != null),

assert(secondaryController != null),

assert(widgetChild != null),

assert(transition != null);

Yes, that would be better! However, unfortunately, dartfmt is enabled for this repository giving me no control over the formatting here :(

packages/animations/lib/src/page_transition_switcher.dart

shihaohong · 2019-12-04T16:02:33Z

packages/animations/lib/src/page_transition_switcher.dart

+/// widget data that distinguishes this child from the others).
+///
+/// The same key can be used for a new child as was used for an already-outgoing
+/// child; the two will not be considered related. (For example, if a progress


Suggested change

/// child; the two will not be considered related. (For example, if a progress

/// child; the two will not be considered related. For example, if a progress

shihaohong · 2019-12-04T16:02:38Z

packages/animations/lib/src/page_transition_switcher.dart

+/// indicator with key A is first shown, then an image with key B, then another
+/// progress indicator with key A again, all in rapid succession, then the old
+/// progress indicator and the image will be fading out while a new progress
+/// indicator is fading in.)


Suggested change

/// indicator is fading in.)

/// indicator is fading in.

shihaohong · 2019-12-04T16:06:56Z

packages/animations/lib/src/page_transition_switcher.dart

+  /// durations of transitions already in progress.
+  final Duration duration;
+
+  /// Indicates the direction of the animation when a new [child] is set.


Maybe "Determines whether or not the new [child] will replace the previous [child] from the top or underneath the previous [child]", or something similar to describe the actual transition visually.

shihaohong · 2019-12-04T18:40:22Z

packages/animations/test/page_transition_switcher_test.dart

+    expect(_primaryAnimation[containerTwo], moreOrLessEquals(0.4));
+    expect(_secondaryAnimation[containerTwo], moreOrLessEquals(0.0));
+
+    // Container one is at the bottom.


Suggested change

// Container one is at the bottom.

// Container one is underneath container two.

shihaohong · 2019-12-04T18:47:31Z

packages/animations/test/page_transition_switcher_test.dart

+    _primaryAnimation = _getPrimaryAnimation(
+        <Key>[containerOne, containerTwo, containerThree], tester);
+    _secondaryAnimation = _getSecondaryAnimation(
+        <Key>[containerOne, containerTwo, containerThree], tester);


Maybe comment the expectation for this particular case since it's pretty unique. Like:

// Container one is expected to continue running its primary animation in reverse since it is exiting. // Container two's secondary animation switches from running its secondary animation in reverse to // running forwards since it should now be exiting underneath container three. Container three's primary // animation should be running forwards since it is entering above container two.

shihaohong · 2019-12-04T19:07:59Z

packages/animations/test/page_transition_switcher_test.dart

+    ScaleTransition scale = tester.firstWidget(find.byType(ScaleTransition));
+    expect(fade.opacity.value, moreOrLessEquals(1.0));
+    expect(scale.scale.value, moreOrLessEquals(0.4));
+    await tester.pumpAndSettle();


Maybe add a comment here saying that this pumpAndSettle call is meant to let the entering widget's transition continue to completion

shihaohong · 2019-12-04T19:17:08Z

packages/animations/test/page_transition_switcher_test.dart

+  });
+}
+
+class StatefulTest extends StatefulWidget {


Perhaps rename to make it clearer:

Suggested change

class StatefulTest extends StatefulWidget {

class StatefulTestWidget extends StatefulWidget {

shihaohong · 2019-12-04T19:23:37Z

packages/animations/test/page_transition_switcher_test.dart

+    expect(find.text('1'), findsOneWidget);
+    expect(find.text('2'), findsNothing);
+    await pumpChild(const Text('2'));
+    fade = tester.widget(find.byType(FadeTransition).first);


Shouldn't there only be one widget, since it's not animating and simply updating the value?

Yes, you're right. removed.

HansMuller

Mostly small stuff. Explaining how buildTransitions works is difficult!

HansMuller · 2019-12-04T18:05:12Z

packages/animations/lib/src/page_transition_switcher.dart

@@ -51,8 +51,8 @@ typedef PageTransitionSwitcherTransitionBuilder = Widget Function(
  Animation<double> secondaryAnimation,
 );

-/// A widget that transitions from a previously set child to a new child using
-/// an animation specified by [transitionBuilder].
+/// A widget that transitions from a previously set child to a newly set child


from the current child to a new child?

HansMuller · 2019-12-04T18:10:00Z

packages/animations/lib/src/page_transition_switcher.dart

-/// If the "new" child is the same widget type and key as the "old" child, but
-/// with different parameters, then [PageTransitionSwitcher] will *not* do a
+/// If the *new* child is the same widget type and key as the *previous* child,
+/// but with different parameters, then [PageTransitionSwitcher] will *not* do a


In this case by "but with different parameters" I assume that you mean that the new child was configured differently. Developers may find this a little confusing. You might provide an example. Like: changing the child from SizedBox(width: 10) to SizedBox(width: 100) would not trigger a transition but changing the child from SizedBox(width: 10) to SizedBox(key: Key('foo'), width: 100) would. Similarly, changing the child to Container(width: 10) would trigger a transition.

The AnimatedSwitcher class API doc makes this same point. Might make sense to sync this paragraph with that one.

HansMuller · 2019-12-04T18:12:09Z

packages/animations/lib/src/page_transition_switcher.dart

@@ -128,22 +128,23 @@ class PageTransitionSwitcher extends StatefulWidget {
  /// durations of transitions already in progress.
  final Duration duration;

-  /// Indicates the direction of the animation.
+  /// Indicates the direction of the animation when a new [child] is set.


maybe: when the [child] is changed.

Widgets don't have properties that are set so much as constructor parameters whose values are provided.

HansMuller · 2019-12-04T18:13:29Z

packages/animations/lib/src/page_transition_switcher.dart

-  /// previous child while its primary animation and the secondary animation
-  /// of the previous child are running forward. This is similar to the
-  /// transition associated with pushing a new [PageRoute] on top of another.
+  /// previously set child while its primary animation and the secondary


previously set child => previous child

I'll stop complaining about "child setting" etc.

HansMuller · 2019-12-04T19:13:41Z

packages/animations/lib/src/page_transition_switcher.dart

+/// specified, similar to how the enter and exit transitions of a [PageRoute]
+/// are defined.
+///
+/// The transitions returned by the [transitionBuilder] are driven by two


This is the difficult part of explaining route transitions. I'll take a crack at the whole thing:

When a new [child] is specified, the [transitionBuilder] is effectively applied twice, once to the old child and once to the new one. The old child's secondaryAnimation runs forward, and the value of its primaryAnimation is fixed at 1.0. The new child's primaryAnimation runs forward and the value of its secondary animation is fixed at 0.0. The widget returned by the [transitionBuilder] can incorporate both animations. It will use the primary animation to define how its child appears, and the secondary animation to define how its child disappears. This process is the same as the one used by [PageRoute.buildTransitions].

[And then include simple transitionBuilder example. There's a simple example here: https://api.flutter.dev/flutter/material/MaterialPageRoute/buildTransitions.html; prefer one that uses both animations]

Forgot to add the example, working on that now.

HansMuller · 2019-12-04T20:09:45Z

packages/animations/lib/src/page_transition_switcher.dart

+    final _ChildEntry entry = _ChildEntry(
+      widgetChild: child,
+      transition: KeyedSubtree.wrap(
+        builder(child, primaryController, secondaryController),


Maybe assert that the builder's return value is non-null here and provide a nice error message

HansMuller · 2019-12-04T20:11:30Z

packages/animations/lib/src/page_transition_switcher.dart

+  /// [PageRoute] below it.
+  final bool reverse;
+
+  /// A function that wraps a new [child] with a primary and secondary animation


The new PageTransition child could be null, by the transitionBuilder's child is guaranteed to be non-null, right?

Yes. added documentation.

HansMuller · 2019-12-04T20:13:12Z

packages/animations/lib/src/page_transition_switcher.dart

+  ///
+  /// This duration is applied to the given [child] when that property is set to
+  /// a new child. Changing [duration] will not affect the
+  /// durations of transitions already in progress.


Will a duration change affect in-progress transitions if transitionBuilder is also changed?

HansMuller · 2019-12-04T20:16:00Z

packages/animations/test/page_transition_switcher_test.dart

+    _secondaryAnimation =
+        _getSecondaryAnimation(<Key>[containerOne, containerTwo], tester);
+    // Secondary is running for outgoing widget.
+    expect(_primaryAnimation[containerOne], moreOrLessEquals(1.0));


Maybe moreOrLessEquals isn't needed for cases were we believe the animation's value will have reached 0 or 1

Removed where not needed.

HansMuller · 2019-12-04T20:18:22Z

packages/animations/test/page_transition_switcher_test.dart

+import 'package:flutter_test/flutter_test.dart';
+
+void main() {
+  testWidgets('transitions in a new child.', (WidgetTester tester) async {


Should verify that transitioning to/from a null child works too

I believe there is a subsequent test for this called "handles null children"

Yes, there is one later in this file :)

goderbauer · 2019-12-04T23:04:31Z

PTAL and let me know if the descriptions are clearer now.

shihaohong

LGTM, it's much clearer now :)

packages/animations/lib/src/page_transition_switcher.dart

Co-Authored-By: Shi-Hao Hong <shihaohong@google.com>

HansMuller

LGTM

HansMuller · 2019-12-05T16:33:48Z

packages/animations/lib/src/page_transition_switcher.dart

+///   Animation<double> primaryAnimation,
+///   Animation<double> secondaryAnimation,
+/// ) {
+///   return SlideTransition(


goderbauer added 3 commits November 27, 2019 12:09

switcher works

0ab0817

add actual implementation

1c95b86

typos

782621f

format

d62a35b

shihaohong reviewed Dec 2, 2019

View reviewed changes

review

b0c5f57

shihaohong reviewed Dec 4, 2019

View reviewed changes

HansMuller reviewed Dec 4, 2019

View reviewed changes

review

c7bd29b

better docs

ff1b55e

shihaohong approved these changes Dec 5, 2019

View reviewed changes

packages/animations/lib/src/page_transition_switcher.dart Outdated Show resolved Hide resolved

packages/animations/lib/src/page_transition_switcher.dart Outdated Show resolved Hide resolved

goderbauer and others added 2 commits December 4, 2019 16:39

Update packages/animations/lib/src/page_transition_switcher.dart

11793f3

Co-Authored-By: Shi-Hao Hong <shihaohong@google.com>

Update packages/animations/lib/src/page_transition_switcher.dart

c047a92

Co-Authored-By: Shi-Hao Hong <shihaohong@google.com>

HansMuller approved these changes Dec 5, 2019

View reviewed changes

goderbauer merged commit 4413ccf into flutter:master Dec 5, 2019

goderbauer deleted the switcher branch December 5, 2019 17:23

stuartmorgan pushed a commit that referenced this pull request Oct 31, 2024

encode text transforms (#50)

8d9dce1

-  })  : assert(primaryController != null),
+  }) : assert(primaryController != null),
+       assert(secondaryController != null),
+       assert(widgetChild != null),
+       assert(transition != null);

	/// child; the two will not be considered related. (For example, if a progress
	/// child; the two will not be considered related. For example, if a progress

	// Container one is at the bottom.
	// Container one is underneath container two.

	class StatefulTest extends StatefulWidget {
	class StatefulTestWidget extends StatefulWidget {

PageTransitionSwitcher #50

PageTransitionSwitcher #50

Conversation

goderbauer commented Dec 2, 2019 • edited Loading

goderbauer commented Dec 2, 2019

shihaohong left a comment

Choose a reason for hiding this comment

shihaohong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HansMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer Dec 4, 2019 • edited by HansMuller Loading

Choose a reason for hiding this comment

goderbauer commented Dec 4, 2019

shihaohong left a comment • edited Loading

Choose a reason for hiding this comment

HansMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer commented Dec 2, 2019 •

edited

Loading

shihaohong left a comment •

edited

Loading

goderbauer Dec 4, 2019 •

edited by HansMuller

Loading

shihaohong left a comment •

edited

Loading