Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ios_platform_view] Only remove platform views from flutter view in reset. #41709

Merged
merged 3 commits into from May 16, 2023
Merged

[ios_platform_view] Only remove platform views from flutter view in reset. #41709

merged 3 commits into from May 16, 2023

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented May 3, 2023 •
edited

Add a class type check during reset when removing the PlatformViews.

Fixes: flutter/flutter#125999

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@cyanglaz cyanglaz marked this pull request as ready for review May 3, 2023 20:04
@cyanglaz cyanglaz requested a review from hellohuanlin May 3, 2023 20:04
chinmaygarde
chinmaygarde reviewed May 11, 2023
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm Outdated
@@ -624,6 +624,9 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
void FlutterPlatformViewsController::Reset() {
UIView* flutter_view = flutter_view_.get();
for (UIView* sub_view in [flutter_view subviews]) {
if (![sub_view isKindOfClass:[ChildClippingView class]]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shy away from RTTI-ish things in the engine. Can we avoid the same in ObjC TUs as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_ for the reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will fix it. Also filed flutter/flutter#126868

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting read. tho that means we will need to manually keep track of the child clipping views array, and keep that in sync with UIView::subviews. not sure if it's worth the effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manually keep track of the child clipping views array

We have it: active_composition_order_

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chinmaygarde @hellohuanlin Fixed, ptal

hellohuanlin
hellohuanlin approved these changes May 15, 2023
View reviewed changes
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
UIView* flutter_view = flutter_view_.get();
for (UIView* sub_view in [flutter_view subviews]) {
for (int64_t view_id : active_composition_order_) {
UIView* sub_view = root_views_[view_id].get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment clarifying that there may be non-platform views left in flutter_view's subviews (which was the confusing part that have caused this bug right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There wasn't any bug AFAIK. The old code was just not careful about what to remove.

I agree if we used the old approach with (isKindOf), a comment is helpful.
The new approach doesn't even assume the super view is flutter view. I'm not sure what I should say in a comment to not make it confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label May 16, 2023
@auto-submit auto-submit bot merged commit 3eccedb into flutter:main May 16, 2023
28 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 16, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 16, 2023
@engine-flutter-autoroll
3eccedb11 [ios_platform_view] Only remove platform views from flutter…
3e1447a 
… view in reset. (flutter/engine#41709)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 16, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 16, 2023
@engine-flutter-autoroll
3eccedb11 [ios_platform_view] Only remove platform views from flutter…
e182195 
… view in reset. (flutter/engine#41709)
zanderso pushed a commit to flutter/flutter that referenced this pull request May 16, 2023
@engine-flutter-autoroll
Roll Flutter Engine from c4d4b40c6596 to d7a5de672d1e (4 revisions) (#…
e62e5c4 
…126919)

flutter/engine@c4d4b40...d7a5de6

2023-05-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JCoP2Fekj3MBIqskE... to N4LwCRxg0oIevhQ_O... (flutter/engine#42070)
2023-05-16 zhangzhijian.123@bytedance.com [Impeller] Fix issue about
saveLayer ignoring opacity of paint with advanced blend mode
(flutter/engine#41972)
2023-05-16 ychris@google.com [ios_platform_view] Only remove platform
views from flutter view in reset. (flutter/engine#41709)
2023-05-16 godofredoc@google.com Add drone_dimensions as top level
target property. (flutter/engine#42064)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from JCoP2Fekj3MB to N4LwCRxg0oIe

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
@engine-flutter-autoroll
Roll Flutter Engine from c4d4b40c6596 to d7a5de672d1e (4 revisions) (f…
acae46e 
…lutter#126919)

flutter/engine@c4d4b40...d7a5de6

2023-05-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JCoP2Fekj3MBIqskE... to N4LwCRxg0oIevhQ_O... (flutter/engine#42070)
2023-05-16 zhangzhijian.123@bytedance.com [Impeller] Fix issue about
saveLayer ignoring opacity of paint with advanced blend mode
(flutter/engine#41972)
2023-05-16 ychris@google.com [ios_platform_view] Only remove platform
views from flutter view in reset. (flutter/engine#41709)
2023-05-16 godofredoc@google.com Add drone_dimensions as top level
target property. (flutter/engine#42064)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from JCoP2Fekj3MB to N4LwCRxg0oIe

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chinmaygarde chinmaygarde chinmaygarde left review comments

@hellohuanlin hellohuanlin hellohuanlin approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-ios
Projects
None yet
Milestone
No milestone
3 participants
@cyanglaz @chinmaygarde @hellohuanlin