Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[google_maps_flutter_android] Convert PlatformCameraUpdate to pigeon. #7507

Merged
merged 25 commits into from
Sep 12, 2024
Merged

[google_maps_flutter_android] Convert PlatformCameraUpdate to pigeon. #7507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e0762a1
Path based deps
yaakovschectman Aug 22, 2024
e52567b
Define pigeon classes
yaakovschectman Aug 22, 2024
c59b6f4
Gross quasi union type
yaakovschectman Aug 22, 2024
8bfcb9b
Port dart
yaakovschectman Aug 26, 2024
68b1cdf
Refactor out zoom subtypes
yaakovschectman Aug 26, 2024
2b612b9
Formatting
yaakovschectman Aug 26, 2024
6b75ce6
Upgrade Pigeon
yaakovschectman Aug 30, 2024
a0f9bab
Update pigeons
yaakovschectman Aug 30, 2024
e54cc9d
Java
yaakovschectman Aug 30, 2024
d951404
Update package for platform interface change
yaakovschectman Aug 30, 2024
497af2a
Format
yaakovschectman Aug 30, 2024
0c81c8c
Comment on Object field
yaakovschectman Sep 4, 2024
312aeb4
Update Convert.java
yaakovschectman Sep 5, 2024
ff1e466
Remove dead code
yaakovschectman Sep 9, 2024
69c361c
Expand unit tests to PlatformCameraUpdate cases
yaakovschectman Sep 9, 2024
b2a664d
Formatting
yaakovschectman Sep 9, 2024
0ee9e29
Remove path dependency
yaakovschectman Sep 9, 2024
cdecbae
Remove path dep
yaakovschectman Sep 9, 2024
e80cae6
Formatting
yaakovschectman Sep 9, 2024
f507163
Merge branch 'main' into pigeon_cam_update
yaakovschectman Sep 9, 2024
e28d715
Versioning
yaakovschectman Sep 9, 2024
a798791
Rename unit tests
yaakovschectman Sep 10, 2024
686b2d6
Merge branch 'main' into pigeon_cam_update
yaakovschectman Sep 10, 2024
0fe823a
Merge branch 'main' into pigeon_cam_update
yaakovschectman Sep 11, 2024
5fea8f4
Update interface dependency
yaakovschectman Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/google_maps_flutter/google_maps_flutter_android/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.14.6

* Converts 'PlatformCameraUpdate' to pigeon.

## 2.14.5

* Converts `JointType` to enum.
Expand Down
138 changes: 59 additions & 79 deletions ...gle_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -316,10 +315,6 @@ public static BitmapDescriptor getBitmapFromAsset(
return bitmapDescriptorFactory.fromAsset(assetKey);
}

private static boolean toBoolean(Object o) {
return (Boolean) o;
}

