Prepare for RenderDecorator.computeBaseline changes. #146363
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
framework
LongCatIsLooong
force-pushed
the
input-decorator-baseline
branch
from
485f679 to
a30b462
Compare
9 tasks
justinmc
approved these changes
Apr 5, 2024
Contributor
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 as long as the Google tests pass.
Thanks for cleaning this up. _layout especially is scarily fragile. The diff was hard to read at times but overall it looks good if it passes tests.
| RenderBox? get prefixIcon => childForSlot(_DecorationSlot.prefixIcon); | ||
| RenderBox? get suffixIcon => childForSlot(_DecorationSlot.suffixIcon); | ||
| RenderBox? get helperError => childForSlot(_DecorationSlot.helperError); | ||
| RenderBox get helperError => childForSlot(_DecorationSlot.helperError)!; |
Contributor
There was a problem hiding this comment.
Why is this no longer nullable? Is this like the first one in a greater planned refactor?
Contributor
Author
There was a problem hiding this comment.
The _layout method assumes it's non-null so I think this makes the assumption more explicit.
| // This method applies layout to all of the renderers except the container. | ||
| // For convenience, the container is laid out in performLayout(). | ||
| _RenderDecorationLayout _layout(BoxConstraints layoutConstraints) { | ||
| _RenderDecorationLayout _layout(BoxConstraints constraints) { |
Contributor
There was a problem hiding this comment.
That seems less redundant 👍
| // end. | ||
| final double t = textAlignVertical.y; | ||
| return middle + (end - middle) * t; | ||
| static double _interpolateThree(double begin, double middle, double end, TextAlignVertical textAlignVertical) { |
Contributor
There was a problem hiding this comment.
+1 to making stuff like this static.
| double computeMinIntrinsicWidth(double height) { | ||
| return _minWidth(icon, height) | ||
| + (prefixIcon != null ? 0.0 : (textDirection == TextDirection.ltr ? contentPadding.left : contentPadding.right)) | ||
| + (prefixIcon != null ? 0.0 : contentPadding.start) |
Contributor
There was a problem hiding this comment.
Good call making this EdgeInsetsDirectional.
| final double helperErrorBaseline = helperError.getDistanceToBaseline(TextBaseline.alphabetic)!; | ||
| final double counterBaseline = counter?.getDistanceToBaseline(TextBaseline.alphabetic)! ?? 0.0; | ||
|
|
||
| double start, end; |
Contributor
There was a problem hiding this comment.
Could these be late final?
Contributor
Author
There was a problem hiding this comment.
they are updated after laying out each child.
| expect(intrinsicHeight, equals(height)); | ||
| }); | ||
|
|
||
| testWidgets('Error message for negative baseline', (WidgetTester tester) async { |
Contributor
Author
There was a problem hiding this comment.
I added that in #74326. There used to be a helper method that lays out a child and computes its baseline, even for center aligned children. It's removed. Having negative baselines may look ugly but I don't think it asserts anymore.
LongCatIsLooong
force-pushed
the
input-decorator-baseline
branch
from
79cfd2f to
18ace04
Compare
LongCatIsLooong
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Apr 7, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Apr 7, 2024
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Apr 7, 2024
flutter/flutter@477ebd8...98d23f7 2024-04-06 [email protected] Prepare for RenderDecorator.computeBaseline changes. (flutter/flutter#146363) 2024-04-06 [email protected] Roll Flutter Engine from 563bdb1d7976 to 605b3f35fa0a (1 revision) (flutter/flutter#146393) 2024-04-06 [email protected] Roll Flutter Engine from 482172d16528 to 563bdb1d7976 (1 revision) (flutter/flutter#146385) 2024-04-06 [email protected] Roll Flutter Engine from b0d7ac5425e9 to 482172d16528 (1 revision) (flutter/flutter#146382) 2024-04-06 [email protected] Roll Flutter Engine from df9f7433fc70 to b0d7ac5425e9 (2 revisions) (flutter/flutter#146377) 2024-04-06 [email protected] Roll Flutter Engine from b5039157cbc1 to df9f7433fc70 (1 revision) (flutter/flutter#146373) 2024-04-05 [email protected] Make FileSystem dependency explicit througout. (flutter/flutter#146008) 2024-04-05 [email protected] Roll Flutter Engine from 48604dfd9d49 to b5039157cbc1 (3 revisions) (flutter/flutter#146370) 2024-04-05 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.24.9 to 3.24.10 (flutter/flutter#146368) 2024-04-05 [email protected] Roll Flutter Engine from da995a1061c5 to 48604dfd9d49 (2 revisions) (flutter/flutter#146366) 2024-04-05 [email protected] Roll Flutter Engine from 6a478d6931b2 to da995a1061c5 (1 revision) (flutter/flutter#146364) 2024-04-05 [email protected] Roll Flutter Engine from 62df3bd5f681 to 6a478d6931b2 (3 revisions) (flutter/flutter#146360) 2024-04-05 [email protected] Copy part files and sourcemaps when building with dart2js. (flutter/flutter#146356) 2024-04-05 [email protected] Roll Flutter Engine from d048b9aed529 to 62df3bd5f681 (1 revision) (flutter/flutter#146357) 2024-04-05 [email protected] Reland "Bump to AGP 8.1/Gradle 8.3 (almost) everywhere" (flutter/flutter#146307) 2024-04-05 [email protected] Roll Flutter Engine from 6974dbac35a1 to d048b9aed529 (1 revision) (flutter/flutter#146355) 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 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gilnobrega
pushed a commit
to gilnobrega/flutter
that referenced
this pull request
Apr 22, 2024
Minor changes to make the `RenderDecorator.computeBaseline` change a bit easier to make. No semantic changes.
TecHaxter
pushed a commit
to TecHaxter/flutter_packages
that referenced
this pull request
May 22, 2024
flutter/flutter@477ebd8...98d23f7 2024-04-06 [email protected] Prepare for RenderDecorator.computeBaseline changes. (flutter/flutter#146363) 2024-04-06 [email protected] Roll Flutter Engine from 563bdb1d7976 to 605b3f35fa0a (1 revision) (flutter/flutter#146393) 2024-04-06 [email protected] Roll Flutter Engine from 482172d16528 to 563bdb1d7976 (1 revision) (flutter/flutter#146385) 2024-04-06 [email protected] Roll Flutter Engine from b0d7ac5425e9 to 482172d16528 (1 revision) (flutter/flutter#146382) 2024-04-06 [email protected] Roll Flutter Engine from df9f7433fc70 to b0d7ac5425e9 (2 revisions) (flutter/flutter#146377) 2024-04-06 [email protected] Roll Flutter Engine from b5039157cbc1 to df9f7433fc70 (1 revision) (flutter/flutter#146373) 2024-04-05 [email protected] Make FileSystem dependency explicit througout. (flutter/flutter#146008) 2024-04-05 [email protected] Roll Flutter Engine from 48604dfd9d49 to b5039157cbc1 (3 revisions) (flutter/flutter#146370) 2024-04-05 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.24.9 to 3.24.10 (flutter/flutter#146368) 2024-04-05 [email protected] Roll Flutter Engine from da995a1061c5 to 48604dfd9d49 (2 revisions) (flutter/flutter#146366) 2024-04-05 [email protected] Roll Flutter Engine from 6a478d6931b2 to da995a1061c5 (1 revision) (flutter/flutter#146364) 2024-04-05 [email protected] Roll Flutter Engine from 62df3bd5f681 to 6a478d6931b2 (3 revisions) (flutter/flutter#146360) 2024-04-05 [email protected] Copy part files and sourcemaps when building with dart2js. (flutter/flutter#146356) 2024-04-05 [email protected] Roll Flutter Engine from d048b9aed529 to 62df3bd5f681 (1 revision) (flutter/flutter#146357) 2024-04-05 [email protected] Reland "Bump to AGP 8.1/Gradle 8.3 (almost) everywhere" (flutter/flutter#146307) 2024-04-05 [email protected] Roll Flutter Engine from 6974dbac35a1 to d048b9aed529 (1 revision) (flutter/flutter#146355) 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 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minor changes to make the
RenderDecorator.computeBaselinechange a bit easier to make. No semantic changes.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.