Skip to content

[google_maps_flutter] Convert top-level classes to Swift in _sdk* packages - #12307

Open
stuartmorgan-g wants to merge 19 commits into
flutter:mainfrom
stuartmorgan-g:maps-swift-top-level
Open

[google_maps_flutter] Convert top-level classes to Swift in _sdk* packages#12307
stuartmorgan-g wants to merge 19 commits into
flutter:mainfrom
stuartmorgan-g:maps-swift-top-level

Conversation

@stuartmorgan-g

Copy link
Copy Markdown
Collaborator

This converts GoogleMapsPlugin, GoogleMapFactory, and GoogleMapController to Swift, and restructures the source code to typical migration state of having the Swift code be the primary target, depending on a new *_objc target that contains the unmigrated code. Because google_maps_flutter_ios cannot adopt Swift (see discussion in linked issue), this removes that package from the code sharing. Going forward, development will be limited to the _sdk* packages.

For reasons explained here, the Pigeon API layer is still Objective-C for now, which does lead to some temporary non-ideal code in GoogleMapController. That will be cleaned up in the last step when we are able to switch to Swift Pigeon generation.

The remaining Obj-C code will be migrated in a series of follow-up PRs to keep the scope of each PR reasonable for review.

The conversion process was:

  • Initial conversion via Gemini, with explicit instruction to keep the structure the same.
  • Side-by-side manual review of the old and new versions of the files.
  • Manual fixes and improvements.

The last step was more extensive than usual because the init method had longstanding design issues (flutter/flutter#104121) that became hard errors under Swift. There was some refactoring of helper methods and reordering of initialization steps to make the code work with the strict two-phase init process.

Part of flutter/flutter#119108
Fixes flutter/flutter#104121

Pre-Review Checklist

Footnotes

  1. 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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 converts parts of the iOS Google Maps implementation to Swift across the SDK 9, SDK 10, and shared code packages, replacing several Objective-C controllers and plugins. The build configurations, synchronization tools, and tests are updated to support the new Swift files. Feedback on these changes includes correcting a regular expression typo in the utility script, addressing typos in documentation and comments, avoiding unsafe force-unwrapping and force-casting in the map factory, and resolving an inconsistency in how uninitialized map views are handled.

Comment thread packages/google_maps_flutter/google_maps_flutter_ios_shared_code/tool/utils.dart Outdated
Comment thread packages/google_maps_flutter/google_maps_flutter_ios/CONTRIBUTING.md Outdated
@stuartmorgan-g

Copy link
Copy Markdown
Collaborator Author

We'll want to hold off on landing this until 3.47 reaches stable, since that's when nobody on stable will have an actual need to keep using google_maps_flutter_ios, making it reasonable to stop adding features to it. I wanted to have it reviewed and ready to go at that point though, so I'm opening it now.

@vashworth per offline discussion, this punts on the question of whether we switch the endorsed implementation to _sdk9. We can do that in a follow-up if we decide to go that route.

For review, it may be helpful to look at some of the individual commits. 5a27e87 in particular, which is where I reworked the initialization.

#expect(mapView.frameObserverCount == 0)
}

@Test func mapsServiceSync() {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is removed because in the Swift version, all the custom logic for calling this exactly once was replaced by us being able to use Swift's lazy static handling, and since that's a core language behavior rather than custom logic, there's no value in testing it.

)
}

@Test func frameObserverRemovedOnDeinitIfNeverFired() {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added to cover an issue found by Gemini in local review of the conversion.

#expect(mapView.frameObserverCount == 0)
}

@Test func styleErrorPersistsAcrossConfigUpdates() {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added to cover a case I accidentally regressed when reworking init, also found by Gemini in a local review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[google_maps_flutter] ios, avoid sending message to self in init

1 participant