-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[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
Changes from 7 commits
1c21fd5
8340c92
0f7eb5e
4fbb635
5d2fa59
2f88081
c656c24
a995242
6e6264c
cf2db77
fd4d680
99d3bb1
a4a6fe3
8b9878e
8fb6c08
5108d94
fd35ad3
08d3ded
0efe9d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,7 @@ | ||
| ## 2.17.1 | ||
|
|
||
| * Adds GoogleMapsFlutterAndroid.warmup(). | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
|
|
||
| ## 2.17.0 | ||
|
|
||
| * Updates `com.google.android.gms:play-services-maps` to 19.2.0. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,9 @@ class GoogleMapsFlutterAndroid extends GoogleMapsFlutterPlatform { | |
| /// Creates a new Android maps implementation instance. | ||
| GoogleMapsFlutterAndroid({ | ||
| @visibleForTesting MapsApi Function(int mapId)? apiProvider, | ||
| }) : _apiProvider = apiProvider ?? _productionApiProvider; | ||
| @visibleForTesting MapsInitializerApi? initializerApi, | ||
| }) : _apiProvider = apiProvider ?? _productionApiProvider, | ||
| _initializerApi = initializerApi ?? MapsInitializerApi(); | ||
|
|
||
| /// Registers the Android implementation of GoogleMapsFlutterPlatform. | ||
| static void registerWith() { | ||
|
|
@@ -77,6 +79,8 @@ class GoogleMapsFlutterAndroid extends GoogleMapsFlutterPlatform { | |
| // A method to create MapsApi instances, which can be overridden for testing. | ||
| final MapsApi Function(int mapId) _apiProvider; | ||
|
|
||
| final MapsInitializerApi _initializerApi; | ||
|
|
||
| /// The per-map handlers for callbacks from the host side. | ||
| @visibleForTesting | ||
| final Map<int, HostMapMessageHandler> hostMapHandlers = | ||
|
|
@@ -532,16 +536,25 @@ class GoogleMapsFlutterAndroid extends GoogleMapsFlutterPlatform { | |
| preferredRenderer = null; | ||
| } | ||
|
|
||
| final MapsInitializerApi hostApi = MapsInitializerApi(); | ||
| final PlatformRendererType initializedRenderer = | ||
| await hostApi.initializeWithPreferredRenderer(preferredRenderer); | ||
| final PlatformRendererType initializedRenderer = await _initializerApi | ||
| .initializeWithPreferredRenderer(preferredRenderer); | ||
|
|
||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like this, thanks for the suggestion! Implemented. |
||
| /// when a map is shown for the first time. | ||
| /// | ||
| /// 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). | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove the parenthetical here, per the discussion in the README.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| Future<void> warmup() async { | ||
| await _initializerApi.warmup(); | ||
| } | ||
|
|
||
| Widget _buildView( | ||
| int creationId, | ||
| PlatformViewCreatedCallback onPlatformViewCreated, { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -786,6 +786,15 @@ abstract class MapsInitializerApi { | |
| @async | ||
| PlatformRendererType initializeWithPreferredRenderer( | ||
| PlatformRendererType? type); | ||
|
|
||
| /// Asks the Google Maps SDK to do the thread-blocking work it normally does | ||
| /// when a map is shown for the first time. | ||
| /// | ||
| /// 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). | ||
|
filiph marked this conversation as resolved.
Outdated
|
||
| @async | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I missed this in previous review: why is this marked
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| void warmup(); | ||
| } | ||
|
|
||
| /// Dummy interface to force generation of the platform view creation params, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @filiph I think this is still blocking.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| environment: | ||
| sdk: ^3.6.0 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.