[google_maps_flutter] Marker APIs are now widget based (Android) #1229
Conversation
iskakaushik
changed the title
|
I've made changes to the iOS impl at https://github.com/iskakaushik/plugins/pull/2/files, keeping it separate for now though we would want to merge it into this branch before we land this feature. |
iskakaushik
changed the title
bparrishMines
changed the title
note: This commit only deals with Android side of changes.
| @@ -178,6 +153,7 @@ public void onMapReady(GoogleMap googleMap) { | |||
| googleMap.setOnCameraIdleListener(this); | |||
| googleMap.setOnMarkerClickListener(this); | |||
| updateMyLocationEnabled(); | |||
| this.markersController.setGoogleMap(googleMap); | |||
| * | ||
| * <p>To be kept in sync with MarkerId on dart side. | ||
| */ | ||
| public class MarkerId { |
There was a problem hiding this comment.
I'd just keep the marker IDs as Strings in the Android implementation, saves us some boilerplate.
There was a problem hiding this comment.
I was thinking the same, I was hoping that having a type here will let us distinguish between dart marker id and google map api's marker id (which we are keeping as a string).
There was a problem hiding this comment.
I think the name should cover that well enough, it's used in a relatively narrow scope anyway...
| methodChannel.invokeMethod("marker#onTap", Convert.toJson(markerId)); | ||
| MarkerController markerController = markerIdToController.get(markerId); | ||
| if (markerController != null) { | ||
| return markerController.consumeTapEvents(); |
There was a problem hiding this comment.
Why do we need to do this here?
There was a problem hiding this comment.
The method contract is:
true if the listener has consumed the event (i.e., the default behavior should not occur); false otherwise (i.e., the default behavior should occur). The default behavior is for the camera to move to the marker and an info window to appear.
| return; | ||
| } | ||
| for (Object rawMarkerId : markerIdsToRemove) { | ||
| if (rawMarkerId != null) { |
There was a problem hiding this comment.
uber nit: my personal preference would be to reverse the condition and continue to save one nesting level. Sending you some better articulated explanation offline.
| @@ -117,6 +118,15 @@ static Object toJson(CameraPosition position) { | |||
| return data; | |||
| } | |||
|
|
|||
| static Object toJson(MarkerId markerId) { | |||
There was a problem hiding this comment.
Don't change it as part of this PR to stay consistent, but all of these overloads of function with the same name feels error prone to me, I'd consider given each of them a different name, Also it's not converting to JSON but to a map.
There was a problem hiding this comment.
filed: flutter/flutter#28363
…tter#1229) * Marker APIs are now widget based (Android) note: This commit only deals with Android side of changes. * Fix format * Remove MarkerId class
note: This commit only deals with Android side of changes.
Additional Context: Maps Plugin is in the process of being
moved away from a controller based API to a widget based api.
This is to facilitate easier state management and address a
lot of the common issues.