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.

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

[google_maps_flutter] Semi-convert remaining iOS host API calls to Pigeon #7079

Merged

Conversation

stuartmorgan
Copy link
Contributor

Does a "shallow" Pigeon conversion of the remaining host API calls in the iOS implementation. All of the actual method calls are now Pigeon, but for the most part the data structures are not yet converted, and the Pigeon representations of the map objects are instead placeholders that currently just wrap the existing JSON serialization. This is done for a few reasons:

  • Keeps the incremental PR relatively small and easy to understand.
  • Quickly gets us to a state where any new APIs added will automatically use Pigeon, reducing further accumulation of technical debt.
  • Avoids duplication of handling code until [pigeon] Make the codec public flutter#150631 is resolved. As noted in a TODO added in this PR, almost all of the data structures that are passed in these methods are also passed through the PlatformView factory constructor, and Pigeon doesn't yet support using the Pigeon codec in non-Pigeon-generated code, so we would need to have both the structured and JSON handler for all of these objects if we converted them now.

Future PRs will be able to incrementally convert each map object to a structured form. Converting the Flutter APIs (Java->Dart) will also be done in a follow-up, to limit the scope of this PR.

This is the iOS equivalent of #6980, and the Dart code is largely the same.

Part of flutter/flutter#117907

Pre-launch Checklist

@@ -188,18 +183,18 @@ - (void)testMapViewTypeFromTypeValue {
XCTAssertEqual(kGMSTypeNone, [FLTGoogleMapJSONConversions mapViewTypeFromTypeValue:@5]);
}

- (void)testCameraUpdateFromChannelValueNewCameraPosition {
- (void)testcameraUpdateFromArrayNewCameraPosition {
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
- (void)testcameraUpdateFromArrayNewCameraPosition {
- (void)testCameraUpdateFromArrayNewCameraPosition {

@@ -503,19 +337,23 @@ - (void)setMyLocationButtonEnabled:(BOOL)enabled {
self.mapView.settings.myLocationButton = enabled;
}

/// Sets the map style, returing any error string as well as storing that error in `mapStyle` for
/// later access.
- (NSString *)setMapStyle:(NSString *)mapStyle {
Copy link
Member

Choose a reason for hiding this comment

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

Is the return string needed any more since you added the styleError setter side-effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically no, but I think it's easier to understand return [self.controller setMapStyle:style]; in the Pigeon method implementation than:

[self.controller setMapStyle:style];
// This value has to be read after the call above because it's set as a side effect of that call.
return self.styleError;

@stuartmorgan stuartmorgan requested a review from jmagman July 9, 2024 01:41
@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 9, 2024
@auto-submit auto-submit bot merged commit edb38e0 into flutter:main Jul 9, 2024
74 checks passed
@stuartmorgan stuartmorgan deleted the maps-pigeon-ios-host-api-shallow branch July 9, 2024 18:53
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 10, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 10, 2024
flutter/packages@14341d1...ea35fc6

2024-07-10 [email protected] [camera_avfoundation] Adds Swift Package Manager compatibility (flutter/packages#7080)
2024-07-10 [email protected] [webview_flutter_wkwebview] Adds Swift Package Manager compatibility (flutter/packages#7091)
2024-07-10 [email protected] [webview_flutter_web] Migrate to package:web. (flutter/packages#6792)
2024-07-10 [email protected] [camera] Clean up `maxDuration` code (flutter/packages#7039)
2024-07-10 [email protected] Update espresso dependencies (flutter/packages#7048)
2024-07-09 [email protected] [camera] Fix iOS torch mode regression (flutter/packages#7085)
2024-07-09 [email protected] [google_maps_flutter] Convert Obj-C->Dart calls to Pigeon (flutter/packages#7086)
2024-07-09 [email protected] Roll Flutter from fafd67d to 5103d75 (27 revisions) (flutter/packages#7084)
2024-07-09 [email protected] [camera_avfoundation] fix sample times not being numeric after pause/resume. (flutter/packages#6897)
2024-07-09 [email protected] [camera] Convert Windows to Pigeon (flutter/packages#6925)
2024-07-09 [email protected] [camera] Deprecate `maxDuration` in platform interface (flutter/packages#7078)
2024-07-09 [email protected] [google_maps_flutter] Semi-convert remaining iOS host API calls to Pigeon (flutter/packages#7079)
2024-07-09 [email protected] [path_provider] Remove `win32` (flutter/packages#7073)
2024-07-08 [email protected] [google_maps_flutter] Move iOS inspector to Pigeon (flutter/packages#6937)
2024-07-08 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump com.android.tools.build:gradle from 7.3.0 to 8.5.0 in /packages/camera/camera_android_camerax/android (flutter/packages#7072)
2024-07-08 49699333+dependabot[bot]@users.noreply.github.com [local_auth]: Bump com.android.tools.build:gradle from 7.3.1 to 8.5.0 in /packages/local_auth/local_auth_android/android (flutter/packages#7069)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_maps_flutter platform-ios
Projects
None yet
2 participants