Skip to content

[google_map_flutter] Fix map object regression due to async changes #4171

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

Merged
merged 10 commits into from
Jun 16, 2023
Merged

[google_map_flutter] Fix map object regression due to async changes #4171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6693a3d
Extract fake platform impl to a separate file
stuartmorgan-g Jun 9, 2023
0c41040
Wire up a platform recorder
stuartmorgan-g Jun 9, 2023
8b2580b
Update all map object tests to use the new recorder
stuartmorgan-g Jun 9, 2023
4c0d146
Convert remaining test and remove old fake controller
stuartmorgan-g Jun 9, 2023
7791289
Add new multi-update tests with fake delays
stuartmorgan-g Jun 9, 2023
4ea63c7
Remove incorrect awaits
stuartmorgan-g Jun 9, 2023
80f0354
Version bump
stuartmorgan-g Jun 9, 2023
488becc
Merge branch 'main' into maps-marker-regression-fix
stuartmorgan-g Jun 9, 2023
3faf721
Merge branch 'main' into maps-marker-regression-fix
stuartmorgan-g Jun 13, 2023
a4c502c
Add a dependency override in the web example
stuartmorgan-g Jun 14, 2023
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
4 changes: 3 additions & 1 deletion packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## NEXT
## 2.3.1

* Fixes a regression from 2.2.8 that could cause incorrect handling of a
rapid series of map object updates.
* Fixes stale ignore: prefer_const_constructors.
* Updates minimum supported SDK version to Flutter 3.10/Dart 3.0.

Expand Down
20 changes: 10 additions & 10 deletions packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,41 +360,41 @@ class _GoogleMapState extends State<GoogleMap> {
return;
}
final GoogleMapController controller = await _controller.future;
await controller._updateMapConfiguration(updates);
unawaited(controller._updateMapConfiguration(updates));
_mapConfiguration = newConfig;
}

Future<void> _updateMarkers() async {
final GoogleMapController controller = await _controller.future;
await controller._updateMarkers(
MarkerUpdates.from(_markers.values.toSet(), widget.markers));
unawaited(controller._updateMarkers(
MarkerUpdates.from(_markers.values.toSet(), widget.markers)));
_markers = keyByMarkerId(widget.markers);
}

Future<void> _updatePolygons() async {
final GoogleMapController controller = await _controller.future;
await controller._updatePolygons(
PolygonUpdates.from(_polygons.values.toSet(), widget.polygons));
unawaited(controller._updatePolygons(
PolygonUpdates.from(_polygons.values.toSet(), widget.polygons)));
Comment on lines -376 to +377
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any idea on what can be improved in the plugin so it can go back to the non-unawaited version? This feels a little bit like the plugin is relying in race conditions all the time to work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whether the overall design of these enclosing functions not every being awaited can be addressed without breaking changes to clients I'm not sure yet. I believe these could be fixed locally to await by changing the flow of each one to:

  • copy the current value of _foo to oldFoo
  • update _foo (currently done after the unawaited async)
  • do an awaited call using oldFoo instead of _foo as the previous value argument.

I considered doing that in this PR, or leaving a TODO in the code to do it later, but decided that since the only current use of these private methods is from a non-async function that can't await them anyway it wasn't clear that there was a real benefit to doing so. We could certainly revisit though, since I agree it feels a bit weird.

_polygons = keyByPolygonId(widget.polygons);
}

Future<void> _updatePolylines() async {
final GoogleMapController controller = await _controller.future;
await controller._updatePolylines(
PolylineUpdates.from(_polylines.values.toSet(), widget.polylines));
unawaited(controller._updatePolylines(
PolylineUpdates.from(_polylines.values.toSet(), widget.polylines)));
_polylines = keyByPolylineId(widget.polylines);
}

Future<void> _updateCircles() async {
final GoogleMapController controller = await _controller.future;
await controller._updateCircles(
CircleUpdates.from(_circles.values.toSet(), widget.circles));
unawaited(controller._updateCircles(
CircleUpdates.from(_circles.values.toSet(), widget.circles)));
_circles = keyByCircleId(widget.circles);
}

