Fix UiKitView which wrongly unconditionally repaints #111790
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. |
|
Test passes here.
Without the fix, test fails:
|
| set viewController(UiKitViewController viewController) { | ||
| assert(viewController != null); | ||
| if (_viewController == viewController) { | ||
| return; | ||
| } | ||
| final bool needsSemanticsUpdate = _viewController.id != viewController.id; | ||
| _viewController = viewController; |
There was a problem hiding this comment.
This is confusing code, because the local viewController is shadowing the getter viewController. Perhaps while you are here you could rename the parameter to something harmless like value?
There was a problem hiding this comment.
Done (in latest commit).
Btw this seems also commonly seen in flutter framework code (though I agree value is neater), such as:
|
auto label is removed for flutter/flutter, pr: 111790, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead. |
|
auto label is removed for flutter/flutter, pr: 111790, due to Validations Fail. |
|
Oops seems to because need 2 approvals |
How the bug is found
This bug is surely not found by human eyes reading each and every line of Flutter code :)
I proposed the idea that a linter can be created (dart-code-checker/dart-code-metrics#997) to validate whether RenderObject field setters have correct early-return code. @incendial implemented it and ran it (dart-code-checker/dart-code-metrics#1003).
Thus, thanks @incendial for implementing the linter and run it through flutter framework code!
Bug description
As we know, when implementing a setter in RenderObject, we usually early halt if the new value is equal to the old value (such as this one, indeed almost all fields in RenderObject child classes are examples). This is very necessary, because we are setting fields in
updateRenderObjectunconditionally. If we do not early return, we will execute the full logic like markNeedsPaint and so on unconditionally on everyupdateRenderObject. That will be performance penalty.The current PR fixes one bug of such case. In more details, the
viewControllerfield setter lacks such early-return, and thus unconditionally calls markNeedsPaint. The setter is called fromupdateRenderObjectas follows:And _UiKitPlatformView is used here:
In other words, the controller is not changed in every frame. Instead, in common cases, it should be the same one for many frames. However, it triggers repaint for each and every rebuild.
Close #111788
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.