[google_maps_flutter_android] Convert Circle and Marker to Pigeon #7326

yaakovschectman · 2024-08-06T21:15:00Z

Utilize static typing for pigeon message objects for Marker and Circle in google_maps_flutter_android.

Draft for now to run tests.

List which issues are fixed by this PR. You must list at least one issue.

Pre-launch Checklist

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

stuartmorgan-g

A few initial notes.

I'll update and land #7207 tomorrow now that the other PRs it was going to collide with have landed; that will make the diffs here (and in follow-up object conversion PRs) much easier to review because you'll be able to actually replace the JSON codepaths instead of duplicating them, so it will be very easy to see what's the same and what's changing.

packages/google_maps_flutter/google_maps_flutter_platform_interface/lib/src/types/marker.dart

packages/google_maps_flutter/google_maps_flutter_android/CHANGELOG.md

...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart

stuartmorgan-g · 2024-08-08T00:14:21Z

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

+    this.visible = true,
+    this.strokeWidth = 10,
+    this.zIndex = 0.0,
+    this.radius = 0,


Optional: you can generally just make everything required in a Pigeon definition for a class like this that corresponds pretty directly to an existing class, because in practice there are usual no calls that don't provide every field due to the nature of how we use them.

At least one case where they were not all provided is the Java unit tests prior to this PR. Technically there was only one field at that time, the json, but that json field only had the fields specified that were actually tested by the unit tests. If any of the non-null fields are not provided to the Builder nested class, build will throw an exception. I was hoping that providing default values would mean that the Builder would use them and not require them all to be specified, avoiding this problem, but it appears that the generated Java code does not use the defaults anyway.

Yes, default values haven't been implemented for any of the just languages. Usually for unit tests we just make a helper that builds the object with mostly default values.

@yaakovschectman did you and stuart agree not to make these values required?

stuartmorgan-g · 2024-08-08T00:14:55Z

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

+    this.rotation = 0.0,
+    this.visible = true,
+    this.zIndex = 0.0,
+    this.clusterManagerId,


Same note here.

