Add some more dart doc to RepaintBoundary #17183
Conversation
| /// | ||
| /// While propagating up, `markNeedsPaint` will no longer be marked on | ||
| /// [RenderObject]s past the first ancestor [RepaintBoundary]. The repainting | ||
| /// children can re-record its display list without re-recording the display list |
There was a problem hiding this comment.
The upward propagation of the markNeedsPaint renderer flag will stop when a RepaintBoundary is reached. Only the repaintBoundary's subtree will recreate and redraw its display-list.
| /// | ||
| /// The [RepaintBoundary] is also automatically inserted by the framework in | ||
| /// widgets that are likely to mark natural separation points in apps. For | ||
| /// instance, contents in Material Design drawers usually don't simultaneously |
There was a problem hiding this comment.
I don't think that's exactly the reason that a repaint boundary around the drawer makes sense. Although they drawer and the app's content might change at the same time, the drawer is opaque, so one change will not affect the other.
There was a problem hiding this comment.
Yeah the reason we put a repaint boundary there is because the drawer is being moved, but the contents of the drawer don't change as the drawer is moved. So we want to be able to update just the transform without rerecoding the drawer's display list.
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [RepaintBoundary] which can be used to contain repaints when unchanged |
| @@ -109,6 +114,9 @@ bool debugCheckIntrinsicSizes = false; | |||
| /// equivalent. | |||
| /// | |||
| /// * The discussion at [RendererBinding.drawFrame]. | |||
| /// | |||
| /// * [RepaintBoundary] which can be used to contain repaints when unchanged | |||
There was a problem hiding this comment.
comma before "which"
also, for consistency maybe remove the newlines between the bullets, not sure why we had newlines here
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [RepaintBoundary] to contain render objects marked with [markNeedsPaint] |
| /// This is useful since [RenderObject.paint] may be triggered even if its | ||
| /// associated [Widget] instances' did not change or rebuild, such as when an | ||
| /// ancestor scrolls or when an ancestor or descendent's widget is going through | ||
| /// an animation. |
There was a problem hiding this comment.
Maybe a better way to phrase this:
A [RenderObject] will repaint whenever any [RenderObject] that shares the same [Layer] is marked as
being dirty and needing paint (see [RenderObject.markNeedsPaint]). The [RepaintBoundary] widget
creates a [RenderObject] that always has a [Layer], decoupling the ancestor render objects from the
descendant render objects.For example, if a render object's rendering is being animated, its parent will typically also be repainted
each frame. By placing a [RepaintBoundary] between the animated child and the parent, the parent
is decoupled from the child and so will no longer repaint each frame of the animation.
There was a problem hiding this comment.
Added in your wording, though I spread them out a bit in various paragraphs so reading the whole thing sequentially doesn't interlace problem statements with solutions back and forth
|
I could copyedit this further but I'll let you apply the suggested high level changes here before trying to get more fine-grained about grammar and such. :-) |
|
PTAL |
| /// | ||
| /// * [RepaintBoundary], to contain render objects marked with | ||
| /// [markNeedsPaint] to only render subtrees that visually changed for best | ||
| /// performance. |
There was a problem hiding this comment.
How about: to scope a subtree of render objects to their own layer, thus limiting the number of nodes that [markNeedsPaint] must mark dirty. ?
| /// | ||
| /// such as when an | ||
| /// ancestor scrolls or when an ancestor or descendent's widget is going through | ||
| /// an animation. |
There was a problem hiding this comment.
i assume this should just be removed
| /// the surrounding parts of the tree. | ||
| /// | ||
| /// This is useful since [RenderObject.paint] may be triggered even if its | ||
| /// associated [Widget] instances' did not change or rebuild. A [RenderObject] |
There was a problem hiding this comment.
not sure what "instances'" here is referring to. Is there a missing noun? Or is the apostrophe extraneous?
There was a problem hiding this comment.
Ah yes, an extra apostrophe
| /// | ||
| /// When a [RenderObject] is flagged as needing to paint via | ||
| /// [RenderObject.markNeedsPaint], the nearest ancestor [RenderObject] up to | ||
| /// possibly the root of the application with [RenderObject.isRepaintBoundary] |
There was a problem hiding this comment.
move "up to possibly the root of the application" to after "[RenderObject.isRepaintBoundary]"
| /// When a [RenderObject] is flagged as needing to paint via | ||
| /// [RenderObject.markNeedsPaint], the nearest ancestor [RenderObject] up to | ||
| /// possibly the root of the application with [RenderObject.isRepaintBoundary] | ||
| /// is alerted to repaint. That nearest ancestor's repaint process will cause |
There was a problem hiding this comment.
is requested to repaint. "alerted to" something means "told about"; "told about repaint" doesn't really make sense.
| /// When a [RenderObject] is flagged as needing to paint via | ||
| /// [RenderObject.markNeedsPaint], the nearest ancestor [RenderObject] up to | ||
| /// possibly the root of the application with [RenderObject.isRepaintBoundary] | ||
| /// is alerted to repaint. That nearest ancestor's repaint process will cause |
There was a problem hiding this comment.
"repaint process" -> "[paint] method"
| /// possibly the root of the application with [RenderObject.isRepaintBoundary] | ||
| /// is alerted to repaint. That nearest ancestor's repaint process will cause | ||
| /// _all_ of its descendent [RenderObject]s to repaint in the absence of any | ||
| /// [RepaintBoundary]. |
There was a problem hiding this comment.
"descendent" -> "descendant" (4 occurrences)
There was a problem hiding this comment.
_all_ of its descendent [RenderObject]s to repaint in the absence of any [RepaintBoundary].
That's not quite true. There's other ways to insert layers than [RepaintBoundary]s. It's just that it'll cause everyone within the layer to repaint.
| /// has a [Layer], decoupling ancestor render objects from the descendant | ||
| /// render objects. | ||
| /// | ||
| /// The upward propagation of the `markNeedsPaint` flag will stop when a |
There was a problem hiding this comment.
markNeedsPaint flag -> either [markNeedsPaint] flag or "needs paint" flag or "needs paint" annotation
There was a problem hiding this comment.
This is on a Widget subclass so I'm assuming just saying [markNeedsPaint] won't properly link to [RenderObject.markNeedsPaint] if I understood what you meant correctly.
| /// | ||
| /// The upward propagation of the `markNeedsPaint` flag will stop when a | ||
| /// [RepaintBoundary]'s render object is reached. The repainting children can | ||
| /// re-record its display list without re-recording the display list for the |
| /// The upward propagation of the `markNeedsPaint` flag will stop when a | ||
| /// [RepaintBoundary]'s render object is reached. The repainting children can | ||
| /// re-record its display list without re-recording the display list for the | ||
| /// surround tree. |
| /// | ||
| /// While traversing down, the repainting ancestor can re-use the display list | ||
| /// we recorded previously on descendents behind a [RepaintBoundary], stopping | ||
| /// the repaint traversal beyond that subtree branch. |
There was a problem hiding this comment.
i feel like by this paragraph you've made this point three times already.
There was a problem hiding this comment.
removed this and the previous paragraph
| /// time cost of rasterizing and caching the pixel values of the subtree for | ||
| /// faster future GPU re-rendering speed. | ||
| /// | ||
| /// The [RepaintBoundary] is also automatically inserted by the framework in |
There was a problem hiding this comment.
strike "also" since you haven't mentioned any way that it is automatically inserted yet
| /// The [RepaintBoundary] is also automatically inserted by the framework in | ||
| /// widgets that are likely to mark natural separation points in apps. For | ||
| /// instance, contents in Material Design drawers typically don't change during | ||
| /// the drawer toggle. So repaints are automatically contained inside the drawer |
There was a problem hiding this comment.
"during the drawer toggle" -> "while the drawer opens and closes"
| /// widgets that are likely to mark natural separation points in apps. For | ||
| /// instance, contents in Material Design drawers typically don't change during | ||
| /// the drawer toggle. So repaints are automatically contained inside the drawer | ||
| /// or outside the drawer when using the [Drawer] widget during toggle |
There was a problem hiding this comment.
"contained outside the drawer" sounds very weird
| /// widgets that are likely to mark natural separation points in apps. For | ||
| /// instance, contents in Material Design drawers typically don't change during | ||
| /// the drawer toggle. So repaints are automatically contained inside the drawer | ||
| /// or outside the drawer when using the [Drawer] widget during toggle |
| /// Containing [RenderObject.paint] to parts of the render subtree that are | ||
| /// actually visually changing using [RepaintBoundary] explicitly or implicitly | ||
| /// is therefore critical to minimizing redundant work and improving the app's | ||
| /// performance. Render tree repaints can be monitored via debug flags such as |
There was a problem hiding this comment.
Reading this again, the first sentence here seems unnecessary. It says something that sounds important but doesn't actually end up saying anything. I think we should remove it and move the second sentence to a "See also" list.
There was a problem hiding this comment.
Agree with the 2 flags. Moved to a see also list.
I think we should keep the first sentence. The first few paragraphs are essentially saying 3- what's happening, 4- why it's important, 5- how it works. I think it's useful for understanding the concept and why users should care.
| /// [RepaintBoundary] is therefore used both while propagating the | ||
| /// `markNeedsPaint` flag up the render tree and while traversing down the | ||
| /// render tree via [RenderObject.paintChild] to strategically contain repaints | ||
| /// to the render subtree that visually changed for performance. This is done |
There was a problem hiding this comment.
this sentence is too long. We need to split it somehow or add commas or something to make it clearer.
| /// has a [Layer], decoupling ancestor render objects from the descendant | ||
| /// render objects. | ||
| /// | ||
| /// [RepaintBoundary] has the further side-effect of possibly hinting the engine |
| /// render objects. | ||
| /// | ||
| /// [RepaintBoundary] has the further side-effect of possibly hinting the engine | ||
| /// to further optimize animation performance when the render subtree behind the |
There was a problem hiding this comment.
to -> that it should
when -> if
| /// time cost of rasterizing and caching the pixel values of the subtree for | ||
| /// faster future GPU re-rendering speed. | ||
| /// | ||
| /// The [RepaintBoundary] is automatically inserted by the framework in |
There was a problem hiding this comment.
it's not automatic, we wrote the code for it. Maybe instead, "Several framework widgets insert [RepaintBoundary] widgets to mark natural separation points in applications."
| /// The [RepaintBoundary] is automatically inserted by the framework in | ||
| /// widgets that are likely to mark natural separation points in apps. For | ||
| /// instance, contents in Material Design drawers typically don't change while | ||
| /// the drawer opens and closes. So repaints are automatically contained |
There was a problem hiding this comment.
the section starting from "so" is a sentence fragment. Replace the period with a comma and lowercase So.
|
Sorry about nit-picking this to death. It's a subtle piece of documentation and the clearer we make it the more it will help. Thanks for putting up with me. |
|
No worries. It's subtle and impactful so it's good to get it right. |
|
Any more feedback on this PR @Hixie? |
|
|
* master: Remove race conditions involving finding available ports (flutter#18488) Skip painting hidden children of a sliver list (flutter#18465) Fixed some typos (flutter#18516) update Tristate checkbox semantics to consider indeterminate as "unchecked" (flutter#18297) Add flutter tool support for creating app-specific VM snapshots. (flutter#18417) mention Text.rich on RichText (flutter#18434) invert children for real (flutter#18489) Fix clipping for SliverLists (flutter#18410) Keep TextFields visible when keyboard comes up (flutter#18291) Update all packages (flutter#18506) Prepare for upgrading video player in Gallery to newest version (flutter#18501) Update Android dependencies (flutter#18499) Enable running with prebuilt test-only apk (flutter#18453) Revert "Update all packages (flutter#18471)" (flutter#18492) Add some more dart doc to RepaintBoundary (flutter#17183) Update all packages (flutter#18471) Clean up output of "flutter run --release" (flutter#18380) Ensure errors launching emulators are exposed to the daemon (flutter#18446) Check video widget is mounted on call to setState (flutter#18467) Fixed concurrent list modification in vmservice.dart
No description provided.