Repository navigation
[google_maps_flutter] Convert heatmap controller to Swift - #12713
Merged
auto-submit[bot] merged 18 commits intoSep 4, 2026
Merged
auto-submit[bot] merged 18 commits into
auto-submit[bot] merged 18 commits into
Conversation
Contributor
There was a problem hiding this comment.
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.
vashworth
reviewed
Sep 2, 2026
Comment on lines
+89
to
+97
| var gmsMapViewType: GMSMapViewType { | ||
| switch self { | ||
| case .none: return .none | ||
| case .normal: return .normal | ||
| case .satellite: return .satellite | ||
| case .terrain: return .terrain | ||
| case .hybrid: return .hybrid | ||
| @unknown default: return .normal | ||
| } |
Contributor
There was a problem hiding this comment.
With this being a var, is it set once and then never changed? Is it possible for the map type to change?
Collaborator
Author
There was a problem hiding this comment.
It's just a var because that's the syntax for a computed property; because there's no set defined for it, it's not actually assignable. FGMPlatformMapType is an enum, so an individual instance of it can't change value.
| self.heatmapTileLayer = tileLayer | ||
| self.mapView = mapView | ||
| super.init() | ||
| HeatmapController.update(tileLayer, from: heatmap, mapView: mapView) |
Contributor
There was a problem hiding this comment.
Doesn't this need to pass self.tileLayer?
Collaborator
Author
There was a problem hiding this comment.
self.heatmapTypeLayer and tileLayer are the same at this point, so it doesn't actually matter which is passed.
|
|
||
| var image: UIImage? | ||
|
|
||
| switch bitmap { |
Contributor
There was a problem hiding this comment.
add back comment
Suggested change
| switch bitmap { | |
| // See comment in messages.dart for why this is so loosely typed. See also | |
| // https://github.com/flutter/flutter/issues/117819. | |
| switch bitmap { |
Collaborator
Author
There was a problem hiding this comment.
Fixed. I caught that in the conversion utils, but missed it here.
(Also, good news: this problem is fixed in the next PR, since Swift Pigeon supports a limited form of class hierarchy for this use case.)
| case let bitmap as FGMPlatformBitmapBytes: | ||
| // Deprecated: This message handling for 'fromBytes' has been replaced by 'bytes'. | ||
| // Refer to the flutter google_maps_flutter_platform_interface package for details. | ||
| image = UIImage(data: bitmap.byteData.data, scale: screenScale) |
Contributor
There was a problem hiding this comment.
Is the try catch not necessary here?
Collaborator
Author
There was a problem hiding this comment.
You can't catch NSExceptions in Swift, so if UIImage's constructor can leak NSExceptions in current versions of iOS, Apple has done something very wrong 🙂
A Swift do/catch would catch Swift errors, but this constructor can't throw those because it's not marked as throws.
| } | ||
| case let bitmap as FGMPlatformBitmapBytesMap: | ||
| let bytes = bitmap.byteData | ||
| image = UIImage(data: bytes.data, scale: screenScale) |
Contributor
|
Some tests are failing |
stuartmorgan-g
commented
Sep 2, 2026
Collaborator
Author
There was a problem hiding this comment.
Some tests are failing
Ha. This is a fun little reminder of why we should be moving to Swift, where we have strong uniform typing 🙂
FGMPlatformBitmap *placeholderImage =
[FGMPlatformBitmap makeWithBitmap:[FGMPlatformBitmapDefaultMarker makeWithHue:0]];let placeholderImage = FGMPlatformBitmap.make(
withBitmap: FGMPlatformBitmapDefaultMarker.make(withHue: 0))Looks the same, right? Only, because hue is a nullable double in the Pigeon definition, the Obj-C FGMPlatformBitmap has to declare it as an NSNumber*, not a double. So in the Obj-C, 0 was actually a misspelling of nil... which didn't matter, because they are both just integers.
But in Swift, 0 is an integer in an NSNumber context, so is equivalent to the Obj-C @(0), and since the 0 is an int, it gets encoded with the wrong type, and so explodes on the Dart side.
Fixed by correcting the spelling of nil. (The potential for this kind of problem is eliminated in the next PR, when all of these data classes become Swift.)
Comment on lines
+89
to
+97
| var gmsMapViewType: GMSMapViewType { | ||
| switch self { | ||
| case .none: return .none | ||
| case .normal: return .normal | ||
| case .satellite: return .satellite | ||
| case .terrain: return .terrain | ||
| case .hybrid: return .hybrid | ||
| @unknown default: return .normal | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
It's just a var because that's the syntax for a computed property; because there's no set defined for it, it's not actually assignable. FGMPlatformMapType is an enum, so an individual instance of it can't change value.
| self.heatmapTileLayer = tileLayer | ||
| self.mapView = mapView | ||
| super.init() | ||
| HeatmapController.update(tileLayer, from: heatmap, mapView: mapView) |
Collaborator
Author
There was a problem hiding this comment.
self.heatmapTypeLayer and tileLayer are the same at this point, so it doesn't actually matter which is passed.
|
|
||
| var image: UIImage? | ||
|
|
||
| switch bitmap { |
Collaborator
Author
There was a problem hiding this comment.
Fixed. I caught that in the conversion utils, but missed it here.
(Also, good news: this problem is fixed in the next PR, since Swift Pigeon supports a limited form of class hierarchy for this use case.)
| case let bitmap as FGMPlatformBitmapBytes: | ||
| // Deprecated: This message handling for 'fromBytes' has been replaced by 'bytes'. | ||
| // Refer to the flutter google_maps_flutter_platform_interface package for details. | ||
| image = UIImage(data: bitmap.byteData.data, scale: screenScale) |
Collaborator
Author
There was a problem hiding this comment.
You can't catch NSExceptions in Swift, so if UIImage's constructor can leak NSExceptions in current versions of iOS, Apple has done something very wrong 🙂
A Swift do/catch would catch Swift errors, but this constructor can't throw those because it's not marked as throws.
| } | ||
| case let bitmap as FGMPlatformBitmapBytesMap: | ||
| let bytes = bitmap.byteData | ||
| image = UIImage(data: bytes.data, scale: screenScale) |
Contributor
|
Tests still failing: |
Collaborator
Author
|
I forgot to sync the last changes to the other copies. (Who came up with this crazy multi-copy system? 🙃) |
vashworth
approved these changes
Sep 3, 2026
This was referenced Sep 7, 2026
pull Bot
pushed a commit
to Mattlk13/flutter
that referenced
this pull request
Sep 9, 2026
…er#192485) flutter/packages@9af9c60...36e088a 2026-09-09 daniel.leon@cloudsufi.com [quick_actions] Adopt code-excerpts for README (flutter/packages#12643) 2026-09-08 65155920+0xharkirat@users.noreply.github.com [camera_web] Fix TypeError when reading the torch capability (flutter/packages#12647) 2026-09-08 gibbonsj97@gmail.com [google_maps_flutter_web] Avoid replacing advanced marker content on move (flutter/packages#11952) 2026-09-08 mit@google.com [material_ui][cupertino_ui] Change issue tracker label in pubspec.yaml (flutter/packages#12792) 2026-09-08 saurabhmirajkar000@gmail.com [material_ui] Fix FilledButton Material 3 default style docs (flutter/packages#12620) 2026-09-08 21270878+elliette@users.noreply.github.com [infra] Use a modified no-response workflow in flutter/packages (flutter/packages#12745) 2026-09-08 21270878+elliette@users.noreply.github.com [material_ui] Migrate M3 Banner template to use new gen_defaults (flutter/packages#12734) 2026-09-08 engine-flutter-autoroll@skia.org Roll Flutter from 63170e9 to b444e78 (13 revisions) (flutter/packages#12791) 2026-09-08 fluttergithubbot@gmail.com Sync release-go_router-18.0.1 to main (flutter/packages#12725) 2026-09-08 fluttergithubbot@gmail.com Sync release-material_ui-1.1.1 to main (flutter/packages#12726) 2026-09-08 stuartmorgan@google.com [tool] Adopt `platform` 3.2.0 (flutter/packages#12789) 2026-09-07 50643541+Mairramer@users.noreply.github.com [material_ui] Fix SliverGeometry maxPaintExtent assertion in CarouselView.weighted (flutter/packages#12563) 2026-09-05 engine-flutter-autoroll@skia.org Roll Flutter from 5a6cfa7 to 63170e9 (15 revisions) (flutter/packages#12767) 2026-09-04 engine-flutter-autoroll@skia.org Manual roll Flutter from 70797e1 to 5a6cfa7 (52 revisions) (flutter/packages#12760) 2026-09-04 brackenavaron@gmail.com [material_ui] port drawer tests over from flutter/widgets (flutter/packages#12711) 2026-09-04 tarrinneal@gmail.com add cooldown (flutter/packages#12708) 2026-09-04 joeldumasbg@gmail.com [in_app_purchase] Support StoreKit 2 introductory offer eligibility JWS (flutter/packages#12584) 2026-09-04 21270878+elliette@users.noreply.github.com [material_ui] Migrate M3 Badge template to use new gen_defaults (flutter/packages#12733) 2026-09-04 stuartmorgan@google.com [google_maps_flutter] Convert heatmap controller to Swift (flutter/packages#12713) 2026-09-04 97480502+b-luk@users.noreply.github.com [material_ui] Remove unused `maintainState` constructor parameter in `scaffold_test.dart` (flutter/packages#12754) 2026-09-04 a1rwulf@users.noreply.github.com [video_player_avfoundation] Route video over AirPlay (flutter/packages#12490) 2026-09-04 jerome.dellamaria@proton.me [google_fonts] Add google_fonts_lite file to allow tree-shaking of the other huge files (flutter/packages#11433) 2026-09-04 engine-flutter-autoroll@skia.org Roll Flutter from 0cbd1a4 to 70797e1 (27 revisions) (flutter/packages#12727) 2026-09-04 stuartmorgan@google.com Update Chrome for stable tests (flutter/packages#12747) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni
pushed a commit
to victorsanni/packages
that referenced
this pull request
Sep 9, 2026
) 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 [^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.
victorsanni
pushed a commit
to victorsanni/packages
that referenced
this pull request
Sep 9, 2026
) 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 [^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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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:
make(from:)toMapsSDKClassName()method on the Pigeon type (I'm not sold on that naming pattern; alternate suggestions welcome)mapthat conversion function, sincemapis a simple and idiomatic pattern in Swift, unlike the loop-and-add construction that had been required in Obj-C.The test bridging header is removed since there are no longer any
_Testheaders.Part of flutter/flutter#119108
Pre-Review Checklist
[shared_preferences]///).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