Unbreak WidgetsApp when only a builder specified by dnfield · Pull Request #23976 · flutter/flutter

dnfield · 2018-11-06T05:02:43Z

The WidgetsApp refactor was intended to be completely backwards compatible, but missed out on a case like this:

      WidgetsApp(
        builder: (BuildContext context, Widget child) {
          return SomeWidget();
        },
        color: const Color(0xFF123456),
      ),

This is valid because the default _onGenerateRoute handler will never get invoked in this case, so we don't need to know about how to build page transitions. It's non-sensical to require a user to provide them here.

Adds a test for this and fixes the assert.

/cc @Hixie @HansMuller @xster - I'm not entirely sure who would be most appropriate to review this.

ice-cream-coder · 2018-11-06T05:14:30Z

packages/flutter/lib/src/widgets/app.dart

+         pageRouteBuilder != null,
+         'If neither builder nor onGenerateRoute are provided, the '
+         'pageRouteBuilder must be specified so that the default handler '
+         'will know what kind of PageRoute transition to build.'


I'm not sure if it makes sense to put the emphasis in this message on pageRouteBuilder. I feel like this could prod somebody in the wrong direction if they actually need builder, or onGenerateRoute. Is it sufficient to just say "builder, onGenerateRoute, or pageRouteBuilder must be specified."? (Although I guess that pretty much just restates the assert expression)

There are other asserts covering various scenarios around needing a builder vs home/routes/onGenerateRoute/etc. This one in particular will fire after those, and at this point should only fire for someone who has specified home or routes but not builder, onGenerateRoute, or pageRouteBuilder.

Awesome! Thanks for the quick reply.

Cretezy

LGTM!

dnfield · 2018-11-06T07:41:02Z

/cc @goderbauer as well

HansMuller · 2018-11-07T17:38:55Z

The constructor doc isn't quite right. It says:

  /// If the [builder] is null, the [onGenerateRoute] and [pageRouteBuilder]
  /// arguments are required. T

The builder parameter should be unconditionally optional.

The home parameter can be used to specify a WidgetsApp child.

HansMuller

Need to ensure that just specifying home is a supported configuration. And some tests to make sure that we continue to support all of the supported WidgetsApp configurations.

HansMuller · 2018-11-07T18:19:04Z

packages/flutter/lib/src/widgets/app.dart

-         'so that the default handler will know what kind of PageRoute transition '
-         'bo build.'),
+       assert(
+         builder != null ||


It looks like this assert doesn't allow just specifying home.

That doesn't work currently (before this change). If you specify home, you have to specify how to do transitions in case there are multiple routes. Working on updating the docs for that.

I suppose we could special case just have a home and no other routes, or having a routes list that only contains home (so not ransitions will be required)?

For the sake of this PR, I guess we should leave things as they are.

A separate PR, that relaxes the constraints on WidgetsApp's parameters, would be useful.

HansMuller · 2018-11-07T18:23:19Z

packages/flutter/test/widgets/app_overrides_test.dart

    expect(find.byType(CheckedModeBanner), findsOneWidget);
  });

+  testWidgets('WidgetsApp with builder only', (WidgetTester tester) async {


This file appears to be for tests that check the showFooOverride flags.

We should have a app_test.dart that verifies that the supported WidgetsApp configurations DTRT

I'll move this to another file.

HansMuller

I think the updated documentation is an improvement, since it lays out the truth as it is now.

In the future, if we backwards compatibly relax the constraints on WidgetsApp's parameters or provide some alternative named constructors that can be explained more simply, we might be able to simplify the docs as well.

HansMuller

LGTM

xster

Sorry for the late review

xster · 2018-11-08T01:36:54Z

packages/flutter/lib/src/widgets/app.dart

+  ///  _not_ contain an entry for `'/'`.  Conversely, if [home] is omitted, [routes]
+  /// _must_ contain an entry for `'/'`.
+  ///
+  /// If [home] or [routes] are not null, then either the [pageRoutebuilder] or


pageRouteBuilder

xster · 2018-11-08T01:38:05Z

packages/flutter/lib/src/widgets/app.dart

+  /// the [builder] parameter is required. These parameters will be used so
+  /// that the default routing implementation in [WidgetsApp] can wrap routes in
+  /// appropriate transitions. For example, [MaterialApp] will provide a
+  /// [pageRoutebuilder] that creates Material compliant hero animations between


'hero animation' is specifically the page's inner content that's wrapped in a Hero that flies above the page transition. Here, the MaterialApp is providing a pageRouteBuilder that creates a Material compliant page transition rather than hero animations. MaterialApp also makes sure the hero animation is Material compliant, but that's via the createRectTween in the HeroController rather than via pageRouteBuilder

So the pageRouteBuilder is actually creating a MaterialPage, which uses those animations - do you want this to be clarified here?

Depends what you mean by 'those animations'. pageRouteBuilder and MaterialPageRoute etc are mostly orthogonal to hero animations. Their page transition animations drive the hero animations but they don't define the hero animations.

xster · 2018-11-08T01:39:07Z

packages/flutter/lib/src/widgets/app.dart

+  /// routes, whereas the [CupertinoApp] provides Cupertino compliant hero
+  /// animations. Other implementations can provide other custom transitions here.
+  ///
+  /// The [builder] parameter is optional in all cases. It can be used to ensure that


This is a bit unclear. You just said If [home] or [routes] are not null, then either the [pageRoutebuilder] or the [builder] parameter is required and here you're saying it's optional in all cases. I think you meant it can be additionally provided in all cases or some such.

Inclusive or.. I'll try to clarify.

update assert add test

e372f96

googlebot added the cla: yes label Nov 6, 2018

dnfield mentioned this pull request Nov 6, 2018

Refactor of cupertino/material/widgets app #22161

Merged

ice-cream-coder reviewed Nov 6, 2018

View reviewed changes

Cretezy approved these changes Nov 6, 2018

View reviewed changes

dnfield changed the title ~~Unbreak WidgetsApp with only a builder specified~~ Unbreak WidgetsApp when only a builder specified Nov 6, 2018

dnfield requested a review from HansMuller November 7, 2018 17:14

HansMuller reviewed Nov 7, 2018

View reviewed changes

update docs, move test

cb23773

HansMuller reviewed Nov 7, 2018

View reviewed changes

HansMuller approved these changes Nov 7, 2018

View reviewed changes

remove whitespace

14e588c

dnfield merged commit 3d01e89 into flutter:master Nov 7, 2018

dnfield deleted the widgets_app_assert branch November 7, 2018 20:16

xster reviewed Nov 8, 2018

View reviewed changes

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

Conversation

dnfield commented Nov 6, 2018 • 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!

Cretezy left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Nov 6, 2018

Uh oh!

HansMuller commented Nov 7, 2018

Uh oh!

HansMuller left a comment

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!

HansMuller Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

xster left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnfield commented Nov 6, 2018 •

edited

Loading

HansMuller Nov 7, 2018 •

edited

Loading