[google_maps_flutter] Add missing exports#11196
[google_maps_flutter] Add missing exports#11196stuartmorgan-g wants to merge 1 commit intoflutter:mainfrom
Conversation
Like much of the `google_maps_flutter` APIs currently, advanced markers rely on clients using types that are defined in the platform interface package. This re-exports them, and fixes the examples to not import the platform interface. Ideally we would have more separation between the API layers, but this is deeply ingrained in the API design of the plugin at this point, so changing it would require a comprehensive overhaul.
There was a problem hiding this comment.
Code Review
This pull request correctly adds missing re-exports for advanced marker-related classes from the google_maps_flutter_platform_interface package into the main google_maps_flutter package. The example files have been updated to reflect these changes by removing direct imports from the platform interface, which improves the API's consistency and usability. The changelog and pubspec.yaml have also been updated appropriately. The changes are well-aligned with the pull request's objective.
|
@jokerttu I ran into one potentially thorn issue here that I completely missed in review: one of the example pages is calling Don't clients need to know whether advanced marker support is available before passing the advanced marker type to |
bparrishMines
left a comment
There was a problem hiding this comment.
LGTM pending test question
|
|
||
| export 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart' | ||
| show | ||
| AdvancedMarker, |
There was a problem hiding this comment.
Does this need a test like we did for webview_flutter: https://github.com/flutter/packages/blob/main/packages/webview_flutter/webview_flutter/test/webview_flutter_export_test.dart
There was a problem hiding this comment.
Good point, I'll add that so we don't regress on any future refactorings.
tarrinneal
left a comment
There was a problem hiding this comment.
Good rule of thumb, apps shouldn't be importing the platform interface if they are only using the primary plugin?
You are absolutely right; there is indeed a chicken-and-egg problem here. I completely missed this too when I took over the implementation PR. Since this design flaw has already landed, here is my suggestion:: We could implement new GoogleMapsFlutterPlatform.instance.prepare method that implement same functionality as warmup on Android for all platforms, in addition to be able to take Something like this: Future<PrepareResult> prepare([PrepareOptions? options]);
@immutable
class PrepareOptions {
const PrepareOptions({this.mapId});
/// Cloud map ID to check capabilities for.
final String? mapId;
}
@immutable
class PrepareResult {
const PrepareResult({
required this.capabilities,
});
final MapCapabilities capabilities;
}
@immutable
class MapCapabilities {
const MapCapabilities({
required this.advancedMarkersAvailable,
});
final bool advancedMarkersAvailable;
}This would allow developers to query the MapCapabilities before MapWidgets. Not sure how performant this prepare functionality would be on each platform without testing this approach first, but in most cases developers can do this early on app initialization before map views are most likely even needed to be shown yet. MapCapabilities could be extended later with following options if seen imporant: Android, iOS. On Android there is also following listener available OnMapCapabilitiesChangedListener, which existence tells that the capabilities could change dynamically, but it is not documented, on what situation this could happen. This could potentially mess up things as well. Implementing this prepare would need similar After this is landed, the old If this approach sounds good to you, I could implement this in quite short notice. The current implementation creates a really poor developer experience when developers want to use advanced markers (sticking with legacy markers avoids this issue entirely). To use advanced markers, developers need to first initialize the map without markers to be able check its capability to show advanced markers, and then branch their logic based on the response:
In both cases, this required roundtrip prevents developers from adding markers during map initialization, which will likely cause a visible lag before markers appear on the screen. Also, if a proper GMP MapID is given to the MapWidget and billing is set up properly for the Google Cloud project, then mapCapabilities should return Sorry for late response. This PR LGTM |
Yes. Having platform interface imports in app-facing package examples is usually a red flag (although things like the renderer selection are a legitimate use case for that), and I really should have caught that in the initial review. |
It's not clear to me how that would work; looking at the native implementations of the current queries, they are calling methods on a specific maps instance, not a global call like Android warmup. I've filed flutter/flutter#183892 to continue discussion/investigation of this; I think the next step would be a short design document about how the flow of querying for support and then enabling should work. It seems like if the query is something we can only map on a maps instance, we may need to make enabling it something that happens post-initialization, although as you point out that means the map can't have initial markers. I think this needs more exploration in a doc. |
This adds exports that should have been in #7882, but I missed in review.
Like much of the
google_maps_flutterAPIs currently, advanced markers rely on clients using types that are defined in the platform interface package. This re-exports them, and fixes the examples to not import the platform interface.Ideally we would have more separation between the API layers, but this is deeply ingrained in the API design of the plugin at this point, so changing it would require a comprehensive overhaul.
Pre-Review Checklist
[shared_preferences]///).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