Use the engine's TextOverflow API to implement ellipsizing#6104
Use the engine's TextOverflow API to implement ellipsizing#6104jason-simmons merged 1 commit intoflutter:masterfrom
Conversation
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.
|
|
jason-simmons
force-pushed
the
overflow_ellipsis
branch
from
f118b5d to
5c126f2
Compare
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. |
jason-simmons
force-pushed
the
overflow_ellipsis
branch
from
5c126f2 to
d8498b1
Compare
|
Updated for the new engine API. PTAL |
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
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)
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.
|
|
jason-simmons
force-pushed
the
overflow_ellipsis
branch
from
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
3 participants
Fixes #4478