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] Add ability to animate camera with duration #7648

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[google_maps_flutter] Add ability to animate camera with duration #7648

wants to merge 12 commits into from

Conversation

jokerttu
Copy link
Contributor

@jokerttu jokerttu commented Sep 14, 2024 •
edited
Loading

Adds ability to configure camera animation duration on Android and iOS.

Resolves #39810
Resolves #44284

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from fbf20bc to 68e52df Compare September 14, 2024 12:57
@jokerttu jokerttu changed the title [google_maps_flutter] add ability to animate camera with duration [google_maps_flutter] Add ability to animate camera with duration Sep 14, 2024
@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from 68e52df to 2c8caf1 Compare September 15, 2024 09:03
@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch 2 times, most recently from 2256a07 to c39c721 Compare November 7, 2024 16:41
@jokerttu jokerttu marked this pull request as ready for review November 7, 2024 16:58
@stuartmorgan stuartmorgan added the federated: all_changes PR that contains changes for all packages for a federated plugin change label Nov 7, 2024
stuartmorgan
stuartmorgan requested changes Nov 7, 2024
View reviewed changes
...ps_flutter_platform_interface/lib/src/method_channel/method_channel_google_maps_flutter.dart Outdated Show resolved Hide resolved
...ps_flutter_platform_interface/lib/src/method_channel/method_channel_google_maps_flutter.dart Outdated Show resolved Hide resolved
...maps_flutter_platform_interface/lib/src/platform_interface/google_maps_flutter_platform.dart Outdated Show resolved Hide resolved
...maps_flutter_platform_interface/lib/src/platform_interface/google_maps_flutter_platform.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/lib/google_maps_flutter.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart Outdated Show resolved Hide resolved
.../google_maps_flutter/google_maps_flutter_ios/example/ios14/ios/RunnerTests/GoogleMapsTests.m Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/pigeons/messages.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_web/lib/src/google_maps_flutter_web.dart Show resolved Hide resolved
...flutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java Show resolved Hide resolved
@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from 7c30a33 to 167b519 Compare November 11, 2024 14:13
@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from 167b519 to 6078134 Compare November 21, 2024 11:42
@jokerttu jokerttu mentioned this pull request Nov 21, 2024
Open
@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch 2 times, most recently from eb71ec7 to b6807ec Compare November 22, 2024 08:24
@stuartmorgan stuartmorgan added triage-ios Should be looked at in iOS triage triage-android Should be looked at in Android triage labels Nov 22, 2024
stuartmorgan
stuartmorgan requested changes Nov 22, 2024
View reviewed changes
...le_maps_flutter/google_maps_flutter_ios/example/ios14/integration_test/google_maps_test.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FGMCATransactionWrapper.h Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FGMCATransactionWrapper.h Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/FGMCATransactionWrapper.m Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapController.m Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapController_Test.h Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapController_Test.h Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/ios/Classes/GoogleMapController_Test.h Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter_ios/test/google_maps_flutter_ios_test.dart Outdated Show resolved Hide resolved
.../google_maps_flutter/google_maps_flutter_ios/example/ios14/ios/RunnerTests/GoogleMapsTests.m Show resolved Hide resolved
@stuartmorgan
Copy link
Contributor

My comments are all minor, so once the iOS and Android reviewers have signed off this will be ready to split out the first sub-PR.

@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from b6807ec to 6bc7717 Compare November 25, 2024 08:00
@jesswrd jesswrd requested review from a team and jesswrd November 26, 2024 19:34
@reidbaker reidbaker requested a review from camsim99 December 3, 2024 19:22
@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from 6bc7717 to 12856e2 Compare December 5, 2024 16:39
camsim99
camsim99 reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

The changes LGTM :) but left some questions/comments on the stability of the integration test.

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart Outdated
_getCameraUpdateForType(_cameraUpdateTypeVariants.currentValue!);
await controller.animateCamera(cameraUpdate);

// Check that position is not updated immediately to the target.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not deeply familiar with the APIs here but is it possible this could be flaky? Is it guaranteed that right after we make the call, the position will not update before the check here completes?

