[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing #38981

dkwingsmt · 2023-01-18T17:41:50Z

This PR adds FlutterViewController.id, and formalizes how FlutterViewController is initialized by FlutterEngine:

When FlutterViewController is set to the engine, the engine calls FlutterViewController#attachToEngine:withId:.
When FlutterViewController is unset from the engine, the engine calls FlutterViewController#detachFromEngine.

This allows the ID to be generated by the engine.

The default View ID is now 1 instead of 0. In this way, we can use 0 as an empty value, the value for FlutterViewController.id when the FVC is not attached to an engine. (IDs are meant to be opaque, and views should be a map, not an array anyway.)
This PR moves almost all initialization code into FVC's CommonInit to ensure that all init path produces the expected result.
With this PR, FlutterEngine no long supports replacing viewController (from non-null to a different non-null). Setting the view controller (from null to non-null) is intended to allow pre-warming an engine before displaying it in add-to-app, but replacing the view controller does not make much sense.

If the developer wants to persist the state of the view between different windows, they could just attach the view controller to different windows.
In fact, Flutter doesn't even have a way to create a FlutterViewController that is unattached to an engine. All initializers either require an engine or create an engine.

As a result, unit tests that used to create FVC with the wrong initializer are fixed.

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.

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

loic-sharma · 2023-01-19T18:55:42Z

shell/platform/darwin/macos/framework/Source/FlutterView.h

@@ -18,7 +18,7 @@
 * backward compatibility, single-view APIs will always operate the view with
 * this ID. Also, the first view assigned to the engine will also have this ID.
 */
-constexpr uint64_t kFlutterDefaultViewId = 0;
+constexpr uint64_t kFlutterDefaultViewId = 1;


Just so this comment isn't lost: #38855 (comment)

Consider double-checking with @goderbauer that the framework does not have any expectations that the default view has an ID of 0.

The framework makes no assumption about the value of the id. Why does this have to change from 0 to 1, though?

loic-sharma

LGTM, but please get sign-off from folks that are more familiar with the macOS embedder

a-wallen

LGTM

shell/platform/darwin/macos/framework/Headers/FlutterEngine.h

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

goderbauer · 2023-01-24T00:31:16Z

shell/platform/darwin/macos/framework/Source/FlutterView.h

@@ -18,7 +18,7 @@
 * backward compatibility, single-view APIs will always operate the view with
 * this ID. Also, the first view assigned to the engine will also have this ID.
 */
-constexpr uint64_t kFlutterDefaultViewId = 0;
+constexpr uint64_t kFlutterDefaultViewId = 1;


The framework makes no assumption about the value of the id. Why does this have to change from 0 to 1, though?

shell/platform/darwin/macos/framework/Source/FlutterViewController.mm

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

cbracken · 2023-01-24T18:42:04Z

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

+/**
+ * The identifier for this view controller.
+ *
+ * The ID is assigned when the view controller is added to FlutterEngine.


Maybe expand on this, like:

Suggested change

* The ID is assigned when the view controller is added to FlutterEngine.

* The ID is assigned when the view controller is assigned to the

* FlutterEngine.viewController property.

The mechanism is a little more complicated than this, so I decided to explain everything in FlutterViewController's doc. Can you take a look?

shell/platform/darwin/macos/framework/Headers/FlutterViewController.h

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

dkwingsmt · 2023-01-25T20:40:40Z

@cbracken @goderbauer I should have addressed all your comments. Can you take another look?

goderbauer

LGTM

goderbauer · 2023-01-25T21:19:14Z

shell/platform/darwin/macos/framework/Headers/FlutterEngine.h

+ * Some single-view APIs will eventually be replaced by their multi-view
+ * variant. During the deprecation period, the single-view APIs will coexist with
+ * and work with the multi-view APIs as if the other views don't exist.  For
+ * backward compatibility, single-view APIs will always operate the view with


nit: operate on (or something like that)?

…, and prohibit replacing (#38981)" This reverts commit e3b2782.

…, and prohibit replacing (#38981)" (#39144) This reverts commit e3b2782.

…ion flow, and prohibit replacing (flutter#38981)" (flutter#39144)" This reverts commit 36db005.

…, and prohibit replacing" (#39145) * Revert "Revert "[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing (#38981)" (#39144)" This reverts commit 36db005. * Fix?

…ow, and prohibit replacing (flutter/engine#38981)

…119208) * e3b278239 [macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing (flutter/engine#38981) * 36db005dd Revert "[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing (#38981)" (flutter/engine#39144) * a20609120 Roll buildroot (flutter/engine#39141) * 8f1e5dc1b Reland "[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing" (flutter/engine#39145)

dkwingsmt added 2 commits January 18, 2023 09:35

Impl

48a4c66

Format

7f10f9d

flutter-dashboard bot added the platform-macos label Jan 18, 2023

dkwingsmt added 4 commits January 18, 2023 10:24

Fix provider

1d02bc9

Uncomment test

dc01584

Format

fa5ccc0

Fix all tests, banning running engine

764a157

dkwingsmt requested review from goderbauer and cbracken January 18, 2023 18:48

dkwingsmt mentioned this pull request Jan 18, 2023

[macOS] Move A11yBridge to FVC #38855

Merged

8 tasks

dkwingsmt requested review from a-wallen and loic-sharma January 18, 2023 20:14

loic-sharma reviewed Jan 19, 2023

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm Outdated Show resolved Hide resolved

loic-sharma reviewed Jan 19, 2023

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm Show resolved Hide resolved

loic-sharma reviewed Jan 19, 2023

View reviewed changes

loic-sharma approved these changes Jan 19, 2023

View reviewed changes

Doc

c74f450

a-wallen approved these changes Jan 19, 2023

View reviewed changes

Better assertion

320312b

dkwingsmt self-assigned this Jan 19, 2023

goderbauer reviewed Jan 24, 2023

View reviewed changes

cbracken reviewed Jan 24, 2023

View reviewed changes

dkwingsmt added 2 commits January 24, 2023 11:34

Comment

e47f697

Assert id

570fca3

dkwingsmt added 2 commits January 25, 2023 12:52

better const

2c47d71

Move const to engine

4c3ecb2

goderbauer approved these changes Jan 25, 2023

View reviewed changes

dkwingsmt merged commit e3b2782 into flutter:main Jan 25, 2023

dkwingsmt added a commit that referenced this pull request Jan 25, 2023

Revert "[macOS] Formalize FlutterViewController's initialization flow…

4ba3381

…, and prohibit replacing (#38981)" This reverts commit e3b2782.

dkwingsmt mentioned this pull request Jan 25, 2023

Revert "[macOS] Formalize FlutterViewController's initialization flow… #39144

Merged

8 tasks

dkwingsmt added a commit that referenced this pull request Jan 25, 2023

Revert "[macOS] Formalize FlutterViewController's initialization flow…

36db005

…, and prohibit replacing (#38981)" (#39144) This reverts commit e3b2782.

dkwingsmt added a commit to dkwingsmt/engine that referenced this pull request Jan 25, 2023

Revert "Revert "[macOS] Formalize FlutterViewController's initializat…

7a47987

…ion flow, and prohibit replacing (flutter#38981)" (flutter#39144)" This reverts commit 36db005.

dkwingsmt mentioned this pull request Jan 25, 2023

Reland "[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing" #39145

Merged

8 tasks

engine-flutter-autoroll mentioned this pull request Jan 26, 2023

Roll Flutter Engine from 308ce918f67f to 8f1e5dc1b124 (4 revisions) flutter/flutter#119208

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2023

e3b278239 [macOS] Formalize FlutterViewController's initialization fl…

80bda21

…ow, and prohibit replacing (flutter/engine#38981)

cbracken mentioned this pull request Mar 15, 2023

[macOS] Rename FlutterViewController.id to viewId #40323

Merged

8 tasks

dkwingsmt deleted the add-fvc-id branch June 30, 2023 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing #38981

[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing #38981

dkwingsmt commented Jan 18, 2023 •

edited

loic-sharma Jan 19, 2023

goderbauer Jan 24, 2023

loic-sharma left a comment

a-wallen left a comment

goderbauer Jan 24, 2023

cbracken Jan 24, 2023

dkwingsmt Jan 24, 2023

dkwingsmt commented Jan 25, 2023

goderbauer left a comment

goderbauer Jan 25, 2023

	* The ID is assigned when the view controller is added to FlutterEngine.
	* The ID is assigned when the view controller is assigned to the
	* FlutterEngine.viewController property.

[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing #38981

[macOS] Formalize FlutterViewController's initialization flow, and prohibit replacing #38981

Conversation

dkwingsmt commented Jan 18, 2023 • edited

Pre-launch Checklist

loic-sharma Jan 19, 2023

Choose a reason for hiding this comment

goderbauer Jan 24, 2023

Choose a reason for hiding this comment

loic-sharma left a comment

Choose a reason for hiding this comment

a-wallen left a comment

Choose a reason for hiding this comment

goderbauer Jan 24, 2023

Choose a reason for hiding this comment

cbracken Jan 24, 2023

Choose a reason for hiding this comment

dkwingsmt Jan 24, 2023

Choose a reason for hiding this comment

dkwingsmt commented Jan 25, 2023

goderbauer left a comment

Choose a reason for hiding this comment

goderbauer Jan 25, 2023

Choose a reason for hiding this comment

dkwingsmt commented Jan 18, 2023 •

edited