`RenderBox.{compute,get}DryBaseline` #138369

LongCatIsLooong · 2023-11-13T19:56:18Z

Introduce methods for computing the baseline location of a RenderBox without affecting the current layout (see packages/flutter/lib/src/rendering/box.dart for the signatures).

In follow-up PRs, the new methods will be called mostly in computeDryLayout for RenderBoxes with baseline-aligned children. Unfortunately LayoutBuilders still don't have the ability to compute the intrinsic dimensions sideeffect-free.

Issues that depend on this:
#138794
#71687

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.
All existing and new tests are passing.

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

…into text-intrinsics

goderbauer

I only looked at box.dart so far.

goderbauer · 2023-11-27T18:31:58Z

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

+  /// When the input constraints equal to the constraints given to this
+  /// [RenderBox] by the parent, the returned baseline offset value is guaranteed
+  /// to be consistent with the value returned by [getDistanceToBaseline]. It's


nit: I find it hard to follow what constraints you mean here by "input" constraints and "given" constraints. I assume it should say if the constraints passed to this method match the constraints that will be (or were) given to the render object during layout, then its return value is guaranteed to be consistent with getDistanceToBaseline. Or something like that.

goderbauer · 2023-11-27T18:35:36Z

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

-    return _computeDryLayout(constraints);
+  Size getDryLayout(covariant BoxConstraints constraints) => _intrinsicsCache.getOrCompute(_IntrinsicsCacheType.dryLayout, constraints, _computeDryLayout);
+
+  /// Returns the distance from the top of the box to the first baseline of the


Questions I have after reading this doc that I wish the doc would answer:

What's the difference to getDistanceToBaseline? When should I call this new method, when the other one? (if the only difference is that one is destructive and the other one isn't, why do we still offer the destructive one?)

getDistanceToBaseline has restrictions on when it can be called (only after layout), same for this one? Different restrictions?

I am implementing my own RenderObject, what do I need to do to implement getDryBaseline (e.g. should I override this method or computeDryBaseline or something different)?

What should I do if I cannot implement getDryBaseline for my custom render object (presumably, for certain layout builder situation, this cannot actually be implemented in a non-destructive way?) Also, what are common cases where this cannot be implemented?

Maybe explain what makes this "dry" - what would be a "wet" baseline? See the corresponding explanation on "computeDryLayout".

goderbauer · 2023-11-27T18:38:19Z

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

@@ -1873,6 +1906,21 @@ abstract class RenderBox extends RenderObject {
    return result;
  }

