LayerLink can temporary allow multiple leaders #95977
Conversation
| LayerLink? layerLink; | ||
|
|
||
| @override | ||
| bool isRepaintBoundary = true; |
There was a problem hiding this comment.
Just to double check: There isn't a problem if we are not a repaint boundary?
There was a problem hiding this comment.
It will still be a problem.
The reason i made it a repaint boundary is for easy testing. If it is not a repaint boundary, the markNeedsPaint will go through the ancestor chain to find the nearest repaint boundary and markNeedsPaint on it. That means I would have to create a parent repaint boundary in the test anyway.
There was a problem hiding this comment.
But that case is also fixed by this fix, right?
There was a problem hiding this comment.
Yes, In the layer tree, the non-repaint boundary renderobject does not contributing additional node, so there is no difference between layer added by non-repaint boundary renderobject and layer added by the nearest repaint boundary.
| // layer may be appended to a new parent layer before it is removed from | ||
| // the old parent layer. | ||
| dirtyNodes.sort((RenderObject a, RenderObject b) => b.depth - a.depth); | ||
| for (final RenderObject node in dirtyNodes) { |
There was a problem hiding this comment.
Since this is performance critical code: We are not doing any extra work now, right? We are just doing the reset upfront instead of later?
There was a problem hiding this comment.
yes, but it becomes two for-loops instead of one. Not sure whether there will be any impact on perf. If this is not acceptable, I will then have to do something similar to globalkey duplication that temporary allows child to be attached to two parents and adds a postframecallback to make sure the previous parent give up the child.
This will be more complicated than the global key cases though, since it involves detaching/attaching logic.
|
Gold has detected about 8 new digest(s) on patchset 3. |
|
@goderbauer PTAL |
|
Why does this change goldens? Is that expected? |
|
@goderbauer that is previous commit, which broke a lot of tests, the latest commit does not have golden change |
|
@goderbauer PTAL |
| Set<LeaderLayer>? _debugPreviousLeaders; | ||
| bool _debugLeaderCheckScheduled = false; | ||
|
|
||
| /// schedule the check as post frame callback to make sure the |
There was a problem hiding this comment.
schedule -> Schedule
| assert(link._leader != this); | ||
| assert((){ | ||
| if (link._leader != null) { | ||
| link._debugPreviousLeaders ??= <LeaderLayer>{}; | ||
| link._debugPreviousLeaders!.add(link._leader!); | ||
| link._debugScheduleLeadersCleanUpCheck(); | ||
| } | ||
| return true; | ||
| }()); |
There was a problem hiding this comment.
I would move all of this into a _registerLeader method on LeaderLink to keep this a little cleaner here. This is all impl. detail of leaderLink.
| assert(link._leader != null); | ||
| if (link._leader == this) { | ||
| link._leader = null; | ||
| } else { | ||
| assert((){ | ||
| if (link._leader != this) { | ||
| return link._debugPreviousLeaders!.remove(this); | ||
| } | ||
| return true; | ||
| }()); | ||
| } |
There was a problem hiding this comment.
Same with this.
| @@ -2197,7 +2247,10 @@ class LeaderLayer extends ContainerLayer { | |||
| if (_link == value) { | |||
| return; | |||
| } | |||
| _link._leader = null; | |||
| if (attached) { | |||
There was a problem hiding this comment.
wrote a test switching layer link of an attached leader layer should not crash for this change
|
@goderbauer PTAL, I also fixed another corner case. |
|
This pull request is not suitable for automatic merging in its current state.
|
…pt one give up their leaderships at the end
fixes #95974
If a child layer is moved to a new container layer in the same frame. the assert will throw if the new container is painted before the old container is painted. This pr fixes it so that the pipelineowner resets all the dirty container layer before any of them is painted
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
The text was updated successfully, but these errors were encountered: