[google_maps_flutter_web] Fix getScreenCoordinate, Circle zIndex #4298

ditman · 2021-09-02T01:42:53Z

This PR passes the zIndex attribute to Circle geometry objects.

This PR ensures that the getScreenCoordinate method works as expected on the web platform.

The getScreenCoordinate(LatLng) method (along with its inverse getLatLng(ScreenCoordinate) require a fully rendered map to be tested, because they need the physical size in pixels, current center and zoom level of a map to work.

So far, these two methods weren't tested, but this PR adds happy-case tests for both (see projection_test.dart)

In order to test those methods, the Controller of the rendered map must be available on the test, so we can retrieve information from the map. While doing this, I noticed that the map Controller was being sent to the programmer before it was truly ready (while the map was still initializing).

Instead of littering the test with imprecise timeouts that may make these tests slower (or flakier) than needed, this PR also changes the initialization process of a GMap slightly so when its Controller is returned to the user (onPlatformViewCreated method call), it is truly ready.

For this, we now call init on the Controller immediately as it is created, then wait for the first onTilesloaded event coming from the JS SDK, and only then we return the Controller to the user.

This change happens within "private" sections of the plugin, so programmers using the plugin "normally" shouldn't notice any difference whatsoever (only that the GMap might load slightly faster, and the onPlatformViewCreated callback might be firing a few hundred milliseconds later).

(This PR also triggered changes in the mock files, that can be safely ignored, since those are auto-generated.)

Pre-launch Checklist

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

ditman · 2021-09-02T01:47:26Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_flutter_web.dart

@@ -305,11 +308,16 @@ class GoogleMapsPlugin extends GoogleMapsFlutterPlatform {
      polylines: polylines,
      circles: circles,
      mapOptions: mapOptions,
-    );
+    )..init(); // Initialize the controller


This is the crux of the early initialization. Before the flow was:

buildView -> onPlatformViewCreated (plugin) -> init (plugin) -> init (controller) -> JS SDK

but now, it's:

buildView + init (controller) -> JS SDK -> onPlatformViewCreated (plugin) -> init (plugin, noop)

ditman · 2021-09-02T01:49:20Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_flutter_web.dart

+    // The internal instance of our controller is initialized eagerly in `buildView`,
+    // so we don't have to do anything in this method, which is left intentionally
+    // blank.
+    assert(_map(mapId) != null, 'Must call buildWidget before init!');


This assertion is added to preserve some of the validation that happened before, mainly that calling init before buildWidget would cause quite a lot of breakage.

This method used to call init in the controller, but no longer. We do that immediately after creating the controller in buildView.

ditman · 2021-09-02T01:52:27Z

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_controller.dart

