Leader not always dirty #92598

goderbauer · 2021-10-27T20:09:20Z

Prior to this change, LeaderLayers had alwaysNeedsAddToScene unconditionally set to true, but they only need to always be added to scene while actually connected to a FollowerLayer. This change makes it so and with that allows retained (= raster cachable) rendering of subtrees that contain LeaderLayers, which are currently not connected to a follower.

Change is motivated by customer "money": Fading in a route with a textfield was thrashing the raster cache because the textfield was adding a LeaderLayer that prevented retained rendering due to its alwaysNeedsAddToScene flag. The LeaderLayer is used to properly position toolbars and selection handles. However, when both are not visible (and their corresponding FollowerLayers are therefore not in the tree) there was no reason for this behavior. Raster caching the textfield (along any other content) during the fade transition is desirable for performance reason. This change also adds tests to ensure that a textfield without selection handles and toolbars visible remains raster-cachable.

Helps with b/203470598

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

goderbauer · 2021-10-27T20:13:47Z

/cc @dnfield

dnfield · 2021-10-27T20:20:48Z

packages/flutter/lib/src/rendering/layer.dart

@@ -2302,6 +2378,21 @@ class FollowerLayer extends ContainerLayer {
  ///  * [unlinkedOffset], for when the layer is not linked.
  Offset? linkedOffset;

+  LayerLinkHandle? _leaderHandler;


nit: _leaderHandle

LongCatIsLooong

LGTM but I'm not too familiar with how layers work. I think @Hixie originally wrote the leader/follower layers.

LongCatIsLooong · 2021-10-27T20:32:18Z

packages/flutter/lib/src/rendering/editable.dart

@@ -2354,7 +2354,9 @@ class RenderEditable extends RenderBox with RelayoutWhenSystemFontsChangeMixin,
      _clipRectLayer.layer = null;
      _paintContents(context, offset);
    }
