Initial framework support for iOS platform views. by amirh · Pull Request #23412 · flutter/flutter

amirh · 2018-10-23T17:13:11Z

This PR adds the full framework stack (layer->render object->widget, and
service) for embedding iOS views with minimal functionality (see below for what's still missing).

I allowed myself to throw the entire framework stack in one PR since we're mostly
mirroring the structure we already established for embedded Android views, so this PR
is a little bigger than usual. I'm happy to break it down to the
different pieces of the stack if reviewers prefer.

Specifically this PR adds:

A UiKitView widget for embedding a UIView in the widget tree.
A RenderUiKitView which is the render object for showing a
UIView.
A PlatformViewLayer which denotes the position of a UIView in the
layer tree.
The iOS platform_views system channel client code in services/platform_views.dart
Splits the fake platform views controller to an Android and iOS
controllers.

TBD in following PRs:

Plumb the layout direction all the way to the platform view (currently
there is still no engine support for it).
Integrate with gesture arenas (engine support is still missing as
well).

#19030

This PR adds the full framework stack (layer->render object->widget, and service) for embedding iOS views with minimal functionality. I allowed myself to throw the entire framework stack in one PR since we're mostly mirroring the structure we already established for embedded Android views, so this PR is a little bigger than usual. I'm happy to break it down to the different pieces of the stack if reviewers prefer. Specifically this PR adds: * A UiKitView widget for embedding a UIView in the widget tree. * A RenderUiKitView which is the render object for showing a UIView. * A PlatformViewLayer which denotes the position of a UIView in the layer tree. * The iOS platform_views system channel client code in services/platform_views.dart * Splits the fake platform views controller to an Android and iOS controllers. TBD in following PRs: * Plumb the layout direction all the way to the platform view (currently there is still no engine support for it). * Integrate with gesture arenas (engine support is still missing as well).

xster · 2018-10-23T23:53:51Z

 /// [RenderAndroidView] is responsible for sizing, displaying and passing touch events to an
 /// Android [View](https://developer.android.com/reference/android/view/View).
 ///
+/// {@template platform_render_layout}


Be more specific about namespacing. See other usages of @template

xster · 2018-10-23T23:53:55Z


+/// A layer that shows an embedded [UIView](https://developer.apple.com/documentation/uikit/uiview)
+/// on iOS.
+class PlatformViewLayer extends Layer {


Are we ever going to give a view id to fuchsia and non-iOS to compose? I worry that we'll add actual iOS specific stuff to this class later on and dilute the 'PlatformView' nomenclature since we've been fairly good at calling Android things RenderAndroidView etc.

I can imagine this getting used for non-iOS use-cases.
Also note that in the engine we abstracted by the platform-agnostic Surface.

xster · 2018-10-23T23:54:24Z

+    @required this.viewId,
+  }): assert(rect != null), assert(viewId != null);
+
+  /// Bounding rectangle of this layer.


From whose perspective?

Mentioned it's in the global coordinate space, hope that clarifies?

xster · 2018-10-23T23:56:05Z

+///
+/// [RenderUiKitView] is responsible for sizing and displaying an iOS
+/// [UIView](https://developer.apple.com/documentation/uikit/uiview).
+///


Add more details about hit testing and how it delegates painting.

Added a note about how the UIViews are composited (this is still churning so I'm purposely keeping it pretty abstract here).

Hit testing is explained on PlatformViewHitTestBehavior, skimming through proxy_box.dart I see that this is the common practice (e.g we don't normally describe hit testing behavior in the render objects class docs).

Right now touch support is not yet implemented, I'm planning to add some docs about it when adding the support.

xster · 2018-10-23T23:58:43Z

+       _viewId = viewId;
+
+
+  /// The unique identifier of the UIView controlled by this controller.


Cross reference platform view services here again?

xster · 2018-10-24T00:22:14Z

+  bool _initialized = false;
+
+  static final Set<Factory<OneSequenceGestureRecognizer>> _emptyRecognizersSet =
+  Set<Factory<OneSequenceGestureRecognizer>>();


xster · 2018-10-24T00:22:50Z

+  @override
+  Widget build(BuildContext context) {
+    if (_controller == null) {
+      return const SizedBox.expand();


Should we allow for a custom specifiable placeholder for android and uikitview?

That's probably useful, though I'd keep it for a future PR, this one is already doing too many things.

xster · 2018-10-24T00:24:42Z

+      return;
+    }
+    _initialized = true;
+    _layoutDirection = _findLayoutDirection();


I don't think you need this one. Since didChangeDependencies is always called and it always does this anyway.

We need _layoutDirection set before calling _createNewUiKitView for the fist time...

Still feel like there's some redundancy. Maybe move lines 339-341 2 lines up?

Good suggestion, done, it's cleaner now.

xster · 2018-10-24T00:26:22Z

+    if (widget.onPlatformViewCreated != null) {
+      widget.onPlatformViewCreated(_id);
+    }
+    setState(() {});


final local controller and setState(() { _controller = controller; }); just to be explicit? Or at least add some docs.

xster · 2018-10-24T00:27:21Z

  }
 }
+
+class _UiKitPlatformView extends LeafRenderObjectWidget {


I think the convention is calling these XRenderObjectWidget (if you global search for extends LeafRenderObjectWidget)

Doesn't seem to be a convention...

$ git grep "extends LeafRenderObject" | wc -l 23 $ git grep "RenderObjectWidget extends LeafRenderObject" | wc -l 6 $ git grep "extends LeafRenderObject" ... lib/src/widgets/basic.dart:class RichText extends LeafRenderObjectWidget { lib/src/widgets/basic.dart:class RawImage extends LeafRenderObjectWidget { lib/src/widgets/basic.dart:class WidgetToRenderBoxAdapter extends LeafRenderObjectWidget { lib/src/widgets/editable_text.dart:class _Editable extends LeafRenderObjectWidget { lib/src/widgets/framework.dart:class ErrorWidget extends LeafRenderObjectWidget { lib/src/widgets/performance_overlay.dart:class PerformanceOverlay extends LeafRenderObjectWidget { ...

Hixie · 2018-10-24T17:23:19Z

+  /// Sets the layout direction for the Android view.
+  Future<void> setLayoutDirection(TextDirection layoutDirection) async {
+    if (_disposed)
+      throw FlutterError('trying to set a layout direction for a disposed Android View. View id: $id');


i would only check this in debug mode

Hixie · 2018-10-24T17:23:34Z

+  /// The unique identifier of the iOS view controlled by this controller.
+  final int id;
+
+  bool _disposed = false;


i would call this _debugDisposed and only track it in debug mode

Hixie · 2018-10-24T17:24:46Z

+  ///
+  /// {@macro platform_view_constructor_params}
+  UiKitView({ // ignore: prefer_const_constructors_in_immutables
+    // TODO(aam): Remove lint ignore above once dartbug.com/34297 is fixed


can you make that link a full link (https://... etc)? That way it comes up in searches for links when I do audits. Thanks.

Hixie · 2018-10-24T17:25:12Z

+  final TextDirection layoutDirection;
+
+  @override
+  State createState() => _UiKitViewState();


State<UiKitView> or _UiKitViewState, not just State.

Hixie · 2018-10-24T17:26:53Z

+      viewType: widget.viewType,
+      layoutDirection: _layoutDirection,
+    );
+    if (widget.onPlatformViewCreated != null) {


since this is async, you might get disposed before initUiKitView returns. You should check mounted is true here and return early if it is not.

Good catch!
Fixed and added a unit test.

Hixie · 2018-10-24T17:27:12Z

+    if (widget.onPlatformViewCreated != null) {
+      widget.onPlatformViewCreated(_id);
+    }
+    setState(() {});


always put a comment in empty setState blocks explaining what state changed.

changed to be non empty setState.

Hixie · 2018-10-24T17:27:19Z

+
+  @override
+  void dispose() {
+    _controller.dispose();


_controller might be null here

fixed (and tested)

Hixie · 2018-10-24T17:28:47Z

+    _layoutDirection = newLayoutDirection;
+
+    if (widget.viewType != oldWidget.viewType) {
+      _controller.dispose();


_controller might be null here

amirh

Thanks @xster ! PTAL

amirh · 2018-10-24T16:36:26Z

 /// apply to it as well.
 ///
-/// The widget fill all available space, the parent of this object must provide bounded layout
+/// {@template platform_view_layout}


amirh · 2018-10-24T16:36:29Z

 /// ```
 ///
-/// The Android view's lifetime is the same as the lifetime of the [State] object for this widget.
+/// {@template platform_view_lifetime}


amirh · 2018-10-24T16:37:10Z


  /// The codec used to encode `creationParams` before sending it to the
-  /// platform side. It should match the codec passed to the constructor of [PlatformViewFactory](/javadoc/io/flutter/plugin/platform/PlatformViewFactory.html#PlatformViewFactory-io.flutter.plugin.common.MessageCodec-).
+  /// platform side.It should match the codec passed to the constructor of [PlatformViewFactory](/javadoc/io/flutter/plugin/platform/PlatformViewFactory.html#PlatformViewFactory-io.flutter.plugin.common.MessageCodec-).


amirh · 2018-10-24T16:37:24Z

+  bool _initialized = false;
+
+  static final Set<Factory<OneSequenceGestureRecognizer>> _emptyRecognizersSet =
+  Set<Factory<OneSequenceGestureRecognizer>>();


amirh · 2018-10-24T16:38:21Z

+  @override
+  Widget build(BuildContext context) {
+    if (_controller == null) {
+      return const SizedBox.expand();


That's probably useful, though I'd keep it for a future PR, this one is already doing too many things.

amirh · 2018-10-24T17:21:44Z

+    TextDirection layoutDirection,
+  ) : assert(id != null),
+      assert(layoutDirection != null),
+      _layoutDirection = layoutDirection;


The layoutDirection will be passed as an argument of the create call (this is not yet implemented in the engine).

amirh · 2018-10-24T17:23:05Z

+      _layoutDirection = layoutDirection;
+
+
+  /// The unique identifier of the iOS view controlled by this controller.


amirh · 2018-10-24T17:23:51Z

+  /// Sets the layout direction for the Android view.
+  Future<void> setLayoutDirection(TextDirection layoutDirection) async {
+    if (_disposed)
+      throw FlutterError('trying to set a layout direction for a disposed Android View. View id: $id');


amirh · 2018-10-24T17:24:17Z

+  /// Disposes the view.
+  ///
+  /// The [UiKitViewController] object is unusable after calling this.
+  /// The identifier of the platform view cannot be reused after the view is


amirh · 2018-10-24T17:26:11Z

+///
+/// Embedding iOS views is an expensive operation and should be avoided when a Flutter
+/// equivalent is possible.
+///


Added a TODO, there's going to be some churn in the way it works with the incremental progress on the engine side, I'll describe it here once it settled.

Hixie · 2018-10-24T17:28:58Z

+    }
+
+    if (didChangeLayoutDirection) {
+      _controller.setLayoutDirection(_layoutDirection);


_controller might be null here

Hixie · 2018-10-24T17:49:38Z

+    if (didChangeLayoutDirection) {
+      // The native view will update asynchronously, in the meantime we don't want
+      // to block the framework. (so this is intentionally not awaiting).
+      _controller.setLayoutDirection(_layoutDirection);


_controller might be null here

Hixie · 2018-10-24T17:50:02Z

+      RenderUiKitView(
+        viewId: viewId,
+        hitTestBehavior: hitTestBehavior,
+      );


nit: avoid line breaks in => expressions

Hixie · 2018-10-24T17:50:14Z

+      return false;
+    final FakeIosPlatformView typedOther = other;
+    return id == typedOther.id &&
+        type == typedOther.type;


nit: indent

Hixie · 2018-10-24T17:50:29Z

+  }
+}
+
+class FakeIosPlatformView {


/me peers suspiciously at the "Ios"

Renamed to FakeUiKitView

Hixie · 2018-10-24T17:51:07Z

+
+  @override
+  bool operator ==(dynamic other) {
+    if (other is! FakeIosPlatformView)


compare runtimeType instead of using is in operator==, otherwise subclasses will have asymmetric == operators

done (here and in FakeAndroidPlatformView)

Hixie · 2018-10-24T17:52:22Z


-    expect(factoryInvocationCount, 1);
  });
 }


consider adding tests that hit the "_controller is null" side of everywhere that you end up testing whether _controller is null (doing so will involve creating a bunch of interesting tests that test edge cases of things changing at various times, disposing early, etc, which is generally valuable)

added a 'disposeUIView before creation completed' test case

amirh · 2018-10-24T18:08:17Z

Thanks @Hixie ! PTAL

xster

LGTM

xster · 2018-10-24T18:35:51Z

+      return;
+    }
+    _initialized = true;
+    _layoutDirection = _findLayoutDirection();


Still feel like there's some redundancy. Maybe move lines 339-341 2 lines up?

xster · 2018-10-24T18:36:44Z

+}
+
+class FakeAndroidPlatformView {
+


nit: symmetrical white spaces

xster · 2018-10-24T18:36:47Z

+}
+
+class FakeUiKitView {
+


xster · 2018-10-24T18:37:21Z

+        layoutDirection: TextDirection.ltr,
+      );
+      expect(
+              () =>


some formatting issues here

reformatted with dartfmt. (and it's weird)

xster · 2018-10-24T18:37:45Z

+          unorderedEquals(<FakeUiKitView>[
+            FakeUiKitView(0, 'webview'),
+            FakeUiKitView(1, 'webview'),
+          ]));


trailing commas might get the formatter to return something cleaner

amirh

Thanks, I'll wait for some relevant engine stuff to land before landing this.

amirh · 2018-10-24T19:10:55Z

+      return;
+    }
+    _initialized = true;
+    _layoutDirection = _findLayoutDirection();


Good suggestion, done, it's cleaner now.

amirh · 2018-10-24T19:13:13Z

+}
+
+class FakeAndroidPlatformView {
+


amirh · 2018-10-24T19:13:34Z

+}
+
+class FakeUiKitView {
+


amirh · 2018-10-24T19:15:41Z

+        layoutDirection: TextDirection.ltr,
+      );
+      expect(
+              () =>


reformatted with dartfmt. (and it's weird)

amirh · 2018-10-24T19:17:17Z

+          unorderedEquals(<FakeUiKitView>[
+            FakeUiKitView(0, 'webview'),
+            FakeUiKitView(1, 'webview'),
+          ]));


This reverts commit 67ffe1c.

…23779) This reverts commit 67ffe1c.

…3412)" (#23779)" This reverts commit b20e7a2.

Re-landing #23412 with a fix to the PlatformLayer's addToScene signature. #23412 was broken by the change to Layer done in #23434. It seemed green as the presubmits were done before #23434 was landed, and when #23412 landed it broke the build. Reverts #23779

This PR adds the full framework stack (layer->render object->widget, and service) for embedding iOS views with minimal functionality. I allowed myself to throw the entire framework stack in one PR since we're mostly mirroring the structure we already established for embedded Android views, so this PR is a little bigger than usual. I'm happy to break it down to the different pieces of the stack if reviewers prefer. Specifically this PR adds: * A UiKitView widget for embedding a UIView in the widget tree. * A RenderUiKitView which is the render object for showing a UIView. * A PlatformViewLayer which denotes the position of a UIView in the layer tree. * The iOS platform_views system channel client code in services/platform_views.dart * Splits the fake platform views controller to an Android and iOS controllers. TBD in following PRs: * Plumb the layout direction all the way to the platform view (currently there is still no engine support for it). * Integrate with gesture arenas (engine support is still missing as well).

)" (flutter#23779) This reverts commit 67ffe1c.

…23781) Re-landing flutter#23412 with a fix to the PlatformLayer's addToScene signature. flutter#23412 was broken by the change to Layer done in flutter#23434. It seemed green as the presubmits were done before flutter#23434 was landed, and when flutter#23412 landed it broke the build. Reverts flutter#23779

googlebot added the cla: yes label Oct 23, 2018

amirh requested a review from xster October 23, 2018 17:13

xster reviewed Oct 24, 2018

View reviewed changes

Hixie reviewed Oct 24, 2018

View reviewed changes

xster review comments followup

e8d6025

Hixie reviewed Oct 24, 2018

View reviewed changes

amirh commented Oct 24, 2018

View reviewed changes

Hixie reviewed Oct 24, 2018

View reviewed changes

hixie's review comments followup

24491a7

xster reviewed Oct 24, 2018

View reviewed changes

review comments followup

c11a522

amirh commented Oct 24, 2018

View reviewed changes

Add WIP comments

377482d

amirh merged commit 67ffe1c into flutter:master Oct 31, 2018

amirh deleted the uikit_view branch October 31, 2018 13:01

amirh added a commit that referenced this pull request Oct 31, 2018

Revert "Initial framework support for iOS platform views. (#23412)"

66c6f5b

This reverts commit 67ffe1c.

amirh mentioned this pull request Oct 31, 2018

Revert "Initial framework support for iOS platform views." #23779

Merged

amirh added a commit that referenced this pull request Oct 31, 2018

Revert "Initial framework support for iOS platform views. (#23412)" (#…

b20e7a2

…23779) This reverts commit 67ffe1c.

amirh mentioned this pull request Oct 31, 2018

Maintain dirtiness and use retained engine layers #23434

Merged

amirh added a commit that referenced this pull request Oct 31, 2018

Revert "Revert "Initial framework support for iOS platform views. (#2…

c4cee3a

…3412)" (#23779)" This reverts commit b20e7a2.

amirh mentioned this pull request Oct 31, 2018

Re-land "Initial framework support for iOS platform views." #23781

Merged

Xavjer pushed a commit to Xavjer/flutter that referenced this pull request Nov 1, 2018

Revert "Initial framework support for iOS platform views. (flutter#23412

c3ad27b

)" (flutter#23779) This reverts commit 67ffe1c.

github-actions Bot locked as resolved and limited conversation to collaborators Aug 9, 2021

		_viewId = viewId;


		/// The unique identifier of the UIView controlled by this controller.

		_layoutDirection = layoutDirection;


		/// The unique identifier of the iOS view controlled by this controller.

Conversation

amirh commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh commented Oct 23, 2018 •

edited

Loading