`RenderFlex` baseline intrinsics #145483

LongCatIsLooong · 2024-03-20T17:12:47Z

Implements RenderFlex intrinsics calculation when children are baseline aligned in the cross axis.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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

goderbauer · 2024-03-20T21:28:18Z

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

-        reason: 'Baseline metrics are only available after a full layout.',
-      ));
-      return Size.zero;
+    return _computeSizes(constraints, ChildLayoutHelper.dryLayoutChild, ChildLayoutHelper.getDryBaseline).$1;


Instead of using $1 could you give the items in the record meaningful names that could be used here? That would also remove the guesswork from figuring out what the "double" represents?

goderbauer · 2024-03-20T21:33:09Z

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

@@ -1419,33 +1420,46 @@ class RenderBaseline extends RenderShiftedBox {
    markNeedsLayout();
  }

+  (Size, double) _computeSizes(covariant BoxConstraints constraints, ChildLayouter layoutChild, ChildBaselineGetter computeBaseline) {


nit: shoud the last argument be named getBaseline (to avoid the confusion of whether the method passed in here should use the getBaseline methods over the computeBaseline methods)?

goderbauer · 2024-03-20T21:35:39Z

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

+    }
+    final BoxConstraints childConstraints = constraints.loosen();
+    final Size childSize = layoutChild(child, childConstraints);
+    final double childBaseline = computeBaseline(child, childConstraints, baselineType) ?? childSize.height;


For my own understanding: There is a slight semantics change here, right? Previously, we would blow up if the child didn't have a baseline? Now, we assume the baseline to be the child's height. I suspect that this is fine, just making sure that this is intentional... (Maybe also worthwhile to add a test for this case)

Ah right now it returns null if the child doesn't have a baseline.

goderbauer · 2024-03-20T21:39:56Z

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

+    if (result1 == null || result2 == null) {
+      return this.baseline;
+    }
+    return this.baseline + result1 - result2;


I know, the new dry baseline and dry layout support is indirectly already tested, but I think this logic deserves some dedicated tests.

goderbauer · 2024-03-20T21:43:41Z

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

+// A type that describes a 2D size along the main axis and the cross axis of a
+// [RenderFlex].


I don't understand what this represents. Isn't every size by default aligned with the cross and main axis? What does it mean to "convert" a size to a direction?

Also: Should this just be a regular class with two fields: mainAxisExtent and crossAxisExtent? How do we benefit from wrapping Size here?

Yeah it's just ({double mainAxisExtent, double crossAxisExtent}). Under the hood it's still a size so I don't have to reimplement methods like BoxConstraints.constrained(Size size).

goderbauer · 2024-03-20T22:02:58Z

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

+
+  static const _LayoutAxisDimensions empty = _LayoutAxisDimensions((axisSize: _AxisSize(Size.zero), ascentDescent: null));
+
+  // Updates the `current` _LayoutAxisDimensions with the dimensions of the given


TBH, it is not clear to me what updating a _LayoutAxisDimensions with a _AxisSize means semantically? What does this do?

Replaced the class with _AscentDscent and _AxisSize. Both of them have a + binary operator to add to the sizes when a new child is laid out.

goderbauer · 2024-03-20T22:16:13Z

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

-          }
-          inflexibleSpace += mainSize;
-          maxCrossSize = math.max(maxCrossSize, crossSize);
+      Size computeIntrinsicCrossExtentForChild(RenderBox child, BoxConstraints constraints) {


nit: these names seem inaccurate. It does not only return the extend, it returns a full size.

FWIW, I found this code easier to follow when it was implemented in terms of mainExtend and crossExtend and not in terms of width/height.

goderbauer · 2024-03-20T22:16:45Z

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

-      }
-
-      return maxCrossSize;
+      return _computeSizes(constraints: extentConstraints, layoutChild: computeIntrinsicCrossExtentForChild, computeBaseline: ChildLayoutHelper.getDryBaseline).size.crossAxisExtent;


here and elsewhere in this method: these long method calls should probably be formatted over multiple lines.

goderbauer · 2024-03-20T22:29:56Z

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

@@ -12,6 +12,84 @@ import 'layer.dart';
 import 'layout_helper.dart';
 import 'object.dart';

+// A type that describes a 2D size along the main axis and the cross axis of a


Overall, I found the sizing and positioning algorithm way harder to follow with the _AxisSize and _LayoutAxisDimensions abstractions in place - compared to what it was before. Part of the problem is that after reading everything I couldn't explain what an _AxisSize or a _LayoutAxisDimensions is, which makes me think that these are not helpful abstractions?

goderbauer · 2024-03-20T22:33:15Z

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

+    // `defaultComputeDistanceToFirstActualBaseline` does not work even for
+    // Columns, because the children reorder with VerticalDirection.up.


