[google_maps_flutter_android] Add PlatformCap pigeon type.
#7639
Conversation
yaakovschectman
requested review from
reidbaker and
stuartmorgan-g
as code owners
...ges/google_maps_flutter/google_maps_flutter_android/lib/src/google_maps_flutter_android.dart
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Instead of saying "the Cap class" consider linking to the source of that class.
I believe this is the correct class. https://developers.google.com/maps/documentation/android-sdk/reference/com/google/android/libraries/maps/model/Cap
There was a problem hiding this comment.
Should this link to google map's SDK's Cap? I would have thought a link to the platform interface's Cap would be more appropriate, but do you disagree?
There was a problem hiding this comment.
I think the fact that you and I were thinking of different classes is a great explanation of why "the cap class" is not the right level of documentation.
There was a problem hiding this comment.
I've linked here to the platform interface Cap, and from a couple other comments in this PR to the Maps SDK's Cap. Let me know if you agree with my choice of links.
There was a problem hiding this comment.
Link to documentation so that someone can better understand these values.
There was a problem hiding this comment.
No action required: You are following the pattern of the rest of this file but both this method name and documentation do not help me understand what is being converted here or how to learn about either the to or from object.
There was a problem hiding this comment.
We could trivially improve them all a fair amount by using [] references for both class names. That was just an oversight on my part when I started converting.
There was a problem hiding this comment.
Please use dart doc linking notation for the classes.
There was a problem hiding this comment.
Link to cap documentation.
There was a problem hiding this comment.
We could trivially improve them all a fair amount by using [] references for both class names. That was just an oversight on my part when I started converting.
yaakovschectman
force-pushed
the
pigeon_cap
branch
from
b0c553d to
e78e7bc
Compare
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
LGTM with one incorrect comment removed.
| // any time, so provide a fallback that ensures this won't break when used | ||
| // with a version that contains new values. This is deliberately outside | ||
| // the switch rather than a `default` so that the linter will flag the | ||
| // switch as needing an update. |
There was a problem hiding this comment.
This last sentence does not apply in this context. String-value enums cannot be exhaustive, so the linter will never flag this.
...google_maps_flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/Convert.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Please use dart doc linking notation for the classes.
...s/google_maps_flutter/google_maps_flutter_android/test/google_maps_flutter_android_test.dart
Show resolved
Hide resolved
|
The last updates look to me like they address Reid's feedback, so I'm dismissing his change request to unblock landing since he's OOO for the moment. |
flutter/packages@9d00fb1...f1a3da2 2024-10-09 matanlurey@users.noreply.github.com Remove additional (harmless but annoying) native stack traces. (flutter/packages#7837) 2024-10-09 engine-flutter-autoroll@skia.org Roll Flutter from 0917e9d to 2d45fb3 (50 revisions) (flutter/packages#7836) 2024-10-09 109111084+yaakovschectman@users.noreply.github.com [camera_android] Begin conversion to Pigeon (flutter/packages#7755) 2024-10-09 109111084+yaakovschectman@users.noreply.github.com [google_maps_flutter_android] Add `PlatformCap` pigeon type. (flutter/packages#7639) 2024-10-09 43054281+camsim99@users.noreply.github.com [camerax] Revert "Explicitly remove READ_EXTERNAL_STORAGE permission" (flutter/packages#7826) 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 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
3 participants
Replace old JSON implementation of polyline start/end cap with a structured Pigeon.
flutter/flutter#154737
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.