@@ -163,6 +185,10 @@ class GoogleMapController {

  // Funnels map gmap events into the plugin's stream controller.
  void _attachMapEvents(gmaps.GMap map) {
+    map.onTilesloaded.first.then((event) {


There's no map.onReady event, so we synthesize it from the first onTilesloaded event 🤷 (years of discussion)

stuartmorgan · 2021-09-03T16:04:55Z

packages/google_maps_flutter/google_maps_flutter_web/example/pubspec.yaml

+  build_runner: ^2.1.1
+  google_maps: ^5.2.0
+  google_maps_flutter: # Used for projection_test.dart
+    path: ../../google_maps_flutter


This feels sketchy, but I don't have a better suggestion offhand :(

(Maybe long term some of the widgets should be in the interface package, not the app-facing package?)

(Maybe long term some of the widgets should be in the interface package, not the app-facing package?)

Yes, I'm not a fan of the current widget. There's a circular dependency in the code that didn't allow me to completely separate it from the GoogleMapsController that also lives in the core plugin, here (I'm not a fan of the controller accessing the map state directly, but never had the time to make it better :/).

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

…nfo.

stuartmorgan

A comment nit and license format nit, but otherwise LGTM

packages/google_maps_flutter/google_maps_flutter_web/lib/src/convert.dart

packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_flutter_web.dart

...utter/google_maps_flutter_web/lib/src/third_party/to_screen_location/to_screen_location.dart

ditman · 2021-09-11T00:19:04Z

I'll manually squash and merge when master recovers, I want to have a proper commit message for this one.

…Circles (flutter/plugins#4298)

…Circles (flutter/plugins#4298) (#89895)

…lutter#4298) This commit: * uses the zIndex attribute when converting Circle geometry objects. * ensures that the getScreenCoordinate method works as expected on the web platform. * adds tests that can use a fully-rendered Google Map (see projection_test.dart) * changes the initialization flow of the web Google Map, so the Controller is only returned to the main plugin when it's ready to work. In order to test the getScreenCoordinate method, the Controller of a fully-rendered map must be available on the test, so we can retrieve information from an actual map instance. While working on this, it was observed that the Controller was being sent to the programmer before it was truly ready (while the map was still initializing). Instead of littering the test with imprecise timeouts that may make these tests slower (and flakier) than needed, this PR also changes the initialization process of a GMap slightly so when its Controller is returned to the user of the plugin (onPlatformViewCreated method call), it is truly ready. For this: * Controller.init is immediately called after the controller is created, * The plugin waits for the first onTilesloaded event coming from the JS SDK, and then * The Controller is sent to the user This change happens within "private" sections of the plugin, so programmers using the plugin "normally" shouldn't notice any difference whatsoever (only that the GMap might load slightly faster, and the onPlatformViewCreated callback might be firing a few hundred milliseconds later).

ditman requested a review from stuartmorgan September 2, 2021 01:42

ditman requested a review from cyanglaz as a code owner September 2, 2021 01:42

github-actions bot added p: google_maps_flutter platform-web labels Sep 2, 2021

google-cla bot added the cla: yes label Sep 2, 2021

ditman commented Sep 2, 2021

View reviewed changes

stuartmorgan reviewed Sep 3, 2021

View reviewed changes

ditman force-pushed the maps-get-screen-coordinate branch from a84a694 to e2bc20b Compare September 9, 2021 22:23

ditman added 13 commits September 9, 2021 16:50

Add _latLngToPixel convert function

c147de5

Bump google_maps dep to the one that fixed the original issue.

2f2ec62

Add test for the projection of the map in a known-size widget.

c19e5cf

Update dependencies and mock files.

6ef4084

Wait until the first onTilesloaded to call onPlatformViewCreated.

a475d90

Add tests for projection methods.

8c58a52

Make google_maps a dev dependency of the example app.

063dc54

Dartfmt tests

b254ab0

dartfmt lib

0bc5a7f

Bump changelog and version

1241d6d

Ensure example app uses fixed API too.

2016cd7

Move to_screen_location to src/third_party and add proper licensing i…

c8ecb6e

…nfo.

Use zIndex when creating Circles.

9bbdfa9

ditman force-pushed the maps-get-screen-coordinate branch from e2bc20b to 9bbdfa9 Compare September 10, 2021 00:46

ditman changed the title ~~[google_maps_flutter_web] Fix getScreenCoordinate method~~ [google_maps_flutter_web] Fix getScreenCoordinate, Circle zIndex Sep 10, 2021

ditman added 3 commits September 9, 2021 17:58

Update CHANGELOG

3ca2f71

Update license check command to recognize a new third_party license.

544b366

dart format and some commas

c39eabf

ditman requested review from stuartmorgan and removed request for cyanglaz September 10, 2021 01:41

stuartmorgan approved these changes Sep 10, 2021

View reviewed changes

Do not use we, make LICENSE more legible.

a447daa

ditman added waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. labels Sep 11, 2021

ditman merged commit 70c314c into flutter:master Sep 11, 2021

engine-flutter-autoroll mentioned this pull request Sep 11, 2021

Roll Plugins from a4f0e88fca1e to 70c314ce53cf (1 revision) flutter/flutter#89895

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2021

70c314c [google_maps_flutter_web] Fix getScreenCoordinate, zIndex of …

4f6723c

…Circles (flutter/plugins#4298)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Sep 11, 2021

70c314c [google_maps_flutter_web] Fix getScreenCoordinate, zIndex of …

3a60854

…Circles (flutter/plugins#4298) (#89895)

ditman deleted the maps-get-screen-coordinate branch September 13, 2021 16:20

ditman mentioned this pull request Jun 14, 2022

[google_maps_flutter] Switch web to structured options #5965

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter_web] Fix getScreenCoordinate, Circle zIndex #4298

[google_maps_flutter_web] Fix getScreenCoordinate, Circle zIndex #4298

ditman commented Sep 2, 2021 •

edited

ditman Sep 2, 2021

ditman Sep 2, 2021

ditman Sep 2, 2021

stuartmorgan Sep 3, 2021

ditman Sep 3, 2021

stuartmorgan left a comment

ditman commented Sep 11, 2021

[google_maps_flutter_web] Fix getScreenCoordinate, Circle zIndex #4298

[google_maps_flutter_web] Fix getScreenCoordinate, Circle zIndex #4298

Conversation

ditman commented Sep 2, 2021 • edited

Pre-launch Checklist

ditman Sep 2, 2021

Choose a reason for hiding this comment

ditman Sep 2, 2021

Choose a reason for hiding this comment

ditman Sep 2, 2021

Choose a reason for hiding this comment

stuartmorgan Sep 3, 2021

Choose a reason for hiding this comment

ditman Sep 3, 2021

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

ditman commented Sep 11, 2021

ditman commented Sep 2, 2021 •

edited