Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document RenderObject._relayoutBoundary and its invariant; small refactors #150527

Merged
merged 11 commits into from
Jun 25, 2024
78 changes: 55 additions & 23 deletions packages/flutter/lib/src/rendering/object.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1853,7 +1853,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
assert(child._parent == this);
assert(child.attached == attached);
assert(child.parentData != null);
child._cleanRelayoutBoundary();
_cleanChildRelayoutBoundary(child);
child.parentData!.detach();
child.parentData = null;
child._parent = null;
Expand Down Expand Up @@ -2185,6 +2185,32 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
}
bool _needsLayout = true;

/// The nearest relayout boundary enclosing this render object, if known.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: cross reference [markNeedsLayout] for what a "relayout boundary" is? I feel the text below doesn't explain that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or say that a relayout boundary is the closest ancestor node (or this) whose parent doesn't care about the layout of the subtree rooted at the relayout boundary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, I agree. Just pushed a commit that adds an explanation of that; PTAL.

///
/// When a render object is marked as needing layout, its parent may
/// as a result also need to be marked as needing layout.
/// For details, see [markNeedsLayout].
/// A render object where relayout does not require relayout of the parent
/// (because its size cannot change on relayout, or because
/// its parent does not use the child's size for its own layout)
/// is a "relayout boundary".
Comment on lines +2193 to +2196
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the description in parentheses here exactly covers the four cases found in layout:

    final bool isRelayoutBoundary = !parentUsesSize || sizedByParent || constraints.isTight || parent is! RenderObject;

The second and third are two different possible reasons why the size can't change on relayout: the child chooses to base its size only on the constraints, or the constraints dictate a size. The first and last are two different possible ways we can know the parent doesn't use the size: because it said so when calling layout, or because there is no parent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The description also seems clean enough — it makes good logical sense on its own — that I think it's suitable for documentation. The analysis of the actual code is there just for fact-checking, for my own verification that what I'm saying not only sounds logical but is accurate as to the code's actual behavior.)

///
/// This property is set in [layout], and consulted by [markNeedsLayout] in
/// deciding whether to recursively mark the parent as also needing layout.
///
/// This property is initially null, and becomes null again if this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not sure this is an invariant / will be super useful since it seems that relayout boundary is an optimization the render tree does on a best effort basis (markNeedsLayout still has to handle the case where relayout boundary is null). I feel the previous paragraph should suffice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this information about when it's null vs. non-null is less important than the other paragraphs. I initially left it as a TODO comment, and replaced that with this paragraph after the thought was prompted by a subthread above: #150527 (comment)

If you'd prefer to take the paragraph out again, I'd be happy with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I guess if you want to turn _relayoutBoundary into a boolean flag you'll have to rewrite most of the docs here.

/// render object is removed from the tree (with [dropChild]);
/// it remains null until the first layout of this render object
/// after it was most recently added to the tree.
/// This property can also be null while an ancestor in the tree is
/// currently doing layout, until this render object itself does layout.
///
/// When not null, the relayout boundary is either this render object itself
/// or one of its ancestors, and all the render objects in the ancestry chain
/// up through that ancestor have the same [_relayoutBoundary].
/// Equivalently: when not null, the relayout boundary is either this render
/// object itself or the same as that of its parent. (So [_relayoutBoundary]
/// is one of `null`, `this`, or `parent!._relayoutBoundary!`.)
RenderObject? _relayoutBoundary;

/// Whether [invokeLayoutCallback] for this render object is currently running.
Expand Down Expand Up @@ -2222,7 +2248,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
/// (where it will always be false).
static bool debugCheckingIntrinsics = false;

