Started bridging the scene delegate to the lifecycle delegate for shortcuts

[gaaclarke]

testing: There integration tests in the plugins repo.
fixes: #169928

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.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

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

[jmagman]

keep the readwrite implementation property as-is, add readonly to this.

[gaaclarke]

I removed the direct usage of the lifecycle delegate and instead started calling directly into the app delegate as we discussed in our meeting. That's a bit more scary but I think it technically is the safer thing to do.

[hellohuanlin]

could you share a bit more details (for me and future readers) on app delegate vs life cycle delegate decision? Thx!

[gaaclarke]

Jenn pointed out that users could be subclassing FlutterAppDelegate and override application:performActionForShortcutItem:completionHandler: and be performing their own logic, like say analytics for the usage of shortcuts.

Talking directly the the lifecycle delegate would break them, but fix the plugin problems. So, I decided to call the delegate directly.

[gaaclarke]

quick actions tested locally (since the test won't run as part of presubmit), it works:

[hellohuanlin]

looks reasonable. can we add some unit tests?

[gaaclarke]

looks reasonable. can we add some unit tests?

We have integration tests. We can't add unit tests because it relies on the application delegate being a FlutterAppDelegate which we don't support in our unit test harness.

[hellohuanlin]

We can't add unit tests because it relies on the application delegate being a FlutterAppDelegate which we don't support in our unit test harness.

Could you explain more? OCMock should be able to mock the app delegate (I may misunderstand the problem tho).

id mockApplication = OCMClassMock([UIApplication class]);
OCMStub([mockApplication shared]).andReturn(mockApplication);
id mockAppDelegate = OCMClassMock([FlutterAppDelegate class]);
OCMStub([mockApplication delegate]).andReturn(mockApplication)

[gaaclarke]

Could you explain more? OCMock should be able to mock the app delegate (I may misunderstand the problem tho).

Thanks, I forgot you could mock like that. Done.

[auto-submit]

autosubmit label was removed for flutter/flutter/170180, because - The status or check suite Linux gradle_plugin_fat_apk_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

[gaaclarke]

rerunning Linux gradle_plugin_fat_apk_test, issue filed

[gaaclarke]

@hellohuanlin friendly ping

[hellohuanlin]

I have some concerns around relaying scene delegate calls to app delegate, mainly the fact that we are changing Apple's behavior that may surprise us in the future. See more details here: #170037 (comment)

If there's no other way around, I'm fine with relaying the calls, but just wanna hear your thoughts on it first.

[hellohuanlin]

If we do it for one method, we should probably do it for all methods for minimal surprise.

[gaaclarke]

This method is special where apple has specifically called out that they will not call the old app delegate method. Things like "will resign active" are still called. The goal of this PR is to fix broken behavior.

[hellohuanlin]

Things like "will resign active" are still called.

IIRC from my experiment a few days ago, "will resign active" are not called

[hellohuanlin]

Same as "background/foreground" callbacks (there are some discussions around it a few days ago in the chat channel)

[hellohuanlin]

Related: #170178

[gaaclarke]

I think we should be test driven. I don't think we have any failing tests related to resign active. We should double check, but I think that should be addressed in a different PR.

[hellohuanlin]

also reported in #170037

[hellohuanlin]

I think that should be addressed in a different PR.

Either way sounds good to me. I just wanna have a consistent behavior - if we relay 1 call, we should relay all similar calls.

[AquaGeek]

I have some concerns around relaying scene delegate calls to app delegate, mainly the fact that we are changing Apple's behavior that may surprise us in the future. See more details here: #170037 (comment)

If there's no other way around, I'm fine with relaying the calls, but just wanna hear your thoughts on it first.

I agree. This feels like the kind of thing that addresses the short-term pain of everyone having to move their code to the scene delegate (or add the relay to their own code) but will create headaches long-term. I would much rather see Flutter take the stance that code should be moved to align with platform expectations.

Another approach would be to spin up a subclass that does the relaying. Teams that don't want that functionality would simply use the superclass instead.

[gaaclarke]

I switched back to talking the the lifecycle delegate directly. While I think in general we should make our decisions based on concrete concerns and tests, not calling the FlutterAppDelegate isn't technically a breaking change even if it can break some customers. So, I'm fine doing the more reserved solution of talking to the delegate directly.

Keeping the logic inline with Apple's will be more understandable at least for the odd scenarios where it may matter.

[hellohuanlin]

What do you think about we firstly start with relaying the calls, and adding the warning in tooling against app delegate usage (apple's deprecation warning isn't sufficient as we discussed yesterday), and then remove the relay in the future? This allows a smoother transition for developers.

[gaaclarke]

What do you think about we firstly start with relaying the calls, and adding the warning in tooling against app delegate usage (apple's deprecation warning isn't sufficient as we discussed yesterday), and then remove the relay in the future? This allows a smoother transition for developers.

I don't think it's necessary. The people this effects is low, there is no longer an automatic migration and and it's not a Breaking Change. I think that will just complicate the migration. Who knows what decision will be made in the future but migrating may never be an automatic thing considering the events of the last few days. I plan on filing an issue for the static warning like we discussed yesterday as part of resolving this PR, so that idea doesn't get lost.

[hellohuanlin]

Sounds good. I think i'm comfortable with this. Before stamping this, @jmagman FYI this reverts to the original implementation that you had concerns: #170180 (comment)

[hellohuanlin]

LGTM!