Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add platform channel System.exitApplication and System.requestAppExit support #39836

Merged
merged 15 commits into from
Mar 3, 2023
Merged

Add platform channel System.exitApplication and System.requestAppExit support #39836

merged 15 commits into from
Mar 3, 2023

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Feb 24, 2023 •
edited
Loading

Description

This adds support for System.exitApplication and sending and handling the response from System.requestAppExit to the platform channel.

It also introduces a FlutterApplication class that is a subclass of NSApplication, and overrides the terminate message so that it can ignore it if the framework decides to cancel the termination, and to allow for async communication with the framework before termination.

If the application doesn't use a FlutterApplication as its NSPrincipalClass in Info.plist for the application, then it all works as before: the application will be unable to cancel the request. In debug mode it does log once per run, if the application is accessed, that some capabilities are disabled (like exitApplication). I'm not really sure if we should be doing the log message. It seems like a good prompt to remind people to migrate, and how, but I could easily be convinced that it's log spam.

Paired with flutter/flutter#121378 to enable the framework to respond properly. Before that lands, if anyone switches to use FlutterApplication as their principal class, termination requests will go unanswered.

Related Issues

Tests

  • Adds tests for the round trip from framework request for exit, to application termination request from the engine to framework, and then a response to the engine to actually terminate.

@flutter-dashboard flutter-dashboard bot added platform-macos platform-web Code specifically for the web engine labels Feb 24, 2023
@gspencergoog gspencergoog mentioned this pull request Feb 24, 2023
Merged
cbracken
cbracken reviewed Feb 24, 2023
View reviewed changes
shell/platform/darwin/macos/framework/Headers/FlutterApplication.h Show resolved Hide resolved
shell/platform/darwin/macos/framework/Headers/FlutterApplication.h Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterAppDelegate.mm Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterAppDelegate.mm Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterAppDelegate.mm Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterAppDelegate_internal.h Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterAppDelegate.mm Outdated Show resolved Hide resolved
@gspencergoog gspencergoog force-pushed the app_exit branch 2 times, most recently from bdc025a to 41e319e Compare February 27, 2023 23:54
@gspencergoog gspencergoog marked this pull request as ready for review February 27, 2023 23:54
@gspencergoog
Copy link
Contributor Author

gspencergoog commented Feb 28, 2023 •
edited
Loading

Ok, PTAL: I've added a test that goes the round trip: handles a call to exit the application from the framework, asks the framework if that's OK (so that if other listeners are registered, they get asked), and then either exits or doesn't depending on the response. If it had come from the OS, then it would just do the last part (query the framework and then possibly exit).

The only thing I couldn't figure out how to test was the actually call to [[FlutterApplication sharedApplication] terminate:nil], since that requires that there be a real application running (making a mock subclass would just bypass the thing I'm trying to test). There's only a couple of lines of code there.

I was able to cut down the necessary additions to FlutterAppDelegate to be just an internal property that is the termination handler.

The termination handler is an internal class, and so is the property on FlutterAppDelegate for now. I considered making them public, but I'd like to see a use case first.

cbracken
cbracken reviewed Mar 1, 2023
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple quick comments before I grab dinner. Will finish the rest of the review in a bit!

