-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[google_maps_flutter] Convert plugin class to Swift #10545
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
base: main
Are you sure you want to change the base?
[google_maps_flutter] Convert plugin class to Swift #10545
Conversation
Converts the top-level plugin class and its supporting platform view factory from Objective-C to Swift. This changes very little implementation, but lays the groundwork for future incremental conversions. Part of flutter/flutter#119108
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.
Code Review
This pull request successfully converts the top-level plugin class and its associated platform view factory from Objective-C to Swift. The changes are well-contained and include necessary updates to the podspec, module definitions, and test files to support the new Swift implementation. The core logic, including the handling of shared GMSServices, is correctly preserved.
My review includes one suggestion to improve the robustness of the new Swift code by replacing force-unwrapping and force-casting with guard statements. This will make the plugin more resilient to unexpected inputs and prevent potential crashes.
| // Precache shared map services, if needed. Initializing this prepares GMSServices | ||
| // on a background thread controlled by the GoogleMaps framework. | ||
| let mapServices = GoogleMapFactory.sharedMapServices | ||
|
|
||
| return FLTGoogleMapController( | ||
| frame: frame, viewIdentifier: viewId, | ||
| creationParameters: args as! FGMPlatformMapViewCreationParams, registrar: registrar!) |
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.
The use of force casting (as!) and force unwrapping (!) can make the app crash if the assumptions about args type or registrar being non-nil are violated. It's safer to use guard let with conditional casting (as?) and optional binding to validate these before use. This provides more robust error handling and clearer failure messages if something goes wrong, which is especially useful for plugin consumers.
Also, using _ = ... is more idiomatic for calling something for its side effects while ignoring the result, which avoids an 'unused variable' warning.
// Precache shared map services, if needed. Initializing this prepares GMSServices
// on a background thread controlled by the GoogleMaps framework.
_ = GoogleMapFactory.sharedMapServices
guard let creationParams = args as? FGMPlatformMapViewCreationParams else {
// This is a programmer error on the Dart side, so crashing is reasonable.
fatalError("Invalid creation parameters for Google Map: \(String(describing: args))")
}
guard let registrar = self.registrar else {
// This should not happen in a normal lifecycle.
fatalError("Registrar is nil when creating Google Map.")
}
return FLTGoogleMapController(
frame: frame, viewIdentifier: viewId,
creationParameters: creationParams, registrar: registrar)| XCTAssertEqual(mapView.frameObserverCount, 0); | ||
| } | ||
|
|
||
| - (void)testMapsServiceSync { |
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.
|
Well, this is fun. We appear to have a SwiftPM-style problem with CocoaPods as well if we adopt any Swift in the target: It goes away if I change the podspec's |
Converts the top-level plugin class and its supporting platform view factory from Objective-C to Swift. This changes very little implementation, but lays the groundwork for future incremental conversions.
Part of flutter/flutter#119108
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3