Make ParentDataWidget usable with different ancestor RenderObjectWidget types by goderbauer · Pull Request #48541 · flutter/flutter

goderbauer · 2020-01-10T00:02:38Z

Description

Prior to this change, a ParentDataWidget was bound to a particular ancestor RenderObjectWidget type, whose RenderObject was responsible for setting up the parent data that the ParentDataWidget would write to. A given ParentDataWidget could not be re-used with another RenderObjectWidget type as ancestor. For example, a new implementation of Stack (e.g. SuperStack widget) was not able to re-use the Positioned widget, because it was bound to the existing Stack widget.

This change loosens the contract between ParentDataWidget and ancestor RenderObjectWidget types. Any RenderObjectWidget type can be used as an ancestor for a ParentDataWidget as long as the RenderObject of the RenderObjectWidget sets up the ParentData type expected by the ParentDataWidget.

Related Issues

Required for fixing #45797.

Tests

I added the following tests:

Test to verify that a ParentDataWidget can be used with different RenderObjectWidget types as ancestors.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

AFIAK, this is not a breaking change as defined in the breaking change policy, but since this may effect some users negatively, I am providing a migration guide: flutter/website#3525.

…get types

dnfield · 2020-01-10T00:17:19Z

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

 ///  * [StatefulWidget] and [State], for widgets that can build differently
 ///    several times over their lifetime.
