Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

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

Merged
auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
cyanglaz:platform_view_reset_fix
May 16, 2023
Merged

[ios_platform_view] Only remove platform views from flutter view in reset.#41709
auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
cyanglaz:platform_view_reset_fix

Conversation

@cyanglaz

@cyanglaz cyanglaz commented May 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm Outdated
void FlutterPlatformViewsController::Reset() {
UIView* flutter_view = flutter_view_.get();
for (UIView* sub_view in [flutter_view subviews]) {
if (![sub_view isKindOfClass:[ChildClippingView class]]) {

@chinmaygarde chinmaygarde May 11, 2023

Copy link
Copy Markdown
Contributor

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.

@chinmaygarde chinmaygarde May 11, 2023

Copy link
Copy Markdown
Contributor

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.

@cyanglaz cyanglaz May 15, 2023

Copy link
Copy Markdown
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

@hellohuanlin hellohuanlin May 15, 2023

Copy link
Copy Markdown
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.

@cyanglaz cyanglaz May 15, 2023

Copy link
Copy Markdown
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_

@cyanglaz cyanglaz May 15, 2023

Copy link
Copy Markdown
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

@cyanglaz cyanglaz requested a review from hellohuanlin May 15, 2023 19:53
hellohuanlin
hellohuanlin approved these changes May 15, 2023
View reviewed changes
Comment thread 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();

@hellohuanlin hellohuanlin May 15, 2023

Copy link
Copy Markdown
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?)

@cyanglaz cyanglaz May 15, 2023

Copy link
Copy Markdown
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.

@hellohuanlin hellohuanlin May 15, 2023

Copy link
Copy Markdown
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-team-archive:main May 16, 2023
@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
3eccedb [ios_platform_view] Only remove platform views from flutter v…
3e1447a 
…iew in reset. (flutter-team-archive/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
3eccedb [ios_platform_view] Only remove platform views from flutter v…
e182195 
…iew in reset. (flutter-team-archive/engine#41709)
zanderso pushed a commit to flutter/flutter that referenced this pull request May 16, 2023
@engine-flutter-autoroll
Roll Flutter Engine from c4d4b40 to d7a5de6 (4 revisions) (#126919)
e62e5c4 
flutter-team-archive/engine@c4d4b40...d7a5de6

2023-05-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JCoP2Fekj3MBIqskE... to N4LwCRxg0oIevhQ_O... (flutter-team-archive/engine#42070)
2023-05-16 zhangzhijian.123@bytedance.com [Impeller] Fix issue about
saveLayer ignoring opacity of paint with advanced blend mode
(flutter-team-archive/engine#41972)
2023-05-16 ychris@google.com [ios_platform_view] Only remove platform
views from flutter view in reset. (flutter-team-archive/engine#41709)
2023-05-16 godofredoc@google.com Add drone_dimensions as top level
target property. (flutter-team-archive/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 c4d4b40 to d7a5de6 (4 revisions) (flutter#12…
acae46e 
…6919)

flutter-team-archive/engine@c4d4b40...d7a5de6

2023-05-16 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
JCoP2Fekj3MBIqskE... to N4LwCRxg0oIevhQ_O... (flutter-team-archive/engine#42070)
2023-05-16 zhangzhijian.123@bytedance.com [Impeller] Fix issue about
saveLayer ignoring opacity of paint with advanced blend mode
(flutter-team-archive/engine#41972)
2023-05-16 ychris@google.com [ios_platform_view] Only remove platform
views from flutter view in reset. (flutter-team-archive/engine#41709)
2023-05-16 godofredoc@google.com Add drone_dimensions as top level
target property. (flutter-team-archive/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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@chinmaygarde chinmaygarde chinmaygarde left review comments
@hellohuanlin hellohuanlin hellohuanlin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-ios

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ios platform view] FlutterPlatformViewsController::Reset should not assume all the subviews of flutterView are platformviews

3 participants

@cyanglaz @chinmaygarde @hellohuanlin