[google_maps_flutter_ios] Add Advanced marker support

[jokerttu]

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

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter.
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under¹.
• All existing and new tests are passing.

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

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. ↩ ↩² ↩³

[stuartmorgan-g]

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.

[jokerttu]

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:
https://issuetracker.google.com/issues/474141340
Edit: the "long-closed issue" is now re-opened: https://issuetracker.google.com/issues/370536110

[stuartmorgan-g]

Please make this non-nullable to avoid a repeat of flutter/flutter#182540 on iOS (and for consistency with the changes in #11061).

[jokerttu]

I completely forgot that the Android implementation (and the upcoming iOS implementation) will be used with the current version of the app-facing package because of the caret dependency constrain. I had only tested locally against the app-facing version that already includes the updated app-facing implementation; therefore nevers seeing this issue.

I tested Advanced Marker support using dependency_overrides for the Android and iOS implementations:

• Android does indeed crash.
• iOS does not; most likely because, in the enum check, a nil value falls back to the first enum case (0), which happens to work by accident.

I'll update this package to match the Android implementation and force the default value as "marker" if null given, before setting this as ready to review.

Would it be possible to run CI tests for the app-facing packages as well whenever there are changes in the endorsed federated package (using dependency_overrides), when the app-facing package version matches the dependency constraint? This would prevent this kind of accidents from happening again.

[stuartmorgan-g]

Would it be possible to run CI tests for the app-facing packages as well whenever there are changes in the endorsed federated package (using dependency_overrides), when the app-facing package version matches the dependency constraint?

That's flutter/flutter#52115. The tl;dr is that I'm still not convinced the CI cost is worthwhile given the rarity of issues showing up only in integration tests.

[stuartmorgan-g]

In this case, I think the first question would be: why are the integration tests that crash on Android not in the Android implementation package already?

[jokerttu]

Yes, indeed!
And ther were no real reason to pass null value to the platfom layer in any case.
This is now fixed, and null value is not sent to the platform layer on iOS implementation either.
Still missing unit test for the null case, will add that before resolving this comment.

[jokerttu]

@stuartmorgan-g
As this iOS implementation package still has an issue with Advanced Marker rendering when the marker icon is created using PinConfig (tracked here: https://issuetracker.google.com/issues/370536110).

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 PinConfig icons are not supported and are affected by this issue. (Related comment at #7882 (comment))

Since PinConfig and AdvancedMarker are defined in packages/google_maps_flutter/google_maps_flutter_platform_interface (and not in this iOS implementation package), what would be the best place to document this behavior so that Devs really are aware of this?

Option 1:
Add an assert in _platformMarkerFromMarker conversion method to ensure marker.icon is not PinConfig.

Option 2:
Throw an UnsupportedError in _platformMarkerFromMarker conversion method if marker.icon is PinConfig.

Option 3:
Just log a warning to the console.
(This would most likely cause unnecessary issue reports)

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?

[stuartmorgan-g]

Since PinConfig and AdvancedMarker are defined in packages/google_maps_flutter/google_maps_flutter_platform_interface (and not in this iOS implementation package), what would be the best place to document this behavior so that Devs really are aware of this?

We'll have to bypass our usual guidance against platform-specific docs in the platform interface in this case, and document it on PinConfig.

Option 1: Add an assert in _platformMarkerFromMarker conversion method to ensure marker.icon is not PinConfig.

Option 2: Throw an UnsupportedError in _platformMarkerFromMarker conversion method if marker.icon is PinConfig.

Option 3: Just log a warning to the console. (This would most likely cause unnecessary issue reports)

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 UnsupportedError... then a week later, a bugfix release of the Google Maps iOS SDK goes out that fixes the issue. Now google_maps_flutter is broken even though the SDK isn't.

I think we should just put a prominent comment on the PinConfig class and constructor, and on the pinConfig factory method. We should phrase it in such a way that it won't be actively wrong if we haven't removed/updated it yet once the issue is fixed; something like:

WARNING: On iOS, using a PinConfig may result in the marker not showing. For details and updates, see
https://issuetracker.google.com/issues/370536110. If this issue has not been fixed in the version of the
Google Maps SDK you are using, consider using an asset or bitmap for customization on iOS.

Additionally, this should be documented in the iOS implementation package’s README.md?

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.