Fix errors when using multiple build/pipeline owners #113817
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
flutter-dashboard
bot
added
the
framework
| static RenderObject? get debugActiveLayout => _debugActiveLayout; | ||
| static RenderObject? _debugActiveLayout; | ||
|
|
||
| /// Set [debugActiveLayout] to null when [inner] callback is called. |
There was a problem hiding this comment.
Almost same (as mentioned there), but with one more thing:
This PR makes this function public instead of private. Then, users of the secondary pipeline owners can call that function. If only have that PR, then cannot.
|
Can you update this PR with the latest master to make sure what is actually changed by this? |
# Conflicts: # packages/flutter/lib/src/rendering/object.dart
|
Done merging (wait for ci though, but the idea is clear) |
|
ping :) @CaseyHillers, since
|
You can ignore this failure for now. Google Testing can only run if a Flutter hacker approves your PR. Internally, it says there's no LGTMs, and it marks the status as failed (there's an internal tracking bug for making this status show pending instead of failing) |
|
@CaseyHillers Thanks, get it |
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
Since the layout/paint cycle is managed by pipeline owner, should the _debugActiveLayout property in the PipelineOwner instead? that way we don't need to open up dangerous API to clear the value.
|
Not very sure, it was at RenderObject previously so I think there may be some design reasons? |
Since this will may also affect multi window, so cc @goderbauer. I think the current logic(that it uses static variable on RenderObject) is wrong it basically eliminate the possibility to have two pipelineowners to run their cycle in parallel. This pr feels like a bandage and may be hard to maintain |
|
I agree with @chunhtai here. We would need to design a more maintainable solution around this. I am going to close this one, looks like we already have an issue on file for this problem. |
4 participants
Close #114002
We all know that, Flutter allows us to create our own
BuildOwnerandPipelineOwner, and here is even an official example: https://github.com/flutter/flutter/blob/master/examples/api/lib/widgets/framework/build_owner.0.dart. The doc also agrees with that. For example:And
Therefore, theoretically, we should be able to happily use our own
BuildOwnerandPipelineOwneranywhere freely. However, it has a bug as follows: If I callpipelineOwner.flushPaint();(and sibling methods) inside the layout phase of the main PipelineOwner, then I get an assertion error in debug mode.The root cause is that, even though the self-managed PipelineOwner is isolated from the flutter-managed PipelineOwner, the debug variable
RenderObject.debugActiveLayoutis shared. Therefore, when calling flushPaint on self-manged PipelineOwner within a call of flushLayout of flutter-managed PipelineOwner, the assertions get confused and wrongly throws.My hack can be seen in https://github.com/fzyzcjy/flutter_smooth/blob/0c5db0ff270aa0c8cff28ea19055999627a8df6d/packages/smooth/lib/src/infra/auxiliary_tree_pack.dart#L214. Copy it here for completeness:
... _temporarilyRemoveDebugActiveLayout(() { pipelineOwner.flushPaint(); }); ... void _temporarilyRemoveDebugActiveLayout(VoidCallback f) { // NOTE we have to temporarily remove debugActiveLayout // b/c [SecondTreeRootView.paint] is called inside [preemptRender] // which is inside main tree's build/layout. // thus, if not set it to null we will see error // https://github.com/fzyzcjy/yplusplus/issues/5783#issuecomment-1254974511 // In short, this is b/c [debugActiveLayout] is global variable instead // of per-tree variable // and also // https://github.com/fzyzcjy/yplusplus/issues/5793#issuecomment-1256095858 final oldDebugActiveLayout = RenderObject.debugActiveLayout; RenderObject.debugActiveLayout = null; try { f(); } finally { RenderObject.debugActiveLayout = oldDebugActiveLayout; } }However, for this to work, the
debugActiveLayoutsetter must be public.The most naive solution is to make it public, but that may violate encapsulation. Thus, in the proposed PR, I create a method to wrap that.
Reproduction code and output
Details
yields
Details
Performance overhead
Using compiler explorer, we can see that it does not generate worse assembly (as long as we use the prefer-inline pragma)
https://godbolt.org/z/EoznoWex7
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.