-    _paintHandleLayers(context, getEndpointsForSelection(selection!));
+    if (selection!.isValid) {
+      _paintHandleLayers(context, getEndpointsForSelection(selection!));


It sounds like getEndpointForSelection should return an empty list if the selection is not valid?

Sounds reasonable. However, when doing that, 142 tests are starting to fail. Let's leave that refactoring to a later PR.

LongCatIsLooong · 2021-10-27T22:43:48Z

packages/flutter/lib/src/rendering/layer.dart

+  /// purposes.
+  LeaderLayer? get debugLeader {
+    LeaderLayer? result;
+    if (kDebugMode) {


Question: is there a difference between this and an assert? Assert blocks don't get compiled unless it's in debug mode?

AFAIK, no. Since kDebugMode is a const bool the same rules regarding tree shaking should apply to this and this should not end up in a release binary.

Some packages, like local_hero, depends on the LayerLink.leader field.
Can it be exposed by a getter method again?
@goderbauer

Re-exposed with the changes in #96486.

LongCatIsLooong · 2021-10-27T22:47:53Z

packages/flutter/lib/src/rendering/layer.dart

+  ///
+  /// When the [FollowerLayer] no longer wants to follow the [LeaderLayer],
+  /// [LayerLinkHandle.dispose] must be called to disconnect the link.
+  LayerLinkHandle registerFollower() {


Should this be private? register/dispose seems to be already managed in FollowerLayer's lifecycle event callbacks. Do developers still have call these methods themselves at some point?

Good point. This and the entire LayerLinkHandle class can just be private. Changed it.

LongCatIsLooong · 2021-10-27T22:48:39Z

packages/flutter/lib/src/rendering/layer.dart

+/// [LeaderLayer].
+///
+/// If the link is no longer needed, [dispose] must be called to disconnect it.
+class LayerLinkHandle {


Is it possible to make this private?

LongCatIsLooong · 2021-10-27T22:49:46Z

packages/flutter/test/material/text_field_test.dart

@@ -9967,4 +9967,73 @@ void main() {
      containsPair('hintText', 'placeholder text'),
    );
  });
+


nit: these tests are probably better suited in editable_test.dart?

My thinking for placing them here was: I want to make sure that TextField never pushes these kind of layers. That currently Editable was the source of the bug is an implementation detail. If we reshuffle the implementation, the only thing that's important is that textfield continues to not do this.

LongCatIsLooong · 2021-10-27T22:58:44Z

packages/flutter/lib/src/rendering/layer.dart


  /// {@macro flutter.rendering.FollowerLayer.alwaysNeedsAddToScene}
  @override
-  bool get alwaysNeedsAddToScene => true;
+  bool get alwaysNeedsAddToScene => _link._connectedFollowers > 0;


Question: when alwaysNeedsAddToScene gets called the layer tree is finalized right? So it's not possible for a FollowerLayer to connect after this gets called and accidentally skips the add to scene step?

alwaysNeedsAddtoScene is called while building the scene. However, this logic happens synchronously and is completed before the next frame build. So no, you can't end up adding a FollowerLayer until the next frame build, which will be working with a new scene.

Yeah, this is evaluated during renderView.compositeFrame() in the snippet below after the layer tree has been finalized:

flutter/packages/flutter/lib/src/rendering/binding.dart

Lines 495 to 505 in 30d3866

void drawFrame() {

assert(renderView != null);

pipelineOwner.flushLayout();

pipelineOwner.flushCompositingBits();

pipelineOwner.flushPaint();

if (sendFramesToEngine) {

renderView.compositeFrame(); // this sends the bits to the GPU

pipelineOwner.flushSemantics(); // this also sends the semantics to the OS.

_firstFrameSent = true;

}

}

dnfield

Layer changes LGTM

dnfield

Layer changes LGTM

zanderso · 2021-11-01T03:59:49Z

Internal benchmark results for this change: https://flutter-flutter-perf.skia.org/e/?begin=1635396934&end=1635464072&keys=X79fae4dec0e50b8f554a8a6f79f45812&num_commits=50&request_type=1&xbaroffset=26369

creativecreatorormaybenot · 2022-01-28T17:35:26Z

@goderbauer Making _leader prviate here is a breaking change - is this necessary for any reason?

We have a package that depends on this currently and I am wondering how I should handle it now. If there is a motivation for the change, could you explain that - I would love to understand it.
Otherwise, I would probably like to propose to open a PR to expose it again ~

creativecreatorormaybenot · 2022-01-28T17:37:05Z

For Flutter a change is only breaking if it breaks one of the contributed tests (see also https://github.com/flutter/flutter/wiki/Tree-hygiene#1-determine-if-your-change-is-a-breaking-change). Since this change didn't require any changes to those tests it isn't considered breaking and no migration guide has been published. Please read the link above to learn how you can contribute tests to avoid being broken in the future.

@goderbauer This answer seems nice on the surface and I understand - but it is not always feasible to go through with the necessary overhead. Can you explain how to handle the breaking change?

goderbauer · 2022-01-28T17:51:06Z

The leader property has been exposed again as you can see in

flutter/packages/flutter/lib/src/rendering/layer.dart

Line 2113 in 28ca7bd

LeaderLayer? get leader => _leader;

.

To avoid that your package breaks on things like this in the future I highly recommend contributing your tests to https://github.com/flutter/tests.

Hixie · 2022-02-03T23:49:05Z

@creativecreatorormaybenot There shouldn't be much overhead if the package is already on GitHub... it's literally just a tiny file that specifies how to check out the package and run the tests. I'm very interested in hearing your experience if this is too much overhead, so that we can make it smoother.

creativecreatorormaybenot · 2022-02-04T00:32:16Z

@Hixie hey, I really appreciate it. I actually started to write a full repo of public-facing tests for the company I work(ed) at, but I ended up not publishing it because it was even too much overhead to finish it, basically needing to maintain another set of tests.

For packages, I fully agree. The flutter_portal package in particular does not have sufficient test coverage I would say, but if I have a package that qualifies, I will certainly contribute!

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 27, 2021

google-cla bot added the cla: yes label Oct 27, 2021

goderbauer requested a review from LongCatIsLooong October 27, 2021 20:11

dnfield reviewed Oct 27, 2021

View reviewed changes

LongCatIsLooong approved these changes Oct 27, 2021

View reviewed changes

dnfield reviewed Oct 28, 2021

View reviewed changes

dnfield approved these changes Oct 28, 2021

View reviewed changes

goderbauer added the waiting for tree to go green label Oct 28, 2021

goderbauer added 8 commits October 28, 2021 09:56

LeaderLayer only need add to scene when connected to followers

ad718ed

remove visibleForTesting

8f23523

doc fix

7ae1b36

more docs

cabd561

dan review

f2e594b

fix doc

c5df759

review

ce8550e

fix docs

7c2c4f6

goderbauer force-pushed the leaderNotAlwaysDirty branch from 118b88d to 7c2c4f6 Compare October 28, 2021 16:56

disable toolbar tests on platforms that do not support toolbars

c4614ff

goderbauer merged commit ca788d8 into flutter:master Oct 28, 2021

goderbauer deleted the leaderNotAlwaysDirty branch October 28, 2021 17:35

clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021

Leader not always dirty (flutter#92598)

416fd07

WizzXu pushed a commit to WizzXu/flutter that referenced this pull request Nov 19, 2021

Leader not always dirty (flutter#92598)

045edbd

selcukguvel mentioned this pull request Dec 17, 2021

Document breaking change in LayerLink class #95464

Closed

goderbauer mentioned this pull request Jan 4, 2022

Do not crash on LeaderLayer.applyTransform after retained rendering #96144

Merged

cchamm mentioned this pull request Jan 11, 2022

flutter 2.8 support letsar/local_hero#7

Closed

goderbauer mentioned this pull request Jan 11, 2022

enable more retained rendering opportunities for LeaderLayer #96486

Merged

goderbauer mentioned this pull request Jan 12, 2022

Expose the leader field of the LayerLink class. #96451

Closed

8 tasks

darshankawar mentioned this pull request Jan 27, 2022

Slider in Draggable.feedback does not work as expected and throws an error LeaderLayer anchor must come before FollowerLayer in paint order, but the reverse was true. #97292

Open

This was referenced Mar 1, 2022

CP request: #96486 into stable #99349

Closed

Add tests for the flutter_portal package flutter/tests#126

Merged

Include package tests in flutter/tests fzyzcjy/flutter_portal#46

Closed

fzyzcjy mentioned this pull request Mar 17, 2022

Failed assertion: '_lastOffset != null' in various cases, which should exist in Flutter 2.8~2.10 and flutter_portal from old to new fzyzcjy/flutter_portal#56

Closed

	void drawFrame() {
	assert(renderView != null);
	pipelineOwner.flushLayout();
	pipelineOwner.flushCompositingBits();
	pipelineOwner.flushPaint();
	if (sendFramesToEngine) {
	renderView.compositeFrame(); // this sends the bits to the GPU
	pipelineOwner.flushSemantics(); // this also sends the semantics to the OS.
	_firstFrameSent = true;
	}
	}

Leader not always dirty #92598

Leader not always dirty #92598

Uh oh!

Conversation

goderbauer commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

goderbauer commented Oct 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment

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!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Nov 1, 2021

Uh oh!

creativecreatorormaybenot commented Jan 28, 2022

Uh oh!

creativecreatorormaybenot commented Jan 28, 2022

Uh oh!

goderbauer commented Jan 28, 2022

Uh oh!

Hixie commented Feb 3, 2022

Uh oh!

creativecreatorormaybenot commented Feb 4, 2022

Uh oh!

Uh oh!

goderbauer commented Oct 27, 2021 •

edited

Loading