UiKitView will be rendered with borders #30664
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. 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. |
|
cc @cyanglaz |
|
Isn't this the same as |
yes, I know this API, I don't use this api case by i feel I was want change the FlutterView default property value. |
|
I think it was to make things consistent with Android. There also might hav ebeen some performance related concerns, but I don't remember there being any benchmark changes for this. That said, this patch needs a test. |
|
Do we have benchmark tests which can test this change? |
|
I'd try running some devicelab iOS tests with and without this change, e.g. flutter_gallery__transitions_perf |
|
@JsouLiang I think the benchmark already exists, but we also need tests to make sure this value is set to NO if we are going to land this. |
|
@cyanglaz yes, I think so, we need to verify the impact of this change on performance. And is the benchmark test on the remote CI? |
|
@JsouLiang I strongly doubt this will impact any benchmark we have, and if it does impact a benchmark unexpectedly we can always revert it. I wouldn't worry too much about the benchmark. However, we do want a unit test (probably one in https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterViewControllerTest.mm) that tests that this behavior is as expected. |
|
It would probably be good to have a golden test that verifies we don't regress the alpha blending this fixes as well, but that may be harder to do particularly in the engine repo. |
|
So what should I do? |
|
@dnfield I think this change doesn't add test, because we can't be sure there are show 'border'. If you in iPhone see the border and you snapshot screen the picture is no border. |
|
@JsouLiang For this particular change, you can add an unit-test to check if |
|
@cyanglaz Hey, my phone is iPhoneXsMax. And I run the demo which has render border, then I use |
|
@cyanglaz hey, the pr has any progress? |
|
@JsouLiang - I think at this point we're waiting for a unit test to be added as suggested #30664 (comment) |
JsouLiang
force-pushed
the
iOS-UIKitView-Render-Issue
branch
4 times, most recently
from
2d7a0a7 to
47d1f7d
Compare
|
@dnfield The test has been added as requested. Are you able to review the same? |
JsouLiang
force-pushed
the
iOS-UIKitView-Render-Issue
branch
from
47d1f7d to
badc8a4
Compare
|
@cyanglaz hey, I think you are right, and i add a new test case. please help me review it, thanks. |
…ter#30664)" This reverts commit df562b8. See flutter/flutter#99249
|
@cyanglaz @gaaclarke @dnfield I found that this PR causes some performance issues, that's mean we couldn't use this way that change the |
5 participants
The UIView's
opaqueproperty will inform the UIViews color blend, for example the parent is Red and subview is Green,when the subview's alpha not equal 1.0, then iOS GPU will blend the color:
Result = Source + Destination * (1 - SourceAlpha)If the FlutterView
opaque = YESand when it subView has translucent pixel that rendering out may have jagged black edges.So I change the FlutterView
opaque = NOto make blend alpha correctly.And you can use this project to check the effect: https://github.com/JsouLiang/platform_blend_test
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.