bool _debugSubtreeRelayoutRootAlreadyMarkedNeedsLayout() {
bool _debugRelayoutBoundaryAlreadyMarkedNeedsLayout() {
if (_relayoutBoundary == null) {
// We don't know where our relayout boundary is yet.
return true;
Expand Down Expand Up @@ -2281,7 +2307,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
void markNeedsLayout() {
assert(_debugCanPerformMutations);
if (_needsLayout) {
assert(_debugSubtreeRelayoutRootAlreadyMarkedNeedsLayout());
assert(_debugRelayoutBoundaryAlreadyMarkedNeedsLayout());
return;
}
if (_relayoutBoundary == null) {
Expand Down Expand Up @@ -2346,32 +2372,36 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
markParentNeedsLayout();
}

void _cleanRelayoutBoundary() {
if (_relayoutBoundary != this) {
_relayoutBoundary = null;
visitChildren(_cleanChildRelayoutBoundary);
/// Set [_relayoutBoundary] to null throughout this render object's subtree,
/// stopping at relayout boundaries.
// This is a static method to reduce closure allocation with visitChildren.
static void _cleanChildRelayoutBoundary(RenderObject child) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this reduce closure allocation? The original form doesn't seem to be called in an anonymous function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the "reduce closure allocation" part isn't relative to before this PR; it was introduced in 1a79592 / #51439.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ok looks like the instance method was removed so it makes sense the comment is moved here

if (child._relayoutBoundary != child) {
child.visitChildren(_cleanChildRelayoutBoundary);
child._relayoutBoundary = null;
}
}

void _propagateRelayoutBoundary() {
if (_relayoutBoundary == this) {
// This is a static method to reduce closure allocation with visitChildren.
static void _propagateRelayoutBoundaryToChild(RenderObject child) {
if (child._relayoutBoundary == child) {
return;
}
final RenderObject? parentRelayoutBoundary = parent?._relayoutBoundary;
final RenderObject? parentRelayoutBoundary = child.parent?._relayoutBoundary;
assert(parentRelayoutBoundary != null);
if (parentRelayoutBoundary != _relayoutBoundary) {
_relayoutBoundary = parentRelayoutBoundary;
visitChildren(_propagateRelayoutBoundaryToChild);
}
assert(parentRelayoutBoundary != child._relayoutBoundary);
child._setRelayoutBoundary(parentRelayoutBoundary!);
}

// Reduces closure allocation for visitChildren use cases.
static void _cleanChildRelayoutBoundary(RenderObject child) {
child._cleanRelayoutBoundary();
}

static void _propagateRelayoutBoundaryToChild(RenderObject child) {
child._propagateRelayoutBoundary();
/// Set [_relayoutBoundary] to [value] throughout this render object's
/// subtree, including this render object but stopping at relayout boundaries
/// thereafter.
void _setRelayoutBoundary(RenderObject value) {
assert(value != _relayoutBoundary);
// This may temporarily break the _relayoutBoundary invariant at children;
// the visitChildren restores the invariant.
_relayoutBoundary = value;
visitChildren(_propagateRelayoutBoundaryToChild);
}

/// Bootstrap the rendering pipeline by scheduling the very first layout.
Expand All @@ -2396,6 +2426,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge

@pragma('vm:notify-debugger-on-exception')
void _layoutWithoutResize() {
assert(_needsLayout);
assert(_relayoutBoundary == this);
RenderObject? debugPreviousActiveLayout;
assert(!_debugMutationsLocked);
Expand Down Expand Up @@ -2520,8 +2551,7 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
}());

if (relayoutBoundary != _relayoutBoundary) {
_relayoutBoundary = relayoutBoundary;
visitChildren(_propagateRelayoutBoundaryToChild);
_setRelayoutBoundary(relayoutBoundary);
}

if (!kReleaseMode && debugProfileLayoutsEnabled) {
Expand All @@ -2530,13 +2560,15 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
return;
}
_constraints = constraints;

if (_relayoutBoundary != null && relayoutBoundary != _relayoutBoundary) {
// The local relayout boundary has changed, must notify children in case
// they also need updating. Otherwise, they will be confused about what
// their actual relayout boundary is later.
visitChildren(_cleanChildRelayoutBoundary);
}
_relayoutBoundary = relayoutBoundary;

assert(!_debugMutationsLocked);
assert(!_doingThisLayoutWithCallback);
assert(() {
Expand Down