[google_maps_flutter_android] Convert PlatformPolygon and PlatformPolyline to Pigeon
#7406
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yaakovschectman
requested review from
stuartmorgan-g,
loic-sharma and
gmackall
stuartmorgan-g
requested changes
Aug 15, 2024
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java
Show resolved
Hide resolved
| @@ -862,6 +795,15 @@ static List<LatLng> toPoints(Object o) { | |||
| return points; | |||
| } | |||
|
|
|||
| static List<LatLng> toPoints(List<Messages.PlatformLatLng> data) { | |||
There was a problem hiding this comment.
Please call this pointsFromPigeon, matching the style of some of the existing methods, so that we don't have two things with the same name, one of which takes Object, because it makes it hard to clearly reason about which one is being called since List<Messages.PlatformLatLng> is also an Object. Having clearly separable codepaths makes the Object versions less of a foot-gun for as long as we have to have them.
| final List<List<LatLng>> holes = new ArrayList<>(data.size()); | ||
|
|
||
| for (Object rawHole : data) { | ||
| for (List<Messages.PlatformLatLng> rawHole : data) { |
There was a problem hiding this comment.
Just hole now that this is a typed structure instead of an Object.
packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart
Show resolved
Hide resolved
| final int color; | ||
| final bool geodesic; | ||
| final int jointType; | ||
| // TODO(schectman): convert to structured data. |
There was a problem hiding this comment.
This should have detail about the exact meaning/source of the object, like the old json TODO.
stuartmorgan-g
requested changes
Aug 18, 2024
| @@ -784,7 +784,7 @@ static String interpretHeatmapOptions(Map<String, ?> data, HeatmapOptionsSink si | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| static List<LatLng> toPoints(Object o) { | |||
| static List<LatLng> pointsFromPigeon(Object o) { | |||
There was a problem hiding this comment.
You've renamed the non-Pigeon version to say it's for Pigeon. The Pigeon version is the one that takes a List<Messages.PlatformLatLng>.
There was a problem hiding this comment.
Hm, I thought there were still unconverted codepaths that used toPoints, but now that you've changed the names it looks like there aren't; it's marked VisibleForTesting, and there are no references I see within the file. If toPoints is dead code now, it should be removed.
There was a problem hiding this comment.
I see a reference to it remaining at
There was a problem hiding this comment.
That's a unit test, not production code.
packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart
Show resolved
Hide resolved
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 22, 2024
…PlatformPolyline` to Pigeon (flutter/packages#7406)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 22, 2024
…PlatformPolyline` to Pigeon (flutter/packages#7406)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 23, 2024
…PlatformPolyline` to Pigeon (flutter/packages#7406)
auto-submit bot
pushed a commit
to flutter/flutter
that referenced
this pull request
Aug 23, 2024
flutter/packages@c5d03ee...d862279 2024-08-22 jacksongardner@google.com Serve benchmarks with COOP/COEP headers in wasm mode. (flutter/packages#7423) 2024-08-21 109111084+yaakovschectman@users.noreply.github.com [google_maps_flutter_android] Convert `PlatformPolygon` and `PlatformPolyline` to Pigeon (flutter/packages#7406) 2024-08-21 tarrinneal@gmail.com [pigeon] simplifies int handling across the codec, verifies ints in collections (flutter/packages#7392) 2024-08-21 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 80c2e84 to 5874a72 (9 revisions) (flutter/packages#7472) 2024-08-21 49699333+dependabot[bot]@users.noreply.github.com [interactive_media_ads]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/interactive_media_ads/android (flutter/packages#7384) 2024-08-21 engine-flutter-autoroll@skia.org Roll Flutter from e7da16d to b8f89f7 (28 revisions) (flutter/packages#7471) 2024-08-21 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds a contribution doc (flutter/packages#7460) 2024-08-21 49699333+dependabot[bot]@users.noreply.github.com [webview]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/webview_flutter/webview_flutter_android/android (flutter/packages#7373) 2024-08-21 paulberry@google.com [script] Remove unnecessary breaks in default clauses of switch statements (flutter/packages#7469) 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
Buchimi
pushed a commit
to Buchimi/flutter
that referenced
this pull request
Sep 2, 2024
flutter/packages@c5d03ee...d862279 2024-08-22 jacksongardner@google.com Serve benchmarks with COOP/COEP headers in wasm mode. (flutter/packages#7423) 2024-08-21 109111084+yaakovschectman@users.noreply.github.com [google_maps_flutter_android] Convert `PlatformPolygon` and `PlatformPolyline` to Pigeon (flutter/packages#7406) 2024-08-21 tarrinneal@gmail.com [pigeon] simplifies int handling across the codec, verifies ints in collections (flutter/packages#7392) 2024-08-21 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 80c2e84 to 5874a72 (9 revisions) (flutter/packages#7472) 2024-08-21 49699333+dependabot[bot]@users.noreply.github.com [interactive_media_ads]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/interactive_media_ads/android (flutter/packages#7384) 2024-08-21 engine-flutter-autoroll@skia.org Roll Flutter from e7da16d to b8f89f7 (28 revisions) (flutter/packages#7471) 2024-08-21 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds a contribution doc (flutter/packages#7460) 2024-08-21 49699333+dependabot[bot]@users.noreply.github.com [webview]: Bump androidx.annotation:annotation from 1.8.1 to 1.8.2 in /packages/webview_flutter/webview_flutter_android/android (flutter/packages#7373) 2024-08-21 paulberry@google.com [script] Remove unnecessary breaks in default clauses of switch statements (flutter/packages#7469) 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace the old JSON-based encoding of polygons and polylines to use structured pigeon type.
flutter/flutter#152926
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.