Add error message when lerping between TextStyles different `inherit` values by LongCatIsLooong · Pull Request #112837 · flutter/flutter

LongCatIsLooong · 2022-10-04T01:55:20Z

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.

LongCatIsLooong · 2022-10-04T01:57:37Z

packages/flutter/lib/src/painting/text_style.dart

I wonder if I should mention this is just mimicking Theme.of. Maybe there're better examples?

This error report is probably comprehensive enough without this additional explanation. Also: it's not clear that we really want to advocate calling ThemeData.localize, since that's done automatically when the app's locale is finalized. I think it would be best to leave this part of the error report out.

There's no ThemeData.merge so the example is really awkward. Should I say instead of transitioning to Theme.of(context) you should transition to isDark ? ThemeData.dark() : ThemeData()?

HansMuller

LGTM. Just some wordsmithing and error message smithing.

HansMuller · 2022-10-04T23:43:11Z

packages/flutter/lib/src/painting/text_style.dart

+  /// [inherit] value.
+  ///
+  /// Properties that don't have explicit values or other default values to fall
+  /// back to will revert to the defaults defined in the text shaper: white in


"text shaper"? Will developers know what that refers to? Unless "text shaper" refers to something that will make this point easier to understand, just explaining what the defaults are is probably enough.

Note also: I'm surprised that the default color is white (instead of black).

I mostly copied these defaults without actually checking (the font size is correct). It could be black let me check.

yeah it's correct. Should I say "defined in ui.Paragraph" instead?

got rid of "text shaper"

HansMuller · 2022-10-04T23:45:16Z

packages/flutter/lib/src/painting/text_style.dart

  /// The package name should be provided by the `package` argument in the
  /// constructor.
-  List<String>? get fontFamilyFallback => _package != null && _fontFamilyFallback != null ? _fontFamilyFallback!.map((String str) => 'packages/$_package/$str').toList() : _fontFamilyFallback;
+  List<String>? get fontFamilyFallback => _package != null ? _fontFamilyFallback?.map((String str) => 'packages/$_package/$str').toList() : _fontFamilyFallback;


This might be slightly easier to ready if the simple case was first:

List<String>? get fontFamilyFallback => _package == null ? _fontFamilyFallback : _fontFamilyFallback?.map((String str) => 'packages/$_package/$str').toList();

HansMuller · 2022-10-04T23:47:12Z

packages/flutter/lib/src/painting/text_style.dart

  }

-  /// Interpolate between two text styles.
+  /// Interpolate between two text styles for animated transitions.


Why include the "for animation transitions" qualifier?

The implementation does things to keep the transition smooth (e.g.: fontSize: ui.lerpDouble(a.fontSize ?? b.fontSize, b.fontSize ?? a.fontSize, t),), that doesn't really make sense for anything other than transitions.

HansMuller · 2022-10-04T23:48:00Z

packages/flutter/lib/src/painting/text_style.dart

-  /// Interpolate between two text styles.
+  /// Interpolate between two text styles for animated transitions.
+  ///
+  /// This will not work well if the styles don't set the same fields. When this


Suggested change

/// This will not work well if the styles don't set the same fields. When this

/// Interpolation will not work well if the styles don't specify the same fields. When this

HansMuller · 2022-10-04T23:51:27Z

packages/flutter/lib/src/painting/text_style.dart

+  /// Interpolate between two text styles for animated transitions.
+  ///
+  /// This will not work well if the styles don't set the same fields. When this
+  /// happens, to keep the transition smooth, the implementation uses the


Suggested change

/// happens, to keep the transition smooth, the implementation uses the

/// happens, to keep the interpolated transition smooth, the implementation uses the

HansMuller · 2022-10-04T23:55:03Z

packages/flutter/lib/src/painting/text_style.dart

+        ErrorSpacer(),
+        ErrorHint(
+          'In general, TextStyle.lerp only works well when both TextStyles have '
+          'the same "inherit" value, and set roughly the same fields.',


Suggested change

'the same "inherit" value, and set roughly the same fields.',

'the same "inherit" value, and specify roughly the same fields.',

Also: Do we really need to say "roughly" the same fields?

removed roughly

HansMuller · 2022-10-04T23:58:49Z

packages/flutter/lib/src/painting/text_style.dart

This error report is probably comprehensive enough without this additional explanation. Also: it's not clear that we really want to advocate calling ThemeData.localize, since that's done automatically when the app's locale is finalized. I think it would be best to leave this part of the error report out.

…lutter into text-style-lerp-guards

…uards

…inherit` values (flutter/flutter#112837)

LongCatIsLooong requested review from HansMuller and darrenaustin October 4, 2022 01:55

flutter-dashboard 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 Oct 4, 2022

LongCatIsLooong commented Oct 4, 2022

View reviewed changes

TextStyle.lerp error message

df34286

LongCatIsLooong force-pushed the text-style-lerp-guards branch from 282de92 to df34286 Compare October 4, 2022 02:02

LongCatIsLooong added 2 commits October 3, 2022 19:12

reee

987d531

Update text_style.dart

1d66033

HansMuller approved these changes Oct 5, 2022

View reviewed changes

LongCatIsLooong added 3 commits October 5, 2022 10:40

review

3882d50

Merge branch 'text-style-lerp-guards' of github.com:LongCatIsLooong/f…

931b8ef

…lutter into text-style-lerp-guards

Merge remote-tracking branch 'upstream/master' into text-style-lerp-g…

5e6580f

…uards

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 7, 2022

auto-submit bot merged commit 5d4f240 into flutter:master Oct 7, 2022

LongCatIsLooong deleted the text-style-lerp-guards branch October 7, 2022 06:06

This was referenced Oct 7, 2022

Roll Flutter from 98c04987963b to eec8d9d9cc29 (32 revisions) flutter/packages#2687

Merged

Roll Flutter from be86a916548a to eec8d9d9cc29 (28 revisions) flutter/plugins#6551

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 7, 2022

5d4f240 Add error message when lerping between TextStyles different `…

bb3eb83

…inherit` values (flutter/flutter#112837)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 7, 2022

5d4f240 Add error message when lerping between TextStyles different `…

d2d7515

…inherit` values (flutter/flutter#112837)

	/// This will not work well if the styles don't set the same fields. When this
	/// Interpolation will not work well if the styles don't specify the same fields. When this

	/// happens, to keep the transition smooth, the implementation uses the
	/// happens, to keep the interpolated transition smooth, the implementation uses the

	'the same "inherit" value, and set roughly the same fields.',
	'the same "inherit" value, and specify roughly the same fields.',

Conversation

LongCatIsLooong commented Oct 4, 2022

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!

HansMuller left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants