Documentation improvements#122787
Conversation
flutter-dashboard
bot
added
a: animation
There was a problem hiding this comment.
This is slightly incorrect isn't it? context.visitAncestorElements returns false at the first ancestor, not the _getParent() function, because the latter returns a BuildContext.
There was a problem hiding this comment.
good catch. s/returns false/stops/
thanks!
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Read the rest of the changes, mostly commenting on the nice new description of when and how rebuilds happen at [Element.rebuild].
(The PR description says it fixes #3354, but that's an old PR and not a docs issue; perhaps a typo?)
There was a problem hiding this comment.
(Hmm, now I wonder how often applications in the ecosystem fall into this trap. All it takes is using a widget from one library that made the wrong choice here…)
There was a problem hiding this comment.
yeah. i mean, this is true in general of any method that isn't otherwise overridden, as i understand it. @nonvirtual is the annotation intended to help with this, but i'm sure people ignore the lints sometimes.
There was a problem hiding this comment.
Ah, good to know about @nonVirtual! Which I see is on [Widget.==] (and that implementation otherwise just vacuously defers to super.)
I'm sure people ignore it sometimes, but that is still fairly reassuring about how prevalent this mistake would be — it makes it a lot less silent of a trap to fall into.
|
updated with review comments! |
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
All looks great! One new nit.
Fixes flutter#112676. Fixes flutter#97015. Fixes flutter#62107. Fixes flutter#38740. Fixes flutter#31911. Fixes flutter#29958. Fixes flutter#18108. Fixes flutter#17160. Fixes flutter#14243. Fixes flutter#3354.
|
thanks for the review! |
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/flutter, pr: 122787, due to - The status or check suite Windows customer_testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
| /// This hint tells the compositor not to cache the layer containing this | ||
| /// render object because the cache will not be used in the future. If this | ||
| /// hint is not set, the compositor will apply its own heuristics to decide | ||
| /// whether this layer is likely to be reused in the future. |
There was a problem hiding this comment.
I think these docs would be better updated to not refer to a raster cache at all. Impeller doesn't use a raster cache and it seems unlikely that it will at this point.
This is a hint that tells the compositor whether the developer knows this layer will be around next frame or not. The compositor may or may not use that information.
There was a problem hiding this comment.
Do the hints have any effect on Impeller at all?
(Might be worth doing an "Impeller docs update" PR separate from this that updates our docs to match the new world with mixed Impeller/Skia backends.)
| /// heuristics to decide whether the this layer is complex enough to benefit | ||
| /// from caching. | ||
| /// heuristics to decide whether the layer containing this render object is | ||
| /// complex enough to benefit from caching. |
There was a problem hiding this comment.
I think this actually gets added on the picture instead of the layer, but same feedback as below - I think we should try to get rid of docs referring to raster cache, since it's a private implementation detail of the compositor that may not even be present.
There was a problem hiding this comment.
pictures are layers at the framework level
| /// A widget that builds itself based on the latest snapshot of interaction with | ||
| /// a [Future]. | ||
| /// | ||
| /// {@youtube 560 315 https://www.youtube.com/watch?v=ek8ZPdWj4Qo} |
There was a problem hiding this comment.
it got replaced by the second one.
|
(It's fine to follow up in my comments in a separate PR if more convenient - none of what you added really makes things worse than they already were) |
|
@dnfield gonna land this as-is for now but we should talk about doing a pass for impeller, there's lots of places that mention the raster cache and i'm sure lots of other things need updating too |
4 participants
Fixes #112676.
Fixes #97015.
Fixes #62107.
Fixes #38740.
Fixes #31911.
Fixes #29958.
Fixes #18108.
Fixes #17160.
Fixes #14243.
Fixes #3554.