Theme.of provides all TextStyle properties by yjbanov · Pull Request #12552 · flutter/flutter

yjbanov · 2017-10-15T05:31:27Z

Fixes #12548
Fixes #12550
See also #12537

This adds decoration property to TextStyles vended by Theme.of.

tvolkert · 2017-10-15T07:19:25Z

packages/flutter/test/material/theme_test.dart

It's too bad there's no way to force this test to be updated if any properties are added to TextStyle...

There is one way. Have a class in this file that "implements" TextStyle. The analyzer will flag this file whenever we add a member.

Wow, this trick is so devious that I implemented it.

tvolkert · 2017-10-15T07:22:20Z

LGTM, but I'd wait for @Hixie to confirm that this is the right way to go before merging.

Hixie · 2017-10-16T20:40:08Z

I would prefer the output of Theme.of to be a ThemeData whose text styles are all inherit:false. That way, if we add anything else to TextStyle, we don't have to keep updating ThemeData.

yjbanov · 2017-10-17T17:24:14Z

I would prefer the output of Theme.of to be a ThemeData whose text styles are all inherit:false.

Done (see 6885e8a). I wonder, however, if people would want to get ThemeData from Theme.of, alter it a bit, then feed it back to a Theme widget. We currently expect that ThemeData supplied to Theme is inherit:true so we can localize it.

Test case:

var original = Theme.of(context);
return new Theme(
  // This will fail during localization.
  theme: original.copyWith(
    accentTextTheme: someOtherTextTheme,
  ),
  child: someWidget,
);

One way to solve this is to not localize text styles that are inherit:false. Then in the example above we will localize someOtherTextTheme (if it's inherit:true), but not others.

Hixie · 2017-10-17T18:04:06Z

Discussion:

TextStyle.merge should return other if other.inherit is false (rather than asserting)
TextStyle should sprout a debugLabel (and have merge, lerp et al update it accordingly)
update docs everywhere to be the right way round for inherit wrt Typography, ThemeData, etc.

yjbanov · 2017-10-20T00:18:19Z

039a61f and dd85edf change the other.inherit logic, update docs and add a debugLabel field to TextStyle.

yjbanov · 2017-10-20T00:18:22Z

PTAL

Hixie · 2017-10-20T17:08:39Z

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

-    accentTextTheme ??= accentIsDark ? typography.white : typography.black;
+    textTheme ??= (isDark ? typography.white : typography.black).apply(decoration: TextDecoration.none);
+    primaryTextTheme ??= (primaryIsDark ? typography.white : typography.black).apply(decoration: TextDecoration.none);
+    accentTextTheme ??= (accentIsDark ? typography.white : typography.black).apply(decoration: TextDecoration.none);


how would you feel about either removing these additions (relying on the way we inherit from a base that doesn't have an underline) or hard-coding them into the constants? It seems weird to have these done in the middle-ground manner.

Yeah, now that the result of Theme.of no longer inherits from text styles, this property is not strictly necessary. However, I added the value to the Typography constants to be explicit that by default we do not have decorations rather than not having them as a side-effect of never setting them.

Hixie · 2017-10-20T17:16:25Z

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

    String package,
  }) : fontFamily = package == null ? fontFamily : 'packages/$package/$fontFamily',
-       assert(inherit != null);
+       debugLabel = debugLabel == null ? _kDefaultDebugLabel : debugLabel,


debugLabel = debugLabel ?? _kDefaultDebugLabel

Or, if it doesn't interfere with other logic, just make it a default and assert non-null.

Alternatively, default to null, which will be more efficient in release builds and in comparisons against the default value, and only bringing _kDefaultDebugLabel when you need to merge strings (using debugLabel ?? _kDefaultDebugLabel)

Went with the last option: default to null, only bring in the default label when merging strings.

Hixie · 2017-10-20T17:16:59Z

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

-       assert(inherit != null);
+       debugLabel = debugLabel == null ? _kDefaultDebugLabel : debugLabel,
+       assert(inherit != null),
+       assert(debugLabel == null || debugLabel.length <= 100);


I'd avoid the maximum length. It's likely that we'll accidentally exceed it at some point when creating a lerp of a lerp of a lerp or some crazy thing.

(...but need to see the whole length because it's meaningful in some way to what we're debugging, or something)

I guess maybe you added this because in regular operation we generate 1000-long messages? I defer to your judgement on this. If we do generate ridiculous chains in normal operation, let me know, though. That seems surprising.

Removed the cap. I was just paranoid. Easy to add later if we find a legitimate use-case for infinite chains of styles.

Hixie · 2017-10-20T17:17:20Z

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

+  /// A human-readable description of this text style.
+  ///
+  /// This property is not considered when comparing text styles using `==` or
+  /// [compareTo], and it does not affect [hashCode].


say something about it only being managed in debug builds.

Hixie · 2017-10-20T17:18:37Z

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

