Extract common functionality of iOS platformviews into superclasses #128716

yaakovschectman · 2023-06-12T17:43:59Z

Move most functionality of UiKitView and its supporting classes into superclasses named DarwinPlatformView, etc., and create trivial or near-trivial subclasses with the same names as the old classes.

I am currently awaiting approval for a macOS workstation that would allow me to run the iOS/macOS tests and make sure all existing functionality is preserved by this refactor. I can ensure that tests will pass, but doing so may need to wait for a while.

Addresses Add AppKitView

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

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

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

goderbauer

For my own understanding: The motivation for this refactoring is to add a AppKitPlatformView later that will reuse the DarwinPlatformView machinery?

goderbauer · 2023-06-12T18:16:52Z

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

-///  * [PlatformViewsService] which is a service for controlling platform views.
-class RenderUiKitView extends RenderBox {
-  /// Creates a render object for an iOS UIView.
+/// Superclass for common fucntionality of platform view RenderBoxes


Can you add a little more color to this doc? See also our style guide for writing prompts: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#writing-prompts-for-good-documentation

Questions to answer:

What is the common functionality that this provides?

What responsibilities are left to the subclasses?

Can this class be use directly? (it's documented as a super class, but it isn't abstract, so what do I get if I instantiate it directly?)

Document what the generic type parameter T is

What responsibilities are left to the subclasses?

Looks like at least gesture handling needs to be provided by the sub class?

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

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

goderbauer · 2023-06-12T18:39:37Z

Some checks are failing.

Co-authored-by: Loïc Sharma <[email protected]>

Co-authored-by: Michael Goderbauer <[email protected]>

cbracken · 2023-06-12T23:02:22Z

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

+  }
+
+  /// {@macro flutter.rendering.PlatformViewRenderBox.updateGestureRecognizers}
+  void updateGestureRecognizers(Set<Factory<OneSequenceGestureRecognizer>> gestureRecognizers) {


Gesture handling is not yet supported on macOS, but will likely be within the next week or two. Given that, perhaps we should push this up to the superclass now rather than later. API-wise this is pretty safe to support even before we actually have the embedder side work done I think. (We're cheating and testing by using UIKitView already as-is).

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

yaakovschectman · 2023-06-13T18:08:30Z

@cbracken

perhaps we should push this up to the superclass now rather than later

Because the constructor for the gesture recognizer takes a UiKitViewController parameter, this would require either all RenderDarwinPlatformView to hold a UiKitViewController, or update _UiKitViewGestureRecognizer to take the superclass DarwinPlatformViewController instead.

flutter-dashboard · 2023-06-28T20:08:30Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Co-authored-by: Michael Goderbauer <[email protected]>

cbracken

lgtm.

Summing up a discussion over chat re: composition vs inheritance. API-wise, a composition-based approach affords a tiny bit more future wiggle room should UIKit and AppKit based subclasses ever significantly diverge, since it's (relatively) easy to add a superclass that wasn't there, but (relatively) breaking to remove one that was. Further, composition avoids inserting a superclass that doesn't serve a significant conceptual purpose API-wise (users are likely to assign UIViews to PlatformViews but not likely to need a DarwinView specifically. However, composition involves duplicating a ton of API (across all the layers of the framework) and having dozens of calls out to some common private delegate. Overall, we think it's very unlikely that our UIKit and AppKit platformview classes diverge very significantly, and inheritance save a significant amount of code.

Hixie · 2023-07-17T21:40:49Z

test-exempt: code refactor with no semantic change

…lasses (flutter/flutter#128716)

Roll Flutter from 6f09064 to d07e8ae (60 revisions) flutter/flutter@6f09064...d07e8ae 2023-07-19 [email protected] Roll Flutter Engine from 09389b16d684 to eb2285205f25 (3 revisions) (flutter/flutter#130879) 2023-07-19 [email protected] Roll Flutter Engine from 0293a7cb7887 to 09389b16d684 (5 revisions) (flutter/flutter#130867) 2023-07-19 [email protected] Move TapAndDragGestureRecognizer code under gestures (flutter/flutter#119508) 2023-07-19 [email protected] Roll Flutter Engine from 29de67c7d009 to 0293a7cb7887 (1 revision) (flutter/flutter#130858) 2023-07-19 [email protected] Roll Flutter Engine from 39d60be72ffb to 29de67c7d009 (4 revisions) (flutter/flutter#130855) 2023-07-19 [email protected] Roll Flutter Engine from b3bfc744bb61 to 39d60be72ffb (1 revision) (flutter/flutter#130847) 2023-07-19 [email protected] Roll Flutter Engine from adf6142f6738 to b3bfc744bb61 (7 revisions) (flutter/flutter#130843) 2023-07-18 [email protected] Update app_builder_test.dart for M3 (flutter/flutter#130794) 2023-07-18 [email protected] Catch errors in loadStructuredData (flutter/flutter#130748) 2023-07-18 [email protected] Fix super tiny space formatting (hope we have auto formatter in the future) (flutter/flutter#127479) 2023-07-18 [email protected] Roll Flutter Engine from 71bbecee3010 to adf6142f6738 (2 revisions) (flutter/flutter#130831) 2023-07-18 [email protected] Update SnackBar tests for M2/M3 (flutter/flutter#130717) 2023-07-18 [email protected] Roll pub packages (flutter/flutter#130821) 2023-07-18 [email protected] Relax syntax for gen-l10n (flutter/flutter#130736) 2023-07-18 [email protected] Roll Flutter Engine from 45851af55bd6 to 71bbecee3010 (7 revisions) (flutter/flutter#130820) 2023-07-18 [email protected] Roll pub packages (flutter/flutter#130608) 2023-07-18 [email protected] Roll Flutter Engine from 831da7e9dc3b to 45851af55bd6 (2 revisions) (flutter/flutter#130814) 2023-07-18 [email protected] [Android] Deletes deprecated splash screen meta-data element (flutter/flutter#130744) 2023-07-18 [email protected] Updated `ThemeData.useMaterial3` API doc, default is `true` (flutter/flutter#130764) 2023-07-18 [email protected] [labeler] Mark sync-labels as empty (flutter/flutter#130642) 2023-07-18 [email protected] Roll Flutter Engine from c27658cc5ade to 831da7e9dc3b (2 revisions) (flutter/flutter#130810) 2023-07-18 [email protected] Roll Packages from 6889cca to 3e8b813 (9 revisions) (flutter/flutter#130802) 2023-07-18 [email protected] Update `AppBar` and `AppBarTheme` tests for M2/M3 (flutter/flutter#130790) 2023-07-18 [email protected] Update app tests for M3 (flutter/flutter#130792) 2023-07-18 [email protected] Add lint check to make sure samples are linked and have tests (flutter/flutter#130523) 2023-07-18 [email protected] Roll Flutter Engine from aaec42812a1f to c27658cc5ade (2 revisions) (flutter/flutter#130799) 2023-07-18 [email protected] Extract common functionality of iOS platformviews into superclasses (flutter/flutter#128716) 2023-07-18 [email protected] Roll Flutter Engine from 88be39be7b07 to aaec42812a1f (1 revision) (flutter/flutter#130787) 2023-07-18 [email protected] Roll Flutter Engine from b46a8baf8ed9 to 88be39be7b07 (1 revision) (flutter/flutter#130784) 2023-07-18 [email protected] Roll Flutter Engine from 777fe158f4e7 to b46a8baf8ed9 (1 revision) (flutter/flutter#130782) 2023-07-18 [email protected] Fix `iconTheme` in `AppBar` doesn't apply custom `Colors.white` in the dark mode for M3 (flutter/flutter#130574) 2023-07-18 [email protected] Roll Flutter Engine from c6e23288db8d to 777fe158f4e7 (2 revisions) (flutter/flutter#130779) 2023-07-18 [email protected] Roll Flutter Engine from 116eedf769be to c6e23288db8d (3 revisions) (flutter/flutter#130778) 2023-07-18 [email protected] Roll Flutter Engine from 9d018f00d687 to 116eedf769be (2 revisions) (flutter/flutter#130774) 2023-07-18 [email protected] Roll Flutter Engine from 77ec92371846 to 9d018f00d687 (1 revision) (flutter/flutter#130772) 2023-07-18 [email protected] Document stack's clipping behaviour better (flutter/flutter#130749) 2023-07-18 [email protected] Roll Flutter Engine from f2958f9229a4 to 77ec92371846 (1 revision) (flutter/flutter#130769) 2023-07-18 [email protected] Roll Flutter Engine from 3cceb705007e to f2958f9229a4 (1 revision) (flutter/flutter#130767) 2023-07-18 [email protected] Roll Flutter Engine from 09689d37e1d6 to 3cceb705007e (2 revisions) (flutter/flutter#130763) 2023-07-18 [email protected] Stabilize hybrid_android_views_integration_test rendering tree (flutter/flutter#130751) 2023-07-17 [email protected] update link to good first issues (flutter/flutter#130759) 2023-07-17 [email protected] Prevent `InputDecorator` from supplying its descendants with non-normalized constraints (flutter/flutter#130460) 2023-07-17 [email protected] Roll Flutter Engine from 15c15fd75743 to 09689d37e1d6 (3 revisions) (flutter/flutter#130758) 2023-07-17 [email protected] [tools/ios_build_ipa] fallback to CFBundleName if CFBundleDisplayName is absent (flutter/flutter#130752) 2023-07-17 [email protected] Resolve TODOs in channels integration test (flutter/flutter#130745) 2023-07-17 [email protected] Roll Flutter Engine from ddbe23b374d8 to 15c15fd75743 (2 revisions) (flutter/flutter#130746) ...

…lutter#128716) Move most functionality of `UiKitView` and its supporting classes into superclasses named `DarwinPlatformView`, etc., and create trivial or near-trivial subclasses with the same names as the old classes. I am currently awaiting approval for a macOS workstation that would allow me to run the iOS/macOS tests and make sure all existing functionality is preserved by this refactor. I can ensure that tests will pass, but doing so may need to wait for a while. Addresses [Add AppKitView](flutter#128519) ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Loïc Sharma <[email protected]> Co-authored-by: Michael Goderbauer <[email protected]> Co-authored-by: Chris Bracken <[email protected]>

yaakovschectman added 4 commits June 12, 2023 13:40

Start extracting superclasses

fb50318

Shuffle comments

56cfe20

Privatize _DarwinView

0d62247

Clean comment

0f580b5

yaakovschectman added platform-ios iOS applications specifically platform-mac Building on or for macOS specifically a: platform-views Embedding Android/iOS views in Flutter apps a: desktop Running on desktop labels Jun 12, 2023

yaakovschectman requested review from cbracken, cyanglaz, goderbauer, hellohuanlin and loic-sharma June 12, 2023 17:43

yaakovschectman self-assigned this Jun 12, 2023

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. and removed platform-ios iOS applications specifically labels Jun 12, 2023

loic-sharma reviewed Jun 12, 2023

View reviewed changes

packages/flutter/lib/src/rendering/platform_view.dart Outdated Show resolved Hide resolved

loic-sharma reviewed Jun 12, 2023

View reviewed changes

packages/flutter/lib/src/rendering/platform_view.dart Outdated Show resolved Hide resolved

goderbauer reviewed Jun 12, 2023

View reviewed changes

yaakovschectman and others added 4 commits June 12, 2023 15:53

Update packages/flutter/lib/src/rendering/platform_view.dart

41ad0de

Co-authored-by: Loïc Sharma <[email protected]>

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

829d55c

Co-authored-by: Michael Goderbauer <[email protected]>

PR feedback

0e0c5fa

Remove trailing space

aba5532

yaakovschectman marked this pull request as ready for review June 12, 2023 20:49

cbracken reviewed Jun 12, 2023

View reviewed changes

yaakovschectman and others added 2 commits June 13, 2023 10:34

Format and comment

f306282

Merge branch 'master' into extract_platformview

6009d74

yaakovschectman requested a review from cbracken June 15, 2023 14:26

Merge branch 'master' into extract_platformview

d60da93

goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Jul 5, 2023

yaakovschectman requested a review from cbracken July 13, 2023 20:20

Add missing commas

8bb7a84

Co-authored-by: Michael Goderbauer <[email protected]>

cbracken approved these changes Jul 17, 2023

View reviewed changes

Merge branch 'master' into extract_platformview

cf77c1e

yaakovschectman merged commit 203ef6f into flutter:master Jul 18, 2023

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Flutter from c40173f114fa to 203ef6f7a32f (111 revisions) flutter/packages#4503

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

75829a3

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Flutter from c40173f114fa to 3b0b8348fd3e (112 revisions) flutter/packages#4505

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

65f6965

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Flutter from c40173f114fa to 8b768de18f29 (113 revisions) flutter/packages#4506

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

c8fd65e

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Flutter from c40173f114fa to 86385dce684e (114 revisions) flutter/packages#4507

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

72f8f13

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Flutter from c40173f114fa to b460d49199e5 (115 revisions) flutter/packages#4508

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

d37207e

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Manual roll Flutter from c40173f114fa to 86385dce684e (114 revisions) flutter/packages#4509

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

4d196d1

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Flutter from c40173f114fa to 94179a8e3fd3 (116 revisions) flutter/packages#4510

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

ede07fe

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 18, 2023

Roll Flutter from c40173f114fa to fc2626a790ae (117 revisions) flutter/packages#4512

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 18, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

3ee447a

…lasses (flutter/flutter#128716)

engine-flutter-autoroll mentioned this pull request Jul 19, 2023

Roll Flutter from 6f09064e785b to d07e8aece184 (60 revisions) flutter/packages#4522

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 19, 2023

203ef6f Extract common functionality of iOS platformviews into superc…

4303abf

…lasses (flutter/flutter#128716)

Extract common functionality of iOS platformviews into superclasses #128716

Extract common functionality of iOS platformviews into superclasses #128716

Uh oh!

Conversation

yaakovschectman commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

goderbauer Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goderbauer commented Jun 12, 2023

Uh oh!

cbracken Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yaakovschectman commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Jun 28, 2023

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jul 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yaakovschectman commented Jun 12, 2023 •

edited

Loading

yaakovschectman commented Jun 13, 2023 •

edited

Loading