From 7dc900929e8328228c5480da5cca9736c6e9b4b5 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Thu, 22 Jun 2023 18:49:59 -0700 Subject: [PATCH 01/17] fix: Clean up subscriptions when disposing, instead of firing and forgetting --- .../lib/src/controller.dart | 116 ++++++++++++------ 1 file changed, 80 insertions(+), 36 deletions(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index dedb8400a32e..54d753e3de44 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -18,6 +18,9 @@ class GoogleMapController { /// The mapId for this controller final int mapId; + /// Subscriptions to events from the native side of this controller. + final List> _subscriptions = []; + /// Initialize control of a [GoogleMap] with [id]. /// /// Mainly for internal use when instantiating a [GoogleMapController] passed @@ -38,47 +41,85 @@ class GoogleMapController { void _connectStreams(int mapId) { if (_googleMapState.widget.onCameraMoveStarted != null) { - GoogleMapsFlutterPlatform.instance - .onCameraMoveStarted(mapId: mapId) - .listen((_) => _googleMapState.widget.onCameraMoveStarted!()); + _subscriptions.add( + GoogleMapsFlutterPlatform.instance + .onCameraMoveStarted(mapId: mapId) + .listen((_) => _googleMapState.widget.onCameraMoveStarted!()), + ); } if (_googleMapState.widget.onCameraMove != null) { - GoogleMapsFlutterPlatform.instance.onCameraMove(mapId: mapId).listen( - (CameraMoveEvent e) => _googleMapState.widget.onCameraMove!(e.value)); + _subscriptions.add( + GoogleMapsFlutterPlatform.instance.onCameraMove(mapId: mapId).listen( + (CameraMoveEvent e) => + _googleMapState.widget.onCameraMove!(e.value), + ), + ); } if (_googleMapState.widget.onCameraIdle != null) { - GoogleMapsFlutterPlatform.instance - .onCameraIdle(mapId: mapId) - .listen((_) => _googleMapState.widget.onCameraIdle!()); + _subscriptions.add( + GoogleMapsFlutterPlatform.instance + .onCameraIdle(mapId: mapId) + .listen((_) => _googleMapState.widget.onCameraIdle!()), + ); } - GoogleMapsFlutterPlatform.instance - .onMarkerTap(mapId: mapId) - .listen((MarkerTapEvent e) => _googleMapState.onMarkerTap(e.value)); - GoogleMapsFlutterPlatform.instance.onMarkerDragStart(mapId: mapId).listen( - (MarkerDragStartEvent e) => - _googleMapState.onMarkerDragStart(e.value, e.position)); - GoogleMapsFlutterPlatform.instance.onMarkerDrag(mapId: mapId).listen( - (MarkerDragEvent e) => - _googleMapState.onMarkerDrag(e.value, e.position)); - GoogleMapsFlutterPlatform.instance.onMarkerDragEnd(mapId: mapId).listen( - (MarkerDragEndEvent e) => - _googleMapState.onMarkerDragEnd(e.value, e.position)); - GoogleMapsFlutterPlatform.instance.onInfoWindowTap(mapId: mapId).listen( - (InfoWindowTapEvent e) => _googleMapState.onInfoWindowTap(e.value)); - GoogleMapsFlutterPlatform.instance - .onPolylineTap(mapId: mapId) - .listen((PolylineTapEvent e) => _googleMapState.onPolylineTap(e.value)); - GoogleMapsFlutterPlatform.instance - .onPolygonTap(mapId: mapId) - .listen((PolygonTapEvent e) => _googleMapState.onPolygonTap(e.value)); - GoogleMapsFlutterPlatform.instance - .onCircleTap(mapId: mapId) - .listen((CircleTapEvent e) => _googleMapState.onCircleTap(e.value)); - GoogleMapsFlutterPlatform.instance - .onTap(mapId: mapId) - .listen((MapTapEvent e) => _googleMapState.onTap(e.position)); - GoogleMapsFlutterPlatform.instance.onLongPress(mapId: mapId).listen( - (MapLongPressEvent e) => _googleMapState.onLongPress(e.position)); + + _subscriptions + ..add( + GoogleMapsFlutterPlatform.instance + .onMarkerTap(mapId: mapId) + .listen((MarkerTapEvent e) => _googleMapState.onMarkerTap(e.value)), + ) + ..add( + GoogleMapsFlutterPlatform.instance + .onMarkerDragStart(mapId: mapId) + .listen( + (MarkerDragStartEvent e) => + _googleMapState.onMarkerDragStart(e.value, e.position), + ), + ) + ..add( + GoogleMapsFlutterPlatform.instance.onMarkerDrag(mapId: mapId).listen( + (MarkerDragEvent e) => + _googleMapState.onMarkerDrag(e.value, e.position), + ), + ) + ..add( + GoogleMapsFlutterPlatform.instance.onMarkerDragEnd(mapId: mapId).listen( + (MarkerDragEndEvent e) => + _googleMapState.onMarkerDragEnd(e.value, e.position), + ), + ) + ..add( + GoogleMapsFlutterPlatform.instance.onInfoWindowTap(mapId: mapId).listen( + (InfoWindowTapEvent e) => + _googleMapState.onInfoWindowTap(e.value), + ), + ) + ..add( + GoogleMapsFlutterPlatform.instance.onPolylineTap(mapId: mapId).listen( + (PolylineTapEvent e) => _googleMapState.onPolylineTap(e.value), + ), + ) + ..add( + GoogleMapsFlutterPlatform.instance.onPolygonTap(mapId: mapId).listen( + (PolygonTapEvent e) => _googleMapState.onPolygonTap(e.value), + ), + ) + ..add( + GoogleMapsFlutterPlatform.instance + .onCircleTap(mapId: mapId) + .listen((CircleTapEvent e) => _googleMapState.onCircleTap(e.value)), + ) + ..add( + GoogleMapsFlutterPlatform.instance + .onTap(mapId: mapId) + .listen((MapTapEvent e) => _googleMapState.onTap(e.position)), + ) + ..add( + GoogleMapsFlutterPlatform.instance.onLongPress(mapId: mapId).listen( + (MapLongPressEvent e) => _googleMapState.onLongPress(e.position), + ), + ); } /// Updates configuration options of the map user interface. @@ -270,6 +311,9 @@ class GoogleMapController { /// Disposes of the platform resources void dispose() { + for (final subscription in _subscriptions) { + subscription.cancel(); + } GoogleMapsFlutterPlatform.instance.dispose(mapId: mapId); } } From 565def228b6b653315cad26f0d963c5d37c87d5c Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Thu, 22 Jun 2023 19:01:42 -0700 Subject: [PATCH 02/17] doc: update CHANGELOG.md for google_maps_flutter --- packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md b/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md index 6edda3e08049..db403251fe1c 100644 --- a/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md +++ b/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md @@ -1,3 +1,7 @@ +## 2.3.2 + +* Implemented a proper disposal mechanism for event subscriptions in `GoogleMapController`. Previously, subscriptions were not cancelled upon disposal of the controller, which could lead to memory leaks or unexpected behavior. The controller now tracks all event subscriptions and cancels them when `dispose()` is called. + ## 2.3.1 * Fixes a regression from 2.2.8 that could cause incorrect handling of a From d3a4f849967a9e922b6f5140dee9235f97563292 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Fri, 23 Jun 2023 09:26:37 -0700 Subject: [PATCH 03/17] fix: don't initialize if widget isn't mounted --- .../google_maps_flutter/lib/src/google_map.dart | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart index 08c2286527fb..1e8483b0d666 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/google_map.dart @@ -398,6 +398,9 @@ class _GoogleMapState extends State { } Future onPlatformViewCreated(int id) async { + if (!mounted) { + return; + } final GoogleMapController controller = await GoogleMapController.init( id, widget.initialCameraPosition, From e38f413f21374ae43b90030e60599687a8c227b0 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Fri, 23 Jun 2023 09:26:55 -0700 Subject: [PATCH 04/17] chore: version bump --- packages/google_maps_flutter/google_maps_flutter/pubspec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/pubspec.yaml b/packages/google_maps_flutter/google_maps_flutter/pubspec.yaml index e56ff513e7a4..26b4dfd930c6 100644 --- a/packages/google_maps_flutter/google_maps_flutter/pubspec.yaml +++ b/packages/google_maps_flutter/google_maps_flutter/pubspec.yaml @@ -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.1 +version: 2.3.2 environment: sdk: ">=3.0.0 <4.0.0" From 0aab33eddae912153a45f0dd48574aee0d14fc6b Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 13:26:10 -0700 Subject: [PATCH 05/17] test: add unit test to assert that StreamSubscription are cancelled --- .../lib/src/controller.dart | 144 ++++++++++-------- .../test/stream_subscription_test.dart | 81 ++++++++++ 2 files changed, 161 insertions(+), 64 deletions(-) create mode 100644 packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index 54d753e3de44..d552274e6096 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -18,8 +18,16 @@ class GoogleMapController { /// The mapId for this controller final int mapId; - /// Subscriptions to events from the native side of this controller. - final List> _subscriptions = []; + /// State map of the stream subscriptions for this controller. + /// + /// The map stores a [StreamSubscription] as key and a [bool] as value. + /// Each [StreamSubscription] represents a subscription to an event from the native side. + /// The corresponding [bool] indicates whether the subscription has been cancelled (true) or not (false). + /// + /// This map allows for efficient management of subscriptions, facilitating operations + /// like mass cancellation during controller disposal. + final Map, bool> streamSubscriptionsState = + , bool>{}; /// Initialize control of a [GoogleMap] with [id]. /// @@ -41,14 +49,14 @@ class GoogleMapController { void _connectStreams(int mapId) { if (_googleMapState.widget.onCameraMoveStarted != null) { - _subscriptions.add( + _addSubscription( GoogleMapsFlutterPlatform.instance .onCameraMoveStarted(mapId: mapId) .listen((_) => _googleMapState.widget.onCameraMoveStarted!()), ); } if (_googleMapState.widget.onCameraMove != null) { - _subscriptions.add( + _addSubscription( GoogleMapsFlutterPlatform.instance.onCameraMove(mapId: mapId).listen( (CameraMoveEvent e) => _googleMapState.widget.onCameraMove!(e.value), @@ -56,70 +64,74 @@ class GoogleMapController { ); } if (_googleMapState.widget.onCameraIdle != null) { - _subscriptions.add( + _addSubscription( GoogleMapsFlutterPlatform.instance .onCameraIdle(mapId: mapId) .listen((_) => _googleMapState.widget.onCameraIdle!()), ); } - _subscriptions - ..add( - GoogleMapsFlutterPlatform.instance - .onMarkerTap(mapId: mapId) - .listen((MarkerTapEvent e) => _googleMapState.onMarkerTap(e.value)), - ) - ..add( - GoogleMapsFlutterPlatform.instance - .onMarkerDragStart(mapId: mapId) - .listen( - (MarkerDragStartEvent e) => - _googleMapState.onMarkerDragStart(e.value, e.position), - ), - ) - ..add( - GoogleMapsFlutterPlatform.instance.onMarkerDrag(mapId: mapId).listen( - (MarkerDragEvent e) => - _googleMapState.onMarkerDrag(e.value, e.position), - ), - ) - ..add( - GoogleMapsFlutterPlatform.instance.onMarkerDragEnd(mapId: mapId).listen( - (MarkerDragEndEvent e) => - _googleMapState.onMarkerDragEnd(e.value, e.position), - ), - ) - ..add( - GoogleMapsFlutterPlatform.instance.onInfoWindowTap(mapId: mapId).listen( - (InfoWindowTapEvent e) => - _googleMapState.onInfoWindowTap(e.value), - ), - ) - ..add( - GoogleMapsFlutterPlatform.instance.onPolylineTap(mapId: mapId).listen( - (PolylineTapEvent e) => _googleMapState.onPolylineTap(e.value), - ), - ) - ..add( - GoogleMapsFlutterPlatform.instance.onPolygonTap(mapId: mapId).listen( - (PolygonTapEvent e) => _googleMapState.onPolygonTap(e.value), - ), - ) - ..add( - GoogleMapsFlutterPlatform.instance - .onCircleTap(mapId: mapId) - .listen((CircleTapEvent e) => _googleMapState.onCircleTap(e.value)), - ) - ..add( - GoogleMapsFlutterPlatform.instance - .onTap(mapId: mapId) - .listen((MapTapEvent e) => _googleMapState.onTap(e.position)), - ) - ..add( - GoogleMapsFlutterPlatform.instance.onLongPress(mapId: mapId).listen( - (MapLongPressEvent e) => _googleMapState.onLongPress(e.position), - ), - ); + _addSubscription( + GoogleMapsFlutterPlatform.instance + .onMarkerTap(mapId: mapId) + .listen((MarkerTapEvent e) => _googleMapState.onMarkerTap(e.value)), + ); + + _addSubscription( + GoogleMapsFlutterPlatform.instance.onMarkerDragStart(mapId: mapId).listen( + (MarkerDragStartEvent e) => + _googleMapState.onMarkerDragStart(e.value, e.position), + ), + ); + + _addSubscription( + GoogleMapsFlutterPlatform.instance.onMarkerDrag(mapId: mapId).listen( + (MarkerDragEvent e) => + _googleMapState.onMarkerDrag(e.value, e.position), + ), + ); + + _addSubscription( + GoogleMapsFlutterPlatform.instance.onMarkerDragEnd(mapId: mapId).listen( + (MarkerDragEndEvent e) => + _googleMapState.onMarkerDragEnd(e.value, e.position), + ), + ); + + _addSubscription( + GoogleMapsFlutterPlatform.instance.onInfoWindowTap(mapId: mapId).listen( + (InfoWindowTapEvent e) => _googleMapState.onInfoWindowTap(e.value), + ), + ); + _addSubscription( + GoogleMapsFlutterPlatform.instance.onPolylineTap(mapId: mapId).listen( + (PolylineTapEvent e) => _googleMapState.onPolylineTap(e.value), + ), + ); + _addSubscription( + GoogleMapsFlutterPlatform.instance.onPolygonTap(mapId: mapId).listen( + (PolygonTapEvent e) => _googleMapState.onPolygonTap(e.value), + ), + ); + _addSubscription( + GoogleMapsFlutterPlatform.instance + .onCircleTap(mapId: mapId) + .listen((CircleTapEvent e) => _googleMapState.onCircleTap(e.value)), + ); + _addSubscription( + GoogleMapsFlutterPlatform.instance + .onTap(mapId: mapId) + .listen((MapTapEvent e) => _googleMapState.onTap(e.position)), + ); + _addSubscription( + GoogleMapsFlutterPlatform.instance.onLongPress(mapId: mapId).listen( + (MapLongPressEvent e) => _googleMapState.onLongPress(e.position), + ), + ); + } + + void _addSubscription(StreamSubscription subscription) { + streamSubscriptionsState[subscription] = false; } /// Updates configuration options of the map user interface. @@ -311,9 +323,13 @@ class GoogleMapController { /// Disposes of the platform resources void dispose() { - for (final subscription in _subscriptions) { - subscription.cancel(); + for (final StreamSubscription subscription + in streamSubscriptionsState.keys) { + subscription.cancel().then((void value) { + streamSubscriptionsState[subscription] = true; + }); } + streamSubscriptionsState.clear(); GoogleMapsFlutterPlatform.instance.dispose(mapId: mapId); } } diff --git a/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart b/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart new file mode 100644 index 000000000000..e0c6c092cd50 --- /dev/null +++ b/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart @@ -0,0 +1,81 @@ +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_google_maps_flutter_platform.dart'; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + group('GoogleMap widget', () { + testWidgets('Subscriptions are canceled on dispose', + (WidgetTester tester) async { + GoogleMapsFlutterPlatform.instance = FakeGoogleMapsFlutterPlatform(); + + final FakePlatformViewsController fakePlatformViewsController = + FakePlatformViewsController(); + + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform_views, + fakePlatformViewsController.fakePlatformViewsMethodHandler, + ); + + final ValueNotifier controllerNotifier = + ValueNotifier(null); + + final GoogleMap googleMap = GoogleMap( + onMapCreated: (GoogleMapController controller) { + controllerNotifier.value = controller; + }, + initialCameraPosition: const CameraPosition( + target: LatLng(0, 0), + ), + ); + + await tester.pumpWidget(Directionality( + textDirection: TextDirection.ltr, + child: googleMap, + )); + + await tester.pump(); + + final GoogleMapController? controller = controllerNotifier.value; + + if (controller != null) { + expect(controller.streamSubscriptionsState, isNotEmpty); + + controller.streamSubscriptionsState.forEach((_, bool isCanceled) { + expect(isCanceled, isFalse); + }); + + await tester.pumpWidget(const Directionality( + textDirection: TextDirection.ltr, + child: SizedBox(), + )); + + await tester.binding.runAsync(() async { + await tester.pump(); + }); + + controller.streamSubscriptionsState.forEach((_, bool isCanceled) { + expect(isCanceled, isTrue); + }); + } else { + fail('GoogleMapController not created'); + } + }); + }); +} + +class FakePlatformViewsController { + Future fakePlatformViewsMethodHandler(MethodCall call) { + switch (call.method) { + case 'create': + return Future.value(1); + default: + return Future.value(null); + } + } +} From 971a143f1981aa893a3c81a88010ab362e453c79 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 13:37:12 -0700 Subject: [PATCH 06/17] doc: add license to new test file --- .../google_maps_flutter/test/stream_subscription_test.dart | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart b/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart index e0c6c092cd50..8aab7a098b72 100644 --- a/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart +++ b/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart @@ -1,3 +1,7 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// 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'; From 4065b901ae330ba93d30cb20142d58c98138b4f7 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 13:56:21 -0700 Subject: [PATCH 07/17] chore: correct linting/analyze issues --- .../google_maps_flutter/test/stream_subscription_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart b/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart index 8aab7a098b72..1ca7bc1de649 100644 --- a/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart +++ b/packages/google_maps_flutter/google_maps_flutter/test/stream_subscription_test.dart @@ -79,7 +79,7 @@ class FakePlatformViewsController { case 'create': return Future.value(1); default: - return Future.value(null); + return Future.value(); } } } From 509fe6c2a2bca1dff61b94469e50a225fa62ecc0 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 14:25:50 -0700 Subject: [PATCH 08/17] doc: add doc string for _addSubscription --- .../google_maps_flutter/lib/src/controller.dart | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index d552274e6096..208fc3abe605 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -130,6 +130,15 @@ class GoogleMapController { ); } + /// Adds a new stream subscription to the `streamSubscriptionsState` map. + /// + /// This method accepts a `StreamSubscription` object and adds it to the `streamSubscriptionsState` map, + /// initializing its state to `false` to indicate that the subscription has not been canceled yet. + /// + /// The purpose of maintaining this map is to keep track of all active stream subscriptions, + /// so they can be properly canceled and their states updated when the object is disposed. + /// + /// @param subscription The `StreamSubscription` object to be added to the `streamSubscriptionsState` map. void _addSubscription(StreamSubscription subscription) { streamSubscriptionsState[subscription] = false; } From b7783494c482adc6bbe95b751e4886e98639a848 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 14:30:42 -0700 Subject: [PATCH 09/17] doc: fixup doc string for _addSubscription --- .../google_maps_flutter/lib/src/controller.dart | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index 208fc3abe605..40132a8463a2 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -130,15 +130,16 @@ class GoogleMapController { ); } - /// Adds a new stream subscription to the `streamSubscriptionsState` map. + /// Adds a new stream subscription to the [streamSubscriptionsState] map. /// - /// This method accepts a `StreamSubscription` object and adds it to the `streamSubscriptionsState` map, - /// initializing its state to `false` to indicate that the subscription has not been canceled yet. + /// The method takes a [StreamSubscription] and sets its status to `false` + /// in the [streamSubscriptionsState] map. This indicates that the subscription + /// has not been canceled yet. /// - /// The purpose of maintaining this map is to keep track of all active stream subscriptions, - /// so they can be properly canceled and their states updated when the object is disposed. + /// The [streamSubscriptionsState] map keeps track of all active stream subscriptions, + /// allowing them to be properly managed and disposed when necessary. /// - /// @param subscription The `StreamSubscription` object to be added to the `streamSubscriptionsState` map. + /// [subscription] is the [StreamSubscription] to be added to the [streamSubscriptionsState] map. void _addSubscription(StreamSubscription subscription) { streamSubscriptionsState[subscription] = false; } From a21690ee1cf8b034b1d2e11380039ee426872abf Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 14:35:25 -0700 Subject: [PATCH 10/17] doc: fixup CHANGELOG --- packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md b/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md index db403251fe1c..c21abce0a7e0 100644 --- a/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md +++ b/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md @@ -1,6 +1,6 @@ ## 2.3.2 -* Implemented a proper disposal mechanism for event subscriptions in `GoogleMapController`. Previously, subscriptions were not cancelled upon disposal of the controller, which could lead to memory leaks or unexpected behavior. The controller now tracks all event subscriptions and cancels them when `dispose()` is called. +* Implemented a proper disposal mechanism for stream subscriptions in `GoogleMapController`. Previously, subscriptions were not cancelled upon disposal of the controller, which could lead to memory leaks or unexpected behavior. The controller now tracks all stream subscriptions and cancels them when `dispose()` is called. ## 2.3.1 From 61c293eade2b018b4b0322e9a19255675340848a Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 15:05:05 -0700 Subject: [PATCH 11/17] chore: limit expose to streamSubscriptionsState through getter --- .../lib/src/controller.dart | 38 ++++++++++++++----- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index 40132a8463a2..e2de0b4556f3 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -18,7 +18,7 @@ class GoogleMapController { /// The mapId for this controller final int mapId; - /// State map of the stream subscriptions for this controller. + /// State map of the stream subscriptions for this controller, used internally. /// /// The map stores a [StreamSubscription] as key and a [bool] as value. /// Each [StreamSubscription] represents a subscription to an event from the native side. @@ -26,9 +26,27 @@ class GoogleMapController { /// /// This map allows for efficient management of subscriptions, facilitating operations /// like mass cancellation during controller disposal. - final Map, bool> streamSubscriptionsState = + /// + /// This field is private to prevent external mutation. Access is provided through + /// the [streamSubscriptionsState] getter, which returns an unmodifiable view of this map. + final Map, bool> _streamSubscriptionsState = , bool>{}; + /// Returns an unmodifiable view of the map that holds the active StreamSubscriptions + /// and their corresponding states. + /// + /// This getter provides a way to inspect the current state of active StreamSubscriptions + /// in the `GoogleMapController` class without allowing any outside mutations. + /// + /// The map contains StreamSubscription instances as keys, and bool as values. + /// A value of `true` indicates that the subscription is active and `false` indicates it is disposed. + /// + /// This is useful in situations where the need arises to monitor or debug the + /// lifecycle of various StreamSubscriptions handled within this class. + Map, bool> get streamSubscriptionsState => + Map, bool>.unmodifiable( + _streamSubscriptionsState); + /// Initialize control of a [GoogleMap] with [id]. /// /// Mainly for internal use when instantiating a [GoogleMapController] passed @@ -130,18 +148,18 @@ class GoogleMapController { ); } - /// Adds a new stream subscription to the [streamSubscriptionsState] map. + /// Adds a new stream subscription to the [_streamSubscriptionsState] map. /// /// The method takes a [StreamSubscription] and sets its status to `false` - /// in the [streamSubscriptionsState] map. This indicates that the subscription + /// in the [_streamSubscriptionsState] map. This indicates that the subscription /// has not been canceled yet. /// - /// The [streamSubscriptionsState] map keeps track of all active stream subscriptions, + /// The [_streamSubscriptionsState] map keeps track of all active stream subscriptions, /// allowing them to be properly managed and disposed when necessary. /// - /// [subscription] is the [StreamSubscription] to be added to the [streamSubscriptionsState] map. + /// [subscription] is the [StreamSubscription] to be added to the [_streamSubscriptionsState] map. void _addSubscription(StreamSubscription subscription) { - streamSubscriptionsState[subscription] = false; + _streamSubscriptionsState[subscription] = false; } /// Updates configuration options of the map user interface. @@ -334,12 +352,12 @@ class GoogleMapController { /// Disposes of the platform resources void dispose() { for (final StreamSubscription subscription - in streamSubscriptionsState.keys) { + in _streamSubscriptionsState.keys) { subscription.cancel().then((void value) { - streamSubscriptionsState[subscription] = true; + _streamSubscriptionsState[subscription] = true; }); } - streamSubscriptionsState.clear(); + _streamSubscriptionsState.clear(); GoogleMapsFlutterPlatform.instance.dispose(mapId: mapId); } } From 6a399e07343c41e97be00f2814a9715b584f86c8 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 15:12:00 -0700 Subject: [PATCH 12/17] doc: fixup doc strings --- .../google_maps_flutter/lib/src/controller.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index e2de0b4556f3..1e89795f5c67 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -22,7 +22,7 @@ class GoogleMapController { /// /// The map stores a [StreamSubscription] as key and a [bool] as value. /// Each [StreamSubscription] represents a subscription to an event from the native side. - /// The corresponding [bool] indicates whether the subscription has been cancelled (true) or not (false). + /// The corresponding [bool] indicates whether the subscription has been cancelled `true` or not `false`. /// /// This map allows for efficient management of subscriptions, facilitating operations /// like mass cancellation during controller disposal. @@ -36,9 +36,9 @@ class GoogleMapController { /// and their corresponding states. /// /// This getter provides a way to inspect the current state of active StreamSubscriptions - /// in the `GoogleMapController` class without allowing any outside mutations. + /// in the [GoogleMapController] class without allowing any outside mutations. /// - /// The map contains StreamSubscription instances as keys, and bool as values. + /// The map contains [StreamSubscription] instances as keys, and [bool] as values. /// A value of `true` indicates that the subscription is active and `false` indicates it is disposed. /// /// This is useful in situations where the need arises to monitor or debug the From 2a1a5e96ecde7151f1658c3514b65d956462916a Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 15:18:51 -0700 Subject: [PATCH 13/17] doc: fill in more docs and fixup doc strings --- .../lib/src/controller.dart | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index 1e89795f5c67..d914bf37ccd3 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -63,8 +63,21 @@ class GoogleMapController { ); } + /// The state object of the [GoogleMap] widget. + /// + /// This instance holds important properties and methods that are essential for + /// the behavior and state of the [GoogleMap] widget. final _GoogleMapState _googleMapState; + /// Subscribes to the various event streams from the [GoogleMapsFlutterPlatform] for a specific map. + /// + /// This method takes the `mapId` as an argument and uses it to subscribe to the various streams available from the [GoogleMapsFlutterPlatform]. + /// It only subscribes to the streams if the corresponding callback from the widget is not null. This includes streams for camera movements, + /// marker taps, marker drag events, info window taps, polyline taps, polygon taps, circle taps, map taps, and map long presses. + /// + /// Each subscription is added to [_streamSubscriptionsState] for lifecycle management. + /// + /// - [mapId] is the identifier of the map for which to initiate the streams. void _connectStreams(int mapId) { if (_googleMapState.widget.onCameraMoveStarted != null) { _addSubscription( @@ -151,13 +164,14 @@ class GoogleMapController { /// Adds a new stream subscription to the [_streamSubscriptionsState] map. /// /// The method takes a [StreamSubscription] and sets its status to `false` - /// in the [_streamSubscriptionsState] map. This indicates that the subscription - /// has not been canceled yet. + /// in the [_streamSubscriptionsState] map. The `false` status indicates that the subscription + /// is currently active and has not been canceled yet. /// - /// The [_streamSubscriptionsState] map keeps track of all active stream subscriptions, - /// allowing them to be properly managed and disposed when necessary. + /// The [_streamSubscriptionsState] map manages all active stream subscriptions + /// associated with this instance, which facilitates their lifecycle management + /// and ensures that they are properly canceled and disposed when necessary. /// - /// [subscription] is the [StreamSubscription] to be added to the [_streamSubscriptionsState] map. + /// [subscription]: A [StreamSubscription] that will be managed by the [_streamSubscriptionsState] map. void _addSubscription(StreamSubscription subscription) { _streamSubscriptionsState[subscription] = false; } From ccb562cf6fd3817fdb34cd79bbd9a79a02059540 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 17:35:36 -0700 Subject: [PATCH 14/17] doc: fixup doc strings --- .../google_maps_flutter/lib/src/controller.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index d914bf37ccd3..b056099cce45 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -22,7 +22,7 @@ class GoogleMapController { /// /// The map stores a [StreamSubscription] as key and a [bool] as value. /// Each [StreamSubscription] represents a subscription to an event from the native side. - /// The corresponding [bool] indicates whether the subscription has been cancelled `true` or not `false`. + /// The corresponding [bool] indicates whether the subscription has been canceled `true` or not `false`. /// /// This map allows for efficient management of subscriptions, facilitating operations /// like mass cancellation during controller disposal. @@ -39,7 +39,7 @@ class GoogleMapController { /// in the [GoogleMapController] class without allowing any outside mutations. /// /// The map contains [StreamSubscription] instances as keys, and [bool] as values. - /// A value of `true` indicates that the subscription is active and `false` indicates it is disposed. + /// The value indicates if the subscription has been canceled `true` or not `false`. /// /// This is useful in situations where the need arises to monitor or debug the /// lifecycle of various StreamSubscriptions handled within this class. From df4c8353ab4852c5801e55990c6a5b099998a385 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 19:03:33 -0700 Subject: [PATCH 15/17] chore: cleanup whitespace --- .../google_maps_flutter/lib/src/controller.dart | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart index b056099cce45..41f6d9128820 100644 --- a/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart +++ b/packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart @@ -101,34 +101,29 @@ class GoogleMapController { .listen((_) => _googleMapState.widget.onCameraIdle!()), ); } - _addSubscription( GoogleMapsFlutterPlatform.instance .onMarkerTap(mapId: mapId) .listen((MarkerTapEvent e) => _googleMapState.onMarkerTap(e.value)), ); - _addSubscription( GoogleMapsFlutterPlatform.instance.onMarkerDragStart(mapId: mapId).listen( (MarkerDragStartEvent e) => _googleMapState.onMarkerDragStart(e.value, e.position), ), ); - _addSubscription( GoogleMapsFlutterPlatform.instance.onMarkerDrag(mapId: mapId).listen( (MarkerDragEvent e) => _googleMapState.onMarkerDrag(e.value, e.position), ), ); - _addSubscription( GoogleMapsFlutterPlatform.instance.onMarkerDragEnd(mapId: mapId).listen( (MarkerDragEndEvent e) => _googleMapState.onMarkerDragEnd(e.value, e.position), ), ); - _addSubscription( GoogleMapsFlutterPlatform.instance.onInfoWindowTap(mapId: mapId).listen( (InfoWindowTapEvent e) => _googleMapState.onInfoWindowTap(e.value), From fd940d67ba4aa06941b5d0dbfed05f84470fdbec Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Tue, 27 Jun 2023 19:21:15 -0700 Subject: [PATCH 16/17] doc: cleanup CHANGELOG.md --- packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md b/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md index c21abce0a7e0..8ed643b9f543 100644 --- a/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md +++ b/packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md @@ -1,6 +1,8 @@ ## 2.3.2 -* Implemented a proper disposal mechanism for stream subscriptions in `GoogleMapController`. Previously, subscriptions were not cancelled upon disposal of the controller, which could lead to memory leaks or unexpected behavior. The controller now tracks all stream subscriptions and cancels them when `dispose()` is called. +* Fixes memory leak by implementing mechanism for disposing stream subscriptions in `GoogleMapController`. + Previously, subscriptions were not canceled upon disposal of the controller, which could lead to memory leaks or + unexpected behavior. The controller now tracks all stream subscriptions and cancels them when `dispose()` is called. ## 2.3.1 From 19bc07bfe42b707212af964b9b366125e2956a72 Mon Sep 17 00:00:00 2001 From: Patrick Wolf Date: Wed, 28 Jun 2023 08:43:00 -0700 Subject: [PATCH 17/17] doc: add myself to authors --- packages/google_maps_flutter/google_maps_flutter/AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/google_maps_flutter/google_maps_flutter/AUTHORS b/packages/google_maps_flutter/google_maps_flutter/AUTHORS index 9f1b53ee2667..82923e29f668 100644 --- a/packages/google_maps_flutter/google_maps_flutter/AUTHORS +++ b/packages/google_maps_flutter/google_maps_flutter/AUTHORS @@ -65,3 +65,4 @@ Anton Borries Alex Li Rahul Raj <64.rahulraj@gmail.com> Taha Tesser +Patrick Wolf