[google_maps_flutter] Moves Java->Dart calls to Pigeon #7040

stuartmorgan · 2024-07-01T18:47:44Z

Converts all of the Java->Dart calls from raw method channels to Pigeon. This finishes conversion of all of the invocations in google_maps_flutter_android

Part of flutter/flutter#117907

Pre-launch Checklist

stuartmorgan · 2024-07-01T18:50:35Z

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/CirclesController.java

+
+ @Override
+ public void error(@NonNull Throwable error) {}
+ });


These are because we don't currently have a way in Pigeon to ignore responses. Longer term I think we'll want to convert this over to event channels though; I'm not sure why maps wasn't written using them in the first place, since a stream of one-way calls without responses are the use cases event channels were written for IIUC.

(I'm not doing that conversion as part of this because we don't have event channel support for Pigeon yet; probably in Q3 sometime.)

I didnt know event channels existed so maybe the author didnt either.

Now that I think of it, I'm not sure when event channels were added, so this code may just predate them.

stuartmorgan · 2024-07-01T18:51:52Z

...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

- return data;
- }
-
- static Map<String, Object> tileOverlayArgumentsToJson(


I can change this to stay here and do a Pigeon build if you prefer, but since we have to pass all the arguments individually anyway (vs passing a single object to convert) it seemed easier to just inline it.

stuartmorgan · 2024-07-01T18:52:14Z

...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java

@@ -464,36 439,6 @@ static LatLng latLngFromPigeon(Messages.PlatformLatLng latLng) {
 return new LatLng(latLng.getLatitude(), latLng.getLongitude());
 }

- static Object clusterToJson(String clusterManagerId, Cluster<MarkerBuilder> cluster) {


This is pure removal because the Pigeon version (just below) was added in a previous PR.

stuartmorgan · 2024-07-01T18:52:51Z

...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java

@@ -182,10 190,6 @@ void init() {
 mapView.getMapAsync(this);
 }

- private CameraPosition getCameraPosition() {


Opportunistic cleanup; the IDE flagged that this was unused.

stuartmorgan · 2024-07-01T18:53:45Z

...tter_android/android/src/main/java/io/flutter/plugins/googlemaps/TileProviderController.java

@@ -66,37 65,43 @@ Tile getTile() {
 return TileProvider.NO_TILE;
 }
 try {
- return Convert.interpretTile(result);
+ if (result == null) {


Opportunistic safety fix; the IDE was flagging that we weren't handling null result here.

stuartmorgan · 2024-07-01T18:54:13Z

...s/google_maps_flutter/google_maps_flutter_android/test/google_maps_flutter_android_test.dart

+ expect((await markerDragEndStream.next).value.value, equals(dragEndId));
+ });
+
+ test('markers send tap events to correct stream', () async {


Longer because I added a bunch of missing test coverage.

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/CirclesController.java

reidbaker · 2024-07-02T14:44:26Z

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/CirclesController.java

+
+ @Override
+ public void error(@NonNull Throwable error) {}
+ });


I didnt know event channels existed so maybe the author didnt either.

reidbaker · 2024-07-02T14:46:16Z

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/CirclesController.java

+ public void success() {}
+
+ @Override
+ public void error(@NonNull Throwable error) {}


I struggle with our guidance to only log actionable errors. My gut is always to log because otherwise you are left guessing if you hit an error condition.
No action needed.

The main reason I didn't add it in this PR is that I know we have a class of issues around lifetime mismatch between native and Dart in Google Maps, and I worry that just adding logging here without deeply investigating those first would cause a bunch of spam where the call fails only because the instance has been eliminated on the Dart side, in which case silently dropping them is better than logging.

...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/PolylinesController.java

reidbaker · 2024-07-02T14:53:33Z

...tter_android/android/src/main/java/io/flutter/plugins/googlemaps/TileProviderController.java

+ }
+ @SuppressWarnings("unchecked")
+ final Map<String, ?> tileJson = (Map<String, ?>) result.getJson();
+ return Convert.interpretTile(tileJson);
 } catch (Exception e) {


This pr is not the right place but I think we should prefer a more tightly caught exception since this reads like it is catching json exceptions.

…tter/packages#7040)

flutter/packages@97bad7e...14341d1 2024-07-08 49699333 dependabot[bot]@users.noreply.github.com [sign_in]: Bump com.android.tools.build:gradle from 7.2.1 to 8.5.0 in /packages/google_sign_in/google_sign_in_android/android (flutter/packages#7070) 2024-07-08 49699333 dependabot[bot]@users.noreply.github.com [path_provider]: Bump com.android.tools.build:gradle from 7.3.1 to 8.5.0 in /packages/path_provider/path_provider_android/android (flutter/packages#7068) 2024-07-07 [email protected] Roll Flutter from af913a7 to fafd67d (41 revisions) (flutter/packages#7062) 2024-07-06 [email protected] [rfw] Update goldens. (flutter/packages#6941) 2024-07-06 [email protected] [google_maps_flutter] Moves Java->Dart calls to Pigeon (flutter/packages#7040) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

…r#151417) flutter/packages@97bad7e...14341d1 2024-07-08 49699333 dependabot[bot]@users.noreply.github.com [sign_in]: Bump com.android.tools.build:gradle from 7.2.1 to 8.5.0 in /packages/google_sign_in/google_sign_in_android/android (flutter/packages#7070) 2024-07-08 49699333 dependabot[bot]@users.noreply.github.com [path_provider]: Bump com.android.tools.build:gradle from 7.3.1 to 8.5.0 in /packages/path_provider/path_provider_android/android (flutter/packages#7068) 2024-07-07 [email protected] Roll Flutter from af913a7 to fafd67d (41 revisions) (flutter/packages#7062) 2024-07-06 [email protected] [rfw] Update goldens. (flutter/packages#6941) 2024-07-06 [email protected] [google_maps_flutter] Moves Java->Dart calls to Pigeon (flutter/packages#7040) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/ doc/main/autoroll/README.md

Converts all of the Obj-C->Dart calls from raw method channels to Pigeon. This finishes conversion of all of the invocations in `google_maps_flutter_ios` This is the iOS equivalent of #7040, and much of the Dart code is based on that PR. Part of flutter/flutter#117907

stuartmorgan added 3 commits July 1, 2024 14:46

Convert native->Dart calls to FlutterApi

ae2f470

Update Pigeon to get data class equality for tests

7f310ee

Version bump

a3ad521

stuartmorgan requested a review from reidbaker as a code owner July 1, 2024 18:47

github-actions bot added p: google_maps_flutter platform-android labels Jul 1, 2024

stuartmorgan commented Jul 1, 2024

View reviewed changes

Convert tile fetching to a real object

c678de4

Rexios80 reviewed Jul 3, 2024

View reviewed changes

...s_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/CirclesController.java Show resolved Hide resolved

reidbaker approved these changes Jul 3, 2024

View reviewed changes

stuartmorgan added 3 commits July 3, 2024 15:57

Merge branch 'main' into maps-pigeon-android-flutter-api

bbda7da

Consolidate no-op handlers

da6da68

Actually add new file

5f0aaae

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 6, 2024

auto-submit bot merged commit 0b57eca into flutter:main Jul 6, 2024
74 checks passed

stuartmorgan deleted the maps-pigeon-android-flutter-api branch July 7, 2024 01:25

engine-flutter-autoroll mentioned this pull request Jul 8, 2024

Roll Packages from 97bad7ecf569 to e1bf4791db52 (3 revisions) flutter/flutter#151408

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2024

0b57eca32 [google_maps_flutter] Moves Java->Dart calls to Pigeon (flu…

f7a2180

…tter/packages#7040)

engine-flutter-autoroll mentioned this pull request Jul 8, 2024

Roll Packages from 97bad7ecf569 to 14341d1800f4 (5 revisions) flutter/flutter#151417

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2024

0b57eca32 [google_maps_flutter] Moves Java->Dart calls to Pigeon (flu…

999cc5a

…tter/packages#7040)

This was referenced Jul 9, 2024

[google_maps_flutter] Convert to Pigeon flutter/flutter#117907

Open

[google_maps_flutter] Convert Obj-C->Dart calls to Pigeon #7086

Merged

reidbaker mentioned this pull request Jul 25, 2024

[many] Ignore NewerVersionAvailable in android lint #7218

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter] Moves Java->Dart calls to Pigeon #7040

[google_maps_flutter] Moves Java->Dart calls to Pigeon #7040

stuartmorgan commented Jul 1, 2024

stuartmorgan Jul 1, 2024

reidbaker Jul 2, 2024

stuartmorgan Jul 3, 2024

stuartmorgan Jul 1, 2024

stuartmorgan Jul 1, 2024

stuartmorgan Jul 1, 2024

stuartmorgan Jul 1, 2024

stuartmorgan Jul 1, 2024

reidbaker Jul 2, 2024

reidbaker Jul 2, 2024

stuartmorgan Jul 3, 2024

reidbaker Jul 2, 2024

[google_maps_flutter] Moves Java->Dart calls to Pigeon #7040

[google_maps_flutter] Moves Java->Dart calls to Pigeon #7040

Conversation

stuartmorgan commented Jul 1, 2024

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment