Expose Text foreground from engine #18347
Conversation
|
Believe this is hitting up against dart-lang/sdk#26980 I want the const constructor to be able to assert that one of two objects is null. Dart doesn't like that. |
|
I think this approach makes sense, but still would like some feedback. Here's the intention of this code is to assert early if a user tries to specify both More specifically:
|
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
Update engine.version update tests for TextStyle changes in engine (flutter#17982) * update tests for TextStyle changes in engine * roll engine, support Foreground on TextStyle * Support for saving Dart compilation trace on device (WIP) Expose foreground in framework TextStyle use identical instead of == Add @istest annotation to testGesture (flutter#18311) Notifies IDEs this is a test method, and helps e.g. the flutter plugin recognize test methods and display them in the structure view in intellij. Update gallery assets version with optipng (flutter#18327) Passing any to named params require the name of the parameter itself. (flutter#18361) Update typedef syntax to use Function notation and turn on lint for old notation. (flutter#18362) Now that Dart 1 is turned off, reapplying my change to turn on the prefer_generic_function_type_aliases analysis option, and fix all the typedefs to Dart 2 preferred syntax. Also eliminated the unused analysis_options_repo.yaml file and turned on public_member_api_docs in analysys_options.yaml. No logic changes, just changing the typedef syntax for all typedefs, and updating analysis options. More flexible timeout logic in flutter_test (flutter#18256) This should reduce the number of flakes without actually increasing the timeout, so we'll still find out quickly if a test is hanging. The numbers here might need tweaking. Maybe the default two seconds is too short for CI bots. merge/apply/lerp prefer foreground doc updates and update for copyWith
|
CLAs look good, thanks! |
| }) : fontFamily = package == null ? fontFamily : 'packages/$package/$fontFamily', | ||
| assert(inherit != null); | ||
| assert(inherit != null), | ||
| assert(identical(color, null) || identical(foreground, null), _kColorForegroundWarning); |
There was a problem hiding this comment.
Please add a TODO pointing at that dart bug you commented on. That we have to do this trick with "identical" is silly.
|
|
||
| /// The color to use when painting the text. | ||
| /// | ||
| /// If [foreground] is specified, this value must be null. [color] is shorthand for |
There was a problem hiding this comment.
To avoid starting the sentence with a lowercase letter, consider The [color] property is...
| /// will appear like the style changed, which will result in unnecessary | ||
| /// updates all the way through the framework. | ||
| /// | ||
| /// If [color] is specified, this value must be null. [color] is shorthand for |
| styles.add(new DoubleProperty('${prefix}height', height, unit: 'x', defaultValue: null)); | ||
| styles.add(new StringProperty('${prefix}locale', locale?.toString(), defaultValue: null, quoted: false)); | ||
| styles.add(new StringProperty('${prefix}foreground', foreground?.toString(), defaultValue: null, quoted: false)); | ||
| styles.add(new StringProperty('${prefix}background', background?.toString(), defaultValue: null, quoted: false)); |
There was a problem hiding this comment.
for locale, foreground, and background, we should use DiagnosticsProperty with an appropriate type, e.g. DiagnosticsProperty<Paint>.
| /// If the given text style is null, returns this text style. | ||
| /// | ||
| /// Resolution of conflicts between [color] and [foreground] are handled as in | ||
| /// [copyWith]. |
There was a problem hiding this comment.
rather than deferring to copyWith, let's just be explicit about it here
|
This is great. Can you add a golden test? See https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package%3Aflutter |
|
Tried to redo the Linux tests for fun after adding the center widget - they still fail. I've disabled them for now with some comments that they can be re-enabled when the goldens are properly generated. Added the center widget (inside a RepaintBoundary, which is how it is on the other Text goldens). This should be good to go. |
|
This seems to be failing on an unrelated test now for the nav bar... |
|
yeah the navbar thing is from #18469. It's been reverted, if you point to the latest thing on the goldens repo you should be good, modulo the issues of it not matching in the first place. |
|
So now it looks like there's a time out https://travis-ci.org/flutter/flutter/jobs/394338937 I think that's the one you were mentioning @Hixie ? |
|
One thing I noticed is that drawing text with a linear gradient behaves different than the Android canvas api. In Android, the gradient shader is positioned absolutely in the canvas (based on the rect that was passed when the shader was created). In Flutter, the gradient is positioned relative to the drawn paragraph. It is kind of inconsistent because for all other drawing operations the shader is positioned absolutely. In my experience the relative positioning is a lot less useful than the absolute positioning. Here is a quick visualization: |
|
@sir-boformer can you share the code for that test on the Flutter side? |
|
Looking through the AOSP, I'm not entirely sure how they're doing it so much differently than the engine does it here. But I think this would have to be at the engine level anyway - the code in this patch doesn't really have a sense for any point in the canvas. Can you share the android code as well? |
|
The timeout resolved itself by just rekicking the job... |
|
Like you said, it's a problem in the engine, not with the code in this PR. My concern is that once this feature is shipped, changing the behavior would be a breaking change. Btw, the HTML5 canvas API also behaves like the android canvas API. The android demonstration can be found here: https://github.com/sir-boformer/android-canvas-gradients Screenshot:
|
|
My first guess is that The If my assertion is right, I would propose the exposal of this simpler text painting API in the Flutter engine (probably this skia function). Maybe it would even increase the performance for single line text. But that would be unrelated to this PR. |
|
Engine uses @Hixie - do we have a sense of whether this is intended functionality in I think exposing |
|
The |
|
@Hixie It leads to the same result. This is the code I used to create the gradient paint: final colors = [
Color(0xffff0000),
Color(0xffffff00),
Color(0xff00ff00),
Color(0xff00ffff),
Color(0xff0000ff),
Color(0xffff00ff),
Color(0xffff0000),
];
final colorStops = [0 / 6, 1 / 6, 2 / 6, 3 / 6, 4 / 6, 5 / 6, 6 / 6];
final gradient = new ui.Gradient.linear(
Offset(0.0, 0.0),
Offset(0.0, size.height),
colors,
colorStops,
);
final gradientPaint = new Paint()..shader = gradient;
|
|
well that's more surprising. |
|
Please file a bug on that. |
|
|
|
@Hixie Where would I file the bug? Flutter? Skia? |
|
This is causing Travis to fail, I believe because of this line: That should be |
|
Rather, Travis failed on this merge because of the above mentioned line - that wasn't part of this PR |
|
@sir-boformer Let's start with Flutter. |
4 participants
This exposes the new
foregroundpaint in the framework'sTextStyle.I could use some feedback on the
merge,apply, andlerpportions. As is, I suspect this could cause some confusion for a user that tries to merge/apply/lerp two TextStyles (a and b) where a hascolorset and b hasforegroundset. We could try to give priority tocolororforegroundin these cases, or we could makecolora pseudo property here that really does just instantiate aPaint(but this would impactlerp, which would no longer actually lerp between colors).