-
Notifications
You must be signed in to change notification settings - Fork 6k
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.
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
Add platform channel System.exitApplication and System.requestAppExit support #40033
Add platform channel System.exitApplication and System.requestAppExit support #40033
Conversation
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. |
@gspencergoog I haven't tested this - can you confirm this is using the API as intended? |
I've started platform plugin tests in #39992 - will land that first to use that framework. |
4aca3ef
to
a189b31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this all looks right now.
a189b31
to
f7af019
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c5f6154
to
ce2137f
Compare
From Triage: May I land this? |
I think not yet. Robert's still working out a segfault in the engine. Right @robert-ancell ? |
Yes, I wanted to be sure the crash wasn't due to this change first. |
Crash now fixed, will land once the tests complete. |
FlView* self) { | ||
fl_platform_plugin_request_app_exit(self->platform_plugin); | ||
// Stop the event from propagating. | ||
return TRUE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line totally blocks the additional listener provided by plugins, such like window_manager
. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, it does, but the alternative is returning FALSE, which would not give the app a chance to stop the exit when the last window goes away.
Perhaps this should only make the exit request and return TRUE if there is only one window open. That would still block the plugin listeners on the last window, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the change prevents *_plugin_dispose(GObject* object)
from being called.
The some_plugin_dispose
in the generated template will never be called.
static void some_plugin_dispose(GObject* object) {
G_OBJECT_CLASS(some_plugin_parent_class)->dispose(object);
}
static void some_plugin_class_init(SomePluginClass* klass) {
G_OBJECT_CLASS(klass)->dispose = some_plugin_dispose;
}
related issue comes: flutter/engine#40033
related issue comes: flutter/engine#40033
related issue comes: flutter/engine#40033
related issue comes: flutter/engine#40033
This adds support for System.exitApplication and sending and handling the response from System.requestAppExit to the platform channel.
Fixes Linux part of flutter/flutter#30735