Implements `RenderBox.computeDryBaseline` for material render boxes by LongCatIsLooong · Pull Request #146027 · flutter/flutter

LongCatIsLooong · 2024-03-30T05:24:34Z

RenderChip and RenderInputDecorator changes are larger so they are not included.

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.

LongCatIsLooong · 2024-03-30T05:29:30Z

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

Updating documentation, no behavior changes here.

Also the documentation says "if the titles' overall height is greater than 72" but the code checks tileHeight > 72. Typo?

Hm, what am I missing here? Isn't tileHeight > 72 the same as saying the tile hight is greater than 72?

the documentation says "title" but the comment says "tile". These two have different sizes.

Oh, I missed that one is tile and the other one is title. Probably a typo in the docs? Since nobody seems to have complained about the implementation, lets make the doc match the implementation?

goderbauer · 2024-04-01T17:45:14Z

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

-    }
-    final Size size = this.size = _computeSize(constraints, ChildLayoutHelper.layoutChild);
-    final Size childSize = child.size;
+  Size computeDryLayout(BoxConstraints constraints) => child == null ? Size.zero : constraints.biggest;


Oh, nice simplification of what we were doing before. No need to call into the child.

goderbauer · 2024-04-01T17:50:48Z

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

+    child.layout(childConstraints, parentUsesSize: !childConstraints.isTight);
+    final BoxParentData childParentData = child.parentData! as BoxParentData;
+    final Size childSize = childConstraints.isTight ? childConstraints.smallest : child.size;
+      childParentData.offset = _getPositionForChild(size, childSize);


nit: indentation

goderbauer · 2024-04-01T17:55:53Z

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

+    final Size childSize = child.getDryLayout(constraints);
+    return result + Alignment.center.alongOffset(getDryLayout(constraints) - childSize as Offset).dy;
+  }
+


It seems like this code pattern is repeated in a couple of subclasses of RenderShiftedBox. Would it make sense to have RenderShiftedBox implement this as default?

RenderShiftedBox doesn't specify its layout algorithm. To get the baseline of a child computeDryBaseline have to know the offset of each child. computeDryLayout isn't implemented for this abstract class either.

goderbauer · 2024-04-01T17:58:49Z

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

+  /// This is the [alignment] resolved against [textDirection]. Subclasses should
+  /// use [resolvedAlignment] instead of [alignment] directly, for computing the
+  /// child's offset.
+  @protected


Would we still have to expose this if we'd move some dryBaseline base functionality into one of the base classes?

not for _RenderAppBarTitleBox because it changes the constraints before passing it onto its child:

class _RenderAppBarTitleBox extends RenderAligningShiftedBox { @override double? computeDryBaseline(covariant BoxConstraints constraints, TextBaseline baseline) { final BoxConstraints innerConstraints = constraints.copyWith(maxHeight: double.infinity); ... }

goderbauer · 2024-04-01T17:59:34Z

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

+  ///
+  /// This is the [alignment] resolved against [textDirection]. Subclasses should
+  /// use [resolvedAlignment] instead of [alignment] directly, for computing the
+  /// child's offset.


If I rely on this, how do I know it has changed and I need to redo the work depending on it?

If alignment or textDirection changes then markNeedsLayout will be called?

Maybe document that layout will be triggered whenever this changes?

goderbauer · 2024-04-01T18:05:16Z

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

Hm, what am I missing here? Isn't tileHeight > 72 the same as saying the tile hight is greater than 72?

goderbauer · 2024-04-01T18:09:07Z

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


