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.

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

[google_maps_flutter] Add style to widget #6192

Merged
merged 19 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
## NEXT

* Updates minimum supported SDK version to Flutter 3.13/Dart 3.1.
## 2.6.0

* Adds `style` to the GoogleMap widget constructor. This allows setting the map
style during creation, avoiding the possibility of the default style being
displayed briefly.
* Deprecates `GoogleMapController.setMapStyle` in favor of setting the style via
the new widget `style` parameter.
* Updates minimum supported SDK version to Flutter 3.19.

## 2.5.3

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,28 @@ void runTests() {
await mapIdCompleter.future;
},
);

testWidgets('getStyleError reports last error', (WidgetTester tester) async {
final Key key = GlobalKey();
final Completer<GoogleMapController> controllerCompleter =
Completer<GoogleMapController>();

await pumpMap(
tester,
GoogleMap(
key: key,
initialCameraPosition: kInitialCameraPosition,
style: '[[[this is an invalid style',
onMapCreated: (GoogleMapController controller) {
controllerCompleter.complete(controller);
},
),
);

final GoogleMapController controller = await controllerCompleter.future;
final String? error = await controller.getStyleError();
expect(error, isNotNull);
});
}

/// Repeatedly checks an asynchronous value against a test condition.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

// ignore_for_file: public_member_api_docs

