[TextPainter] Don't invalidate layout cache for paint only changes #89515
Conversation
LongCatIsLooong
requested review from
goderbauer,
GaryQian and
jason-simmons
flutter-dashboard
bot
added
the
framework
- Fixes flutter#85108 Defers the rebuilding and the relayout of the ui.Paragraph object to `paint` if there are no layout changes (e.g. only the font color changed) and the width constraints are the same. Additionally if the `TextPainter` is used to help compute the dry layout of a render object, and thus never needs to paint, then color changes alone no longer triggers relayout.
LongCatIsLooong
force-pushed
the
lazier-textpainter-layout
branch
from
5802117 to
a354cd6
Compare
| bool _needsLayout = true; | ||
| // Whether _paragraph contains outdated paint information and needs to be | ||
| // rebuilt. | ||
| bool _needsPaint = true; |
There was a problem hiding this comment.
It's a little confusing that the property is called needsPaint, but the comment talks about needing rebuild...
| if (text == null) { | ||
| throw StateError('TextPainter.text must be set to a non-null value before using the TextPainter.'); |
There was a problem hiding this comment.
For promoting text to non-null for the rest of the function scope. The code would have crashed anyways since we were using ! on _text.
| ); | ||
| } | ||
|
|
||
| if (_needsPaint) { |
There was a problem hiding this comment.
the name _needsPaint seems confusing to me. The TextPainter is drawing/painting the paragraph in line 682 even when this is false.
There was a problem hiding this comment.
_needsRebuildDuringPaint, maybe?
There was a problem hiding this comment.
_rebuildParagraphForLayout (a getter) and _rebuildParagraphForPaint ?
| @@ -196,7 +200,7 @@ class TextPainter { | |||
| /// in framework will automatically invoke this method. | |||
| void markNeedsLayout() { | |||
| _paragraph = null; | |||
| _needsLayout = true; | |||
| _needsPaint = true; | |||
There was a problem hiding this comment.
I believe, technically setting _paragraph to null here is probably enough?
| @@ -496,23 +511,23 @@ class TextPainter { | |||
| /// | |||
| /// Valid only after [layout] has been called. | |||
| double get minIntrinsicWidth { | |||
| assert(!_needsLayout); | |||
There was a problem hiding this comment.
Maybe slightly more readable to define a getter bool get _needsLayout => _paragraph != null and use it here and elsewhere?
There was a problem hiding this comment.
Yeah I can do that. Do we still need these asserts with NNBD?
There was a problem hiding this comment.
_paragraph is declared nullable, so even with NNBD these could be null at this point and that would be a bug.
| // since we've created a new ui.Paragraph. But there's no extra work being | ||
| // done: if _needsPaint is true and _paragraph is not null, the previous | ||
| // `layout` call didn't invoke _layoutParagraph. | ||
| _layoutParagraph(minWidth, maxWidth); |
There was a problem hiding this comment.
Can we assert that the size calculated by the new paragraph is the same as the size before?
LongCatIsLooong
force-pushed
the
lazier-textpainter-layout
branch
from
bed9828 to
e90c6a6
Compare
LongCatIsLooong
force-pushed
the
lazier-textpainter-layout
branch
from
e90c6a6 to
4c0cfb2
Compare
Defers the rebuilding and the relayout of the ui.Paragraph object to
paintif there are no layout changes (e.g. only the font colorchanged) and the width constraints are the same.
Additionally if the
TextPainteris used to help compute the dry layoutof a render object, and thus never needs to paint, then color changes
alone no longer triggers relayout.
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.