Copy link
Contributor Author

@jokerttu jokerttu Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Improved tests documentation to explain better why this is done.
Yes, this test could potentially be flaky, but it is unlikely.
On android the default animation duration is approximately 300ms. This should be long enought time to mitigate possible communication or SDK delays.
The reason for testing this is to ensure that the camera is animated rather than instantly moved to a new location. We could have also used stopwatch and wait cameraIdleEvent, and see if the event does not come too soon, but then we would need to hardcode limits.
It this test is removed, we can only test that camera is moved to right position, but cannot be aware if it is animated or not.

NOTE: tests are now skipped on android, but I left the code here to be enabled later. animateCamera method does not work without Maps API key on Android platform.

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart Outdated Show resolved Hide resolved
...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart
// For short animation duration, check that the animation is completed
// faster than the middle point of the animation test durations.
expect(stopwatch.elapsedMilliseconds,
lessThan(animationDurationMiddlePoint));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not check that the time that has elapsed running the animation is the same as the requested duration directly?

Copy link
Contributor Author

@jokerttu jokerttu Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This solution is based on suggestion by @stuartmorgan #7648 (comment)
Testing with two times mitigate away the possiblity that animateCamera by default animates with same speed.
The duration won't be exactly same as detected by stopwatch, because internal delays on messaging and SDK functionality, so picking proper threshold for testing duration would be really hard.

Most likely having one test run with longer period, lets say 1 second, could work, but this assumes that default animation speed on each platform is different.

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart
);

// Check that position is not updated immediately to the target.
final CameraPosition beforeFinishedPosition =
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same question here about flakiness as the test before. Is that concern why this is only checked for the longer animation and not the shorter?

Copy link
Contributor Author

@jokerttu jokerttu Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Short period is so short, that there might be possilbity that the latencies on communication and SDK might delay the camera position query so much that it is already animated to target location, causing flaky tests.
For longer duration we could safely say that communication latencies are not causing 1 second delays.
Camera animation is not affected by networking either.

...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart Outdated Show resolved Hide resolved
...gle_maps_flutter/google_maps_flutter_android/example/integration_test/google_maps_tests.dart Outdated Show resolved Hide resolved
@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from 1f16fda to 25cf8ec Compare December 10, 2024 12:16
@jokerttu
Copy link
Contributor Author

jokerttu commented Dec 13, 2024 •
edited
Loading

Updated integration tests, by using Completer instead of StreamQueue to detect OnCameraIdle events.
This improved the test quality, and removed need to drain queue (OnCameraIdle event is sent on camera initialization as well), which was possibly a bit flaky solution.

Integration tests are also now skipped on Android as they work only with Maps API Key which is not available on CI at the moment.
Tests passes on both projects for Android if enabled and Maps API Key is available.

@jokerttu jokerttu force-pushed the feature/animate-camera-duration branch from 2d888c5 to 45f5b8a Compare December 13, 2024 08:45
camsim99
camsim99 reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@camsim99 camsim99 left a comment

Choose a reason for hiding this comment

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

Android changes LGTM! Thanks @jokerttu for the thorough explanations :)

@devnoaman

This comment was marked as off-topic.

Sign in to view
@stuartmorgan stuartmorgan removed the triage-android Should be looked at in Android triage label Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camsim99 camsim99 camsim99 left review comments

@cbracken cbracken Awaiting requested review from cbracken cbracken is a code owner

@reidbaker reidbaker Awaiting requested review from reidbaker reidbaker is a code owner

@ditman ditman Awaiting requested review from ditman ditman is a code owner

@jesswrd jesswrd Awaiting requested review from jesswrd

@stuartmorgan stuartmorgan Awaiting requested review from stuartmorgan stuartmorgan is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
federated: all_changes PR that contains changes for all packages for a federated plugin change p: google_maps_flutter platform-android platform-ios platform-web triage-ios Should be looked at in iOS triage
Projects
None yet
Milestone
No milestone
4 participants
@jokerttu @stuartmorgan @devnoaman @camsim99