static @NonNull CameraPosition cameraPositionFromPigeon(
@NonNull Messages.PlatformCameraPosition position) {
final CameraPosition.Builder builder = CameraPosition.builder();
Expand All @@ -330,47 +325,57 @@ private static boolean toBoolean(Object o) {
return builder.build();
}

static CameraPosition toCameraPosition(Object o) {
final Map<?, ?> data = toMap(o);
final CameraPosition.Builder builder = CameraPosition.builder();
builder.bearing(toFloat(data.get("bearing")));
builder.target(toLatLng(data.get("target")));
builder.tilt(toFloat(data.get("tilt")));
builder.zoom(toFloat(data.get("zoom")));
return builder.build();
}

static CameraUpdate toCameraUpdate(Object o, float density) {
final List<?> data = toList(o);
switch (toString(data.get(0))) {
case "newCameraPosition":
return CameraUpdateFactory.newCameraPosition(toCameraPosition(data.get(1)));
yaakovschectman marked this conversation as resolved.
Show resolved Hide resolved
case "newLatLng":
return CameraUpdateFactory.newLatLng(toLatLng(data.get(1)));
case "newLatLngBounds":
return CameraUpdateFactory.newLatLngBounds(
toLatLngBounds(data.get(1)), toPixels(data.get(2), density));
yaakovschectman marked this conversation as resolved.
Show resolved Hide resolved
case "newLatLngZoom":
return CameraUpdateFactory.newLatLngZoom(toLatLng(data.get(1)), toFloat(data.get(2)));
case "scrollBy":
return CameraUpdateFactory.scrollBy( //
toFractionalPixels(data.get(1), density), //
toFractionalPixels(data.get(2), density));
case "zoomBy":
if (data.size() == 2) {
return CameraUpdateFactory.zoomBy(toFloat(data.get(1)));
} else {
return CameraUpdateFactory.zoomBy(toFloat(data.get(1)), toPoint(data.get(2), density));
yaakovschectman marked this conversation as resolved.
Show resolved Hide resolved
}
case "zoomIn":
return CameraUpdateFactory.zoomIn();
case "zoomOut":
return CameraUpdateFactory.zoomOut();
case "zoomTo":
return CameraUpdateFactory.zoomTo(toFloat(data.get(1)));
default:
throw new IllegalArgumentException("Cannot interpret " + o + " as CameraUpdate");
}
static CameraUpdate cameraUpdateFromPigeon(Messages.PlatformCameraUpdate update, float density) {
Object cameraUpdate = update.getCameraUpdate();
if (cameraUpdate instanceof Messages.PlatformCameraUpdateNewCameraPosition) {
Messages.PlatformCameraUpdateNewCameraPosition newCameraPosition =
(Messages.PlatformCameraUpdateNewCameraPosition) cameraUpdate;
return CameraUpdateFactory.newCameraPosition(
cameraPositionFromPigeon(newCameraPosition.getCameraPosition()));
}
if (cameraUpdate instanceof Messages.PlatformCameraUpdateNewLatLng) {
Messages.PlatformCameraUpdateNewLatLng newLatLng =
(Messages.PlatformCameraUpdateNewLatLng) cameraUpdate;
return CameraUpdateFactory.newLatLng(latLngFromPigeon(newLatLng.getLatLng()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (cameraUpdate instanceof Messages.PlatformCameraUpdateNewLatLngZoom) {
Messages.PlatformCameraUpdateNewLatLngZoom newLatLngZoom =
(Messages.PlatformCameraUpdateNewLatLngZoom) cameraUpdate;
return CameraUpdateFactory.newLatLngZoom(
latLngFromPigeon(newLatLngZoom.getLatLng()), newLatLngZoom.getZoom().floatValue());
}
if (cameraUpdate instanceof Messages.PlatformCameraUpdateNewLatLngBounds) {
Messages.PlatformCameraUpdateNewLatLngBounds newLatLngBounds =
(Messages.PlatformCameraUpdateNewLatLngBounds) cameraUpdate;
return CameraUpdateFactory.newLatLngBounds(
latLngBoundsFromPigeon(newLatLngBounds.getBounds()),
(int) (newLatLngBounds.getPadding() * density));
}
if (cameraUpdate instanceof Messages.PlatformCameraUpdateScrollBy) {
Messages.PlatformCameraUpdateScrollBy scrollBy =
(Messages.PlatformCameraUpdateScrollBy) cameraUpdate;
return CameraUpdateFactory.scrollBy(
scrollBy.getDx().floatValue() * density, scrollBy.getDy().floatValue() * density);
}
if (cameraUpdate instanceof Messages.PlatformCameraUpdateZoomBy) {
Messages.PlatformCameraUpdateZoomBy zoomBy =
(Messages.PlatformCameraUpdateZoomBy) cameraUpdate;
final Point focus = pointFromPigeon(zoomBy.getFocus(), density);
return (focus != null)
? CameraUpdateFactory.zoomBy(zoomBy.getAmount().floatValue(), focus)
: CameraUpdateFactory.zoomBy(zoomBy.getAmount().floatValue());
}
if (cameraUpdate instanceof Messages.PlatformCameraUpdateZoomTo) {
Messages.PlatformCameraUpdateZoomTo zoomTo =
(Messages.PlatformCameraUpdateZoomTo) cameraUpdate;
return CameraUpdateFactory.zoomTo(zoomTo.getZoom().floatValue());
}
if (cameraUpdate instanceof Messages.PlatformCameraUpdateZoom) {
Messages.PlatformCameraUpdateZoom zoom = (Messages.PlatformCameraUpdateZoom) cameraUpdate;
return (zoom.getOut()) ? CameraUpdateFactory.zoomOut() : CameraUpdateFactory.zoomIn();
}
throw new IllegalArgumentException(
"PlatformCameraUpdate's cameraUpdate field must be one of the PlatformCameraUpdate... case classes.");
}

private static double toDouble(Object o) {
Expand Down Expand Up @@ -494,16 +499,16 @@ static Point pointFromPigeon(Messages.PlatformPoint point) {
return new Point(point.getX().intValue(), point.getY().intValue());
}

static Messages.PlatformPoint pointToPigeon(Point point) {
return new Messages.PlatformPoint.Builder().setX((long) point.x).setY((long) point.y).build();
}

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

@reidbaker reidbaker Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@stuartmorgan stuartmorgan Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartmorgan Do you have any remaining concerns?

if (point == null) {
return null;
}
final List<?> data = toList(o);
return new LatLngBounds(toLatLng(data.get(0)), toLatLng(data.get(1)));
return new Point((int) (point.getDx() * density), (int) (point.getDy() * density));
reidbaker marked this conversation as resolved.
Show resolved Hide resolved
}

static Messages.PlatformPoint pointToPigeon(Point point) {
return new Messages.PlatformPoint.Builder().setX((long) point.x).setY((long) point.y).build();
}

private static List<?> toList(Object o) {
Expand All @@ -514,26 +519,6 @@ private static List<?> toList(Object o) {
return (Map<?, ?>) o;
}

private static Map<String, Object> toObjectMap(Object o) {
Map<String, Object> hashMap = new HashMap<>();
Map<?, ?> map = (Map<?, ?>) o;
for (Object key : map.keySet()) {
Object object = map.get(key);
if (object != null) {
hashMap.put((String) key, object);
}
}
return hashMap;
}

private static float toFractionalPixels(Object o, float density) {
return toFloat(o) * density;
}

private static int toPixels(Object o, float density) {
return (int) toFractionalPixels(o, density);
}

private static Bitmap toBitmap(Object o) {
byte[] bmpData = (byte[]) o;
Bitmap bitmap = BitmapFactory.decodeByteArray(bmpData, 0, bmpData.length);
Expand Down Expand Up @@ -563,11 +548,6 @@ private static Bitmap toScaledBitmap(Bitmap bitmap, int width, int height) {
return bitmap;
}

private static Point toPoint(Object o, float density) {
final List<?> data = toList(o);
return new Point(toPixels(data.get(0), density), toPixels(data.get(1), density));
}

private static String toString(Object o) {
return (String) o;
}
Expand Down
4 changes: 2 additions & 2 deletions ...tter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,7 @@ public void moveCamera(@NonNull Messages.PlatformCameraUpdate cameraUpdate) {
throw new FlutterError(
"GoogleMap uninitialized", "moveCamera called prior to map initialization", null);
}
googleMap.moveCamera(Convert.toCameraUpdate(cameraUpdate.getJson(), density));
googleMap.moveCamera(Convert.cameraUpdateFromPigeon(cameraUpdate, density));
}

@Override
Expand All @@ -942,7 +942,7 @@ public void animateCamera(@NonNull Messages.PlatformCameraUpdate cameraUpdate) {
throw new FlutterError(
"GoogleMap uninitialized", "animateCamera called prior to map initialization", null);
}
googleMap.animateCamera(Convert.toCameraUpdate(cameraUpdate.getJson(), density));
googleMap.animateCamera(Convert.cameraUpdateFromPigeon(cameraUpdate, density));
}

@Override
Expand Down
Loading