Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom foreground paint to be used for drawing text #5395

Merged
merged 4 commits into from
Jun 10, 2018

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented May 26, 2018

Use case(s): I want to draw text painted with a gradient, I want to draw stroked text

Today, libtxt specifies its own Paint and just uses the color property to fill in a paint color when drawing text. It's not possible to tell it what painting style or shader to use, etc. This enables support for that at the engine level - upstream changes could further enable it in the Flutter SDK.

image

lib/ui/text.dart Outdated
}) : _encoded = _encodeTextStyle(
Paint foreground,
}) : assert(color == null || foreground == null,
'Cannot provide both a color and a foreground\m'
Copy link
Contributor

@xqwzts xqwzts May 26, 2018

Choose a reason for hiding this comment

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

Shouldn't that be \n?

@dnfield
Copy link
Contributor Author

dnfield commented May 27, 2018

This breaks flutter tests here:

https://github.com/flutter/flutter/blob/master/packages/flutter/test/painting/text_style_test.dart#L166

and at least a few other places in that file because of changes in toString now including the foreground property. Not sure of the best way to handle that (short of not including the property in the toString output).

@dnfield dnfield mentioned this pull request May 27, 2018
10 tasks
@Hixie
Copy link
Contributor

Hixie commented May 28, 2018

This is great!

lib/ui/text.dart Outdated
return result;
}

/// An opaque object that determines the size, position, and rendering of text.
class TextStyle {
/// Creates a new TextStyle object.
///
/// * `color`: The color to use when painting the text.
/// * `color`: The color to use when painting the text. If this is specified, `foreground` should be null.
Copy link
Contributor

Choose a reason for hiding this comment

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

should -> must

@Hixie
Copy link
Contributor

Hixie commented May 28, 2018

We fix the test when we roll this into the framework. You should get a patch ready for the framework so that you can roll as soon as the engine bots finish building artifacts.

if (record.style().has_foreground) {
paint = record.style().foreground;
} else {
paint.setColor(record.style().color);
Copy link
Member

Choose a reason for hiding this comment

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

Call paint.reset() before paint.setColor() to clear any attributes besides the color that might have been set by a previous style with a foreground paint

@chinmaygarde
Copy link
Member

@jason-simmons, @Hixie: Can we land this?

@Hixie
Copy link
Contributor

Hixie commented Jun 6, 2018

As far as I'm aware, but be careful that this will require framework changes also. (the framework TextStyle needs updating, as well as some tests)

@dnfield
Copy link
Contributor Author

dnfield commented Jun 6, 2018

There's a PR for the framework here: flutter/flutter#17982 which fixes the unit tests and updates the commit hash for Engine (although I believe that will have to be updated once this is actually merged in with the correct merge hash). I'm traveling for the next couple days and won't be able to do much with this myself until Friday or early next week.

The framework should get more updates to TextStyle, but I figured it'd be more straightforward to just land this stuff first and then better expose it in the framework in a subsequent PR.

@Hixie
Copy link
Contributor

Hixie commented Jun 7, 2018

@chinmaygarde If you don't mind doing the engine roll as well (using flutter/flutter#17982) then please don't hesitate to land this if you need it. Otherwise it's fine to wait until @dnfield is around.

@dnfield dnfield merged commit 8f023c5 into flutter:master Jun 10, 2018
@dnfield
Copy link
Contributor Author

dnfield commented Jun 10, 2018

Hmm. I foolishly did not check to see that the build was clean before merging this. It looks like it's still failing from #5443

Should I revert this until that one is fixed? Or is that one going to be reverted? I've opened a PR that should make things green again..

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants