Fixes Intrinsics for RenderParagraph and RenderWrap by goderbauer · Pull Request #70656 · flutter/flutter

goderbauer · 2020-11-16T22:27:07Z

For google3 rollers: This change requires dnfield/flutter_svg#450 and cl/343539658.

Description

Fixes #48679 by implementing the solution outlined in #48679 (comment). Going forward, authors of RenderBox subclasses will have to implement a new computeDryLayout method, which calculates nondestructively the Size that the RenderBox would like to be under the given set of constraints.

Furthermore, performResize is now implemented by calling the new computeDryLayout method and does not have to be overridden in subclasses anymore.

Last, but not least, this change adds a computeSizeForNoChild method to RenderProxyBoxMixin, which is called from performLayout and computeDryLayout when the RenderProxy has no child to determine its size based on the current constraints (previously this was done by calling performResize, which was a little odd since performResize is documented to only be called when sizedByParent is true (which it often isn't in these cases)). The default implementation just returns constraints.smallest (which is what performResize used to set the size as as well).

Related Issues

Fixes #48679

Tests

I added the following tests:

Added tests for the cases from the bug
Also: This is self-testing: In every test we check that the size computed by performLayout/performResize matches the size computed by performDryLayout.

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.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a migration guide: Breaking Change entry for dry layout on RenderBox website#5007

Hixie · 2020-11-16T22:58:52Z

packages/flutter/lib/src/rendering/box.dart

also when the layout is lazy, right? e.g. SliverList?

Hixie · 2020-11-16T22:59:41Z

packages/flutter/lib/src/rendering/box.dart

I don't think "Reason given:" really adds anything.

Hixie · 2020-11-16T22:59:54Z

packages/flutter/lib/src/rendering/box.dart

objectRuntimeType

(yes i know this is in an assert but we have a lint now)

(also affects other places in the pr)

Hixie · 2020-11-16T23:01:17Z

packages/flutter/lib/src/rendering/box.dart

should this mention debugCheckingIntrinsics? It seems to be the real reason this function exists...

Hixie · 2020-11-17T00:40:00Z

packages/flutter/lib/src/rendering/box.dart

"(which is used by [getDryLayout] behind the scenes)" sounds mysterious, maybe "(the backend implementation of getDryLayout)" or something?

Hixie · 2020-11-17T00:42:05Z

packages/flutter/lib/src/rendering/custom_paint.dart

this is a weird condition

Hixie · 2020-11-17T00:44:24Z

I'm worried about the amount of duplicate code that this involves. Is there any way we can use this to simplify matters? Maybe find places where the intrinsics can defer to the dry layout? Or ways the dry and wet layouts can be implemented as one method?

Hixie · 2020-11-17T00:45:32Z

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

if this is correct, it definitely deserves a comment. :-)

Hixie · 2020-11-17T00:50:04Z

packages/flutter/lib/src/rendering/layout_helper.dart

layouted -> laid out

Hixie · 2020-11-17T00:50:31Z

packages/flutter/lib/src/rendering/layout_helper.dart

might be worth mentioning that these all implement ChildLayouter?

Hixie · 2020-11-17T00:51:10Z

packages/flutter/lib/src/rendering/flex.dart

can this use ChildLayoutHelper?

Hixie · 2020-11-17T00:51:37Z

packages/flutter/lib/src/rendering/flex.dart

can this use ChildLayoutHelper?

Hixie · 2020-11-17T00:52:29Z

packages/flutter/lib/src/rendering/object.dart

might be worth adding something about RenderBox here

Hixie · 2020-11-17T00:53:01Z

packages/flutter/lib/src/rendering/paragraph.dart

Hixie · 2020-11-17T00:59:26Z

I'm worried about the amount of duplicate code that this involves. Is there any way we can use this to simplify matters? Maybe find places where the intrinsics can defer to the dry layout? Or ways the dry and wet layouts can be implemented as one method?

I guess it's not all that bad. You do end up reusing a bunch of code.

Where does the previous intrinsic/layout logic now defer to the new API to solve the bug?

goderbauer · 2020-11-17T01:28:09Z

Where does the previous intrinsic/layout logic now defer to the new API to solve the bug?

It's used in three places:

RenderParagraph. _computeChildrenHeightWithMinIntrinsics (https://github.com/flutter/flutter/pull/70656/files#diff-bc57c6a6c8e97b4cade139f2b1030fffc8707ee8e9713fcab3684e2a213542deL435)
RenderWrap. _computeIntrinsicWidthForHeight (https://github.com/flutter/flutter/pull/70656/files#diff-cebbc90963c6b6d225fcb82d34a027ab6f1fa2815a36a174766bd37b94bd282bL438)
RenderWrap. _computeIntrinsicHeightForWidth (https://github.com/flutter/flutter/pull/70656/files#diff-cebbc90963c6b6d225fcb82d34a027ab6f1fa2815a36a174766bd37b94bd282bL408)

goderbauer · 2020-11-17T01:50:30Z

I was able to simplify the RenderWrap code a little more by implementing its other intrinsics methods in terms of dryLayout. The new API is now used in the following places:

RenderParagraph. _computeChildrenHeightWithMinIntrinsics (https://github.com/flutter/flutter/pull/70656/files#diff-bc57c6a6c8e97b4cade139f2b1030fffc8707ee8e9713fcab3684e2a213542deL435)
All the four intrinsics methods of RenderWrap: https://github.com/flutter/flutter/pull/70656/files#diff-cebbc90963c6b6d225fcb82d34a027ab6f1fa2815a36a174766bd37b94bd282bL463

Hixie · 2020-11-17T03:18:09Z

Nice.

goderbauer · 2020-11-17T21:39:06Z

I updated this PR with the drayLayout implementations for the remaining RenderBoxes.

Piinks

Does this have implications for the solution we're exploring in #70855?

Piinks · 2020-11-20T01:09:22Z

packages/flutter/lib/src/rendering/proxy_box.dart

Can this TODO be removed?

The other intrinsics in this file are still incorrect.

goderbauer · 2020-11-20T21:29:39Z

Does this have implications for the solution we're exploring in #70855?

I removed the legacy mode from here and closed the other PR since SliverFillRemaining seems to require intrinsics.

gspencergoog

This looks like a good change! You might want to updated the PR description to include the links to the breaking change information.

gspencergoog · 2020-11-20T22:43:01Z

packages/flutter/lib/src/rendering/box.dart

The name feels a little odd: If I was someone seeing it for the first time, I might wonder if there was a wet layout too, and what dry refers to.

Maybe computeDryRunLayout? computeDesiredLayout? computeAPrioriLayout?

I am not really happy about any of these names... I added a paragraph to the docs explaining why this is dry and the other layout is wet.

Instead of the paragraph, why not change the name to computeDryRunLayout? Seems more self-documenting.

tvolkert · 2020-12-02T19:55:27Z

packages/flutter/lib/src/rendering/box.dart

 /// significant optimizations. Classes that use this approach should override
-/// [sizedByParent] to return true, and then override [performResize] to set the
-/// [size] using nothing but the constraints, e.g.:
+/// [sizedByParent] to return true, and then override [computeDryLayout] to


We should update the docs in sizedByParent to point the user in the right direction too

Will open a PR to add a note to sizedByParent.

tvolkert · 2020-12-02T19:58:29Z

packages/flutter/lib/src/rendering/box.dart

  void performResize() {
    // default behavior for subclasses that have sizedByParent = true
-    size = constraints.smallest;
+    size = computeDryLayout(constraints);


This seems like a hugely breaking change. Many (if not most) render objects that set sizedByParent to true were likely relying on the old default behavior in performResize, and now they'll get assertion errors in the default implementation of computeDryLayout.

I know this is listed as a breaking change, but was it intentional for it to break so many places?

In the framework there appear to be a total of 16 RenderObject subclasses that set sizedByParent to true. Of those, only 2 used to rely on the default of performResize (constraints.smallest) and the remaining 14 had custom implementations. If I recall correctly, there also wasn't any customer_test that relied on the old default of performResize. With that, I wouldn't really call this "hugely breaking"...

I considered making the default computeDryLayout return constraints.smallest if sizedByParent was set, but ultimately decided against making that method more complicated (and harder to reason about) since the impact of the change seemed so low.

Having said all that, is there something I missed that makes this change more breaking?

Interesting. When I ran across this, it's because I hit the error in my app; I had three render objects in which I had overridden sizedByParent to return true, and in all three cases, I was relying on the old default performResize behavior.

But based on your data, it seems I may be an outlier.

Side-note: once my app's done (and I write tests), I plan to add them to the customer tests 🙂

tvolkert · 2020-12-02T20:03:33Z

packages/flutter/lib/src/rendering/box.dart

+  /// Computes the value returned by [getDryLayout]. Do not call this
+  /// function directly, instead, call [getDryLayout].
+  ///
+  /// Override in subclasses that implement [performLayout] or [performResize].


... or subclasses of RenderBox that set sizedByParent to true but didn't override performResize (see comment below).

Will open a PR to add this.

google-cla bot added the cla: yes label Nov 16, 2020

Hixie reviewed Nov 16, 2020

View reviewed changes

goderbauer force-pushed the dryLayout branch from f84ef65 to 9c234b6 Compare November 16, 2020 23:26

Hixie reviewed Nov 17, 2020

View reviewed changes

packages/flutter/lib/src/rendering/layout_helper.dart Outdated

Copy link

Contributor

Hixie Nov 17, 2020

Choose a reason for hiding this comment

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

layouted -> laid out

goderbauer reacted with thumbs up emoji

Hixie reviewed Nov 17, 2020

View reviewed changes

packages/flutter/lib/src/rendering/flex.dart Outdated

Copy link

Contributor

Hixie Nov 17, 2020

Choose a reason for hiding this comment

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

can this use ChildLayoutHelper?

goderbauer reacted with thumbs up emoji

Hixie reviewed Nov 17, 2020

View reviewed changes

packages/flutter/lib/src/rendering/flex.dart Outdated

Copy link

Contributor

Hixie Nov 17, 2020

Choose a reason for hiding this comment

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

can this use ChildLayoutHelper?

goderbauer reacted with thumbs up emoji

Hixie reviewed Nov 17, 2020

View reviewed changes

packages/flutter/lib/src/rendering/paragraph.dart Outdated

Copy link

Contributor

Hixie Nov 17, 2020

Choose a reason for hiding this comment

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

dry

goderbauer reacted with thumbs up emoji

goderbauer force-pushed the dryLayout branch from b5d6d2f to 56a8f06 Compare November 17, 2020 01:22

flutter-dashboard bot added the a: tests "flutter test", flutter_test, or one of our tests label Nov 17, 2020

goderbauer force-pushed the dryLayout branch from 1796776 to 9a9e9b7 Compare November 17, 2020 21:33

goderbauer mentioned this pull request Nov 18, 2020

Prepare for dry layout feature dnfield/flutter_svg#450

Merged

Piinks self-requested a review November 18, 2020 22:19

Piinks reviewed Nov 20, 2020

View reviewed changes

gspencergoog approved these changes Nov 20, 2020

View reviewed changes

gspencergoog reviewed Nov 20, 2020

View reviewed changes

goderbauer added 12 commits November 20, 2020 15:09

Add dry layout to RenderBox

1c3c9dd

fix test

a56eb17

renderflex refactor

9001da2

add legacy mode

142b368

doc fixes

2fdd463

++

2890b4a

++

4a61bab

++

297f110

Fix FittedBox

57dc4e6

remove legacy

8e103c7

fixTest

6266ea8

++

548b602

goderbauer force-pushed the dryLayout branch from eb3572c to 548b602 Compare November 20, 2020 23:10

goderbauer mentioned this pull request Nov 21, 2020

Breaking Change entry for dry layout on RenderBox flutter/website#5007

Merged

goderbauer merged commit 7a3a29e into flutter:master Nov 21, 2020

goderbauer deleted the dryLayout branch November 21, 2020 01:25

YeungKC mentioned this pull request Nov 21, 2020

Implement material icon toggles #2693

Closed

tvolkert reviewed Dec 2, 2020

View reviewed changes

daohoangson mentioned this pull request Dec 3, 2020

Implement computeDryLayout for various render objects daohoangson/flutter_widget_from_html#384

Closed

This was referenced Dec 7, 2020

Doc fixes for dry layout #71868

Merged

[regression] TextField in a Wrap in AlertDialog crash #71687

Closed

creativecreatorormaybenot mentioned this pull request Dec 9, 2020

Column intrinsic height measurement broken on latest dev #71990

Closed

creativecreatorormaybenot mentioned this pull request Dec 24, 2020

Correct typos in RenderParagraph #72930

Merged

9 tasks

jamesblasco mentioned this pull request Jan 6, 2021

_RenderSegmentedControl's computeDryLayout function uses wrong constraints #73362

Closed

Conversation

goderbauer commented Nov 16, 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!

Hixie commented Nov 17, 2020

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 Nov 17, 2020

Uh oh!

goderbauer commented Nov 17, 2020

Uh oh!

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

Uh oh!

Uh oh!

Hixie commented Nov 17, 2020

Uh oh!

goderbauer commented Nov 17, 2020

Uh oh!

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

goderbauer commented Nov 20, 2020

Uh oh!

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

goderbauer commented Nov 16, 2020 •

edited

Loading

goderbauer commented Nov 17, 2020 •

edited

Loading