Future<void> _updateTileOverlays() async {
final GoogleMapController controller = await _controller.future;
await controller._updateTileOverlays(widget.tileOverlays);
unawaited(controller._updateTileOverlays(widget.tileOverlays));
}

Future<void> onPlatformViewCreated(int id) async {
Expand Down
2 changes: 1 addition & 1 deletion packages/google_maps_flutter/google_maps_flutter/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ 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.3.0
version: 2.3.1

environment:
sdk: ">=3.0.0 <4.0.0"
Expand Down
152 changes: 79 additions & 73 deletions packages/google_maps_flutter/google_maps_flutter/test/circle_updates_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/services.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:google_maps_flutter/google_maps_flutter.dart';
import 'package:google_maps_flutter_platform_interface/google_maps_flutter_platform_interface.dart';

import 'fake_maps_controllers.dart';
import 'fake_google_maps_flutter_platform.dart';

Widget _mapWithCircles(Set<Circle> circles) {
return Directionality(
Expand All @@ -20,36 +20,24 @@ Widget _mapWithCircles(Set<Circle> circles) {
}

void main() {
TestWidgetsFlutterBinding.ensureInitialized();

final FakePlatformViewsController fakePlatformViewsController =
FakePlatformViewsController();

setUpAll(() {
_ambiguate(TestDefaultBinaryMessengerBinding.instance)!
.defaultBinaryMessenger
.setMockMethodCallHandler(
SystemChannels.platform_views,
fakePlatformViewsController.fakePlatformViewsMethodHandler,
);
});
late FakeGoogleMapsFlutterPlatform platform;

setUp(() {
fakePlatformViewsController.reset();
platform = FakeGoogleMapsFlutterPlatform();
GoogleMapsFlutterPlatform.instance = platform;
});

testWidgets('Initializing a circle', (WidgetTester tester) async {
const Circle c1 = Circle(circleId: CircleId('circle_1'));
await tester.pumpWidget(_mapWithCircles(<Circle>{c1}));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
expect(platformGoogleMap.circlesToAdd.length, 1);
final PlatformMapStateRecorder map = platform.lastCreatedMap;
expect(map.circleUpdates.last.circlesToAdd.length, 1);

final Circle initializedCircle = platformGoogleMap.circlesToAdd.first;
final Circle initializedCircle = map.circleUpdates.last.circlesToAdd.first;
expect(initializedCircle, equals(c1));
expect(platformGoogleMap.circleIdsToRemove.isEmpty, true);
expect(platformGoogleMap.circlesToChange.isEmpty, true);
expect(map.circleUpdates.last.circleIdsToRemove.isEmpty, true);
expect(map.circleUpdates.last.circlesToChange.isEmpty, true);
});

testWidgets('Adding a circle', (WidgetTester tester) async {
Expand All @@ -59,16 +47,15 @@ void main() {
await tester.pumpWidget(_mapWithCircles(<Circle>{c1}));
await tester.pumpWidget(_mapWithCircles(<Circle>{c1, c2}));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
expect(platformGoogleMap.circlesToAdd.length, 1);
final PlatformMapStateRecorder map = platform.lastCreatedMap;
expect(map.circleUpdates.last.circlesToAdd.length, 1);

final Circle addedCircle = platformGoogleMap.circlesToAdd.first;
final Circle addedCircle = map.circleUpdates.last.circlesToAdd.first;
expect(addedCircle, equals(c2));

expect(platformGoogleMap.circleIdsToRemove.isEmpty, true);
expect(map.circleUpdates.last.circleIdsToRemove.isEmpty, true);

expect(platformGoogleMap.circlesToChange.isEmpty, true);
expect(map.circleUpdates.last.circlesToChange.isEmpty, true);
});

testWidgets('Removing a circle', (WidgetTester tester) async {
Expand All @@ -77,13 +64,12 @@ void main() {
await tester.pumpWidget(_mapWithCircles(<Circle>{c1}));
await tester.pumpWidget(_mapWithCircles(<Circle>{}));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
expect(platformGoogleMap.circleIdsToRemove.length, 1);
expect(platformGoogleMap.circleIdsToRemove.first, equals(c1.circleId));
final PlatformMapStateRecorder map = platform.lastCreatedMap;
expect(map.circleUpdates.last.circleIdsToRemove.length, 1);
expect(map.circleUpdates.last.circleIdsToRemove.first, equals(c1.circleId));

expect(platformGoogleMap.circlesToChange.isEmpty, true);
expect(platformGoogleMap.circlesToAdd.isEmpty, true);
expect(map.circleUpdates.last.circlesToChange.isEmpty, true);
expect(map.circleUpdates.last.circlesToAdd.isEmpty, true);
});

testWidgets('Updating a circle', (WidgetTester tester) async {
Expand All @@ -93,13 +79,12 @@ void main() {
await tester.pumpWidget(_mapWithCircles(<Circle>{c1}));
await tester.pumpWidget(_mapWithCircles(<Circle>{c2}));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
expect(platformGoogleMap.circlesToChange.length, 1);
expect(platformGoogleMap.circlesToChange.first, equals(c2));
final PlatformMapStateRecorder map = platform.lastCreatedMap;
expect(map.circleUpdates.last.circlesToChange.length, 1);
expect(map.circleUpdates.last.circlesToChange.first, equals(c2));

expect(platformGoogleMap.circleIdsToRemove.isEmpty, true);
expect(platformGoogleMap.circlesToAdd.isEmpty, true);
expect(map.circleUpdates.last.circleIdsToRemove.isEmpty, true);
expect(map.circleUpdates.last.circlesToAdd.isEmpty, true);
});

testWidgets('Updating a circle', (WidgetTester tester) async {
Expand All @@ -109,11 +94,10 @@ void main() {
await tester.pumpWidget(_mapWithCircles(<Circle>{c1}));
await tester.pumpWidget(_mapWithCircles(<Circle>{c2}));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
expect(platformGoogleMap.circlesToChange.length, 1);
final PlatformMapStateRecorder map = platform.lastCreatedMap;
expect(map.circleUpdates.last.circlesToChange.length, 1);

final Circle update = platformGoogleMap.circlesToChange.first;
final Circle update = map.circleUpdates.last.circlesToChange.first;
expect(update, equals(c2));
expect(update.radius, 10);
});
Expand All @@ -129,12 +113,11 @@ void main() {
await tester.pumpWidget(_mapWithCircles(prev));
await tester.pumpWidget(_mapWithCircles(cur));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
final PlatformMapStateRecorder map = platform.lastCreatedMap;

expect(platformGoogleMap.circlesToChange, cur);
expect(platformGoogleMap.circleIdsToRemove.isEmpty, true);
expect(platformGoogleMap.circlesToAdd.isEmpty, true);
expect(map.circleUpdates.last.circlesToChange, cur);
expect(map.circleUpdates.last.circleIdsToRemove.isEmpty, true);
expect(map.circleUpdates.last.circlesToAdd.isEmpty, true);
});

testWidgets('Multi Update', (WidgetTester tester) async {
Expand All @@ -150,16 +133,15 @@ void main() {
await tester.pumpWidget(_mapWithCircles(prev));
await tester.pumpWidget(_mapWithCircles(cur));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
final PlatformMapStateRecorder map = platform.lastCreatedMap;

expect(platformGoogleMap.circlesToChange.length, 1);
expect(platformGoogleMap.circlesToAdd.length, 1);
expect(platformGoogleMap.circleIdsToRemove.length, 1);
expect(map.circleUpdates.last.circlesToChange.length, 1);
expect(map.circleUpdates.last.circlesToAdd.length, 1);
expect(map.circleUpdates.last.circleIdsToRemove.length, 1);

expect(platformGoogleMap.circlesToChange.first, equals(c2));
expect(platformGoogleMap.circlesToAdd.first, equals(c1));
expect(platformGoogleMap.circleIdsToRemove.first, equals(c3.circleId));
expect(map.circleUpdates.last.circlesToChange.first, equals(c2));
expect(map.circleUpdates.last.circlesToAdd.first, equals(c1));
expect(map.circleUpdates.last.circleIdsToRemove.first, equals(c3.circleId));
});

testWidgets('Partial Update', (WidgetTester tester) async {
Expand All @@ -173,12 +155,11 @@ void main() {
await tester.pumpWidget(_mapWithCircles(prev));
await tester.pumpWidget(_mapWithCircles(cur));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
final PlatformMapStateRecorder map = platform.lastCreatedMap;

expect(platformGoogleMap.circlesToChange, <Circle>{c3});
expect(platformGoogleMap.circleIdsToRemove.isEmpty, true);
expect(platformGoogleMap.circlesToAdd.isEmpty, true);
expect(map.circleUpdates.last.circlesToChange, <Circle>{c3});
expect(map.circleUpdates.last.circleIdsToRemove.isEmpty, true);
expect(map.circleUpdates.last.circlesToAdd.isEmpty, true);
});

testWidgets('Update non platform related attr', (WidgetTester tester) async {
Expand All @@ -190,17 +171,42 @@ void main() {
await tester.pumpWidget(_mapWithCircles(prev));
await tester.pumpWidget(_mapWithCircles(cur));

final FakePlatformGoogleMap platformGoogleMap =
fakePlatformViewsController.lastCreatedView!;
final PlatformMapStateRecorder map = platform.lastCreatedMap;

expect(platformGoogleMap.circlesToChange.isEmpty, true);
expect(platformGoogleMap.circleIdsToRemove.isEmpty, true);
expect(platformGoogleMap.circlesToAdd.isEmpty, true);
expect(map.circleUpdates.last.circlesToChange.isEmpty, true);
expect(map.circleUpdates.last.circleIdsToRemove.isEmpty, true);
expect(map.circleUpdates.last.circlesToAdd.isEmpty, true);
});
}

/// This allows a value of type T or T? to be treated as a value of type T?.
///
/// We use this so that APIs that have become non-nullable can still be used
/// with `!` and `?` on the stable branch.
T? _ambiguate<T>(T? value) => value;
testWidgets('multi-update with delays', (WidgetTester tester) async {
platform.simulatePlatformDelay = true;

const Circle c1 = Circle(circleId: CircleId('circle_1'));
const Circle c2 = Circle(circleId: CircleId('circle_2'));
const Circle c3 = Circle(circleId: CircleId('circle_3'), radius: 1);
const Circle c3updated = Circle(circleId: CircleId('circle_3'), radius: 10);

// First remove one and add another, then update the new one.
await tester.pumpWidget(_mapWithCircles(<Circle>{c1, c2}));
await tester.pumpWidget(_mapWithCircles(<Circle>{c1, c3}));
await tester.pumpWidget(_mapWithCircles(<Circle>{c1, c3updated}));

final PlatformMapStateRecorder map = platform.lastCreatedMap;

expect(map.circleUpdates.length, 3);

expect(map.circleUpdates[0].circlesToChange.isEmpty, true);
expect(map.circleUpdates[0].circlesToAdd, <Circle>{c1, c2});
expect(map.circleUpdates[0].circleIdsToRemove.isEmpty, true);

expect(map.circleUpdates[1].circlesToChange.isEmpty, true);
expect(map.circleUpdates[1].circlesToAdd, <Circle>{c3});
expect(map.circleUpdates[1].circleIdsToRemove, <CircleId>{c2.circleId});

expect(map.circleUpdates[2].circlesToChange, <Circle>{c3updated});
expect(map.circleUpdates[2].circlesToAdd.isEmpty, true);
expect(map.circleUpdates[2].circleIdsToRemove.isEmpty, true);

await tester.pumpAndSettle();
});
}
Loading