-  /// Creates a copy of this text style but with the numeric fields multiplied
-  /// by the given factors and then incremented by the given deltas.
+  static String _capDebugLabelLength(String label) {
+    const int maxLabelLength = 100;


i'd probably remove this entire function but if you do keep it:

use the same constant everywhere (here and in the assert above)

remove excessive braces

Done. Removed.

Hixie · 2017-10-20T17:20:13Z

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

-  /// replaced by the non-null parameters of the given text style. If the given
-  /// text style is null, simply returns this text style.
+  /// Returns a new text style that is a combination of this style and the given
+  /// [other] style, following the following rules.


remove "following the following rules" because the first sentence needs to be able to stand alone.

Hixie · 2017-10-20T17:21:17Z

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

+  ///
+  /// If the given [other] text style has its [TextStyle.inherit] set to true,
+  /// its null properties are replaced with the non-null properties of this text
+  /// style. It is said that the [other] style inherits properties of this


I'd probably strike "It is said that", it's one of those phrases that when you really get down to it doesn't add anything meaningful to the docs (like my personal nemesis, "Note that...").

Done, but the sentence still doesn't quite sound right. Here I am trying introduce our nouns and verbs around the "inherit" flag and "merge" method. When you "a.merge(b)" you can say "b inherits properties of a".

Hixie · 2017-10-20T17:21:45Z

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

+  /// simply returns the given [other] style unchanged. It is said that the
+  /// [other] style does not inherit properties of this style.
+  ///
+  /// If the given text style is null, simply returns this text style.


similarly with "simply". It just leads to people muttering "doesn't seem so simple to me" to themselves.

not that i ever mutter to myself when reading documentation. no sir.

Done. This was there already. I just reformatted the sentence.

Hixie · 2017-10-20T17:23:03Z

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

  void debugFillProperties(DiagnosticPropertiesBuilder properties, { String prefix: '' }) {
    super.debugFillProperties(properties);
+    if (debugLabel != _kDefaultDebugLabel)
+      properties.add(new StringProperty('${prefix}debugLabel', debugLabel, defaultValue: null));


probably showName: false? Or maybe this is a MessageProperty? Check to see what @jacob314 did with the other debugLabels.

showName: false sounds reasonable. I don't see any other debugLabel properties added as diagnosticProperty. Generally, if you thing debugLabel does conceptually represent a property name, StringProperty is what you want. If debugLabel is just a debugging message you want to show than MessageProperty is what you want.

Thanks! Switched to MessageProperty.

Hixie · 2017-10-20T17:24:38Z

packages/flutter/test/painting/text_style_test.dart

+    TextStyle cappedCopy = foo;
+    TextStyle cappedApply = foo;
+    TextStyle cappedLerp = foo;
+    for (int i = 0; i < 50; i++) {


i += 1 (my preference) or ++i

This is gone now.

Hixie · 2017-10-20T17:24:57Z

packages/flutter/test/painting/text_style_test.dart

+    expect(TextStyle.lerp(foo, bar, 0.5).debugLabel, 'lerp(foo, bar)');
+    expect(TextStyle.lerp(foo.merge(bar), baz, 0.5).copyWith().debugLabel, 'copy of lerp(bar < foo, baz)');
+
+    expect(() => new TextStyle(debugLabel: 'a' * 300), throwsA(const isInstanceOf<AssertionError>()));


you can import flutter_test and use throwsAssertionError

This is gone now.

Hixie · 2017-10-20T17:28:26Z

yjbanov requested review from Hixie and tvolkert October 15, 2017 05:31

googlebot added the cla: yes label Oct 15, 2017

tvolkert reviewed Oct 15, 2017

View reviewed changes

yjbanov added 5 commits October 19, 2017 14:34

Theme provides all TextStyle properties

62496ee

match field declaration order in the test

4d187cb

Theme.of returns text style with inherit == false

0ccd573

change TextStyle.inherit logic; docs

039a61f

add TextStyle.debugLabel

dd85edf

yjbanov force-pushed the add-missing-text-style-props branch from 6885e8a to dd85edf Compare October 20, 2017 00:16

Hixie reviewed Oct 20, 2017

View reviewed changes

yjbanov added 2 commits October 20, 2017 14:29

address comments

01ea27e

add debug labels to Typography text styles

4e4e067

yjbanov merged commit 67d16cd into flutter:master Oct 21, 2017

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021

Conversation

yjbanov commented Oct 15, 2017

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!

tvolkert commented Oct 15, 2017

Uh oh!

Hixie commented Oct 16, 2017

Uh oh!

yjbanov commented Oct 17, 2017

Uh oh!

Hixie commented Oct 17, 2017 • edited by yjbanov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yjbanov commented Oct 20, 2017

Uh oh!

yjbanov commented Oct 20, 2017

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!

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!

Hixie commented Oct 20, 2017

Uh oh!

Reviewers

Assignees

Hixie commented Oct 17, 2017 •

edited by yjbanov

Loading