Skip to content

[google_maps_flutter] Convert heatmap controller to Swift - #12713

Open
stuartmorgan-g wants to merge 16 commits into
flutter:mainfrom
stuartmorgan-g:maps-swift-heatmaps-and-utils
Open

[google_maps_flutter] Convert heatmap controller to Swift#12713
stuartmorgan-g wants to merge 16 commits into
flutter:mainfrom
stuartmorgan-g:maps-swift-heatmaps-and-utils

Conversation

@stuartmorgan-g

Copy link
Copy Markdown
Collaborator

This converts HeatmapController and the remaining utility functions to Swift in the _sdk* packages.

The final remaining Obj-C code will be migrated in a follow-up PRs.

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 conversion code utils have much more change than previous PRs, since the direct conversion code felt very non-idiomatic in Swift. Almost all of the free functions for Pigeon<->Maps SDK type conversions were converted to extensions on the Pigeon types:

  • To avoid the fragile pattern of putting extension methods on types we don't control (which can lead to collisions), all the extensions are on the Pigeon types, and so the APIs aren't symmetrical:
    • Maps -> Pigeon is done via a convenience constructor (generally called make(from:)
    • Pigeon -> Maps is done via a toMapsSDKClassName() method on the Pigeon type (I'm not sold on that naming pattern; alternate suggestions welcome)
  • Conversions functions that were array-based have been changed to single-element conversions following the pattern above, and then the call sites changed to just map that conversion function, since map is a simple and idiomatic pattern in Swift, unlike the loop-and-add construction that had been required in Obj-C.
    • This in turn caused some tests to be simplified, since we didn't need to test things like the number of list items converted, and can instead just test the individual conversion.

The test bridging header is removed since there are no longer any _Test headers.

Part of flutter/flutter#119108

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 the heatmap controller and data conversion logic from Objective-C to Swift across the iOS packages (sdk9, sdk10, and shared_code), removing several Objective-C files in favor of Swift implementations. The review feedback highlights critical safety issues, pointing out potential runtime crashes from unsafely unwrapping optional values like heatmap.gradient and heatmap.weightedData. Additionally, the reviewer suggests performance optimizations, such as avoiding unnecessary CLLocation heap allocations by passing CLLocationCoordinate2D directly to path-creation helpers, and refactoring redundant image loading logic in ImageUtils.swift.

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.

1 participant