+  double? _computeDryBaseline((BoxConstraints, TextBaseline) pair) {


nit: this would actually be slightly easier to read if the pair is passed in as individual arguments. Why bother with the pair?

I see why you want the pair to make the computer signature in the cache work. This is fine then.

goderbauer · 2023-11-27T18:44:16Z

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

-  /// doing a full layout), it may be computed by a callback about which the
-  /// render object cannot reason, or the layout is so complex that it
-  /// is impractical to calculate the size in an efficient way.
+  /// compute their size without affecting the current layout. For example,


"affecting the current layout" sounds a little misleading like it would actually change what's shown on screen. Wouldn't it in most cases just invalidate the layout and cause a bunch of additional work to recompute it at layout time? Maybe we could say "without invalidating the current layout" (maybe with the addition of saying something about how this would case extra work during layout).

Reverted the change and deleted the part about baseline.

goderbauer · 2023-11-27T18:46:54Z

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

+  /// a [LayoutBuilder] changes its live render subtree based on the incoming
+  /// constraints, so currently there is no easy way to compute its size
+  /// speculatively given arbitrary constraints, for that alters the [RenderBox]'s
+  /// current layout.


nit: I find the old explanation (minus the baseline talk) a little more approachable. It's not just THE [LayoutBuilder] widget thats affected by this. Any widget that invokes a layout callback (about which the render object cannot reason) has this problem and the note about complexity IMO still holds as well.

goderbauer · 2023-11-27T19:29:38Z

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

+  /// non-debug modes.
+  @nonVirtual
+  @visibleForTesting
+  double? debugGetDistanceToBaseline(TextBaseline baseline) {


It's not clear to me why this method exists. Why not just call getDistanceToBaseline directly instead of this?

Also, do we actually want a debug method to insert things in the cache and with that change future behavior? Seems maybe a little dangerous?

getDistanceToBaseline can't be called in tests (there are a few asserts that would fire if you call that method during the idle phase).

Add that information to the doc comment?

goderbauer · 2023-11-27T19:36:48Z

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

-      // If we have cached data, then someone must have used our data.
-      // Since the parent will shortly be marked dirty, we can forget that they
-      // used the baseline and/or intrinsic dimensions. If they use them again,
-      // then we'll fill the cache again, and if we get dirty again, we'll
-      // notify them again.


Can you keep a comment along these lines on the new clear method of the cache that documents why it is ok to just forget the cached data?

goderbauer · 2023-11-27T19:38:52Z

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

-    if (_clearCachedData() && parent is RenderObject) {
+    // A render object's performLayout implementation may depend on the baseline
+    // location or the intrinsic dimensions of a descendant, even when there are
+    // relayout boundaries between them. The cache being non-null implies that


"the cache being non-null" is not super intuitive to understand here since the following lines don't check any caches for null. Maybe be a bit more explicit here why a true return value of clear means we have to mark the parent dirty?

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

goderbauer · 2023-11-27T19:47:05Z

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

+  /// This method calls [computeDryBaseline] under the hood and caches the result.
+  /// When the input constraints equal to the constraints given to this
+  /// [RenderBox] by the parent, the returned baseline offset value is guaranteed
+  /// to be consistent with the value returned by [getDistanceToBaseline]. It's


Can we auto-test that actual implementations actually fulfill this guarantee? We have the debugCheckIntrinsicSizes (enabled in our tests) that enables a bunch of extra asserts to check that the intrinsics calculations match expectations. Could we add an extra check that compares getDryBaseline and getDistanceToBaseline?

goderbauer · 2023-11-27T19:50:27Z

Can you remind me (and also add it to the PR description) what problem we can now solve by having the ability to compute baselines without non-destructively? Wasn't there some kind of an issue on file that we could link this PR to as a step towards resolving that issue?

LongCatIsLooong · 2023-11-27T21:01:09Z

Can you remind me (and also add it to the PR description) what problem we can now solve by having the ability to compute baselines without non-destructively? Wasn't there some kind of an issue on file that we could link this PR to as a step towards resolving that issue?

#71687 would be an example. But the PR by itself doesn't fix anything. The intrinsics methods have to be updated to use the new method(s) and I'm planning to do that in a different PR.

goderbauer · 2023-11-27T22:10:43Z

#71687 would be an example. But the PR by itself doesn't fix anything. The intrinsics methods have to be updated to use the new method(s) and I'm planning to do that in a different PR.

Could you still mention that issue in the PR description and indicate that this is work towards fixing the issue to leave some breadcrumbs/context for future archeologists?

goderbauer

Review of the rest.

goderbauer · 2023-11-27T22:45:17Z

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

+      child = childAfter(child);
+    }
+    return baselineOffset;
+  }


(this one looks very similar to the previous one, if this is a general pattern, maybe we should provide a helper e.g. in RenderBoxContainerDefaultsMixin?)

It's quite specific, the 2 widgets are similar (newer / older versions of the same UI component).

goderbauer · 2023-11-27T22:51:07Z

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

+      return null;
+    }
+    final Size childSize = child.getDryLayout(enforcedConstraint);
+    final bool isAbove = anchorAbove.dy >= childSize.height - _kToolbarArrowSize.height * 2;


As said elsewhere, my main worry here is the amount of duplication that this introduces. How are we going to remember to fix up this line if we change the "wet" getter "isAbove" below? Same holds true for the enforcedConstraints above. This looks like it could be a maintenance nightmare. Is there a way to share more logic between the wet and dry implementation, similarly to what we are trying to do with performLayout and computeDryLayout for individual RenderObjects?

How are we going to remember to fix up this line if we change the "wet" getter "isAbove" below?

Added a check in RenderBox.debugPaint (can't do that in layout because some children may still be dirty) that verifies the getDryBaseline and getDistanceToBaseline return the same value, and another check in RenderBox.size getter to prevent access to size or child.size during dry baseline calculation.

the amount of duplication

Moved the child positioning code to a common method.

goderbauer · 2023-11-27T22:55:45Z

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

+    final double yAdjustment = clampDouble(childSize.height + padding.bottom - maxExtent, 0, padding.bottom);
+    final double offsetY = constraints.biggest.height - childSize.height - padding.bottom + yAdjustment;


Again, these lines appear to just be copied over from performLayout, EXCEPT to my initial surprise size.height has been replaced with constraints.biggest.height. If you study the code carefully, it will become clear that size.height == constraints.biggest.height for this particular render object, but it adds mental overhead to the maintainability and introduces further room for bugs.

goderbauer · 2023-11-27T23:12:55Z

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

@@ -325,6 +325,15 @@ class _RenderLayoutBuilder extends RenderBox with RenderObjectWithChildMixin<Ren
    return Size.zero;
  }