  RenderBox? get leading => childForSlot(_ListTileSlot.leading);
-  RenderBox? get title => childForSlot(_ListTileSlot.title);
+  RenderBox get title => childForSlot(_ListTileSlot.title)!;


This change is kinda odd for breaking the symmetry with the other getters. Why is it guaranteed that we have a title?

Also strange in combination with line 1124 and 1129 where it is assumed that we may not have a title again...

_RenderListTile assumes the title is not null in its layout methods. Also its render object widget has these fields:

final Widget? leading; final Widget title; final Widget? subtitle; final Widget? trailing;

so unless title doesn't create any render objects it's going to be non-null.

But unfortunatly SlottedContainerRenderObjectMixin uses children in its attach implementation so it's possible the title has not been added as a child when attach is called.

goderbauer · 2024-04-01T18:11:22Z

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

+    BoxConstraints constraints,
+    { _PositionChild? positionChild, }
+  ) {


nit: our formatting style here (I think) is:

Suggested change

BoxConstraints constraints,

{ _PositionChild? positionChild, }

) {

BoxConstraints constraints, {

_PositionChild? positionChild,

}) {

goderbauer

LGTM

… boxes (flutter/flutter#146027)

Roll Flutter from a418568 to e868e2b (34 revisions) flutter/flutter@a418568...e868e2b 2024-04-03 [email protected] Add SegmentedButton expand feature (flutter/flutter#142804) 2024-04-03 [email protected] Roll Flutter Engine from e36b9b10c36f to 56fa2c33a5f7 (1 revision) (flutter/flutter#146205) 2024-04-03 [email protected] Roll Packages from 83f3842 to 0e848fa (3 revisions) (flutter/flutter#146201) 2024-04-03 [email protected] Enhance ColorScheme.fromSeed with a new `variant` parameter (flutter/flutter#144805) 2024-04-03 [email protected] Roll Flutter Engine from 0280de5be276 to e36b9b10c36f (1 revision) (flutter/flutter#146200) 2024-04-03 [email protected] Fix typos in bottom_sheet.dart (flutter/flutter#146188) 2024-04-03 [email protected] Roll Flutter Engine from 979030d81f8d to 0280de5be276 (2 revisions) (flutter/flutter#146199) 2024-04-03 [email protected] Roll Flutter Engine from bef3bbe3f74e to 979030d81f8d (1 revision) (flutter/flutter#146186) 2024-04-03 [email protected] Roll Flutter Engine from 5fc83bc24b2e to bef3bbe3f74e (1 revision) (flutter/flutter#146183) 2024-04-03 [email protected] Roll Flutter Engine from 0da1b2eb370a to 5fc83bc24b2e (1 revision) (flutter/flutter#146180) 2024-04-03 [email protected] Avoid calling `TextPainter.plainText` for simple static text (flutter/flutter#146084) 2024-04-03 [email protected] Roll Flutter Engine from e603f89844a9 to 0da1b2eb370a (2 revisions) (flutter/flutter#146179) 2024-04-03 [email protected] Roll Flutter Engine from ef60a95d78c1 to e603f89844a9 (3 revisions) (flutter/flutter#146177) 2024-04-03 [email protected] Fix chip baseline implementation (flutter/flutter#146162) 2024-04-03 [email protected] Roll Flutter Engine from bb4ec2d7eb39 to ef60a95d78c1 (1 revision) (flutter/flutter#146176) 2024-04-03 [email protected] Add tests for material_state_mouse_cursor.0.dart API example. (flutter/flutter#145987) 2024-04-03 [email protected] Update material_color_utilities package version to latest 0.11.1 (flutter/flutter#145959) 2024-04-03 [email protected] Roll Flutter Engine from 5f6dec8bd877 to bb4ec2d7eb39 (4 revisions) (flutter/flutter#146169) 2024-04-03 [email protected] Roll Flutter Engine from 5dbcfdc2a456 to 5f6dec8bd877 (1 revision) (flutter/flutter#146163) 2024-04-02 [email protected] Dispose FocusNode in tests. (flutter/flutter#146161) 2024-04-02 [email protected] Add `none` language strings to code blocks. (flutter/flutter#146154) 2024-04-02 [email protected] Refactor docs (flutter/flutter#145998) 2024-04-02 [email protected] Roll Flutter Engine from c60b00a20fc3 to 5dbcfdc2a456 (3 revisions) (flutter/flutter#146159) 2024-04-02 [email protected] Marks Linux_pixel_7pro complex_layout_scroll_perf_impeller__timeline_summary to be unflaky (flutter/flutter#140038) 2024-04-02 [email protected] Roll Flutter Engine from 5bf8b94505a4 to c60b00a20fc3 (2 revisions) (flutter/flutter#146157) 2024-04-02 [email protected] Refactor analyze (flutter/flutter#146138) 2024-04-02 [email protected] Implement SelectionArea triple click gestures (flutter/flutter#144563) 2024-04-02 [email protected] Roll Flutter Engine from 6883f7313da0 to 5bf8b94505a4 (2 revisions) (flutter/flutter#146152) 2024-04-02 [email protected] Fix border color is wrong for a focused and hovered TextField (flutter/flutter#146127) 2024-04-02 [email protected] Sync lints and enable `annotate_redeclares` (flutter/flutter#146144) 2024-04-02 [email protected] Implements `RenderBox.computeDryBaseline` for material render boxes (flutter/flutter#146027) 2024-04-02 [email protected] Roll Flutter Engine from 523fc953ebc8 to 6883f7313da0 (2 revisions) (flutter/flutter#146140) 2024-04-02 [email protected] Fix `MenuItemButton` overflow (flutter/flutter#143932) 2024-04-02 [email protected] Roll Packages from d5aff19 to 83f3842 (4 revisions) (flutter/flutter#146134) 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 [email protected],[email protected],[email protected] 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 ...

LongCatIsLooong added 2 commits March 29, 2024 15:27

wip

b58394a

list tile

d5c81d4

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 30, 2024

list tile

270e4d2

LongCatIsLooong force-pushed the material-dry-baseline branch from 6600a16 to 270e4d2 Compare March 30, 2024 05:46

LongCatIsLooong commented Mar 30, 2024

View reviewed changes

LongCatIsLooong marked this pull request as ready for review March 30, 2024 20:55

Merge branch 'master' into material-dry-baseline

7c56e78

LongCatIsLooong requested a review from goderbauer April 1, 2024 17:21

goderbauer reviewed Apr 1, 2024

View reviewed changes

review

cf47106

goderbauer approved these changes Apr 2, 2024

View reviewed changes

Update shifted_box.dart

3d0ca68

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

auto-submit bot merged commit fd98a2f into flutter:master Apr 2, 2024

LongCatIsLooong deleted the material-dry-baseline branch April 2, 2024 18:43

engine-flutter-autoroll mentioned this pull request Apr 3, 2024

Roll Flutter from a418568a6498 to bd4c17155d02 (28 revisions) flutter/packages#6452

Closed

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

fd98a2f Implements RenderBox.computeDryBaseline for material render…

b9a91fc

… boxes (flutter/flutter#146027)

engine-flutter-autoroll mentioned this pull request Apr 3, 2024

Roll Flutter from a418568a6498 to 1326c8ee536e (29 revisions) flutter/packages#6454

Closed

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

fd98a2f Implements RenderBox.computeDryBaseline for material render…

16771c1

… boxes (flutter/flutter#146027)

engine-flutter-autoroll mentioned this pull request Apr 3, 2024

Roll Flutter from a418568a6498 to e868e2b3834f (34 revisions) flutter/packages#6455

Merged

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

fd98a2f Implements RenderBox.computeDryBaseline for material render…

e7fe85b

… boxes (flutter/flutter#146027)

HansMuller mentioned this pull request Apr 5, 2024

Flutter Framework Team: Notable Commits #121415

Closed

Conversation

LongCatIsLooong commented Mar 30, 2024

Pre-launch Checklist

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!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants