Use the engine's TextOverflow API to implement ellipsizing #6104
Conversation
| canvas.saveLayer(bounds, new Paint()); | ||
| else | ||
| canvas.save(); | ||
| canvas.save(); |
There was a problem hiding this comment.
I think we still need to do a saveLayer here if we need to generate the fade effect because the fade effect needs to blend the text against the background.
There was a problem hiding this comment.
Probably _overflowShader is the thing to change the check to.
|
|
f118b5d to
5c126f2
Compare
| double textScaleFactor: 1.0 | ||
| }) : _text = text, _textAlign = textAlign, _textScaleFactor = textScaleFactor { | ||
| double textScaleFactor: 1.0, | ||
| ui.TextOverflow textOverflow, |
There was a problem hiding this comment.
please re-export this so that consumers don't have to get it from dart:ui
There was a problem hiding this comment.
That would conflict with the TextOverflow enum in the framework package
There was a problem hiding this comment.
ok well we definitely shouldn't have both if they're both going to be exposed in the framework API.
|
in general i'd like to not see ui.OverflowEffect anywhere in the framework code. edit: as in, it should just be OverflowEffect, the same way we reexport Color. |
|
TextOverflow is provided by the framework and includes overflow effects that are rendered on the framework side (i.e. fade). ui.TextOverflow only covers overflow policies that are implemented within the engine |
|
Is ui.TextOverflow only used at the lowest levels, like ui.TextStyle? It looks like you have some classes in the framework that take ui.TextOverflow. |
|
I think the best way to resolve this really is to have the engine take a string instead of this enum. That would remove any API conflict. |
|
(the framework would then just pass in the ellipsis character as the string value when its enum is set to the ellipsis value.) |
|
ui.TextOverflow is used by low-level text painting APIs that talk to the engine API (TextPainter and TextStyle/ParagraphStyle) TextOverflow is used by widgets and render objects (RichText and RenderParagraph) |
|
TextPainter is not a low-level API. It's the API you use when you paint on a canvas from user code. It shouldn't use any non-reexported dart:ui types in its API. |
5c126f2 to
d8498b1
Compare
|
Updated for the new engine API. PTAL |
| double textScaleFactor: 1.0 | ||
| }) : _text = text, _textAlign = textAlign, _textScaleFactor = textScaleFactor { | ||
| double textScaleFactor: 1.0, | ||
| String ellipsis |
There was a problem hiding this comment.
nit: these days, when you have a list like this, stick a trailing comma in there so future changes don't need to edit the previous line
| @@ -80,6 +81,18 @@ class TextPainter { | |||
| _needsLayout = true; | |||
| } | |||
|
|
|||
| /// The string used to ellipsize overflowing text. | |||
There was a problem hiding this comment.
Add more documentation, for example:
- Is providing a string sufficient to cause ellipsising?
- Is there a difference between null and the empty string? (If not, consider disallowing one just to remove that axis of freedom.)
- What are the performance implications of this feature?
- What are edge cases you might run into, and how are they handled? e.g. if the ellipsising string is wider than the container, if it contains unbalanced bidi characters, combining characters, or explicit line breaks, that kind of thing.
- How is the ellipsising done? (clipping truncation vs character truncation vs ligature truncation etc)
Weave all these answers into a coherent narrative. :-)
There was a problem hiding this comment.
Clarified the docs and added an assert for empty ellipsis strings
(I don't know the internals of the text rendering engine well enough to precisely describe its behavior)
| @@ -162,7 +175,11 @@ class TextPainter { | |||
| if (_paragraph == null) { | |||
| ui.ParagraphBuilder builder = new ui.ParagraphBuilder(); | |||
| _text.build(builder, textScaleFactor: textScaleFactor); | |||
| ui.ParagraphStyle paragraphStyle = _text.style?.getParagraphStyle(textAlign: textAlign, textScaleFactor: textScaleFactor); | |||
| ui.ParagraphStyle paragraphStyle = _text.style?.getParagraphStyle( | |||
| textAlign: textAlign, | |||
There was a problem hiding this comment.
we usually do a two-character indent here. we'll probably change at some point to match typical dart style but for now let's keep the code consistent.
| ui.ParagraphStyle paragraphStyle = _text.style?.getParagraphStyle( | ||
| textAlign: textAlign, | ||
| textScaleFactor: textScaleFactor, | ||
| ellipsis: _ellipsis |
| return new ui.ParagraphStyle( | ||
| textAlign: textAlign, | ||
| fontWeight: fontWeight, | ||
| fontStyle: fontStyle, | ||
| fontFamily: fontFamily, | ||
| fontSize: fontSize == null ? null : fontSize * textScaleFactor, | ||
| lineHeight: height | ||
| lineHeight: height, | ||
| ellipsis: ellipsis |
| text: text, | ||
| textAlign: textAlign, | ||
| textScaleFactor: textScaleFactor, | ||
| ellipsis: overflow == TextOverflow.ellipsis ? _kEllipsis : null |
|
|
d8498b1 to
0153173
Compare
Changes since last roll: ``` 5613939 Roll src/third_party/skia 7ba1d64f0706..5f0726b01019 (12 commits) (flutter#6104) 47a1ce0 Allow embedders to set the root surface transformation. (flutter#6085) ```
* Roll engine to 5613939.6 Changes since last roll: ``` 5613939 Roll src/third_party/skia 7ba1d64f0706..5f0726b01019 (12 commits) (#6104) 47a1ce0 Allow embedders to set the root surface transformation. (#6085) ``` * Roll engine to f3ff83a. (dart roll). * Add const * Add ignore analyzer prefer_const_constructors_in_immutables
Fixes #4478