Should there be a note here? (my guess is no since id is a string but thought it was worth asking.

stuartmorgan-g · 2024-08-08T17:09:57Z

Okay, #7207 has landed, so you can merge in main here, and then delete the JSON codepaths for anything you've converted. And for future PRs you'll be able to alter then in place instead of duplicating first.

reidbaker · 2024-08-09T20:02:30Z

I feel like I should have known that but thanks for the explanation. Ignore my comment then.

…id/src/main/java/io/flutter/plugins/googlemaps/Convert.java Co-authored-by: Reid Baker <hamilton.reid.baker@gmail.com>

stuartmorgan-g · 2024-08-12T11:52:20Z

Is this ready for me to re-review, or still in progress?

yaakovschectman · 2024-08-12T14:05:19Z

I thought I re requested already.

…

On Mon, Aug 12, 2024, 7:52 AM stuartmorgan ***@***.***> wrote: Is this ready for me to re-review, or still in progress? — Reply to this email directly, view it on GitHub <#7326 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2AOOLDSX7G36DBA4DNIAJTZRCOYTAVCNFSM6AAAAABMDDYARWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBTG43DGNJSGU> . You are receiving this because you were assigned.Message ID: ***@***.***>

stuartmorgan-g

There's one unaddressed comment from the last round, but otherwise LGTM!

stuartmorgan-g · 2024-08-12T15:00:34Z

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


-  private void addJsonMarker(Map<String, ?> marker) {
+  private void addMarker(Messages.PlatformMarker marker) {
    if (marker == null) {


I don't see any changes or response to this.

reidbaker · 2024-08-13T15:08:11Z

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

+    this.visible = true,
+    this.strokeWidth = 10,
+    this.zIndex = 0.0,
+    this.radius = 0,


@yaakovschectman did you and stuart agree not to make these values required?

reidbaker · 2024-08-13T15:08:52Z

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

+  final bool consumeTapEvents;
+  final bool draggable;
+  final bool flat;
+  final Object icon;


This is still missing a note and TODO

reidbaker · 2024-08-13T15:09:08Z

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

+    this.rotation = 0.0,
+    this.visible = true,
+    this.zIndex = 0.0,
+    this.clusterManagerId,


Should there be a note here? (my guess is no since id is a string but thought it was worth asking.

yaakovschectman · 2024-08-13T15:15:55Z

@reidbaker If I am interpreting correctly, Stuart stated his note on whether the fields are required was optional, and approved the PR. I do not see why we would need a note for clusterManagerId, or what that note would be.

stuartmorgan-g · 2024-08-13T15:17:28Z

This is still missing a note and TODO

Oops, I missed that in my re-review. Thanks for the catch!

If I am interpreting correctly, Stuart stated his note on whether the fields are required was optional, and approved the PR.

Yes, I'm fine with there being default values. I have a very slight preference for required since it means we can't forget to carry values over, but the real solution to that is unit tests anyway.

stuartmorgan-g · 2024-08-13T15:20:22Z

@reidbaker For the ID, are you asking because the Dart code uses specialized wrapper objects for each ID type? If so, I've been ignoring that at the Pigeon level since I think it's more busywork than benefit at the level of a transfer protocol, and just using strings directly. The objects have more value at the client-code level where things are being stored, passed around, etc. and could plausibly be passed back to the wrong APIs later by buggy client code.

reidbaker · 2024-08-13T15:22:57Z

@reidbaker For the ID, are you asking because the Dart code uses specialized wrapper objects for each ID type? If so, I've been ignoring that at the Pigeon level since I think it's more busywork than benefit at the level of a transfer protocol, and just using strings directly. The objects have more value at the client-code level where things are being stored, passed around, etc. and could plausibly be passed back to the wrong APIs later by buggy client code.

I was asking because you had a comment saying "same here" and I was trying to figure out what comment applied.

stuartmorgan-g · 2024-08-13T15:25:34Z

@reidbaker For the ID, are you asking because the Dart code uses specialized wrapper objects for each ID type? If so, I've been ignoring that at the Pigeon level since I think it's more busywork than benefit at the level of a transfer protocol, and just using strings directly. The objects have more value at the client-code level where things are being stored, passed around, etc. and could plausibly be passed back to the wrong APIs later by buggy client code.

I was asking because you had a comment saying "same here" and I was trying to figure out what comment applied.

Oh, I see it inline now. I still don't understand how GitHub displays responses sometimes 😐

I should have been clearer about what the "same" was; I meant it to refer to not needing default values for all the fields in that object, not about the comment. Sorry for the confusion!

…geon (flutter/packages#7326)

flutter/packages@e4f2247...86d15a6 2024-08-15 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.26.0 to 3.26.2 (flutter/packages#7417) 2024-08-15 tarrinneal@gmail.com [shared_preferences] remove testing section from README (flutter/packages#7410) 2024-08-13 nathan.wilson1232@gmail.com Fix breakages introduced by `SplashFactory` (flutter/packages#6952) 2024-08-13 engine-flutter-autoroll@skia.org Manual roll Flutter from 2afc452 to cc13cd1 (6 revisions) (flutter/packages#7404) 2024-08-13 109111084+yaakovschectman@users.noreply.github.com [google_maps_flutter_android] Convert Circle and Marker to Pigeon (flutter/packages#7326) 2024-08-13 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#7379) 2024-08-13 49699333+dependabot[bot]@users.noreply.github.com [file_selector]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/file_selector/file_selector_android/android (flutter/packages#7381) 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 flutter-ecosystem@google.com,rmistry@google.com 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

yaakovschectman added 5 commits August 6, 2024 14:35

Specify circle and marker in pigeon

960395f

Update from json to pigeon

2de03da

Convert marker

498a71e

Update tests (not yet run)

f7295ec

Update java tests

b726dff

github-actions bot added p: google_maps_flutter platform-android labels Aug 6, 2024

yaakovschectman and others added 7 commits August 6, 2024 17:29

Public InfoWindow.toJson

089ffad

Update versioning

a094266

Federeated plugin path deps

9984cc9

Analysis

a0034dc

Null issues

bcd1dc0

Disect tests

4580b3b

Merge branch 'main' into android_pigeon_circle

a96d5bb

yaakovschectman marked this pull request as ready for review August 7, 2024 21:42

yaakovschectman requested review from reidbaker and stuartmorgan-g as code owners August 7, 2024 21:42

yaakovschectman requested review from gmackall, jokerttu, reidbaker and stuartmorgan-g and removed request for reidbaker and stuartmorgan-g August 7, 2024 21:42

yaakovschectman self-assigned this Aug 7, 2024

BUilder test args

12c24da

stuartmorgan-g requested changes Aug 8, 2024

View reviewed changes

Fix up tests

b3a9125

yaakovschectman and others added 4 commits August 9, 2024 16:18

Dummy bitmap

6a46050

Supply anchor

40e4651

Update packages/google_maps_flutter/google_maps_flutter_android/andro…

04452e3

…id/src/main/java/io/flutter/plugins/googlemaps/Convert.java Co-authored-by: Reid Baker <hamilton.reid.baker@gmail.com>

Mock BitmapDescriptorFactoryWrapper

39a0369

yaakovschectman force-pushed the android_pigeon_circle branch from fdc0b3c to 39a0369 Compare August 9, 2024 20:30

Formatting

a4425a5

stuartmorgan-g approved these changes Aug 12, 2024

View reviewed changes

NonNull

2f2d85f

reidbaker approved these changes Aug 13, 2024

View reviewed changes

Comment

aab39e8

yaakovschectman and others added 2 commits August 13, 2024 14:30

Format

68f69b4

Merge branch 'main' into android_pigeon_circle

b67683a

yaakovschectman merged commit cec00a5 into flutter:main Aug 13, 2024

engine-flutter-autoroll mentioned this pull request Aug 14, 2024

Roll Packages from e4f2247c3f41 to 66e98d826961 (5 revisions) flutter/flutter#153442

Closed

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

cec00a5 [google_maps_flutter_android] Convert Circle and Marker to Pi…

0a02ca5

…geon (flutter/packages#7326)

engine-flutter-autoroll mentioned this pull request Aug 14, 2024

Roll Packages from e4f2247c3f41 to 66e98d826961 (5 revisions) flutter/flutter#153452

Closed

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

cec00a5 [google_maps_flutter_android] Convert Circle and Marker to Pi…

c773ffa

…geon (flutter/packages#7326)

engine-flutter-autoroll mentioned this pull request Aug 15, 2024

Roll Packages from e4f2247c3f41 to 86d15a65d091 (7 revisions) flutter/flutter#153499

Merged

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

cec00a5 [google_maps_flutter_android] Convert Circle and Marker to Pi…

b923a80

…geon (flutter/packages#7326)

[google_maps_flutter_android] Convert Circle and Marker to Pigeon #7326

[google_maps_flutter_android] Convert Circle and Marker to Pigeon #7326

Uh oh!

Conversation

yaakovschectman commented Aug 6, 2024

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

stuartmorgan-g commented Aug 8, 2024

Uh oh!

reidbaker commented Aug 9, 2024

Uh oh!

stuartmorgan-g commented Aug 12, 2024

Uh oh!

yaakovschectman commented Aug 12, 2024 via email

Uh oh!

stuartmorgan-g left a comment

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!

yaakovschectman commented Aug 13, 2024

Uh oh!

stuartmorgan-g commented Aug 13, 2024

Uh oh!

stuartmorgan-g commented Aug 13, 2024

Uh oh!

reidbaker commented Aug 13, 2024

Uh oh!

stuartmorgan-g commented Aug 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants