The `ForwardingGestureRecognizers` to have back reference to the `PlatformViewsController` so it can access `FlutterViewController` when its available #20708

cyanglaz · 2020-08-22T03:00:10Z

Description

This PR makes PlatformViewsController get FlutterViewController as early as possible when first launch.

Related Issues

Fixes flutter/flutter#64228

Tests

I added the following tests:

testViewFlutterViewControllerIsSetForPlatformView

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

dnfield

This is fine to fix the immediate issue. We still have some problems if someone creates a headless engine and tries to make a platform view before assigning the VC

We also have problems if someone creates the engine first and the VC doesn't get shown soon enough (e.g. they use initWithEngine).

cyanglaz · 2020-08-22T03:30:29Z

I think nevertheless, this helps the PlatformViewsController to get the FlutterViewController instance as early as possible in initWithProject scenarios.

I can also add a fix for headless mode too. Something like I mentioned in flutter/flutter#64228 (comment)

One alternative could be that we cache all the gesture recognizers that were created without 
FlutterViewController and SetFlutterViewController will go through those and set those.
Just need to add locks to make sure there are no synchronization issues since
onCreated is called on UIThread and SetFlutterViewController is called on PlatformThread.

cyanglaz · 2020-08-22T03:34:13Z

Actually, for initWithEngine, we can set view controller to platformviewscontroller here: https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm#L108
If the PlatformViewsController has been initialized. If not, and when initializing PlatformViewsController, we can try to set the view controller again.

dnfield · 2020-08-22T04:00:10Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm

  return pixel[3];
 }

+


nit: remove extra newline

