[google_maps_flutter] Marker APIs are now widget based (Dart Changes) by iskakaushik · Pull Request #1239 · flutter/plugins

iskakaushik · 2019-02-19T22:22:24Z

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.

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.

amirh · 2019-02-19T23:46:29Z

packages/google_maps_flutter/example/lib/scrolling_map.dart

                      ),
-                      gestureRecognizers:
-                          <Factory<OneSequenceGestureRecognizer>>[
+                      markers: {


discussed offline but leaving as a reminder - we can't use set literals here yet.

amirh · 2019-02-19T23:51:00Z

packages/google_maps_flutter/lib/src/google_map.dart

-  void _updateOptions(Map<String, dynamic> updates) async {
+  void _updateOptions(
+      Map<String, dynamic> updates, MarkerUpdates markerUpdates) async {
    if (updates.isEmpty) {


You might want to update the markers even if there is no options update.
Might be clearer to just split the markers update to an _updateMarkers method.

amirh · 2019-02-19T23:51:28Z

packages/google_maps_flutter/lib/src/google_map.dart

    }
    final GoogleMapController controller = await _controller.future;
    controller._updateMapOptions(updates);
+    final MarkerControllers markerControllers = await _markerControllers.future;


nit: markersController

amirh · 2019-02-20T00:09:22Z

packages/google_maps_flutter/lib/src/google_map.dart

-          (String key, dynamic value) => prevOptionsMap[key] == value);
+      ..remove('markers')
+      ..removeWhere((String key, dynamic value) => prevOptionsMap[key] == value)
+      ..putIfAbsent('markerUpdates', () => markerUpdates._toJson());


Took me a little bit to wrap my ahead around who's responsible for the marker updates.

It used to be that _GoogleMapOptions is holds all the options data, this data is passed in the creation parameters 'options' field, _GoogleMapOptions knows to compute update deltas which are passed to _updateMapOptions.

The markers field doesn't follow this same pattern: it is still passed as part of the options in the creation parameters, but _GoogleMapOptions is not responsible for computing updates for it, and _updateMapOptions ignores it.

What do you think about keeping a _markers field in _GoogleMapState?

amirh · 2019-02-20T00:19:39Z

packages/google_maps_flutter/lib/src/controller.dart

-    marker._options = marker._options.copyWith(changes);
-    notifyListeners();
+/// Handles callbacks for events on [Marker] and [InfoWindow].
+class MarkerController {


Making this part of the public API limits are flexibility with breaking changes here, we better keep this and MarkersController private API if we can.

amirh · 2019-02-20T00:20:42Z

packages/google_maps_flutter/lib/src/controller.dart

-    await _removeMarker(marker._id);
-    notifyListeners();
+    final String id = marker.markerId.value;
+    // TODO (kaushik) using id in the plugin handle might not be a good idea.


nit: should be TODO(iskakaushik): ... (here and elsewhere)

amirh · 2019-02-20T00:22:48Z

packages/google_maps_flutter/lib/src/controller.dart

-    marker._options = marker._options.copyWith(changes);
-    notifyListeners();
+/// Handles callbacks for events on [Marker] and [InfoWindow].
+class MarkerController {


The name MarkerController hints to me that it controls everything related to this marker (e.g icon, position, etc...) this is more of a MarkerEventDelegate

amirh · 2019-02-20T00:30:57Z

packages/google_maps_flutter/lib/src/controller.dart

+      <MarkerId, MarkerController>{};
+
+  void update(MarkerUpdates markerUpdates) {
+    markerUpdates.markerUpdates.forEach((MarkerUpdate markerUpdate) {


I think the code would be simpler if we would just use the map method channel for all markers, that way we could save the management of the controllers here and e.g when we get a marker#onTap(markerId) all we need to do is fetch the Marker with id markerId from the markers set we maintain anyway and invoke the callback.

This reverts commit 75956c2.

amirh · 2019-02-20T22:23:16Z

packages/google_maps_flutter/lib/src/google_map.dart

+  }
+
+  static Map<MarkerId, Marker> _createMarkerIdMarkerMap(Set<Marker> markers) {
+    // TODO(iskakaushik): Remove this when collection literals makes it to stable.


An alternative suggestion would be:

return Map<MarkerId, Marker>.fromEntries( markers.map((Marker marker) => MapEntry(marker.id, marker)) );

amirh · 2019-02-21T17:37:41Z

packages/google_maps_flutter/lib/src/google_map.dart

+
+  Map<MarkerId, Marker> _markers;
+
+  dynamic update(Set<Marker> newMarkers) {


avoid dynamic when possible

Will change it to Map<String, dynamic>. It returns the change to be sent to the platform channel

amirh · 2019-02-21T17:58:08Z

packages/google_maps_flutter/lib/src/google_map.dart

      'initialCameraPosition': widget.initialCameraPosition?._toMap(),
-      'options': _GoogleMapOptions.fromWidget(widget).toMap(),
+      'options': _googleMapOptions.toMap(),
+      'markerUpdates': _markerUpdateHandler.update(widget.markers),


nit: this is not really an update, might make sense to call the creation parameter just 'markers'.

I am considering renaming MarkerUpdate class to MarkerChange and call this markerChanges. I will still call it markers in creationParams, but elsewhere I will call this markerChanges let me know what you think.

amirh · 2019-02-21T18:17:54Z

packages/google_maps_flutter/lib/src/google_map.dart

  }
 }

+class _MarkerUpdateHandler {


I find the role of this class a little hard to grasp, it does the following:

Maintains the set of current markers

Proxy calls to MarkerUpdates for computing and returning the diff for a new markers set.

Provides closures for invoking the marker events on a given marker.

I was also a little confused by the name - the initial mental model I had just from seeing the class name was (this is a marker update handler, it probably handles computing the updates and sending them over the platform channel).

The closure generation is particularly a trick I'd rather avoid if it's not necessary as it is harder to follow.

It might be easier to follow if we keep the markers map as a member of _GoogleMapState, then we could avoid the closures as we could have _onMarkerTap(String markerId) as a method of _GoogleMapState.
It looks like the update computation logic can be done in a pretty short method which we can fit in _updateMarkers.

amirh · 2019-02-21T18:21:05Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+
+/// [Marker] update event with the changes.
+@immutable
+class MarkerUpdate {


There's some ambiguity with this being called MarkerUpdate while it can have one [add/remove/update] event type.

I will rename this MarkerChange

amirh · 2019-02-21T18:21:24Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+
+/// [Marker] update event with the changes.
+@immutable
+class MarkerUpdate {


Also - if we keep this class we probably don't want to make it part of the public API.

amirh · 2019-02-21T18:23:26Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+    );
+  }
+
+  /// TODO(iskakaushik): Diff is sufficient, don't need to send the whole update.


TODOs should include a link to a GitHub issue otherwise they are not in our tracking system,.

Will make an issue for this.

amirh · 2019-02-21T18:25:58Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+///
+/// Used in [GoogleMapController] when the map is updated.
+@immutable
+class MarkerUpdates {


Is this essentially a set of MarkerUpdate objects? why do we need a dedicated type for it?

This does a few more things than just being a wrapper:

This class provides a nice place to have .from(previous, current) to compute the diff.

Provides methods for serialization of the update chunk that we need to send over to the platform side.

Though this is a wrapper around a Set<MarkerUpdate>, it succinctly captures a snapshot of all updates done to the markers.

amirh · 2019-02-21T18:26:15Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+///
+/// Used in [GoogleMapController] when the map is updated.
+@immutable
+class MarkerUpdates {


If we end up keeping this class we probably don't want it to be public API.

Will make this private.

still need to do this

amirh · 2019-02-21T18:29:41Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+  remove,
+}
+
+Map<MarkerId, Marker> _toMap(Set<Marker> markers) {


See suggestion on _createMarkerIdMarkerMap, also this seems to be redundant with the logic there?

amirh · 2019-02-21T21:50:48Z

packages/google_maps_flutter/lib/src/google_map.dart

  }
+
+  void onMarkerTap(String markerIdParam) {
+    if (markerIdParam == null) {


do we expect this to ever happen? I guess this should be an assertion

amirh · 2019-02-21T21:51:51Z

packages/google_maps_flutter/lib/src/google_map.dart

+      return;
+    }
+    final MarkerId markerId = MarkerId(markerIdParam);
+    _markers[markerId]?.onTap();


Can you add a comment explaining the scenario in which _markers[markerId] can be null

this cannot happen, got rid of the ?

amirh · 2019-02-21T21:53:39Z

packages/google_maps_flutter/lib/src/google_map.dart

  Future<void> onPlatformViewCreated(int id) async {
-    final GoogleMapController controller =
-        await GoogleMapController.init(id, widget.initialCameraPosition);
+    final GoogleMapController controller = await GoogleMapController.init(


unrelated to this PR: we should make GoogleMapController.init private before we hit API stability.

amirh · 2019-02-21T22:00:51Z

packages/google_maps_flutter/lib/src/controller.dart

    );
-    final Marker marker = Marker(markerId, effectiveOptions);
-    _markers[markerId] = marker;
    notifyListeners();


Not related to this PR - We should re-evaluate the single-listener model before we hit API stability.

amirh · 2019-02-21T22:02:57Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+///
+/// Used in [GoogleMapController] when the map is updated.
+@immutable
+class MarkerUpdates {


still need to do this

amirh · 2019-02-21T22:37:22Z

packages/google_maps_flutter/lib/src/marker.dart

+  }
+}
+
+/// An icon placed at a particular geographical location on the map's surface.


blank line after the summary.

amirh · 2019-02-21T22:40:09Z

packages/google_maps_flutter/lib/src/marker.dart

+  }
+}
+
+/// An icon placed at a particular geographical location on the map's surface.


Is a marker "an icon"?
Maybe just say in the summary "Marks a geographical location on the map"?

[I see that you just moved this, but might be a good opportunity anyway]

I meant to suggest that as the summary line.

amirh · 2019-02-21T22:43:19Z

packages/google_maps_flutter/lib/src/marker.dart

+  /// * has an axis-aligned icon; [rotation] is 0.0
+  /// * is visible; [visible] is true
+  /// * is placed at the base of the drawing order; [zIndex] is 0.0
+  const Marker({


this was previously annotated @VisibleForTesting

This is now the preferred way to construct markers. Now that we do not have the createWithDefaultSettings factory method

amirh · 2019-02-21T22:44:13Z

packages/google_maps_flutter/lib/src/marker.dart

+    this.onTap,
  }) : assert(alpha == null || (0.0 <= alpha && alpha <= 1.0));

+  /// Used to uniquely identify the [Marker].


nit: Uniquely identifies the marker.

amirh · 2019-02-21T22:47:47Z

packages/google_maps_flutter/lib/src/marker.dart

-
-  /// Text content for the info window.
-  final InfoWindowText infoWindowText;
+  /// Window displayed onTap for a marker.


/// A Google Maps InfoWindow. /// /// The window is displayed when the marker is tapped.

amirh · 2019-02-22T01:25:06Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+
+    final Set<MarkerId> markerIdsToRemove =
+        prevMarkerIds.difference(currentMarkerIds);
+    final Set<Marker> markersToAdd = currentMarkerIds


not sure if you've missed the comment or decided to not do the nit (which is ok)

amirh · 2019-02-22T01:26:14Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+        assert(markersToChange != null);
+
+  /// Computes [MarkerUpdates] given previous and current [Marker]s.
+  factory MarkerUpdates.from(Set<Marker> previous, Set<Marker> current) {


amirh · 2019-02-22T01:29:26Z

packages/google_maps_flutter/lib/src/marker.dart

+      (Marker marker) => MapEntry<MarkerId, Marker>(marker.markerId, marker)));
+}
+
+List<dynamic> _serializeMarkerSet(Set<Marker> markers) {


List<Map<String, dynamic>>

amirh · 2019-02-22T01:31:01Z

packages/google_maps_flutter/lib/src/marker.dart

+  }
+}
+
+/// An icon placed at a particular geographical location on the map's surface.


I meant to suggest that as the summary line.

amirh · 2019-02-22T01:35:04Z

packages/google_maps_flutter/test/fake_maps_controllers.dart

+
+  Set<Marker> _deserializeMarkers(dynamic markers) {
+    if (markers == null) {
+      // TODO(iskakaushik): Remove this when collection literals makes it to stable.


include a link to a GitHub issue

amirh · 2019-02-22T01:53:04Z

packages/google_maps_flutter/test/marker_updates_test.dart

+        fakePlatformViewsController.lastCreatedView;
+
+    expect(platformGoogleMap.markersToChange.length, 2);
+    expect(platformGoogleMap.markersToChange, equals(cur));


I don't think you need equals here, also no need to compare the length if we're comparing the sets

amirh · 2019-02-22T01:55:42Z

packages/google_maps_flutter/test/marker_updates_test.dart

+    final Set<Marker> prev = _toSet(m1: m1, m2: m2);
+    m1 = Marker(markerId: MarkerId("marker_1"), visible: false);
+    m2 = Marker(markerId: MarkerId("marker_2"), draggable: true);
+    final Set<Marker> cur = _toSet(m1: m1, m2: m2);


my personal preference here would be [m1, m2].toSet() instead of the _toSet method (and soon enough a set literal).
But if you prefer this keep it.

Filing a github issue to track this: flutter/flutter#28311

amirh · 2019-02-22T03:27:19Z

packages/google_maps_flutter/example/lib/place_marker.dart

+    _markerIdCounter++;
+    final MarkerId markerId = MarkerId(markerIdVal);
+
+    void _onMarkerTapped() {


I think it'll simplify things if we avoid this closure.
We can have a member _onMarkerTapped(MarkerId markerId); and below do:

final Marker marker = Marker( // ... onTap: () { _onMarkerTapped(markerId); }, );

This way the logic one sees in the _add method is purely about adding a marker.

amirh · 2019-02-22T03:37:19Z

packages/google_maps_flutter/example/lib/place_marker.dart

-    _updateSelectedMarker(
-      MarkerOptions(
-        position: LatLng(
+    _updateSelectedMarker((Marker m) {


I feel like _updateSelectedMarker is reversing the "reactive" style that we are introducing in this PR. Since this is sample code I'd err on the side of adding a little more duplication but being more clear on the nature of the API(here and in the other "mutation" methods below).

Something like(pseudo):

Marker selectedMarker = markers[_selectedMarker]; setState(() { markers[_selectedMarker] = selectedMaker.copyWith(position: /* .. */); });

amirh · 2019-02-22T03:38:53Z

packages/google_maps_flutter/example/lib/scrolling_map.dart

+                          // ignore: prefer_collection_literals
+                          Set<Marker>.of(
+                        <Marker>[
+                          Marker(


…er-api-mod-dart Conflicts: packages/google_maps_flutter/example/lib/scrolling_map.dart

amirh

LGTM modulo 2 nits.

Make sure to only land this after the iOS and Android implementations land.

amirh · 2019-02-22T17:35:19Z

packages/google_maps_flutter/lib/src/marker_updates.dart

+///
+/// Used in [GoogleMapController] when the map is updated.
+class _MarkerUpdates {
+  _MarkerUpdates._({


Is this used?

amirh · 2019-02-22T17:39:03Z

packages/google_maps_flutter/example/lib/place_marker.dart


  void _changeAnchor() {
-    final Offset currentAnchor = _selectedMarker.options.anchor;
+    final Marker m = markers[selectedMarker];


s/m/marker/

amirh

LGTM

Don't forget to update the change log and bump the version before submitting. Also make sure to merge this PR after the platform implementations.

…er-api-mod-dart

…flutter#1239) * Marker APIs are now widget based 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. * use collection literals * Address CR comments * Revert "use collection literals" This reverts commit 75956c2. * fix collection literal stuff * Crearte a marker update handler and update TODOs * ignore collection literals * Fix test failures * Move marker updates to their own chunks * Fix failing tests * Improved some docs and added some assertions * Make class private * Fix all hashCode and equals * update formatring * Address all the pending cr comments * fix failing test * Do not use => without return value * remove factory method * User `Marker marker` instead of `Marker m` * Update changelog and pubspec.yaml

iskakaushik requested a review from amirh February 19, 2019 22:22

googlebot added the cla: yes label Feb 19, 2019

Kaushik Iska added 2 commits February 19, 2019 14:24

Marker APIs are now widget based

083297b

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.

use collection literals

75956c2

amirh reviewed Feb 20, 2019

View reviewed changes

Kaushik Iska added 6 commits February 20, 2019 09:51

Address CR comments

e0d2dc9

Revert "use collection literals"

df76178

This reverts commit 75956c2.

fix collection literal stuff

f64218e

Crearte a marker update handler and update TODOs

3fde3b4

ignore collection literals

5c3c8d7

Fix test failures

2af0be2

amirh reviewed Feb 21, 2019

View reviewed changes

Kaushik Iska added 2 commits February 21, 2019 12:16

Move marker updates to their own chunks

2121c8f

Fix failing tests

9424e8e

iskakaushik requested a review from amirh February 21, 2019 21:31

amirh reviewed Feb 21, 2019

View reviewed changes

Kaushik Iska added 4 commits February 21, 2019 15:06

Improved some docs and added some assertions

b3f218a

Make class private

cb1cf37

Fix all hashCode and equals

c7ef2ce

update formatring

9da4b47

bparrishMines added the documentation label Feb 22, 2019

bparrishMines changed the title ~~Marker APIs are now widget based (Dart Changes)~~ [google_maps_flutter] Marker APIs are now widget based (Dart Changes) Feb 22, 2019

amirh reviewed Feb 22, 2019

View reviewed changes

iskakaushik mentioned this pull request Feb 22, 2019

When collection literals make it to stable refactor the marker_update_test to use them. flutter/flutter#28311

Closed

Kaushik Iska added 5 commits February 21, 2019 22:34

Address all the pending cr comments

da44b03

Merge branch 'master' of github.com:flutter/plugins into feature/mark…

4279b06

…er-api-mod-dart Conflicts: packages/google_maps_flutter/example/lib/scrolling_map.dart

fix failing test

6328de5

Do not use => without return value

a155265

remove factory method

661b42f

amirh reviewed Feb 22, 2019

View reviewed changes

User Marker marker instead of Marker m

eea1e76

amirh reviewed Feb 27, 2019

View reviewed changes

Kaushik Iska added 3 commits February 27, 2019 07:21

Merge branch 'master' of github.com:flutter/plugins into feature/mark…

81780f3

…er-api-mod-dart

Merge branch 'master' of github.com:flutter/plugins into feature/mark…

a68ec70

…er-api-mod-dart

Update changelog and pubspec.yaml

1c66880

iskakaushik merged commit d434fee into flutter:master Feb 27, 2019

iskakaushik deleted the feature/marker-api-mod-dart branch February 27, 2019 15:47


		Map<MarkerId, Marker> _markers;

		dynamic update(Set<Marker> newMarkers) {

Conversation

iskakaushik commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment