Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Check FlutterAppDelegate selector support before calling #43425

Merged
auto-submit merged 3 commits into from
Jul 18, 2023
Merged

Check FlutterAppDelegate selector support before calling #43425

auto-submit merged 3 commits into from
Jul 18, 2023

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Jul 5, 2023 •
edited
Loading

Description

This adds checks for the app delegate to make sure that it supports the Flutter-specific selectors before calling them, so that a non FlutterAppDelegate can be used for the NSApplicationDelegate on NSApp.

Related Issues

Tests

  • Added a test to make sure things don't crash if the app delegate isn't a FlutterAppDelegate.

@gspencergoog gspencergoog force-pushed the check_delegate_selectors branch from 5c44501 to af32633 Compare July 6, 2023 19:33
@gspencergoog gspencergoog requested a review from cbracken July 6, 2023 20:45
@gspencergoog
Copy link
Contributor Author

gspencergoog commented Jul 6, 2023

@cbracken How would I set the delegate to a non-FlutterAppDelegate in a test?

@gspencergoog gspencergoog mentioned this pull request Jul 7, 2023
Open
2 tasks
@cbracken
Copy link
Member

cbracken commented Jul 11, 2023

You should be able to just create a class that extends NSObject <NSApplicationDelegate> and set the delegate property of the current application at runtime. IIRC you'll want to hold on to the previous one then set it back at the end of the test to avoid test ordering bugs.

cbracken
cbracken reviewed Jul 11, 2023
View reviewed changes
cbracken
cbracken reviewed Jul 11, 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.

Overall lgtm - just the test to do.

@gspencergoog gspencergoog force-pushed the check_delegate_selectors branch from 85a5c53 to 5fba395 Compare July 14, 2023 19:55
@gspencergoog gspencergoog force-pushed the check_delegate_selectors branch from 5fba395 to 25609e0 Compare July 14, 2023 21:00
@gspencergoog gspencergoog marked this pull request as ready for review July 14, 2023 21:01
gspencergoog
gspencergoog commented Jul 14, 2023
View reviewed changes
Copy link
Contributor Author

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

Okay, this is ready for a "real" review. I added a test.

cbracken
cbracken approved these changes Jul 18, 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.

Looks great! lgtm.

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 18, 2023
@auto-submit auto-submit bot merged commit d1712b2 into flutter:main Jul 18, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 18, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@cbracken cbracken cbracken approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-macos

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gspencergoog @cbracken