PlatformViewLink, handling creation of the PlatformViewSurface and dispose PlatformViewController #37703

cyanglaz · 2019-08-06T16:58:48Z

Description

Introduce PlatformViewLink which in this PR handles the creation and disposal of the platform view.

Related Issues

Tests

I added the following tests:

'PlatformViewController Widget init, should create a SizedBox widget before onPlatformViewCreated and a PlatformViewSurface after'
'PlatformViewController Widget dispose'

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 Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

amirh · 2019-08-13T17:41:21Z

packages/flutter/lib/src/services/platform_views.dart

@@ -729,4 +729,7 @@ abstract class PlatformViewController {

  /// Dispatches the `event` to the platform view.
  void dispatchPointerEvent(PointerEvent event);
+
+  /// Disposes the resources related to ths platform view from engine.


Disposes the platform view.

The [PlatformViewController] is unusable after calling dispose.

amirh · 2019-08-13T17:43:01Z

packages/flutter/lib/src/widgets/platform_view.dart

+    @required this.onPlatformViewCreated}):assert(id != null),
+                          assert(onPlatformViewCreated != null);
+
+  /// The auto generated id for the newly created platform view.


The unique identifier for the new platform view.

amirh · 2019-08-13T17:43:25Z

packages/flutter/lib/src/widgets/platform_view.dart

+  /// [PlatformViewController.viewId] should match this id.
+  final int id;
+
+  /// Notifies when the PlatformView is ready.


Callback invoked after the platform view has been created.

amirh · 2019-08-13T17:55:06Z

packages/flutter/lib/src/widgets/platform_view.dart

+///   Widget build(BuildContext context) {
+///     return PlatformViewLink(
+///       createCallback: createFooWebView,
+///       surfaceFactory: (BuildContext context, PlatformViewController controller, int id) {


I guess int id is leftover from previous iteration?

amirh · 2019-08-13T17:57:37Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// Constructs a [PlatformViewController].
+///
+/// The implementer of a new platform view is responsible to implement this method when constructing a [PlatformViewLink].
+typedef CreatePlatformView = PlatformViewController Function(PlatformViewCreationParams params);


nit: CreatePlatformViewController

amirh · 2019-08-13T18:09:03Z

packages/flutter/lib/src/widgets/platform_view.dart

