Skip to content

Conversation

@LongCatIsLooong
Copy link
Contributor

Fixes: #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.

Pre-launch Checklist

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

@github-actions 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
@LongCatIsLooong LongCatIsLooong force-pushed the TextStyle.height-sentinel branch from 689811c to 3fdd3ea Compare May 24, 2024 17:13
@LongCatIsLooong LongCatIsLooong requested a review from justinmc May 29, 2024 15:44
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Comma at the end of this line.

@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented May 29, 2024

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).

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented May 29, 2024

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.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2024
@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label May 29, 2024
@auto-submit auto-submit bot merged commit 557fca4 into flutter:master May 29, 2024
@LongCatIsLooong LongCatIsLooong deleted the TextStyle.height-sentinel branch May 29, 2024 18:58
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 30, 2024
* master: (115 commits)
  Roll Flutter Engine from 4adf453b6d68 to 19707e811b60 (1 revision) (flutter#149291)
  disable Impeller on external texture test. (flutter#149292)
  Roll Flutter Engine from 8d5d14a1db95 to 4adf453b6d68 (12 revisions) (flutter#149290)
  Update 3.22.1 release notes to include missing fix. (flutter#148999)
  Manual roll Flutter Engine from 60968ee3bde7 to 8d5d14a1db95 (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 30aa720d4999 to 60968ee3bde7 (1 revision) (flutter#149255)
  Roll Flutter Engine from b26e1b023cdb to 30aa720d4999 (7 revisions) (flutter#149249)
  Roll Packages from a933c30 to 31d3329 (6 revisions) (flutter#149246)
  Clean leak in editable_text_test.dart. (flutter#149223)
  ...
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 30, 2024
* master: (115 commits)
  Roll Flutter Engine from 4adf453b6d68 to 19707e811b60 (1 revision) (flutter#149291)
  disable Impeller on external texture test. (flutter#149292)
  Roll Flutter Engine from 8d5d14a1db95 to 4adf453b6d68 (12 revisions) (flutter#149290)
  Update 3.22.1 release notes to include missing fix. (flutter#148999)
  Manual roll Flutter Engine from 60968ee3bde7 to 8d5d14a1db95 (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 30aa720d4999 to 60968ee3bde7 (1 revision) (flutter#149255)
  Roll Flutter Engine from b26e1b023cdb to 30aa720d4999 (7 revisions) (flutter#149249)
  Roll Packages from a933c30 to 31d3329 (6 revisions) (flutter#149246)
  Clean leak in editable_text_test.dart. (flutter#149223)
  ...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 30, 2024
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
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.
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 1, 2024
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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextStyle with existing height cannot be copied with height=null

2 participants