+  @override
+  double? computeDryBaseline(BoxConstraints constraints, TextBaseline baseline) {
+    assert(debugCannotComputeDryLayout(reason:


Doc on debugCannotComputeDryLayout needs to be updated if this may also be called from debugDryBaseline.

(interestingly, debugCannotComputeDryLayout seems to be mostly called from dryLayout implementations that need baseline metrics)

goderbauer · 2023-11-27T23:13:32Z

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

+  @override
+  double? computeDryBaseline(BoxConstraints constraints, TextBaseline baseline) {
+    assert(debugCannotComputeDryLayout(reason:
+      'Calculating the dry layout would require running the layout callback '


should this be specific about dry baseline instead of just generically talking about dry layout? (same for the default ErrorSummary that debugCannotComputeDryLayout produces)

goderbauer · 2023-11-27T23:53:13Z

packages/flutter/test/rendering/baseline_utils.dart

+  }
+}
+
+double? verifyDryBaseline(RenderBox box) {


This method could benefit from a doc comment.

Just from the method name I would have expected that this method would only contain the expect on line 57. It is not clear to me what the beforePaintInstructions/afterPaintInstructions and their associated expects are supposed to test? And the return value is also somewhat surprising (to me it implies that I need to compare it with something in my test in an expect, but the expects seem to be build into this methods...)

I'm considering getting rid of this helper and banking on that the assert in debugPaint would catch any inconsistencies here.

goderbauer · 2023-11-27T23:57:38Z

packages/flutter/test/rendering/baseline_utils.dart

+      box.getDryBaseline(box.constraints, baseline),
+      box.debugGetDistanceToBaseline(baseline),


Isn't getDryBaseline just putting the computed baseline into the cache and debugGetDistanceToBaseline retrieving it from there without redoing the actual calculation? Wouldn't we want to test the implementations behind these instead of just testing the caching behavior?

Maybe it would be worthwhile to add a test for this util that makes sure RenderObjects with non-matching implementations fail this check?

goderbauer · 2023-11-27T23:59:36Z

packages/flutter/test/rendering/baseline_utils.dart

+  );
+
+  expect(beforePaintInstructions, equals(afterPaintInstructions.map(_PaintInstructionMatcher.new)));
+  return box.getDryBaseline(box.constraints, TextBaseline.alphabetic);


Why the alphabetic instead of the other one?

goderbauer · 2023-11-28T00:00:23Z

packages/flutter/test/rendering/baseline_utils.dart

+      }),
+      paintsNothing,
+    ),
+  );


Help me, what is this expect supposed to be asserting?

Sometimes the render object doesn't paint anything so that has to be matched using paintsNothing.

goderbauer · 2023-11-28T00:04:32Z

packages/flutter/test/rendering/baseline_utils.dart

