[google_maps_flutter_android] Convert `PlatformCameraUpdate` to pigeon. #7507

yaakovschectman · 2024-08-26T17:47:29Z

Because this also converts the platform interface Camera type, this will need to be a federated change, unless we come up with a better idea of how to convert that class.

flutter/flutter#152928

Pre-launch Checklist

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

reidbaker · 2024-08-26T20:00:41Z

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

+    }
+    Messages.PlatformNewLatLng newLatLng = update.getNewLatLng();
+    if (newLatLng != null) {
+      return CameraUpdateFactory.newLatLng(latLngFromPigeon(newLatLng.getLatLng()));


I may not be following the types here but is it possible to make this CameraUpdateFactory.newLatLng(latLngFromPigeon(newLatLng); It seems, based on names alone that we should not need so many unboxing/reboxing of lat long data.

newLatLng is an instance of PlatformNewLatLng, which represents the camera move update to the new position. latLngFromPigeon accepts a PlatformLatLng, which is just a boxed pair of numeric coordinates, not a PlatformNewLatLng. I intend to add a CameraUpdate prefix to the relevant classes, including PlatformNewLatLng, as per Stuart's comment, which I believe will make this more clear.

reidbaker · 2024-08-26T20:04:01Z

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

-        throw new IllegalArgumentException("Cannot interpret " + o + " as CameraUpdate");
+  static CameraUpdate cameraUpdateFromPigeon(Messages.PlatformCameraUpdate update, float density) {
+    Messages.PlatformNewCameraPosition newCameraPosition = update.getNewCameraPosition();
+    if (newCameraPosition != null) {


I see how this code is a conversion from what was here before but I think the if statement make it harder to read what should happen. Before the toCameraUpdate method either updated the camera position, the lat/lng or the bounds. Now that is much harder to understand.

Try restructuring this method to make it easier for a human reader and/or adding java doc comments.

reidbaker · 2024-08-26T20:06:38Z

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

+
+  static PlatformCameraUpdate _platformCameraUpdateFromCameraUpdate(
+      CameraUpdate update) {
+    if (update is NewCameraPosition) {


You might have discussed this with @stuartmorgan if so feel free to resolve but can cameraUpdate include an enum of the type of update instead of these long object type checks?

This was discussed a bit for the Pigeon classes (where I don't think the enum probably helps us), but not for the Dart code.

I think for Dart, where we will be creating a new permanent public API surface, it would be better to add a type enum, and to make all of these classes private, so that we're not locking ourselves into a specific class structure forever.

If these classes were private, would that not then make their members inaccessible in this file?

🤦🏻

Sorry, I was thinking about mirroring the Java SDK's approach, but there they only need to consume the properties internally to the SDK.

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

stuartmorgan-g · 2024-08-27T14:22:52Z

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

+
+  static PlatformCameraUpdate _platformCameraUpdateFromCameraUpdate(
+      CameraUpdate update) {
+    if (update is NewCameraPosition) {


This was discussed a bit for the Pigeon classes (where I don't think the enum probably helps us), but not for the Dart code.

I think for Dart, where we will be creating a new permanent public API surface, it would be better to add a type enum, and to make all of these classes private, so that we're not locking ourselves into a specific class structure forever.

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

reidbaker · 2024-09-05T20:53:21Z

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

+      Messages.PlatformCameraUpdateZoom zoom = (Messages.PlatformCameraUpdateZoom) cameraUpdate;
+      return (zoom.getOut()) ? CameraUpdateFactory.zoomOut() : CameraUpdateFactory.zoomIn();
+    }
+    throw new IllegalArgumentException("PlatformCameraUpdate must have one non-null member.");


This error message does not make sense to me since the throw is not related to a value being null.

It made sense before, but now I need to update it.

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

reidbaker · 2024-09-05T20:55:50Z

Please hold on @stuartmorgan's approval as well.

stuartmorgan-g

Some comments, but nothing structural at this point. Feel free to split google_maps_flutter_platform_interface out into its own PR and just addressing the testing in that PR.

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

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

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

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

packages/google_maps_flutter/google_maps_flutter/example/pubspec.yaml

stuartmorgan-g

(I forgot to switch from Comment to Request Changes to reflect the comments.)

yaakovschectman · 2024-09-06T17:37:54Z

Some comments, but nothing structural at this point. Feel free to split google_maps_flutter_platform_interface out into its own PR and just addressing the testing in that PR.

@stuartmorgan I do not think we can address the testing in the PR that updates the platform interface. The tests would be in the android package, and no PR can land that refers to changes in the platform interface until the platform interface's package's PR lands. Perhaps I have misunderstood what you mean..
It occurs to me now you are probably suggesting that the unit tests for each case of CameraUpdate would go in the platform_interface package, rendering my above point moot.

…rived classes to use structured data (#7596) Platform interface part of #7507 flutter/flutter#152928 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] page, which explains my responsibilities. - [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use `dart format`.) - [ ] I signed the [CLA]. - [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. `[shared_preferences]` - [x] I [linked to at least one issue that this PR fixes] in the description above. - [x] I updated `pubspec.yaml` with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated `CHANGELOG.md` to add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes]. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md [Tree Hygiene]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md [relevant style guides]: https://github.com/flutter/packages/blob/main/CONTRIBUTING.md#style [CLA]: https://cla.developers.google.com/ [Discord]: https://github.com/flutter/flutter/blob/master/docs/contributing/Chat.md [linked to at least one issue that this PR fixes]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#overview [pub versioning philosophy]: https://dart.dev/tools/pub/versioning [exempt from version changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version [following repository CHANGELOG style]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog-style [exempt from CHANGELOG changes]: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changelog [test-exempt]: https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#tests

# Conflicts: # packages/google_maps_flutter/google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Messages.java # packages/google_maps_flutter/google_maps_flutter_android/lib/src/messages.g.dart

stuartmorgan-g

Thanks for the expanded testing and Java cleanup! Mostly looks great, just a few comments.

packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart

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

reidbaker · 2024-09-10T13:35:13Z

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

-  private static LatLngBounds toLatLngBounds(Object o) {
-    if (o == null) {
+  @Nullable
+  static Point pointFromPigeon(@Nullable Messages.PlatformOffset point, float density) {


@stuartmorgan is it a normal pattern to have "from" methods in pigeon conversion? As I am reading this pr I find that I very much do not care that the conversions are coming from pidgeon and instead care what the object is being converted into.

FWIW I think the answer is yes because i see a bunch of from methods but I did want to call out my confusion.

I believe I established that naming convention in this file. The problem is that during this transition there have been a bunch of cases where there are two different version of all the methods: one that converts method channel primitives, for the unconverted code, and one that converts Pigeon objects, but both to the same object. E.g., the toPoint method that's finally able to be removed in this PR.

In that context, the fact that the source object is Pigeon is very important. Especially because the old methods take Object which means you can accidentally pass a Pigeon object to them (which will then crash).

I would have no objection to mass renaming these when the conversion is done and the old versions don't exist any more. Or we could mass rename the remaining old methods now to something that clearly identifies them, and then mass rename the Pigeon versions to use the generic names.

That makes sense and makes me glad I asked the question.

@stuartmorgan Do you have any remaining concerns?

stuartmorgan-g

LGTM

…to pigeon. (flutter/packages#7507)

flutter/packages@91caa7a...330581f 2024-09-12 [email protected] Refactor the test benchmark app to make the example easier to follow (flutter/packages#7640) 2024-09-12 [email protected] Fix an if statement with resumed video players on Android. (flutter/packages#7641) 2024-09-12 [email protected] Roll Flutter from 2e221e7 to 303f222 (77 revisions) (flutter/packages#7638) 2024-09-12 [email protected] [google_maps_flutter_android] Convert `PlatformCameraUpdate` to pigeon. (flutter/packages#7507) 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

yaakovschectman requested review from loic-sharma and gmackall August 26, 2024 17:47

yaakovschectman self-assigned this Aug 26, 2024

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

yaakovschectman marked this pull request as ready for review August 26, 2024 19:38

yaakovschectman requested review from reidbaker and stuartmorgan-g as code owners August 26, 2024 19:38

reidbaker reviewed Aug 26, 2024

View reviewed changes

stuartmorgan-g reviewed Aug 27, 2024

View reviewed changes

yaakovschectman requested review from stuartmorgan-g, tarrinneal and reidbaker September 5, 2024 20:44

reidbaker approved these changes Sep 5, 2024

View reviewed changes

stuartmorgan-g reviewed Sep 5, 2024

View reviewed changes

stuartmorgan-g requested changes Sep 6, 2024

View reviewed changes

yaakovschectman mentioned this pull request Sep 6, 2024

[google_maps_flutter_platform_interface] Split CameraUpdate into derived classes to use structured data #7596

Merged

11 tasks

yaakovschectman added 10 commits September 9, 2024 14:04

Path based deps

e0762a1

Define pigeon classes

e52567b

Gross quasi union type

c59b6f4

# Conflicts: # packages/google_maps_flutter/google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Messages.java # packages/google_maps_flutter/google_maps_flutter_android/lib/src/messages.g.dart

Port dart

8bfcb9b

Refactor out zoom subtypes

68b1cdf

Formatting

2b612b9

Upgrade Pigeon

6b75ce6

Update pigeons

a0f9bab

Java

e54cc9d

Update package for platform interface change

d951404

yaakovschectman and others added 7 commits September 9, 2024 14:07

Format

497af2a

Comment on Object field

0c81c8c

Update Convert.java

312aeb4

Remove dead code

ff1e466

Expand unit tests to PlatformCameraUpdate cases

69c361c

Formatting

b2a664d

Remove path dependency

0ee9e29

yaakovschectman force-pushed the pigeon_cam_update branch from 931ac13 to 0ee9e29 Compare September 9, 2024 18:38

yaakovschectman and others added 4 commits September 9, 2024 14:39

Remove path dep

cdecbae

Formatting

e80cae6

Merge branch 'main' into pigeon_cam_update

f507163

Versioning

e28d715

yaakovschectman requested a review from stuartmorgan-g September 9, 2024 19:53

stuartmorgan-g requested changes Sep 9, 2024

View reviewed changes

reidbaker reviewed Sep 10, 2024

View reviewed changes

reidbaker approved these changes Sep 10, 2024

View reviewed changes

yaakovschectman and others added 2 commits September 10, 2024 14:15

Rename unit tests

a798791

Merge branch 'main' into pigeon_cam_update

686b2d6

yaakovschectman requested a review from stuartmorgan-g September 10, 2024 18:41

yaakovschectman and others added 2 commits September 11, 2024 14:17

Merge branch 'main' into pigeon_cam_update

0fe823a

Update interface dependency

5fea8f4

stuartmorgan-g approved these changes Sep 12, 2024

View reviewed changes

yaakovschectman merged commit 11b339e into flutter:main Sep 12, 2024
76 checks passed

engine-flutter-autoroll mentioned this pull request Sep 13, 2024

Roll Packages from 91caa7a6e3dd to 330581face4b (4 revisions) flutter/flutter#155161

Closed

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

11b339e [google_maps_flutter_android] Convert PlatformCameraUpdate …

49f1602

…to pigeon. (flutter/packages#7507)

engine-flutter-autoroll mentioned this pull request Sep 13, 2024

Roll Packages from 91caa7a6e3dd to 330581face4b (4 revisions) flutter/flutter#155171

Merged

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

11b339e [google_maps_flutter_android] Convert PlatformCameraUpdate …

55ba434

…to pigeon. (flutter/packages#7507)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[google_maps_flutter_android] Convert `PlatformCameraUpdate` to pigeon. #7507

[google_maps_flutter_android] Convert `PlatformCameraUpdate` to pigeon. #7507

yaakovschectman commented Aug 26, 2024

reidbaker Aug 26, 2024

yaakovschectman Aug 27, 2024

reidbaker Aug 26, 2024

reidbaker Aug 26, 2024

stuartmorgan-g Aug 27, 2024

yaakovschectman Aug 27, 2024

stuartmorgan-g Aug 28, 2024

stuartmorgan-g Aug 27, 2024

reidbaker Sep 5, 2024

yaakovschectman Sep 5, 2024

reidbaker commented Sep 5, 2024

stuartmorgan-g left a comment

stuartmorgan-g left a comment

yaakovschectman commented Sep 6, 2024 •

edited

Loading

stuartmorgan-g left a comment

reidbaker Sep 10, 2024 •

edited

Loading

stuartmorgan-g Sep 10, 2024 •

edited

Loading

reidbaker Sep 10, 2024

yaakovschectman Sep 12, 2024

stuartmorgan-g left a comment

[google_maps_flutter_android] Convert PlatformCameraUpdate to pigeon. #7507

[google_maps_flutter_android] Convert PlatformCameraUpdate to pigeon. #7507

Conversation

yaakovschectman commented Aug 26, 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

reidbaker commented Sep 5, 2024

stuartmorgan-g left a comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

yaakovschectman commented Sep 6, 2024 • edited Loading

stuartmorgan-g left a comment

Choose a reason for hiding this comment

reidbaker Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan-g Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan-g left a comment

Choose a reason for hiding this comment

[google_maps_flutter_android] Convert `PlatformCameraUpdate` to pigeon. #7507

[google_maps_flutter_android] Convert `PlatformCameraUpdate` to pigeon. #7507

yaakovschectman commented Sep 6, 2024 •

edited

Loading

reidbaker Sep 10, 2024 •

edited

Loading

stuartmorgan-g Sep 10, 2024 •

edited

Loading