lib/ui/platform_dispatcher.dart Outdated Show resolved Hide resolved
lib/ui/platform_dispatcher.dart Show resolved Hide resolved
shell/platform/darwin/macos/framework/Headers/FlutterApplication.h Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterEngine.mm Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm Outdated Show resolved Hide resolved
@gspencergoog gspencergoog mentioned this pull request Mar 1, 2023
Closed
@gspencergoog gspencergoog force-pushed the app_exit branch 3 times, most recently from 33755cf to 5bf896f Compare March 1, 2023 18:32
@gspencergoog gspencergoog force-pushed the app_exit branch 3 times, most recently from 4daadab to 2d76d62 Compare March 1, 2023 21:32
cbracken
cbracken approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 3, 2023
@auto-submit auto-submit bot merged commit 55b6631 into flutter:main Mar 3, 2023
@gspencergoog gspencergoog deleted the app_exit branch March 3, 2023 19:13
@HansMuller HansMuller mentioned this pull request Mar 3, 2023
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 3, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 3, 2023
@engine-flutter-autoroll
55b663140 Add platform channel System.exitApplication and `System.r…
61a9b0a 
…equestAppExit` support (flutter/engine#39836)
@gspencergoog gspencergoog mentioned this pull request Mar 6, 2023
Merged
@tgucio tgucio mentioned this pull request Mar 16, 2023
Closed
@cbracken cbracken mentioned this pull request Apr 4, 2023
Merged
8 tasks
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Apr 4, 2023
@cbracken
[macOS] Handle termination in FlutterAppDelegate
79a0cb5 
Moves application termination handling to FlutterAppDelegate.
Previously, we required macOS applications using Flutter to ensure their
main application class was FlutterApplication. Instead, we now do all
handling in FlutterAppDelegate and FlutterEngine.

There are two termination workflows to consider:
* Termination requested from framework side: In this case, then engine
  receives a `System.exitApplication` method call, and starts the app
  termination workflow via
  `[FlutterEngine requestApplicationTermination:exitType:result]`.
* Termination requested from macOS (e.g. Cmd-Q): In this case,
  `FlutterAppDelegate`'s `applicationShouldTerminate:` handler is
  invoked by AppKit, and the delegate starts the app termination
  workflow via
  `[FlutterEngine requestApplicationTermination:exitType:result]`.

In either case, at this point, if the request is not cancellable, the
app immediately exits. If it is cancellable, the embedder sends a
`System.requestAppExit` method channel invocation to the framework,
which responds with either `exit` or `cancel`. In the case of `exit` we
immediately exit, otherwise we do nothing and the app continues running.

This is a minor refactoring of the original approach we took in:
flutter#39836

This does not remove the FlutterApplication class, since the framework
migration from NSApplication to FlutterApplication still depends on it.
A followup patch with replace the migration with a reverse migration
will land, then FlutterApplication will be removed.

Issue: flutter/flutter#30735
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Apr 4, 2023
@cbracken
[macOS] Handle termination in FlutterAppDelegate
aa9bed4 
Moves application termination handling to FlutterAppDelegate.
Previously, we required macOS applications using Flutter to ensure their
main application class was FlutterApplication. Instead, we now do all
handling in FlutterAppDelegate and FlutterEngine.

There are two termination workflows to consider:
* Termination requested from framework side: In this case, then engine
  receives a `System.exitApplication` method call, and starts the app
  termination workflow via
  `[FlutterEngine requestApplicationTermination:exitType:result]`.
* Termination requested from macOS (e.g. Cmd-Q): In this case,
  `FlutterAppDelegate`'s `applicationShouldTerminate:` handler is
  invoked by AppKit, and the delegate starts the app termination
  workflow via
  `[FlutterEngine requestApplicationTermination:exitType:result]`.

In either case, at this point, if the request is not cancellable, the
app immediately exits. If it is cancellable, the embedder sends a
`System.requestAppExit` method channel invocation to the framework,
which responds with either `exit` or `cancel`. In the case of `exit` we
immediately exit, otherwise we do nothing and the app continues running.

This is a minor refactoring of the original approach we took in:
flutter#39836

This does not remove the FlutterApplication class, since the framework
migration from NSApplication to FlutterApplication still depends on it.
A followup patch with replace the migration with a reverse migration
will land, then FlutterApplication will be removed.

Issue: flutter/flutter#30735
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Apr 5, 2023
@cbracken
[macOS] Handle termination in FlutterAppDelegate
d7d60b4 
Moves application termination handling to FlutterAppDelegate.
Previously, we required macOS applications using Flutter to ensure their
main application class was FlutterApplication. Instead, we now do all
handling in FlutterAppDelegate and FlutterEngine.

There are two termination workflows to consider:
* Termination requested from framework side: In this case, then engine
  receives a `System.exitApplication` method call, and starts the app
  termination workflow via
  `[FlutterEngine requestApplicationTermination:exitType:result]`.
* Termination requested from macOS (e.g. Cmd-Q): In this case,
  `FlutterAppDelegate`'s `applicationShouldTerminate:` handler is
  invoked by AppKit, and the delegate starts the app termination
  workflow via
  `[FlutterEngine requestApplicationTermination:exitType:result]`.

In either case, at this point, if the request is not cancellable, the
app immediately exits. If it is cancellable, the embedder sends a
`System.requestAppExit` method channel invocation to the framework,
which responds with either `exit` or `cancel`. In the case of `exit` we
immediately exit, otherwise we do nothing and the app continues running.

This is a minor refactoring of the original approach we took in:
flutter#39836

This does not remove the FlutterApplication class, since the framework
migration from NSApplication to FlutterApplication still depends on it.
A followup patch with replace the migration with a reverse migration
will land, then FlutterApplication will be removed.

Issue: flutter/flutter#30735
cbracken added a commit that referenced this pull request Apr 5, 2023
@cbracken
[macOS] Handle termination in FlutterAppDelegate (#40929)
8679b67 
Moves application termination handling to FlutterAppDelegate.
Previously, we required macOS applications using Flutter to ensure their
main application class was FlutterApplication. Instead, we now do all
handling in FlutterAppDelegate and FlutterEngine.

There are two termination workflows to consider:
* Termination requested from framework side: In this case, then engine
receives a `System.exitApplication` method call, and starts the app
termination workflow via `[FlutterEngine
requestApplicationTermination:exitType:result]`.
* Termination requested from macOS (e.g. Cmd-Q): In this case,
`FlutterAppDelegate`'s `applicationShouldTerminate:` handler is invoked
by AppKit, and the delegate starts the app termination workflow via
`[FlutterEngine requestApplicationTermination:exitType:result]`.

In either case, at this point, if the request is not cancellable, the
app immediately exits. If it is cancellable, the embedder sends a
`System.requestAppExit` method channel invocation to the framework,
which responds with either `exit` or `cancel`. In the case of `exit` we
immediately exit, otherwise we do nothing and the app continues running.

This is a minor refactoring of the original approach we took in:
#39836

This does not remove the FlutterApplication class, since the framework
migration from NSApplication to FlutterApplication still depends on it.
A followup patch with replace the migration with a reverse migration
will land, then FlutterApplication will be removed.

Issue: flutter/flutter#30735

No new tests since this refactors existing behaviour while retaining the
same app semantics as before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loic-sharma loic-sharma loic-sharma left review comments

@cbracken cbracken cbracken approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-macos platform-web Code specifically for the web engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gspencergoog @cbracken @loic-sharma