-abstract class ParentDataWidget<T extends RenderObjectWidget> extends ProxyWidget {
+abstract class ParentDataWidget<T extends ParentData> extends ProxyWidget {


Is this not a breaking change?

Yes and No. :)

As far as I can tell this will not break any tests. However, I will be writing a migration guide for people that have custom ParentDataWidgets (see PR description :P )

dnfield · 2020-01-10T00:19:29Z

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

-      if (ancestor is ParentDataElement<RenderObjectWidget>)
-        return ancestor;
+      if (ancestor is ParentDataElement<ParentData>) {
+        result = ancestor as ParentDataElement<ParentData>;


I believe that either the as isn't necessary here anymore, or that if it is you should just be doing:

result = ancestor as ParentDataElement<ParentData>; if (result != null) { break;

Without the "as" I am getting:

error: A value of type 'Element' can't be assigned to a variable of type 'ParentDataElement'. (invalid_assignment at [flutter] lib/src/widgets/framework.dart:5096)

And in your code, the "as" would throw if ancestor is not a ParentDataElement, which I don't want.

Ahh I was thinking of C#. Nevermind

i'm surprised you need the as, i expected this case to be handled. cc @leafpetersen

dnfield · 2020-01-10T00:21:08Z

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

+        try {
+          throw FlutterError.fromParts(<DiagnosticsNode>[
+            ErrorSummary('Incorrect use of ParentDataWidget.'),
+            ErrorDescription('The following ParentDataWidgets are providing parent data to the same RenderObject:'),
+            for (final ParentDataElement<ParentData> ancestor in badAncestors)
+              ErrorDescription('- ${ancestor.widget} (typically placed directly inside a ${ancestor.widget.debugTypicalAncestorWidget} widget)'),
+            ErrorDescription('However, a RenderObject can only receive parent data from at most one ParentDataWidget.'),
+            ErrorHint('Usually, this indicates that at least one of the offending ParentDataWidgets listed above is not placed directly inside a compatible ancestor widget.'),
+            ErrorDescription('The ownership chain for the RenderObject that received the parent data was:\n  ${debugGetCreatorChain(10)}'),
+          ]);
+        } on FlutterError catch (e) {
+          _debugReportException(ErrorSummary('while looking for parent data.'), e, e.stackTrace);
+        }


Why isn't this just

_debugReportException(ErrorSummary('while looking for parent data.'), FlutterError.fromParts(...), StackTrace.current);

?

dnfield · 2020-01-10T00:22:20Z

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

    }
-    return null;
+    assert(() {
+      if (result == null || ancestor == null) {


Is it ever possible for the result to have been the root? Or is that condition acceptable?

e.g. if you somehow get to the top most ancestor, and it is the wrong type, we'd have result != null && ancestor == null here.

I don't fully understand what you mean, if we never find the correct type, result would be null as well?

Also, it is fine to not find any ancestor ParentDataElement.

I mean if you find it and it's the root render object

Maybe just q comment then about why we are throwing and catching this way would help

I mean if you find it and it's the root render object

You mean, the root of the element tree? That's fine for the purpose of this check. This check only wants to check how many ParentDataElements there are between this RenderObjectElement and the next RenderObjectElement (or the root).

dnfield · 2020-01-10T00:23:12Z

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

+  void _updateParentData(ParentDataWidget<ParentData> parentDataWidget) {
+    bool applyParentData = true;
+    assert(() {
+      try {


Ditto here. We should avoid using exceptions for flow control - it causes debuggers to fire here when users are debugging their own code.

Or is that whta we want? If so, we should add a comment to that effect.

This is not an exception for control flow. This throws when there's actually something wrong with your app, so if you are debugging your app and you have it set up to fire on exception, you want it to fire here.

I am only catching the exception here directly, because I don't want the exception to bubble up and cause the error widget to swoop in because the tree is in a state where the error widget would cause more exceptions...

Maybe just q comment then about why we are throwing and catching this way would help

Will do.

dnfield · 2020-01-10T00:26:44Z

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

-  Iterable<DiagnosticsNode> debugDescribeInvalidAncestorChain({ String description, DiagnosticsNode ownershipChain, bool foundValidAncestor, Iterable<Widget> badAncestors }) sync* {
+  /// The [RenderObjectWidget] that is typically used to setup the [ParentData]
+  /// that [applyParentData] will write to.
+  Type get debugTypicalAncestorWidget;


It would be helpful if this docstring explained that this is only used for error message purposes - i.e. it is not actually used for comparison in asserts or anything.

dnfield · 2020-01-10T00:27:24Z

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

  }
+
+  @override
+  Type get debugTypicalAncestorWidget => _CupertinoAlertActionsRenderWidget;


Here and elsewhere - does it make any difference to wrap these in asserts so that they're null outside of debug mode?

Since they are only called from on assert, these methods should already be tree-shaken out in release mode.

dnfield

Lgtm

Hixie · 2020-01-10T19:20:32Z

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

-  bool debugIsValidAncestor(RenderObjectWidget ancestor) {
+  /// Checks if this widget can apply its parent data to the provided
+  /// `renderObject`.
+  bool debugIsValidRenderObject(RenderObject renderObject) {


docs should probably mention debugTypicalAncestorWidget and also mention when this is called in the widget lifecycle

Hixie · 2020-01-10T19:20:56Z

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

+  ///
+  /// This is only used in error messages to tell users what widget typically
+  /// wraps this ParentDataWidget.
+  Type get debugTypicalAncestorWidget;


debugTypicalAncestorWidgetClass? or is that too verbose

Hixie · 2020-01-10T19:21:39Z

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

      yield ErrorDescription(
-        '$runtimeType widgets must be placed inside $T widgets.\n'
-        '$description has no $T ancestor at all.'
+        '$description, which has not been setup to receive any ParentData.'


set up, two words

Hixie · 2020-01-10T19:21:54Z

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

      yield ErrorDescription(
-        '$runtimeType widgets must be placed directly inside $T widgets.\n'
-        '$description has a $T ancestor, but there are other widgets between them:'
+        '$description, which has been setup to only accept ParentData of incompatible type ${parentData.runtimeType}.'


i'd probably remove "only" here

Hixie · 2020-01-10T19:26:43Z

…et types (flutter#48541)

Includes fix for Flutter v1.14.0+. Breaking change described here: flutter/flutter#48541

goderbauer added 3 commits January 9, 2020 15:46

ParentDataWidgets can be used with different ancestor RenderObjectWid…

1ddc0c1

…get types

++

88c125d

fix doc

98f3b6b

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Jan 10, 2020

googlebot added the cla: yes label Jan 10, 2020

goderbauer changed the title ~~A ParentDataWidget can be used with different ancestor RenderObjectWidget types~~ Make ParentDataWidget usable with different ancestor RenderObjectWidget types Jan 10, 2020

whitespace

d76c371

dnfield reviewed Jan 10, 2020

View reviewed changes

goderbauer added 3 commits January 9, 2020 16:43

fix doc sample

7550907

dan review

106d58b

add comment

1cdbc40

goderbauer removed the f: cupertino flutter/packages/flutter/cupertino repository label Jan 10, 2020

dnfield approved these changes Jan 10, 2020

View reviewed changes

Hixie reviewed Jan 10, 2020

View reviewed changes

goderbauer mentioned this pull request Jan 10, 2020

Migration Guide for ParentDataWidget generics type change flutter/website#3525

Merged

fluttergithubbot added the f: cupertino flutter/packages/flutter/cupertino repository label Jan 10, 2020

Ian review

068233b

goderbauer added waiting for tree to go green and removed f: cupertino flutter/packages/flutter/cupertino repository labels Jan 10, 2020

fluttergithubbot merged commit 6a5964d into flutter:master Jan 13, 2020

goderbauer deleted the parentdata branch January 13, 2020 18:08

mehere pushed a commit to mehere/flutter that referenced this pull request Jan 14, 2020

Make ParentDataWidget usable with different ancestor RenderObjectWidg…

d8e8122

…et types (flutter#48541)

shyndman pushed a commit to shyndman/flutter_layout_grid that referenced this pull request Feb 20, 2020

Bump version to 0.10.0-dev.0

1dafd20

Includes fix for Flutter v1.14.0+. Breaking change described here: flutter/flutter#48541

goderbauer mentioned this pull request Mar 24, 2020

Be able to chain ParentDataWidgets which refer to the same parent widget. #4912

Closed

znjameswu mentioned this pull request May 7, 2020

Document expected behavior for APIs that expect a return type of Type? #56500

Closed

Conversation

goderbauer commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jan 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

goderbauer commented Jan 10, 2020 •

edited

Loading