-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[google_maps_flutter] Add ability to perform Google Maps SDK warmup #9674
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR introduces a warmup method to mitigate the initial jank on Android, along with dependency injection for testing. Adding onResume()
to the MapView
lifecycle methods in the warmup process would ensure it's as effective as possible.
...lutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapInitializer.java
Outdated
Show resolved
Hide resolved
Hey maintainers, I've finalized this PR and removed the |
Yeah I dont think that should prevent review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
...lutter_android/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapInitializer.java
Show resolved
Hide resolved
@@ -2,7 +2,7 @@ name: google_maps_flutter_android | |||
description: Android implementation of the google_maps_flutter plugin. | |||
repository: https://github.com/flutter/packages/tree/main/packages/google_maps_flutter/google_maps_flutter_android | |||
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+maps%22 | |||
version: 2.17.0 | |||
version: 2.17.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stuartmorgan-g should this be a minor version bump not a patch version bump?
I reviewed https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md but didnt see anything more specific than my general understadning of sem-ver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since it adds new public API semver requires it to be a minor API bump. (That's not a repo policy, it's basic semver.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filiph I think this is still blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was already addressed in 8b9878e ? Or at least I see this as addressed both locally and here on github.
...s/google_maps_flutter/google_maps_flutter_android/test/google_maps_flutter_android_test.dart
Outdated
Show resolved
Hide resolved
...s/google_maps_flutter/google_maps_flutter_android/test/google_maps_flutter_android_test.dart
Outdated
Show resolved
Hide resolved
...le_maps_flutter/google_maps_flutter_android/test/google_maps_flutter_android_test.mocks.dart
Outdated
Show resolved
Hide resolved
…google_maps_flutter_android_test.dart Co-authored-by: Reid Baker <[email protected]>
…google_maps_flutter_android_test.dart Co-authored-by: Reid Baker <[email protected]>
packages/google_maps_flutter/google_maps_flutter_android/README.md
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/README.md
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/README.md
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/README.md
Outdated
Show resolved
Hide resolved
packages/google_maps_flutter/google_maps_flutter_android/pigeons/messages.dart
Outdated
Show resolved
Hide resolved
@@ -2,7 +2,7 @@ name: google_maps_flutter_android | |||
description: Android implementation of the google_maps_flutter plugin. | |||
repository: https://github.com/flutter/packages/tree/main/packages/google_maps_flutter/google_maps_flutter_android | |||
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+maps%22 | |||
version: 2.17.0 | |||
version: 2.17.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since it adds new public API semver requires it to be a minor API bump. (That's not a repo policy, it's basic semver.)
Thanks for the feedback. Implemented. |
packages/google_maps_flutter/google_maps_flutter_android/CHANGELOG.md
Outdated
Show resolved
Hide resolved
/// | ||
/// This gives the developer the option to move that jank to a different | ||
/// part of the map (typically, the app startup, where missed frames | ||
/// aren't going to be noticed). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the parenthetical here, per the discussion in the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
return switch (initializedRenderer) { | ||
PlatformRendererType.latest => AndroidMapRenderer.latest, | ||
PlatformRendererType.legacy => AndroidMapRenderer.legacy, | ||
}; | ||
} | ||
|
||
/// Asks the Google Maps SDK to do the thread-blocking work it normally does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Attempts to trigger any thread-blocking work the Google Maps SDK normally does when a map is shown for the first time."
It's not actually asking the SDK to do this; that implies some officially supported API intended for this purpose. Instead it's trying to trigger it via reliance on internal implementation details of the SDK that could change at any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this, thanks for the suggestion! Implemented.
@@ -1,3 +1,7 @@ | |||
## 2.18.0 | |||
|
|||
* Adds GoogleMapsFlutterAndroid.warmup(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: backticks around the code.
Also, this should provide some client-facing context about what this is, rather than someone needing to go read the API docs to understand what this method is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with
* Adds support for warming up the Google Maps SDK
via `GoogleMapsFlutterAndroid.warmup()`.
I try to balance the laconic style of the changelog with giving some context, but I feel I'm botching it?
|
||
/// Attempts to trigger any thread-blocking work | ||
/// the Google Maps SDK normally does when a map is shown for the first time. | ||
@async |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed this in previous review: why is this marked @async
? There's no async step in the Java code, it's all synchronous. Without the @async
annotation the Java code would be simpler, without the possibility of accidentally not calling the result
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Damn, sorry I missed it. The original code was async but then I found out it wouldn't work so I made it synchronous — and promptly forgot about this being async.
Addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
flutter/packages@f0b2726...6efb759 2025-08-06 [email protected] [camera_android_camerax] Re-land "Force new Surface for each SurfaceRequest" (flutter/packages#9760) 2025-08-06 [email protected] [google_maps_flutter] Add ability to perform Google Maps SDK warmup (flutter/packages#9674) 2025-08-06 [email protected] Roll Flutter from 59fc766 to 9de63a0 (6 revisions) (flutter/packages#9764) 2025-08-06 [email protected] [webview_flutter_wkwebview] Extended Web View API on iOS to add flexibility when working with local HTML content (flutter/packages#8787) 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] 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
…r#173414) flutter/packages@f0b2726...6efb759 2025-08-06 [email protected] [camera_android_camerax] Re-land "Force new Surface for each SurfaceRequest" (flutter/packages#9760) 2025-08-06 [email protected] [google_maps_flutter] Add ability to perform Google Maps SDK warmup (flutter/packages#9674) 2025-08-06 [email protected] Roll Flutter from 59fc766 to 9de63a0 (6 revisions) (flutter/packages#9764) 2025-08-06 [email protected] [webview_flutter_wkwebview] Extended Web View API on iOS to add flexibility when working with local HTML content (flutter/packages#8787) 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] 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
The Android Google Maps SDK janks when the first map is shown, completely blocking the app thread. This PR introduces
GoogleMapsFlutterAndroid.warmup()
which lets Flutter developers do what Android developers do to mitigate this: a fake show of a Google Map that forces the SDK to initialize. This is typically done at app startup so that the jank is less visible than during, say, a page transition.More info: flutter/flutter#28493 (comment)
This PR also introduces injecting
MapsInitializerApi
to theGoogleMapsFlutterAndroid
constructor, for testing, and adds a basic test forinitializeWithRenderer
, too.Fixes flutter/flutter#28493
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3