+import 'package:flutter/rendering.dart';
+import 'package:flutter_test/flutter_test.dart';
+
+class _PaintInstructionMatcher extends Matcher {


Does this matcher provide a clear error message when it fails? I see a lot of our custom matcher implement describeMismatch to help out with that, but this one doesn't.

goderbauer · 2023-11-28T00:07:10Z

How did you decide which RenderBoxes need a custom computeDryBaseline implementation? How should I as a custom render object author determine that I need one (the latter should be added to the docs somewhere)?

…into text-intrinsics

LongCatIsLooong · 2023-12-15T23:12:40Z

I think I've addressed most comments. Right now in debug mode, the RenderBox.getDryBaseline method still returns a valid baseline offset even if there's a LayoutBuilder descendent, if the RenderBox has been previously laid out at that input BoxConstraints and the baseline offset cache hasn't been invalidated. To fix that we'll need to distinguish "dry" value from "wet" value in the cache (but only in debug mode) so I'm not sure if it's worth implementing.

LongCatIsLooong · 2024-03-13T04:08:27Z

Splitting this into smaller PRs.

…without affecting the current layout (#144655) Extracted from #138369 Introduces `RenderBox.{compute,get}DryBaseline` for computing the baseline location in `RenderBox.computeDryLayout`.

RenderBox.{compute,get}DryBaseline

683e875

LongCatIsLooong force-pushed the text-intrinsics branch 2 times, most recently from ae99e7f to db4f937 Compare November 13, 2023 21:36

tests

dafa737

LongCatIsLooong force-pushed the text-intrinsics branch from db4f937 to dafa737 Compare November 13, 2023 22:18

LongCatIsLooong requested a review from goderbauer November 13, 2023 22:19

LongCatIsLooong marked this pull request as ready for review November 13, 2023 22:19

LongCatIsLooong added 3 commits November 13, 2023 19:38

Merge branch 'master' into text-intrinsics

91ace91

fix test failures

6fbdd14

Merge branch 'text-intrinsics' of github.com:LongCatIsLooong/flutter …

c6be509

…into text-intrinsics

LongCatIsLooong force-pushed the text-intrinsics branch from 7f53ebe to c6be509 Compare November 14, 2023 08:36

fix ListTile test failure

cb6b56d

goderbauer reviewed Nov 27, 2023

View reviewed changes

goderbauer reviewed Nov 28, 2023

View reviewed changes

LongCatIsLooong added 2 commits November 28, 2023 12:48

Merge remote-tracking branch 'upstream/main' into text-intrinsics

09c3e74

Merge remote-tracking branch 'upstream/main' into text-intrinsics

490ebd8

LongCatIsLooong marked this pull request as draft November 29, 2023 09:15

LongCatIsLooong added 4 commits November 29, 2023 01:15

nav bar

8854152

Merge branch 'text-intrinsics' of github.com:LongCatIsLooong/flutter …

85abc51

…into text-intrinsics

wip

3d6e7d3

wip

9d9bb92

LongCatIsLooong force-pushed the text-intrinsics branch from 7a9c27b to 9d9bb92 Compare November 30, 2023 09:44

LongCatIsLooong added 4 commits December 4, 2023 16:30

fix more implementations

a519954

Merge remote-tracking branch 'upstream/main' into text-intrinsics

833103e

fix more implementations

79b7fc7

review

9421735

fix RenderAnimatedSize

6540c43

LongCatIsLooong added 9 commits December 10, 2023 18:09

fix RenderAspectRatio

6a82def

review

b89de92

Merge remote-tracking branch 'upstream/main' into text-intrinsics

44d2169

fix dem tests

33e7a3b

revert test changes

b43e64b

syntax

3fbdf2e

revert more tests

432b95d

rename

0a4aa40

Merge remote-tracking branch 'upstream/main' into text-intrinsics

b55a372

LongCatIsLooong marked this pull request as ready for review December 15, 2023 23:14

LongCatIsLooong requested a review from goderbauer December 15, 2023 23:14

Merge remote-tracking branch 'upstream/main' into text-intrinsics

532cfea

LongCatIsLooong mentioned this pull request Mar 6, 2024

Introduce methods for computing the baseline location of a RenderBox without affecting the current layout #144655

Merged

9 tasks

LongCatIsLooong closed this Mar 13, 2024

		final double yAdjustment = clampDouble(childSize.height + padding.bottom - maxExtent, 0, padding.bottom);
		final double offsetY = constraints.biggest.height - childSize.height - padding.bottom + yAdjustment;

		box.getDryBaseline(box.constraints, baseline),
		box.debugGetDistanceToBaseline(baseline),

RenderBox.{compute,get}DryBaseline #138369

RenderBox.{compute,get}DryBaseline #138369

Conversation

LongCatIsLooong commented Nov 13, 2023 • edited

Pre-launch Checklist

goderbauer 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

goderbauer Nov 27, 2023 • edited

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 commented Nov 27, 2023

LongCatIsLooong commented Nov 27, 2023

goderbauer commented Nov 27, 2023

goderbauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goderbauer Nov 27, 2023 • edited

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 commented Nov 28, 2023

LongCatIsLooong commented Dec 15, 2023

LongCatIsLooong commented Mar 13, 2024

`RenderBox.{compute,get}DryBaseline` #138369

`RenderBox.{compute,get}DryBaseline` #138369

LongCatIsLooong commented Nov 13, 2023 •

edited

goderbauer Nov 27, 2023 •

edited

goderbauer Nov 27, 2023 •

edited