Was it a bug that we used defaultComputeDistanceToFirstActualBaseline here before?

Yes. there's a test that would fail if this diff gets reverted.

need to revert this change because of G3 failures. Will create an issue.

…-intrinsics

LongCatIsLooong · 2024-03-22T20:16:37Z

@goderbauer I found it strange that the last flex child gets the remaining main axis extent instead of flex * spacePerFlex (here). That makes the last flex child special if other flex children opt to not take the full main extent allowance offered to them. I changed the implementation and no existing tests fail. Does that sound like a reasonable change to make?

goderbauer · 2024-03-26T18:05:56Z

I found it strange that the last flex child gets the remaining main axis extent instead of flex * spacePerFlex (here).

This does look strange in deed. I am not aware of a reason for this. But is it really worth it to change this since nobody seems to actually depend on it? Also, if we do change it, maybe we should do it in a dedicated change and not as part of this already pretty big change.

LongCatIsLooong · 2024-03-26T18:12:58Z

I tried the change out in a separate PR and there appeared to be no internal failures.

…-intrinsics

goderbauer

LGTM

goderbauer · 2024-03-26T18:27:31Z

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

+//
+// Baseline-aligned children contributes to the cross axis extent of a [RenderFlex]
+// differently from chidren with other [CrossAxisAlignment]s.
+extension type const _AscentDescent._((double ascent, double descent)? ascentDescent) {


This extension type I am still not sure about. What are we getting by wrapping a record here? Seems like this could easily be just a plain old data class with two fields?

The type doesn't exist at runtime I guess? Probably not much.

It's still allocating a record at runtime. It shouldn't matter whether you allocate an instance of a class or a record? Since the extension type isn't giving us anything here my suggestion is to use the simpler concept of a regular class here.

The (double, double)? pair is probably still needed even if switch to using a class, because it needs an additional bit to represent the "no baselines" state.

goderbauer · 2024-03-26T19:30:48Z

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

-  spaceEvenly,
+  spaceEvenly;
+
+  // Returns (leadingSpace, betweenSpace).


Giving it proper names here would make the function signature self-documenting though and remove the need for this comment.

goderbauer · 2024-03-26T19:33:35Z

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

@@ -273,6 +273,11 @@ class RenderConstrainedBox extends RenderProxyBox {
    return height;
  }

+  @override
+  double? computeDryBaseline(covariant BoxConstraints constraints, TextBaseline baseline) {
+    return child?.computeDryBaseline(_additionalConstraints.enforce(constraints), baseline);


Should this call the get* method instead?

goderbauer · 2024-03-26T19:34:34Z

I tried the change out in a separate PR and there appeared to be no internal failures.

If this causes any issues in the future, it will be much easier to bisect and root cause it if those independent changes are submitted separately.

LongCatIsLooong · 2024-03-27T02:40:42Z

I found it strange that the last flex child gets the remaining main axis extent instead of flex * spacePerFlex

well I tried to create a separate PR for the flex factor change, and when writing tests I realized I read the code wrong. There's no difference between the two except for minor floating point errors, which shouldn't make much of a difference because inconsequential overflow is ignored anyway.

…-intrinsics

flutter/flutter@89ea492...8528881 2024-03-29 34871572+gmackall@users.noreply.github.com Remove trailing commas in android dependency version checking gradle plugin (flutter/flutter#145718) 2024-03-29 polinach@google.com Upgrade leak_tracker. (flutter/flutter#145940) 2024-03-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8b893cb3b607 to 68aa9ba386e1 (2 revisions) (flutter/flutter#145946) 2024-03-29 103135467+sealesj@users.noreply.github.com Refactor flutter_plugins (flutter/flutter#145870) 2024-03-29 34871572+gmackall@users.noreply.github.com Point kotlin message in `gradle_errors.dart` towards new place where templates define the kotlin version (flutter/flutter#145936) 2024-03-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from b917b24836fd to 8b893cb3b607 (1 revision) (flutter/flutter#145941) 2024-03-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from c176d114d01e to b917b24836fd (4 revisions) (flutter/flutter#145938) 2024-03-28 31859944+LongCatIsLooong@users.noreply.github.com `RenderFlex` baseline intrinsics (flutter/flutter#145483) 2024-03-28 godofredoc@google.com Use reporter extended consistently. (flutter/flutter#145617) 2024-03-28 36861262+QuncCccccc@users.noreply.github.com Update tokens to v2.3.5 (flutter/flutter#145356) 2024-03-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from a396dc1a03a9 to c176d114d01e (2 revisions) (flutter/flutter#145934) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC dit@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

LongCatIsLooong added 3 commits March 19, 2024 01:33

Implement RenderFlex.computeDryBaseline, and small refactor

4432aa1

RenderBaseline

8ec30fe

test

141fe30

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 20, 2024

more tests

f1655ee

LongCatIsLooong force-pushed the RenderFlex-baseline-intrinsics branch from 9c518bc to f1655ee Compare March 20, 2024 17:13

LongCatIsLooong commented Mar 20, 2024

View reviewed changes

packages/flutter/lib/src/rendering/flex.dart Outdated Show resolved Hide resolved

LongCatIsLooong marked this pull request as ready for review March 20, 2024 18:10

LongCatIsLooong requested a review from goderbauer March 20, 2024 18:10

comment

14c2f26

goderbauer reviewed Mar 20, 2024

View reviewed changes

LongCatIsLooong added 3 commits March 21, 2024 18:34

review

1fc9193

Merge remote-tracking branch 'upstream/main' into RenderFlex-baseline…

a8ebf7b

…-intrinsics

review

fe324c6

LongCatIsLooong force-pushed the RenderFlex-baseline-intrinsics branch from 2a676e7 to 8b7a1f5 Compare March 22, 2024 07:15

review

5f7bc1e

LongCatIsLooong force-pushed the RenderFlex-baseline-intrinsics branch 2 times, most recently from e81b19d to 1503b30 Compare March 22, 2024 08:41

review

899e331

LongCatIsLooong force-pushed the RenderFlex-baseline-intrinsics branch from 1503b30 to 899e331 Compare March 22, 2024 08:47

LongCatIsLooong force-pushed the RenderFlex-baseline-intrinsics branch from 179eaaa to 5e27bbb Compare March 22, 2024 20:46

No special handling for the last flex child

9fac18c

LongCatIsLooong force-pushed the RenderFlex-baseline-intrinsics branch from 5e27bbb to 9fac18c Compare March 22, 2024 20:52

LongCatIsLooong added 5 commits March 25, 2024 11:35

revert intrinsic main size changes

9824369

revert intrinsics calculation changes

4480405

revert computeDistanceToActualBaseline change

6045799

Merge branch 'master' into RenderFlex-baseline-intrinsics

d7b2411

remove failing test

0adfbb7

LongCatIsLooong requested a review from goderbauer March 26, 2024 16:25

Merge remote-tracking branch 'upstream/main' into RenderFlex-baseline…

d297cde

…-intrinsics

goderbauer approved these changes Mar 26, 2024

View reviewed changes

LongCatIsLooong added 3 commits March 26, 2024 19:57

review

a98b910

Merge remote-tracking branch 'upstream/main' into RenderFlex-baseline…

89bf1a5

…-intrinsics

Skip check to avoid slowing down the app too much

eba50c5

LongCatIsLooong force-pushed the RenderFlex-baseline-intrinsics branch from 5b73103 to eba50c5 Compare March 28, 2024 02:31

Merge branch 'master' into RenderFlex-baseline-intrinsics

256d1fc

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 28, 2024

auto-submit bot merged commit 460a5e2 into flutter:master Mar 28, 2024

LongCatIsLooong deleted the RenderFlex-baseline-intrinsics branch March 28, 2024 23:09

engine-flutter-autoroll mentioned this pull request Mar 29, 2024

Roll Flutter from 89ea49204b37 to 1a2f34ab5b9a (10 revisions) flutter/packages#6435

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2024

460a5e2 RenderFlex baseline intrinsics (flutter/flutter#145483)

ba03754

engine-flutter-autoroll mentioned this pull request Mar 29, 2024

Roll Flutter from 89ea49204b37 to 85288818b59e (11 revisions) flutter/packages#6436

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2024

460a5e2 RenderFlex baseline intrinsics (flutter/flutter#145483)

f512bf1

HansMuller mentioned this pull request Mar 29, 2024

Flutter Framework Team: Notable Commits #121415

Closed

		// A type that describes a 2D size along the main axis and the cross axis of a
		// [RenderFlex].


		static const _LayoutAxisDimensions empty = _LayoutAxisDimensions((axisSize: _AxisSize(Size.zero), ascentDescent: null));

		// Updates the `current` _LayoutAxisDimensions with the dimensions of the given

		// `defaultComputeDistanceToFirstActualBaseline` does not work even for
		// Columns, because the children reorder with VerticalDirection.up.

RenderFlex baseline intrinsics #145483

RenderFlex baseline intrinsics #145483

Uh oh!

Conversation

LongCatIsLooong commented Mar 20, 2024

Pre-launch Checklist

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!

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!

LongCatIsLooong commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Mar 26, 2024

Uh oh!

LongCatIsLooong commented Mar 26, 2024

Uh oh!

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

goderbauer commented Mar 26, 2024

Uh oh!

LongCatIsLooong commented Mar 27, 2024

Uh oh!

Uh oh!

`RenderFlex` baseline intrinsics #145483

`RenderFlex` baseline intrinsics #145483

LongCatIsLooong commented Mar 22, 2024 •

edited

Loading