Add some more dart doc to RepaintBoundary by xster · Pull Request #17183 · flutter/flutter

xster · 2018-05-02T03:50:38Z

No description provided.

HansMuller · 2018-05-02T16:39:09Z

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.

Took parts of your wording

HansMuller · 2018-05-02T16:41:33Z

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.

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.

Hixie · 2018-05-02T18:22:19Z

comma before "which"

Hixie · 2018-05-02T18:22:52Z

comma before "which"

also, for consistency maybe remove the newlines between the bullets, not sure why we had newlines here

Hixie · 2018-05-02T18:23:07Z

comma before to

Hixie · 2018-05-02T19:57:52Z

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.

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

Hixie · 2018-05-02T20:00:49Z

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. :-)

xster · 2018-05-10T19:48:32Z

PTAL

Hixie · 2018-05-29T22:08:38Z

How about: to scope a subtree of render objects to their own layer, thus limiting the number of nodes that [markNeedsPaint] must mark dirty. ?

Hixie · 2018-05-29T22:08:58Z

copyediting error

i assume this should just be removed

Oops, attached back

Hixie · 2018-05-29T22:09:43Z

not sure what "instances'" here is referring to. Is there a missing noun? Or is the apostrophe extraneous?

Ah yes, an extra apostrophe

Hixie · 2018-05-29T22:10:46Z

move "up to possibly the root of the application" to after "[RenderObject.isRepaintBoundary]"

Hixie · 2018-05-29T22:11:21Z

is requested to repaint. "alerted to" something means "told about"; "told about repaint" doesn't really make sense.

Hixie · 2018-05-29T22:12:11Z

"repaint process" -> "[paint] method"

Hixie · 2018-05-29T22:12:40Z

"descendent" -> "descendant" (4 occurrences)

_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.

global search replaced.

Hixie · 2018-05-29T22:14:47Z

markNeedsPaint flag -> either [markNeedsPaint] flag or "needs paint" flag or "needs paint" annotation

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.

Hixie · 2018-05-29T22:15:20Z

its -> their

Hixie · 2018-05-29T22:15:28Z

surround -> surrounding

Hixie · 2018-05-29T22:16:29Z

i feel like by this paragraph you've made this point three times already.

removed this and the previous paragraph

Hixie · 2018-05-29T22:17:08Z

strike "also" since you haven't mentioned any way that it is automatically inserted yet

Hixie · 2018-05-29T22:17:38Z

"during the drawer toggle" -> "while the drawer opens and closes"

Hixie · 2018-05-29T22:18:01Z

"contained outside the drawer" sounds very weird

Hixie · 2018-05-29T22:18:18Z

strike "toggle"

Hixie · 2018-06-01T05:28:35Z

+/// 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


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.

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.

Hixie · 2018-06-01T05:29:43Z

+/// [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


this sentence is too long. We need to split it somehow or add commas or something to make it clearer.

Hixie · 2018-06-01T05:30:07Z

+/// has a [Layer], decoupling ancestor render objects from the descendant
+/// render objects.
+///
+/// [RepaintBoundary] has the further side-effect of possibly hinting the engine


hinting to the engine

Hixie · 2018-06-01T05:30:25Z

+/// render objects.
+///
+/// [RepaintBoundary] has the further side-effect of possibly hinting the engine
+/// to further optimize animation performance when the render subtree behind the


to -> that it should
when -> if

Hixie · 2018-06-01T05:31:26Z

+/// 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


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."

Hixie · 2018-06-01T05:31:56Z

+/// 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


the section starting from "so" is a sentence fragment. Replace the period with a comma and lowercase So.

Hixie · 2018-06-01T05:32:35Z

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.

xster · 2018-06-04T23:47:05Z

No worries. It's subtle and impactful so it's good to get it right.

xster · 2018-06-13T19:39:45Z

Any more feedback on this PR @Hixie?

Hixie · 2018-06-14T19:59:31Z

* 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

googlebot added the cla: yes label May 2, 2018

xster force-pushed the repaint-doc branch from e617231 to 9f4d8b1 Compare May 2, 2018 03:53

HansMuller reviewed May 2, 2018

View reviewed changes

Hixie reviewed May 2, 2018

View reviewed changes

Hixie reviewed May 29, 2018

View reviewed changes

xster added 4 commits May 30, 2018 12:09

Add some more dart doc to RepaintBoundary

6b075c1

review

ab15237

review

3e940fd

review

4c52ab6

xster force-pushed the repaint-doc branch from dfc114f to 4c52ab6 Compare May 31, 2018 01:36

review

335282b

Hixie reviewed Jun 1, 2018

View reviewed changes

review

6d305cb

xster merged commit 020e0ef into flutter:master Jun 14, 2018

xster deleted the repaint-doc branch June 14, 2018 20:17

github-actions Bot locked as resolved and limited conversation to collaborators Aug 10, 2021

Conversation

xster commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented May 2, 2018

Uh oh!

xster commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xster May 31, 2018 •

edited

Loading