+  /// * [PlatformViewSurface] for details on the widget returned by `surfaceFactory`.
+  /// * [PlatformViewCreationParams] for how each parameter can be used when implementing `createPlatformView`.
+  const PlatformViewLink({
+    @required PlatformViewSurfaceFactory surfaceFactory,


amirh · 2019-08-13T18:10:34Z

packages/flutter/lib/src/widgets/platform_view.dart

+
+  void _onPlatformViewCreated(int id) {
+     _platformViewCreated = true;
+    setState((){});


setState(() {
_platformViewCreated = true;
});

amirh · 2019-08-13T18:11:29Z

packages/flutter/lib/src/widgets/platform_view.dart

+  }
+
+  @override
+  void didChangeDependencies() {


Can we override initState instead?

done, also removed the _initialized flag.

amirh · 2019-08-13T18:13:08Z

packages/flutter/lib/src/widgets/platform_view.dart

+///   }
+/// }
+/// ```
+class PlatformViewLink extends StatefulWidget {


Document what happens when the widget is rebuild with a different create callback or a different surfaceFactory

done, with the change that made sure PlatformViewSurface only created once.

amirh · 2019-08-13T18:15:42Z

packages/flutter/test/widgets/platform_view_test.dart

+      await tester.pumpWidget(Container());
+
+      expect(disposedController.disposed, true);
+    });


I would add an equivalent test to Android view survives widget tree change

cyanglaz

Updated with review comments.

cyanglaz · 2019-08-14T15:57:45Z

packages/flutter/lib/src/services/platform_views.dart

@@ -729,4 +729,7 @@ abstract class PlatformViewController {

  /// Dispatches the `event` to the platform view.
  void dispatchPointerEvent(PointerEvent event);
+
+  /// Disposes the resources related to ths platform view from engine.


cyanglaz · 2019-08-14T15:58:46Z

packages/flutter/lib/src/widgets/platform_view.dart

+    @required this.onPlatformViewCreated}):assert(id != null),
+                          assert(onPlatformViewCreated != null);
+
+  /// The auto generated id for the newly created platform view.


cyanglaz · 2019-08-14T15:58:50Z

packages/flutter/lib/src/widgets/platform_view.dart

+  /// [PlatformViewController.viewId] should match this id.
+  final int id;
+
+  /// Notifies when the PlatformView is ready.


cyanglaz · 2019-08-14T16:03:44Z

packages/flutter/lib/src/widgets/platform_view.dart

+
+/// Constructs a [PlatformViewController].
+///
+/// The implementer of a new platform view is responsible to implement this method when constructing a [PlatformViewLink].


cyanglaz · 2019-08-14T16:11:28Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// Constructs a [PlatformViewController].
+///
+/// The implementer of a new platform view is responsible to implement this method when constructing a [PlatformViewLink].
+typedef CreatePlatformView = PlatformViewController Function(PlatformViewCreationParams params);


cyanglaz · 2019-08-14T16:19:18Z

packages/flutter/lib/src/widgets/platform_view.dart

+  /// * [PlatformViewSurface] for details on the widget returned by `surfaceFactory`.
+  /// * [PlatformViewCreationParams] for how each parameter can be used when implementing `createPlatformView`.
+  const PlatformViewLink({
+    @required PlatformViewSurfaceFactory surfaceFactory,


cyanglaz · 2019-08-14T16:23:42Z

packages/flutter/lib/src/widgets/platform_view.dart

+  }
+
+  @override
+  void didChangeDependencies() {


done, also removed the _initialized flag.

cyanglaz · 2019-08-14T16:23:54Z

packages/flutter/lib/src/widgets/platform_view.dart

+
+  void _onPlatformViewCreated(int id) {
+     _platformViewCreated = true;
+    setState((){});


cyanglaz · 2019-08-14T17:09:04Z

packages/flutter/test/widgets/platform_view_test.dart

+      await tester.pumpWidget(Container());
+
+      expect(disposedController.disposed, true);
+    });


cyanglaz · 2019-08-14T21:22:07Z

packages/flutter/lib/src/widgets/platform_view.dart

+///   }
+/// }
+/// ```
+class PlatformViewLink extends StatefulWidget {


done, with the change that made sure PlatformViewSurface only created once.

amirh · 2019-08-14T21:56:04Z

packages/flutter/lib/src/widgets/platform_view.dart

+
+  const PlatformViewCreationParams._({
+    @required this.id,
+    @required this.onPlatformViewCreated}):assert(id != null),


style nit: ) : assert

amirh · 2019-08-14T21:56:22Z

packages/flutter/lib/src/widgets/platform_view.dart

+  const PlatformViewCreationParams._({
+    @required this.id,
+    @required this.onPlatformViewCreated}):assert(id != null),
+                          assert(onPlatformViewCreated != null);


fix indentation

amirh · 2019-08-14T21:58:03Z

packages/flutter/lib/src/widgets/platform_view.dart

+
+  /// Callback invoked after the platform view has been created.
+  ///
+  /// Must be invoked as soon as the embedded platform view is ready.


I believe it doesn't have to be called "as soon as" the platform view is created.
I'd skip this paragraph.

amirh · 2019-08-14T21:59:07Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// It is required when constructing a [PlatformViewLink].
+typedef PlatformViewSurfaceFactory = PlatformViewSurface Function(BuildContext context, PlatformViewController controller);
+
+///Constructs a [PlatformViewController].


nit: missing space

amirh · 2019-08-14T22:00:40Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// The factory to construct a [PlatformViewSurface].
+///
+/// It is required when constructing a [PlatformViewLink].
+typedef PlatformViewSurfaceFactory = PlatformViewSurface Function(BuildContext context, PlatformViewController controller);


We probably want to make this return a widget.

amirh · 2019-08-14T22:03:38Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// The factory to construct a [PlatformViewSurface].
+///
+/// It is required when constructing a [PlatformViewLink].
+typedef PlatformViewSurfaceFactory = PlatformViewSurface Function(BuildContext context, PlatformViewController controller);


/// A factory for a surface presenting a platform view as part of the widget hierarchy. /// /// The returned widget should present the platform view associated with `controller`. /// /// See also: /// * [PlatformViewSurface], a common widget for presenting platform views. typedef PlatformViewSurfaceFactory = Widget Function(BuildContext context, PlatformViewController controller);

amirh · 2019-08-14T22:05:11Z

packages/flutter/lib/src/widgets/platform_view.dart

+  final CreatePlatformViewController _createPlatformViewController;
+
+  @override
+  State<StatefulWidget> createState() {


style nit: use => if it fits in one line.

cyanglaz

Updated. Ready for another review.

cyanglaz · 2019-08-15T00:59:41Z

packages/flutter/lib/src/widgets/platform_view.dart

+
+  const PlatformViewCreationParams._({
+    @required this.id,
+    @required this.onPlatformViewCreated}):assert(id != null),


cyanglaz · 2019-08-15T00:59:45Z

packages/flutter/lib/src/widgets/platform_view.dart

+  const PlatformViewCreationParams._({
+    @required this.id,
+    @required this.onPlatformViewCreated}):assert(id != null),
+                          assert(onPlatformViewCreated != null);


cyanglaz · 2019-08-15T00:59:49Z

packages/flutter/lib/src/widgets/platform_view.dart

+
+  /// Callback invoked after the platform view has been created.
+  ///
+  /// Must be invoked as soon as the embedded platform view is ready.


cyanglaz · 2019-08-15T00:59:54Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// The factory to construct a [PlatformViewSurface].
+///
+/// It is required when constructing a [PlatformViewLink].
+typedef PlatformViewSurfaceFactory = PlatformViewSurface Function(BuildContext context, PlatformViewController controller);


cyanglaz · 2019-08-15T00:59:57Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// The factory to construct a [PlatformViewSurface].
+///
+/// It is required when constructing a [PlatformViewLink].
+typedef PlatformViewSurfaceFactory = PlatformViewSurface Function(BuildContext context, PlatformViewController controller);


cyanglaz · 2019-08-15T01:00:05Z

packages/flutter/lib/src/widgets/platform_view.dart

+/// It is required when constructing a [PlatformViewLink].
+typedef PlatformViewSurfaceFactory = PlatformViewSurface Function(BuildContext context, PlatformViewController controller);
+
+///Constructs a [PlatformViewController].


cyanglaz · 2019-08-15T01:00:13Z

packages/flutter/lib/src/widgets/platform_view.dart

+  final CreatePlatformViewController _createPlatformViewController;
+
+  @override
+  State<StatefulWidget> createState() {


amirh

LGTM

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 6, 2019

googlebot added the cla: yes label Aug 6, 2019

cyanglaz requested a review from amirh August 6, 2019 16:59

cyanglaz added the a: platform-views Embedding Android/iOS views in Flutter apps label Aug 6, 2019

cyanglaz changed the title ~~PlatformView link, handling creation of the PlatformViewSurface and dispose PlatformViewController~~ PlatformViewLink, handling creation of the PlatformViewSurface and dispose PlatformViewController Aug 6, 2019

link

75a5a3f

cyanglaz force-pushed the common_platform_link branch from 9fed1dd to 75a5a3f Compare August 6, 2019 17:02

amirh reviewed Aug 13, 2019

View reviewed changes

review fixes

b63303a

cyanglaz commented Aug 14, 2019

View reviewed changes

amirh reviewed Aug 14, 2019

View reviewed changes

review fixes

a99309e

cyanglaz commented Aug 15, 2019

View reviewed changes

remove extra line

a42a0b8

amirh approved these changes Aug 15, 2019

View reviewed changes

cyanglaz merged commit 5acf63d into flutter:master Aug 15, 2019

cyanglaz deleted the common_platform_link branch August 15, 2019 19:16

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

PlatformViewLink, handling creation of the PlatformViewSurface and dispose PlatformViewController #37703

PlatformViewLink, handling creation of the PlatformViewSurface and dispose PlatformViewController #37703

Conversation

cyanglaz commented Aug 6, 2019 • edited

Description

Related Issues

Tests

Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanglaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirh left a comment

Choose a reason for hiding this comment

cyanglaz commented Aug 6, 2019 •

edited