[google_maps_flutter_web] Allow marker position updates

[AsturaPhoenix]

Unconditionally convert the current marker position in convert.dart:_markerOptionsFromMarker, to allow for position updates.

Also adds position changes to marker_test.dart/MarkerController/update and markers_test.dart/MarkersController/changeMarkers. The MarkersController case is fixed by this patch.

Issues

• Grafted from: [google_maps_flutter_web] Allow marker position updates plugins#6753
• Fixes: [google_maps_flutter_web] Map pins don't move flutter#83467

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• 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 listed 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 this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[ditman]

Hey @AsturaPhoenix I did a small tweak to the position update to make it optional so programmers don't always have to pass the position of a marker on every possible update. LMK what you think, and if your use case still works after my change!

[AsturaPhoenix]

@ditman It shouldn't affect my use case, so I'm inclined to defer to your judgment. I'm curious about the rationale though. I did get the distinct impression that Google Maps JS, and for that matter Google Maps Android, really want to push the MVC paradigm and that the current Flutter interface is taking some not insignificant performance hits as a result, so from that angle I understand the desire to have the ability to skip updates.

However, my personal opinion is I'm not a fan of treating Null Island as a sentinel value, nor of Google Maps Flutter behaving differently here from Android and iOS, unless I'm mistaken.

If we're going for performance optimization, I'd suggest taking a hammer to the interface instead, or at least the google_maps_flutter/google_maps_flutter_platform_interface package, even if the Flutter-facing API doesn't change (but that would be scope creep for a much larger PR). At the very least for example, I got about a hundred milliseconds back from just diffing polyline fields for Android so that it wasn't trying to serialize all the unchanging fields across the method channel. (Which, while substantial, wasn't enough to push it over the jank line as GMS Maps still seems to be serializing something heavy across yet another boundary, but I digress.)

[ditman]

I'm curious about the rationale though.

@AsturaPhoenix to me, this is a stop-gap measure so we can land this update, that seems useful, without forcing everybody to re-set the Location of a Marker every time they want to update any other property. Currently, you should be able to set draggable of MarkerId(X) without having to worry about any of its other properties, and I wanted to keep that feature alive.

Do you need to pass the position of every marker on every update on Android?

(I bet you that was the original intent of the code, defaulting to the "currently set position", except I did the default backwards! 🤦)

I agree that using Null Island as a sentinel value is not great, but to remove that, I must fix the interface and make the position of the Marker nullable, which is probably a Major version change, and would take much longer to land than this.

(Once the default position is null, we'll be able to tell it apart from LatLng(0, 0) and remove the epsilon hack for the web. (I'm not sure how Android handles this use case!))

[AsturaPhoenix]

Do you need to pass the position of every marker on every update on Android?

Yes if you're using google_maps_flutter, unless you go really far out of your way (either build without null safety, which I've never tried so I don't know how/if that works, and explicitly set the position to null, or if you don't use google_maps_flutter and write to the method channel directly). I would go so far as to say that's idiomatic of Flutter.

To be precise, as you discovered, the position is optional in the interface and defaults to (0, 0), so if you were to omit it for a build, the marker would move to (0, 0) on Android and iOS. It's not clear to me why that parameter is optional in the interface with its current philosophy.

If the true null case is what you want to support, we could ignore analyzer warnings and add conditionals for null positions anyway, but that doesn't seem like a great direction.

(Once the default position is null, we'll be able to tell it apart from LatLng(0, 0) and remove the epsilon hack for the web. (I'm not sure how Android handles this use case!))

It doesn't, as far as I can tell. Although the Android and iOS platform impls skip setters for anything that's null on the method channel, the only way that happens is if the code using google_maps_flutter ignores null safety, or else writes into the method channel directly without using google_maps_flutter, and none of this is documented anywhere. I don't really understand why we want to bundle what amounts to a hack for a new feature that grew out of a bug into a bugfix, but again, I'm not a stakeholder. Maybe someone else on the issue would be a more appropriate reviewer to weigh in.

It may also be worth calling out that since google_maps_flutter_web doesn't use the method channel, that part isn't a concern here.

[ditman]

the marker would move to (0, 0) on Android and iOS.

I guess I don't need to worry about the web resetting the position then!

[ditman]

I think the position resetting to 0,0 is a bug, and we shouldn't force people to specify a new position on the marker if they don't want to, but let's land this to improve consistency with the mobile implementations!

[ditman]

LGTM!

[auto-submit]

auto label is removed for flutter/packages, pr: 3697, due to This PR has not met approval requirements for merging. You have project association NONE and need 1 more review(s) in order to merge this PR.

• Merge guidelines: You need at least one approved review if you are already a MEMBER or two member reviews if you are not a MEMBER before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[auto-submit]

auto label is removed for flutter/packages, pr: 3697, due to Validations Fail.

[ditman]

My beloved @auto-submit, please try again!