[google_maps_flutter_ios] Add Advanced marker support#10508
Conversation
8f811e9 to
26fd75a
Compare
|
Picking up the discussion from the main issue here: do you have a link to the tracking issue for the blocker here ("an issue with the GMSPinImage not rendering on the map", per this comment)? As noted in that discussion, the link is to a long-closed issue. |
Added tracking issue information to this PR description: |
16c74d8 to
df35a62
Compare
2c08201 to
b1b0038
Compare
packages/google_maps_flutter/google_maps_flutter_ios/lib/src/messages.g.dart
Outdated
Show resolved
Hide resolved
|
@stuartmorgan-g Note that Advanced Markers without any customizations (without PinConfig icon type), or those using a custom image asset, render correctly. The issue only affects markers that use GMSPinImage to customize the marker in native iOS code. We could still publish, but it must be clearly communicated to developers that Since Option 1: Option 2: Option 3: I’ll also create a separate issue describing the issue and link it to the code under TODO Additionally, this should be documented in the iOS implementation package’s README.md? |
b1b0038 to
da25bc6
Compare
We'll have to bypass our usual guidance against platform-specific docs in the platform interface in this case, and document it on
I don't think we should do anything at a code level, because the behavior isn't in code that we control. Let's say we ship an I think we should just put a prominent comment on the PinConfig class and constructor, and on the
This is severe enough that I would put it here rather than in the implementation package, especially since we still mostly haven't "federated" the README platform details yet for this plugin. |
da25bc6 to
d8397ec
Compare
d8397ec to
6a2e5a9
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces advanced marker support to the iOS implementation of google_maps_flutter. It includes updates to the CHANGELOG.md, modifications to integration tests to cover advanced marker clustering and pin configurations, and new example pages demonstrating advanced marker icons and collision behavior. The core changes involve updating Pigeon messages to include PlatformMarkerCollisionBehavior and PlatformMarkerType, and modifying the FGMMarkersController to handle advanced markers based on the markerType configuration. The FGMImageUtils.m file has been updated to support PinConfig for advanced marker icons.
...google_maps_flutter/google_maps_flutter_ios_shared_code/lib/src/google_maps_flutter_ios.dart
Show resolved
Hide resolved
...gle_maps_flutter_ios/Sources/google_maps_flutter_ios/google_maps_flutter_pigeon_messages.g.m
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios_shared_code/lib/src/messages.g.dart
Show resolved
Hide resolved
...gle_maps_flutter_ios/Sources/google_maps_flutter_ios/google_maps_flutter_pigeon_messages.g.m
Show resolved
Hide resolved
...gle_maps_flutter_ios/Sources/google_maps_flutter_ios/google_maps_flutter_pigeon_messages.g.m
Show resolved
Hide resolved
.../google_maps_flutter/google_maps_flutter_ios_shared_code/example/lib/example_google_map.dart
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios_shared_code/lib/src/messages.g.dart
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios_shared_code/lib/src/messages.g.dart
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios_shared_code/lib/src/messages.g.dart
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios_shared_code/lib/src/messages.g.dart
Show resolved
Hide resolved
7eac3ce to
911dfa1
Compare
#11100) Add iOS warning for PinConfig usage in BitmapDescriptor iOS implementation for Advanced Markers: #10508 Related issue: flutter/flutter#155526 ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^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.
911dfa1 to
9d2bacd
Compare
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Generally looks good, just small comments.
What changes were in that last force-push? It came in when I was almost done reviewing, and since it's a force-push I can't see diffs relative to where I had reviewed.
..._flutter_ios/Sources/google_maps_flutter_ios/include/google_maps_flutter_ios/FGMImageUtils.h
Outdated
Show resolved
Hide resolved
...sdk10/example/ios/Runner.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Outdated
Show resolved
Hide resolved
...aps_flutter/google_maps_flutter_ios/example/ios/RunnerTests/ClusterManagersControllerTests.m
Outdated
Show resolved
Hide resolved
| FGMIconFromBitmap([FGMPlatformBitmap makeWithBitmap:pinConfig], assetProvider, screenScale); | ||
|
|
||
| // PinConfig returns nil on iOS versions without GMSPinImageOptions support (< iOS 16.0). | ||
| // On simulators, GMSPinImage may also return a zero-dimension image. Both cases are acceptable |
There was a problem hiding this comment.
Out of curiosity, do we know why this happens in simulators, or is it just an undocumented SDK behavior?
There was a problem hiding this comment.
I updated the comment to better explain the reasoning behind this logic.
...ter_ios/ios/google_maps_flutter_ios/Sources/google_maps_flutter_ios/FGMGoogleMapController.m
Outdated
Show resolved
Hide resolved
| PlatformMarkerCollisionBehavior.optionalAndHidesLowerPriority, | ||
| MarkerCollisionBehavior.requiredAndHidesOptional => | ||
| PlatformMarkerCollisionBehavior.requiredAndHidesOptional, | ||
| }; |
There was a problem hiding this comment.
This must have a fallback behavior (either a default value, or null, whichever makes more sense in this case) to handle changes in the platform interface. Search for // ignore: dead_code in this file for examples to follow.
There was a problem hiding this comment.
this now defaults to the default value PlatformMarkerCollisionBehavior.requiredDisplay;
| borderColor: borderColor, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same here; this code needs some kind of handling of the case where a new glyph type is added to the platform interface as a non-breaking change (even if it's just throwing an UnimplementedError).
There was a problem hiding this comment.
Will throw UnimplementedError for unimplemented glyph types
| return switch (markerType) { | ||
| MarkerType.marker => PlatformMarkerType.marker, | ||
| MarkerType.advancedMarker => PlatformMarkerType.advancedMarker, | ||
| }; |
There was a problem hiding this comment.
Needs fallback handling, in case a third marker type is ever added.
There was a problem hiding this comment.
Will throw UnimplementedError for marker types not yet implemented.
throw UnimplementedError('MarkerType "$markerType" has not been implemented');
packages/google_maps_flutter/google_maps_flutter_ios/pigeons/messages.dart
Show resolved
Hide resolved
...ogle_maps_flutter/google_maps_flutter_ios_sdk10/example/ios/Runner.xcodeproj/project.pbxproj
Show resolved
Hide resolved
9d2bacd to
caf8fa6
Compare
Sorry for the late response. You didn't miss anything in the earlier rebase. |
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM! I'm going to go ahead and land this, and then we can roll out the app-level support (and hope that the SDK issue with PinConfig will be resolved soon).
This PR adds Advanced markers support to the iOS implementation of
google_maps_flutter.Approved combined PR: #7882
Approved and merged platform interface PR: #9737
Issue: flutter/flutter#155526
Blocker:
GMSAdvancedMarker is not rendered when the marker icon is a GMSPinImage instance. Tracking issue:
https://issuetracker.google.com/issues/370536110
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.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.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