[macos] expose runWithEngine constructor in FlutterViewController (#7… #36410

sergiy-sc · 2022-09-26T09:36:41Z

…4735)

Exposes runWithEngine in FlutterViewController as suggested by jmagman in #74735
Fixes: flutter/flutter#74735

Removing [engine setViewController:self]; because view is not loaded, but in that function controller is saved. Later, when the view is loaded (loadView) passed as an argument controller is checked against the saved controller and since they match, the nil view is not replaced with a new view.

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.

…4735)

google-cla · 2022-09-26T09:36:45Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

jmagman · 2022-09-26T17:28:07Z

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

+  } @catch (...) {
+    return false;
+  }


(note for @cbracken not @dubik) I know this pattern was copied from the other tests in this file but why are these tests returning bool (it's not even BOOL...) instead of failing in place? For example this @catch hides the details of any exception instead of letting XCTest fail the test with the actual thing that happened.

That's an excellent question that'll involve some archaeology. We should leave this for now and I'll follow-up with a cleanup PR once I figure out what the history of this was.

zanderso · 2022-09-29T20:17:31Z

From PR review triage: It looks like this PR needs feedback from @cbracken.

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

dkwingsmt · 2022-09-29T21:49:38Z

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

    }
    _engine = engine;
    CommonInit(self);
-    [engine setViewController:self];


Bypassing the initialization logic by using .viewController = instead of setViewController seems very hacky. I'm surprised that viewController is not readonly (I suggest we make it so).

It feels to me that this PR complexes the relationship between FVC and FlutterEngine, which can be seen from the three initializeKeyboard calls throughout the file. It's hard to track how many ways are there we expect to use FVC and whether a line is executed by each, as well as how to add a new line to the initialization.

Commenting only about

Bypassing the initialization logic by using .viewController = instead of setViewController seems very hacky.

That's just Objective-C, they are completely identical under the covers. The dot notation .viewController = is a more idiomatic way to set a property.

It feels to me that this PR complexes the relationship

It does. When the existing engine is passed, it can be running or not. So that increases the complexity of the initializer and the patch adds basically one if which loads FlutterView and initializes the keyboard if it's running. So to me the added complexity doesn't look excessive.

It's hard to track how many ways are there we expect to use FVC and whether a line is executed by each, as well as how to add a new line to the initialization.

There is CommonInit and all initializers use it. initWithEngine handles the case when the engine is running and I could extract that into the method, but I couldn't come up with a good name and it doesn't clarify anything in that context.

three initializeKeyboard calls

To me, the story with the keyboard is rather complicated. There are quite a few cases to handle, and I'm not sure controller is the best place for that. There is a TODO comment in initializeKeyboard which I guess, could solve some of the problems. But fixing that TODO goes way beyond my capabilities/knowledge of the engine's internals.

Engine is not running, then FVC is created (displaying FVC will start the engine)

Engine is running, then FVC is created

Engine is not running, then FVC is created, and the engine is started by the user, and later FVC is shown (not handled by this patch, but viewWillAppear perhaps should check if the keyboard was correctly configured before)

One solution could be that the engine would fire an event when it's started, and FVC would subscribe to that. This can simplify keyboard initialization for cases 1. and 3.

chinmaygarde · 2022-10-06T20:18:55Z

From Triage: Looks like the conversation here didn't come to a clear conclusion on direction. Are we still making progress on this?

sergiy-sc · 2022-10-06T20:36:49Z

@chinmaygarde are you asking the reviewers or me?

chinmaygarde · 2022-10-06T21:31:45Z

Both I guess. Couldn't tell if there was consensus to make progress on this patch.

cbracken

Overall this approach lgtm; we're going to want to add a devicelab test for this in the framework as well, which we'll need to do in a separate patch.

I necessarily agree this makes the code much more complex than it already is, but agree we should do some work to simplify what's already there. It does add one (really two: already running engine, not yet running engine) additional cases to consider for maintainers of the code.

I'll play around with this a bit this evening the finish up the review. Thanks again for the patch.

jmagman · 2022-10-07T16:41:37Z

we're going to want to add a devicelab test for this in the framework as well, which we'll need to do in a separate patch.

It could look like module_test_ios but using flutter build macos-framework integration and need an example app like
https://github.com/flutter/samples/tree/main/add_to_app/prebuilt_module

cbracken

Thanks for your patience - a lot of us were in meetings all week. Spent some time trying to break this and it lgtm :)

[macos] expose runWithEngine constructor in FlutterViewController (#7…

b13d4c7

…4735)

flutter-dashboard bot added the platform-macos label Sep 26, 2022

dubik mentioned this pull request Sep 26, 2022

[macos] expose runWithEngine constructor in FlutterViewController (#7… #35869

Closed

8 tasks

jmagman requested a review from cbracken September 26, 2022 17:24

jmagman reviewed Sep 26, 2022

View reviewed changes

jmagman mentioned this pull request Sep 27, 2022

Expose macOS -[FlutterViewController initWithEngine:::] in header #34012

Closed

8 tasks

dkwingsmt reviewed Sep 29, 2022

View reviewed changes

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

dkwingsmt reviewed Sep 29, 2022

View reviewed changes

[macos] Added an assert when FlutterEngine already bound to a controller

deeae0a

cbracken reviewed Oct 7, 2022

View reviewed changes

cbracken approved these changes Oct 14, 2022

View reviewed changes

cbracken merged commit 578279f into flutter:main Oct 14, 2022

This was referenced Jan 11, 2023

[macOS] Remove usages of FlutterViewController#loadView #38792

Closed

[macOS] Move A11yBridge to FVC #38855

Merged

[macos] expose runWithEngine constructor in FlutterViewController (#7… #36410

[macos] expose runWithEngine constructor in FlutterViewController (#7… #36410

Uh oh!

Conversation

sergiy-sc commented Sep 26, 2022

Pre-launch Checklist

Uh oh!

google-cla bot commented Sep 26, 2022

Uh oh!

jmagman Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

cbracken Oct 7, 2022

Choose a reason for hiding this comment

Uh oh!

zanderso commented Sep 29, 2022

Uh oh!

Uh oh!

dkwingsmt Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

sergiy-sc Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

sergiy-sc Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Oct 6, 2022

Uh oh!

sergiy-sc commented Oct 6, 2022

Uh oh!

chinmaygarde commented Oct 6, 2022

Uh oh!

cbracken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dkwingsmt Sep 29, 2022 •

edited

Loading

cbracken left a comment •

edited

Loading

jmagman commented Oct 7, 2022 •

edited

Loading