[google_maps_flutter] Add ability to animate camera with duration #7648
Conversation
github-actions
bot
added
p: google_maps_flutter
platform-android
platform-ios
platform-web
labels
jokerttu
force-pushed
the
feature/animate-camera-duration
branch
from
fbf20bc
to
68e52df
Compare
jokerttu
changed the title
jokerttu
force-pushed
the
feature/animate-camera-duration
branch
from
68e52df
to
2c8caf1
Compare
jokerttu
force-pushed
the
feature/animate-camera-duration
branch
from
2c8caf1
to
2256a07
Compare
jokerttu
force-pushed
the
feature/animate-camera-duration
branch
from
2256a07
to
c39c721
Compare
|
Web support is not included, as there are still some areas open for discussion. |
jokerttu
requested review from
cbracken,
reidbaker,
ditman and
stuartmorgan
as code owners
stuartmorgan
added
the
federated: all_changes
| @@ -421,8 +421,20 @@ class MethodChannelGoogleMapsFlutter extends GoogleMapsFlutterPlatform { | |||
| CameraUpdate cameraUpdate, { | |||
| required int mapId, | |||
| }) { | |||
| return channel(mapId).invokeMethod<void>('camera#animate', <String, Object>{ | |||
| return channel(mapId).invokeMethod<void>('camera#animate', | |||
| <String, dynamic>{'cameraUpdate': cameraUpdate.toJson()}); | |||
There was a problem hiding this comment.
Why was this changed to dynamic? We only use dynamic when we have an extremely compelling reason to do so as it bypasses a lot of static analysis.
| 'cameraUpdate': cameraUpdate.toJson(), | ||
| 'duration': configuration.duration?.inMilliseconds, |
There was a problem hiding this comment.
We don't generally want to send nulls in collections in method channels.
I would recommend just reverting the changes to this file; we don't use the method channel implementation, it's only still here because removing it requires a breaking change. There's no obligation to implement new features for it.
| /// platform side. | ||
| Future<void> animateCameraWithConfiguration( | ||
| CameraUpdate cameraUpdate, { | ||
| required CameraUpdateAnimationConfiguration configuration, |
There was a problem hiding this comment.
This can be positional rather than named since it's a core parameter.
| required int mapId, | ||
| }) { | ||
| throw UnimplementedError( | ||
| 'animateCameraWithConfiguration() has not been implemented.'); |
There was a problem hiding this comment.
This should call animateCamera; see https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-platform-interface-method-parameters
There was a problem hiding this comment.
And the forwarding should get a unit test in this package.
| @@ -24,6 +24,7 @@ export 'package:google_maps_flutter_platform_interface/google_maps_flutter_platf | |||
| CameraPositionCallback, | |||
| CameraTargetBounds, | |||
| CameraUpdate, | |||
| CameraUpdateAnimationConfiguration, | |||
There was a problem hiding this comment.
We generally don't want to expose these parameter objects at the app-facing package API level; they solve a problem (adding optional parameter being a breaking change for subclasses) that doesn't exist at this level, and add complexity for all clients (vs. just ourselves in implementation packages). The client-facing API should expose the duration directly, and that should be packed into the configuration object as an internal implementation detail.
| @@ -103,6 +103,13 @@ class PlatformCameraUpdateZoomTo { | |||
| final double zoom; | |||
| } | |||
|
|
|||
| /// Pigeon representation of a CameraUpdateAnimationConfiguration. | |||
| class PlatformCameraUpdateAnimationConfiguration { | |||
There was a problem hiding this comment.
Pigeon interfaces are internal APIs, and can be changed at will, so there is no need to use parameter objects in general; the method can just take a durationMilliseconds parameter directly (same on iOS).
| // Assert that the camera zoom is updated after the animation completes | ||
| XCTAssertEqual(controller.mapView.camera.zoom, 10.0, | ||
| "Camera zoom should be updated to 10.0 after animation."); | ||
| } |
There was a problem hiding this comment.
Actually running animations is pretty slow for unit tests; could we just dependency inject a simple wrapper around CATransaction and ensure that's being called as expected, rather than testing the animation itself (which is largely testing the underlying SDK rather than the plugin code)?
| @@ -51,6 +51,13 @@ class PlatformCameraUpdate { | |||
| final Object json; | |||
| } | |||
|
|
|||
| /// Pigeon representation of a CameraUpdateAnimationConfiguration. | |||
| class PlatformCameraUpdateAnimationConfiguration { | |||
There was a problem hiding this comment.
Same note here as on Android.
| required CameraUpdateAnimationConfiguration configuration, | ||
| required int mapId, | ||
| }) { | ||
| return animateCamera(cameraUpdate, mapId: mapId); |
There was a problem hiding this comment.
This will need a TODO referencing an issue, as with the test disabling.
| null); | ||
| } else { | ||
| googleMap.animateCamera(Convert.cameraUpdateFromPigeon(cameraUpdate, density)); | ||
| } |
There was a problem hiding this comment.
There should be a native unit test validating the branching here (via a mock map).
Adds ability to configure camera animation duration on Android and iOS.
Resolves #39810
Resolves #44284
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.