import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart' show rootBundle;
import 'package:google_maps_flutter/google_maps_flutter.dart';
Expand Down Expand Up @@ -59,6 +60,7 @@ class MapUiBodyState extends State<MapUiBody> {
bool _myLocationButtonEnabled = true;
late GoogleMapController _controller;
bool _nightMode = false;
String _mapStyle = '';

@override
void initState() {
Expand Down Expand Up @@ -243,27 +245,18 @@ class MapUiBodyState extends State<MapUiBody> {
return rootBundle.loadString(path);
}

void _setMapStyle(String mapStyle) {
setState(() {
_nightMode = true;
_controller.setMapStyle(mapStyle);
});
}

// Should only be called if _isMapCreated is true.
Widget _nightModeToggler() {
assert(_isMapCreated);
return TextButton(
child: Text('${_nightMode ? 'disable' : 'enable'} night mode'),
onPressed: () {
if (_nightMode) {
setState(() {
_nightMode = false;
_controller.setMapStyle(null);
});
} else {
_getFileData('assets/night_mode.json').then(_setMapStyle);
}
onPressed: () async {
_nightMode = !_nightMode;
final String style =
_nightMode ? await _getFileData('assets/night_mode.json') : '';
setState(() {
_mapStyle = style;
});
},
);
}
Expand All @@ -278,6 +271,7 @@ class MapUiBodyState extends State<MapUiBody> {
cameraTargetBounds: _cameraTargetBounds,
minMaxZoomPreference: _minMaxZoomPreference,
mapType: _mapType,
style: _mapStyle,
rotateGesturesEnabled: _rotateGesturesEnabled,
scrollGesturesEnabled: _scrollGesturesEnabled,
tiltGesturesEnabled: _tiltGesturesEnabled,
Expand Down Expand Up @@ -352,5 +346,13 @@ class MapUiBodyState extends State<MapUiBody> {
_controller = controller;
_isMapCreated = true;
});
// Log any style errors to the console for debugging.
if (kDebugMode) {
_controller.getStyleError().then((String? error) {
if (error != null) {
debugPrint(error);
}
});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ dependencies:
# the parent directory to use the current plugin's version.
path: ../
google_maps_flutter_android: ^2.5.0
google_maps_flutter_platform_interface: ^2.4.0
google_maps_flutter_platform_interface: ^2.5.0

dev_dependencies:
build_runner: ^2.1.10
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,17 @@ class GoogleMapController {
/// Also, refer [iOS](https://developers.google.com/maps/documentation/ios-sdk/style-reference)
/// and [Android](https://developers.google.com/maps/documentation/android-sdk/style-reference)
/// style reference for more information regarding the supported styles.
@Deprecated('Use GoogleMap.style instead.')
Future<void> setMapStyle(String? mapStyle) {
return GoogleMapsFlutterPlatform.instance
.setMapStyle(mapStyle, mapId: mapId);
}

/// Returns the last style error, if any.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say that it will get errors for the current mapid only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MapController is a per-map object; it's created with this ID.

Future<String?> getStyleError() {
return GoogleMapsFlutterPlatform.instance.getStyleError(mapId: mapId);
}

/// Return [LatLngBounds] defining the region that is visible in a map.
Future<LatLngBounds> getVisibleRegion() {
return GoogleMapsFlutterPlatform.instance.getVisibleRegion(mapId: mapId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ class GoogleMap extends StatefulWidget {
const GoogleMap({
super.key,
required this.initialCameraPosition,
this.style,
this.onMapCreated,
this.gestureRecognizers = const <Factory<OneSequenceGestureRecognizer>>{},
this.webGestureHandling,
Expand Down Expand Up @@ -136,6 +137,19 @@ class GoogleMap extends StatefulWidget {
/// The initial position of the map's camera.
final CameraPosition initialCameraPosition;

/// The style for the map.
///
/// Set to null to clear any previous custom styling.
///
/// If problems were detected with the [mapStyle], including un-parsable
/// styling JSON, unrecognized feature type, unrecognized element type, or
/// invalid styler keys, the style is left unchanged, and the error can be
/// retrieved with [GoogleMapController.getStyleError].
///
/// The style string can be generated using the
/// [map style tool](https://mapstyle.withgoogle.com/).
final String? style;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way at all to make this a type safe object?

If not can we at least point users to https://developers.google.com/maps/documentation/cloud-customization which does not require app updates and a stringly typed api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way at all to make this a type safe object?

Not in any useful way. The native objects are opaque objects created from a string that is supposed to be a JSON string. So the options are:

  • Make a do-nothing MapStyle object that just wraps a String, but in practice that would almost certainly just convert every call from style: someString to style: MapStyle(someString)
  • Pre-parse the JSON on the Dart side, only to convert it back to a string and then have the Maps SDK re-parse it anyway.

If not can we at least point users to https://developers.google.com/maps/documentation/cloud-customization which does not require app updates and a stringly typed api.

If you follow the link there in this comment for generating the JSON style, you'll see that we don't really need to do that at the plugin level, as the SDK's docs and tool are very aggressive about this. (Also, there are significant caveats, like cloud style requires a per-load charge, and doesn't work on all Android devices, which is more than I want to get into in the comments here. I'd rather err on the side of being a neutral wrapper of the SDKs, and let the SDK docs steer people.)

Copy link
Member

@ditman ditman Feb 23, 2024

Choose a reason for hiding this comment

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

It can definitely be made a type-safe object (we have a "type-safe-ish version" in the web version); the value of the String version of this property is that it can be generated/tweaked (and then copy-pasted) from here: https://mapstyle.withgoogle.com (and probably others)

HOWEVER, one thing that Google Maps SDK seems to be doing is to call this feature "legacy styling", so maybe this property could be called: legacyStyle and in the documentation point them to the link that you're proposing? That way people can learn about cloud styling, the mapId property and the new way (and cost) of doing things?

There's even documentation on how to migrate from this legacyStyle value to the new cloud-based one: https://developers.google.com/maps/documentation/javascript/cloud-customization/map-styles#import-json-styling

Copy link
Contributor Author

@stuartmorgan stuartmorgan Feb 23, 2024

Choose a reason for hiding this comment

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

It can definitely be made a type-safe object

Sorry, technically there is a third option: we could—in our wrapper that is already under-resourced—reverse-engineer a complex object graph from docs, thus building and maintaining a much more complex API than what the SDKs provide, creating a ton of work for us, only to throw away all that structure and re-encode it as a string anyway at the SDK boundary. And thereby make the existing tools for these styles harder for our clients to use. And also being inconsistent with our setMapStyle, making it significantly harder to migrate to this new API.

It's theoretically possible, but I don't see any actual upsides.

HOWEVER, one thing that Google Maps SDK seems to be doing is to call this feature "legacy styling", so maybe this property could be called: legacyStyle

Their high-level docs call it legacy style. The actual SDKs, and the code-level API docs for those SDKs, do not use that language anywhere that I have seen.

I do not want to be in the business of being significantly more opinionated in the code about what APIs people should or should not use than the people actually maintaining the SDK we are wrapping.

I also don't want setMapStyle (the existing method) and legacyStyle (the new, preferred approach), since that will make the deprecated one seem preferable and almost certainly confuse everyone.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM, legacyStyle seems to be only mentioned in the Cloud Styling docs.


/// True if the map should show a compass when rotated.
final bool compassEnabled;

Expand Down Expand Up @@ -556,5 +570,8 @@ MapConfiguration _configurationFromMapWidget(GoogleMap map) {
trafficEnabled: map.trafficEnabled,
buildingsEnabled: map.buildingsEnabled,
cloudMapId: map.cloudMapId,
// A null style in the widget means no style, which is expressed as '' in
// the configuration to distinguish from no change (null).
style: map.style ?? '',
);
}
14 changes: 7 additions & 7 deletions packages/google_maps_flutter/google_maps_flutter/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ name: google_maps_flutter
description: A Flutter plugin for integrating Google Maps in iOS and Android applications.
repository: https://github.com/flutter/packages/tree/main/packages/google_maps_flutter/google_maps_flutter
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+maps%22
version: 2.5.3
version: 2.6.0

environment:
sdk: ^3.1.0
flutter: ">=3.13.0"
sdk: ^3.3.0
flutter: ">=3.19.0"

flutter:
plugin:
Expand All @@ -21,10 +21,10 @@ flutter:
dependencies:
flutter:
sdk: flutter
google_maps_flutter_android: ^2.5.0
google_maps_flutter_ios: ^2.3.0
google_maps_flutter_platform_interface: ^2.4.0
google_maps_flutter_web: ^0.5.2
google_maps_flutter_android: ^2.7.0
google_maps_flutter_ios: ^2.5.0
google_maps_flutter_platform_interface: ^2.5.0
google_maps_flutter_web: ^0.5.6

dev_dependencies:
flutter_test:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -551,4 +551,32 @@ void main() {

expect(map.mapConfiguration.buildingsEnabled, true);
});

testWidgets('Can update style', (WidgetTester tester) async {
const String initialStyle = '[]';
await tester.pumpWidget(
const Directionality(
textDirection: TextDirection.ltr,
child: GoogleMap(
initialCameraPosition: CameraPosition(target: LatLng(10.0, 15.0)),
style: initialStyle,
),
),
);

final PlatformMapStateRecorder map = platform.lastCreatedMap;

expect(map.mapConfiguration.style, initialStyle);

await tester.pumpWidget(
const Directionality(
textDirection: TextDirection.ltr,
child: GoogleMap(
initialCameraPosition: CameraPosition(target: LatLng(10.0, 15.0)),
),
),
);

expect(map.mapConfiguration.style, '');
});
}