dnfield · 2020-08-22T04:03:50Z

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

 - (void)maybeSetupPlatformViewChannels {
  if (_shell && self.shell.IsSetup()) {
+    if ([self platformViewsController] != nullptr) {
+      [self platformViewsController]->SetFlutterViewController([self viewController]);


If this gets called from headless, we're toast :)

We could consider calling this from setViewController in here. In fact, I'm not clear on why we wouldn't.

But we could still run into the issue where the PlatformViewsController is created after the shell is, and the VC isn't attached until some later point in time.

Why would this fail on headless? Unless they call this as private API, maybeSetupPlatformViewChannels is only called in 2 places in this class and both places look fine.

If the PlatformViewsController is created after the shell, maybeSetupPlatformViewChannels should be called in setViewController when FlutterViewController is available, which then it is possible for the platform views to receive touch events.

It's called here:

engine/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm

Line 538 in cfe0cd2

[self maybeSetupPlatformViewChannels];

And we don't necessarily have a view controller yet.

And that is called by runEngine, which is public API. And it's normal to call that before adding a VC to the engine or add to app.

I guess we can't fix this way then. Probably better to go back to the fix that we discussed in the issue.

cyanglaz · 2020-08-22T16:25:18Z

@dnfield Updated so that PlatformViewsController can update FlutterViewController when available. I haven't added tests yet, will be back to add tests later. Also, it looks like current test for "setting flutter view controller earlier" timed out. I'm not entirely sure why :(

cyanglaz · 2020-08-22T22:47:47Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

 #include "flutter/shell/platform/darwin/ios/ios_context.h"
 #include "third_party/skia/include/core/SkPictureRecorder.h"

+@class FlutterTouchInterceptingView;


Had to forward declare here because now FlutterTouchInterceptingView references to FlutterPlatformViewsController.

LongTerm, we should refactor all the objc class in this file to their own files.

cyanglaz · 2020-08-23T21:00:43Z

@dnfield Ready for another review :)

dnfield

LGTM

…tformViewsController` so it can access `FlutterViewController` when its available (flutter#20708)

…the `PlatformViewsController` so it can access `FlutterViewController` when its available (flutter/engine#20708)

* update dart revision * Fix EGL_BAD_SURFACE when app is paused (#20735) * The `ForwardingGestureRecognizers` to have back reference to the `PlatformViewsController` so it can access `FlutterViewController` when its available (#20708) * Remove image sizes from Picture::GetAllocationSize (#20673) * update licenses golden Co-authored-by: Emmanuel Garcia <[email protected]> Co-authored-by: Chris Yang <[email protected]> Co-authored-by: Jason Simmons <[email protected]>

* Update 1.20 engine to use Dart 2.9.2 * update license goldens * Fix EGL_BAD_SURFACE when app is paused (#20735) * The `ForwardingGestureRecognizers` to have back reference to the `PlatformViewsController` so it can access `FlutterViewController` when its available (#20708) * cherry-pick e09af86 * skip linting since this was enabled in CI after this release branch cut commit. Co-authored-by: Emmanuel Garcia <[email protected]> Co-authored-by: Chris Yang <[email protected]>

flutter-dashboard bot added the platform-ios label Aug 22, 2020

googlebot added the cla: yes label Aug 22, 2020

cyanglaz requested review from chinmaygarde and dnfield August 22, 2020 03:00

cyanglaz mentioned this pull request Aug 22, 2020

[webView_flutter][iOS] WebView is frozen in release mode with flutter 1.20.2 stable version flutter/flutter#64228

Closed

dnfield approved these changes Aug 22, 2020

View reviewed changes

cyanglaz force-pushed the set_flutter_view_eariler branch 2 times, most recently from f271130 to cfe0cd2 Compare August 22, 2020 03:52

dnfield reviewed Aug 22, 2020

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm Outdated

return pixel[3];

}

Copy link

Contributor

dnfield Aug 22, 2020

Choose a reason for hiding this comment

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

nit: remove extra newline

dnfield reviewed Aug 22, 2020

View reviewed changes

cyanglaz changed the title ~~PlatformViewsController gets FlutterViewController as early as possible~~ PlatformViewsController gets FlutterViewController as early as possible, PlatformViewsController update FlutterViewController when available Aug 22, 2020

platforms view controller update vc

4b4be43

cyanglaz force-pushed the set_flutter_view_eariler branch from a2d2782 to 4b4be43 Compare August 22, 2020 16:23

forward gesture recognizer reference to platform views controller

8cea986

cyanglaz commented Aug 22, 2020

View reviewed changes

tests

7f5ddb8

dnfield approved these changes Aug 24, 2020

View reviewed changes

cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 24, 2020

fluttergithubbot merged commit 1c13aba into flutter:master Aug 24, 2020

cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Aug 24, 2020

The ForwardingGestureRecognizers to have back reference to the `Pla…

708297b

…tformViewsController` so it can access `FlutterViewController` when its available (flutter#20708)

engine-flutter-autoroll mentioned this pull request Aug 24, 2020

Roll Engine from 4bb6251015a2 to d16ba48e1d79 (10 revisions) flutter/flutter#64509

Closed

engine-flutter-autoroll mentioned this pull request Aug 24, 2020

Roll Engine from 4bb6251015a2 to 4caee915230b (4 revisions) flutter/flutter#64518

Merged

christopherfujino mentioned this pull request Aug 26, 2020

Flutter Engine Beta 1.21.0-9.2.pre cherry picks #20797

Merged

christopherfujino mentioned this pull request Aug 29, 2020

Flutter 1.20.3 engine cherrypicks #20838

Merged

cyanglaz mentioned this pull request Sep 10, 2020

[iOS][WebView] Gesture recognizers not blocked in iOS 13.4 and 13.5 flutter/flutter#53490

Closed

nwparker mentioned this pull request Nov 3, 2020

GestureDetector producing incorrect results when used with UiKitView on iOS 12 and 14 flutter/flutter#66097

Closed

mkc188 mentioned this pull request Nov 9, 2020

Animation freezes after changing AppLifecycleState or rebuilding complex UI with PlatformView in iOS flutter/flutter#70069

Closed

cyanglaz mentioned this pull request Nov 9, 2020

PlatformViewsController always make sure the touch events are finished #22406

Merged

13 tasks

wreppun mentioned this pull request Mar 2, 2021

Google map and ListView making Its scroll buggy. flutter/flutter#69729

Closed

The ForwardingGestureRecognizers to have back reference to the PlatformViewsController so it can access FlutterViewController when its available #20708

The ForwardingGestureRecognizers to have back reference to the PlatformViewsController so it can access FlutterViewController when its available #20708

Uh oh!

Conversation

cyanglaz commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyanglaz commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

cyanglaz Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

cyanglaz Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 22, 2020

Uh oh!

cyanglaz Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Aug 23, 2020

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

The `ForwardingGestureRecognizers` to have back reference to the `PlatformViewsController` so it can access `FlutterViewController` when its available #20708

The `ForwardingGestureRecognizers` to have back reference to the `PlatformViewsController` so it can access `FlutterViewController` when its available #20708

cyanglaz commented Aug 22, 2020 •

edited

Loading

cyanglaz commented Aug 22, 2020 •

edited

Loading

cyanglaz commented Aug 22, 2020 •

edited

Loading