Add a sentinel value for `TextStyle.height` by LongCatIsLooong · Pull Request #149049 · flutter/flutter

LongCatIsLooong · 2024-05-24T16:57:16Z

The rationale for the choice of the sentinel value: flutter/engine#52940
The exact value of kTextHeightNone should be kept as an implementation detail. It's unfortunate that the current value 0 is dangerously close to TextStyle.height's valid domain. If we ever allow TextStyle.height == 0 (which totally makes sense) then it shouldn't be difficult to change the const.

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.

justinmc

LGTM 👍

Just to talk it through, I wonder if you couldn't also solve this problem within TextStyle.copyWith?

static TextStyle copyWith({
  double? height = kTextHeightNone,
  ...
}) {
  return TextStyle(
    // height: height ?? this.height,
    height: height == kTextHeightNone ? this.height : null,
  );
},

So now you can reset height by explicitly passing null:

myTextStyle.copyWith(
  height: null,
);

justinmc · 2024-05-29T16:44:12Z

packages/flutter/test/painting/text_painter_test.dart

+      textDirection: TextDirection.ltr,
+      text: const TextSpan(
+        style: TextStyle(fontSize: 10, height: 1000),
+        children: <TextSpan> [TextSpan(text: 'A', style: TextStyle(height: kTextHeightNone))]


Nit: Comma at the end of this line.

LongCatIsLooong · 2024-05-29T17:22:16Z

LGTM 👍

Just to talk it through, I wonder if you couldn't also solve this problem within TextStyle.copyWith?
static TextStyle copyWith({
  double? height = kTextHeightNone,
  ...
}) {
  return TextStyle(
    // height: height ?? this.height,
    height: height == kTextHeightNone ? this.height : null,
  );
},
So now you can reset height by explicitly passing null:
myTextStyle.copyWith(
  height: null,
);

myTextStyle.copyWith(height: null);

No this does not reset the height to null. Even if does, null still has conflicting semantics in an InlineSpan because it may inherit the value from the parent span(basically the same problem with copyWith).

auto-submit · 2024-05-29T17:46:23Z

auto label is removed for flutter/flutter/149049, due to - The status or check suite Linux build_tests_1_3 has failed. Please fix the issues identified (or deflake) before re-applying this label.

* master: (115 commits) Roll Flutter Engine from 4adf453 to 19707e8 (1 revision) (flutter#149291) disable Impeller on external texture test. (flutter#149292) Roll Flutter Engine from 8d5d14a to 4adf453 (12 revisions) (flutter#149290) Update 3.22.1 release notes to include missing fix. (flutter#148999) Manual roll Flutter Engine from 60968ee to 8d5d14a (1 revision) (flutter#149263) Reverts "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149279) Unskip expression evaluation test (flutter#149253) temporarily disable SemanticsAction tests to unblock engine change (flutter#149274) Adds benchmark for rrect_blur. (flutter#149261) Prevent LayoutBuilder from rebuilding more than once (flutter#147856) Add test for inherited_theme.0.dart (flutter#149120) Update progress_indicator.dart to indicate the adaptive option is for both macOS and iOS (flutter#145246) _ModalScopeStatus as InheritedModel (flutter#149022) Add test for radio.toggleable.0.dart (flutter#149153) Add a sentinel value for `TextStyle.height` (flutter#149049) Remove dynamic_layouts from issue template (flutter#149252) Roll Flutter Engine from 30aa720 to 60968ee (1 revision) (flutter#149255) Roll Flutter Engine from b26e1b0 to 30aa720 (7 revisions) (flutter#149249) Roll Packages from a933c30 to 31d3329 (6 revisions) (flutter#149246) Clean leak in editable_text_test.dart. (flutter#149223) ...

…149049)

Fixes: flutter#58765 The rationale for the choice of the sentinel value: flutter/engine#52940 The exact value of `kTextHeightNone` should be kept as an implementation detail. It's unfortunate that the current value `0` is dangerously close to `TextStyle.height`'s valid domain. If we ever allow `TextStyle.height == 0` (which totally makes sense) then it shouldn't be difficult to change the const.

flutter/flutter@c85fa6a...7eebe29 2024-05-30 [email protected] Roll Packages from 31d3329 to 910fabb (11 revisions) (flutter/flutter#149321) 2024-05-30 [email protected] Roll Flutter Engine from fb64b9a4e6f2 to 2fedfd3cc6e5 (2 revisions) (flutter/flutter#149320) 2024-05-30 [email protected] Fix `Slider` throws an error when `_labelPainter` text is null (flutter/flutter#148462) 2024-05-30 [email protected] Roll Flutter Engine from 5500c1a3969a to fb64b9a4e6f2 (1 revision) (flutter/flutter#149307) 2024-05-30 [email protected] Enable `explicitChildNodes` for the `AlertDialog` content (flutter/flutter#149130) 2024-05-30 [email protected] Roll Flutter Engine from 0c95e85dfbf4 to 5500c1a3969a (1 revision) (flutter/flutter#149304) 2024-05-30 [email protected] Roll Flutter Engine from 19707e811b60 to 0c95e85dfbf4 (1 revision) (flutter/flutter#149300) 2024-05-30 [email protected] Roll Flutter Engine from 4adf453b6d68 to 19707e811b60 (1 revision) (flutter/flutter#149291) 2024-05-30 [email protected] disable Impeller on external texture test. (flutter/flutter#149292) 2024-05-30 [email protected] Roll Flutter Engine from 8d5d14a1db95 to 4adf453b6d68 (12 revisions) (flutter/flutter#149290) 2024-05-30 [email protected] Update 3.22.1 release notes to include missing fix. (flutter/flutter#148999) 2024-05-30 [email protected] Manual roll Flutter Engine from 60968ee3bde7 to 8d5d14a1db95 (1 revision) (flutter/flutter#149263) 2024-05-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Prevent LayoutBuilder from rebuilding more than once (#147856)" (flutter/flutter#149279) 2024-05-29 [email protected] Unskip expression evaluation test (flutter/flutter#149253) 2024-05-29 [email protected] temporarily disable SemanticsAction tests to unblock engine change (flutter/flutter#149274) 2024-05-29 [email protected] Adds benchmark for rrect_blur. (flutter/flutter#149261) 2024-05-29 [email protected] Prevent LayoutBuilder from rebuilding more than once (flutter/flutter#147856) 2024-05-29 [email protected] Add test for inherited_theme.0.dart (flutter/flutter#149120) 2024-05-29 [email protected] Update progress_indicator.dart to indicate the adaptive option is for both macOS and iOS (flutter/flutter#145246) 2024-05-29 [email protected] _ModalScopeStatus as InheritedModel (flutter/flutter#149022) 2024-05-29 [email protected] Add test for radio.toggleable.0.dart (flutter/flutter#149153) 2024-05-29 [email protected] Add a sentinel value for `TextStyle.height` (flutter/flutter#149049) 2024-05-29 [email protected] Remove dynamic_layouts from issue template (flutter/flutter#149252) 2024-05-29 [email protected] Roll Flutter Engine from 30aa720d4999 to 60968ee3bde7 (1 revision) (flutter/flutter#149255) 2024-05-29 [email protected] Roll Flutter Engine from b26e1b023cdb to 30aa720d4999 (7 revisions) (flutter/flutter#149249) 2024-05-29 [email protected] Roll Packages from a933c30 to 31d3329 (6 revisions) (flutter/flutter#149246) 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

…149049)

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels May 24, 2024

TextStyle.height sentinel value

3fdd3ea

LongCatIsLooong force-pushed the TextStyle.height-sentinel branch from 689811c to 3fdd3ea Compare May 24, 2024 17:13

LongCatIsLooong requested a review from justinmc May 29, 2024 15:44

justinmc approved these changes May 29, 2024

View reviewed changes

Update text_painter_test.dart

e727ddf

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

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2024

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

auto-submit bot merged commit 557fca4 into flutter:master May 29, 2024

LongCatIsLooong deleted the TextStyle.height-sentinel branch May 29, 2024 18:58

engine-flutter-autoroll mentioned this pull request May 30, 2024

Roll Flutter from c85fa6abf4ff to db5c1434e649 (24 revisions) flutter/packages#6834

Closed

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

557fca4 Add a sentinel value for TextStyle.height (flutter/flutter#…

2de7226

…149049)

engine-flutter-autoroll mentioned this pull request May 30, 2024

Roll Flutter from c85fa6abf4ff to 421c8c5ef59c (25 revisions) flutter/packages#6835

Closed

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

557fca4 Add a sentinel value for TextStyle.height (flutter/flutter#…

92c03f2

…149049)

engine-flutter-autoroll mentioned this pull request May 30, 2024

Roll Flutter from c85fa6abf4ff to 7eebe2992aba (26 revisions) flutter/packages#6836

Merged

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

557fca4 Add a sentinel value for TextStyle.height (flutter/flutter#…

cca104b

…149049)

engine-flutter-autoroll mentioned this pull request May 30, 2024

Roll Flutter from c85fa6abf4ff to 3d4fd550c2b5 (29 revisions) flutter/packages#6837

Closed

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

557fca4 Add a sentinel value for TextStyle.height (flutter/flutter#…

b4b99b9

…149049)

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

557fca4 Add a sentinel value for TextStyle.height (flutter/flutter#…

cc77e64

…149049)

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

557fca4 Add a sentinel value for TextStyle.height (flutter/flutter#…

8a4775b

…149049)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a sentinel value for `TextStyle.height`#149049

Add a sentinel value for `TextStyle.height`#149049
auto-submit[bot] merged 2 commits intoflutter:masterfrom
LongCatIsLooong:TextStyle.height-sentinel

LongCatIsLooong commented May 24, 2024

Uh oh!

justinmc left a comment

Uh oh!

justinmc May 29, 2024

Uh oh!

LongCatIsLooong commented May 29, 2024 •

edited

Loading

Uh oh!

auto-submit bot commented May 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

LongCatIsLooong commented May 24, 2024

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc May 29, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

auto-submit bot commented May 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LongCatIsLooong commented May 29, 2024 •

edited

Loading