diff --git a/packages/flutter/lib/src/gestures/multidrag.dart b/packages/flutter/lib/src/gestures/multidrag.dart index 5d7037b056f..cac89c986b5 100644 --- a/packages/flutter/lib/src/gestures/multidrag.dart +++ b/packages/flutter/lib/src/gestures/multidrag.dart @@ -82,13 +82,13 @@ abstract class MultiDragPointerState { _velocityTracker.addPosition(event.timeStamp, event.position); if (_client != null) { assert(pendingDelta == null); + // Call client last to avoid reentrancy. _client.update(new DragUpdateDetails(delta: event.delta)); } else { assert(pendingDelta != null); _pendingDelta += event.delta; checkForResolutionAfterMove(); } - return null; } /// Override this to call resolve() if the drag should be accepted or rejected. @@ -123,34 +123,41 @@ abstract class MultiDragPointerState { assert(client != null); assert(pendingDelta != null); _client = client; - _client.update(new DragUpdateDetails(delta: pendingDelta)); + final DragUpdateDetails details = new DragUpdateDetails(delta: pendingDelta); _pendingDelta = null; + // Call client last to avoid reentrancy. + _client.update(details); } void _up() { assert(_arenaEntry != null); + _arenaEntry = null; if (_client != null) { assert(pendingDelta == null); - _client.end(new DragEndDetails(velocity: _velocityTracker.getVelocity() ?? Velocity.zero)); + final DragEndDetails details = new DragEndDetails(velocity: _velocityTracker.getVelocity() ?? Velocity.zero); + final Drag client = _client; _client = null; + // Call client last to avoid reentrancy. + client.end(details); } else { assert(pendingDelta != null); _pendingDelta = null; } - _arenaEntry = null; } void _cancel() { assert(_arenaEntry != null); + _arenaEntry = null; if (_client != null) { assert(pendingDelta == null); - _client.cancel(); + final Drag client = _client; _client = null; + // Call client last to avoid reentrancy. + client.cancel(); } else { assert(pendingDelta != null); _pendingDelta = null; } - _arenaEntry = null; } /// Releases any resources used by the object. @@ -213,13 +220,16 @@ abstract class MultiDragGestureRecognizer exten T state = _pointers[event.pointer]; if (event is PointerMoveEvent) { state._move(event); + // We might be disposed here. } else if (event is PointerUpEvent) { assert(event.delta == Offset.zero); state._up(); + // We might be disposed here. _removeState(event.pointer); } else if (event is PointerCancelEvent) { assert(event.delta == Offset.zero); state._cancel(); + // We might be disposed here. _removeState(event.pointer); } else if (event is! PointerDownEvent) { // we get the PointerDownEvent that resulted in our addPointer getting called since we @@ -266,7 +276,11 @@ abstract class MultiDragGestureRecognizer exten } void _removeState(int pointer) { - assert(_pointers != null); + if (_pointers == null) { + // We've already been disposed. It's harmless to skip removing the state + // for the given pointer because dispose() has already removed it. + return; + } assert(_pointers.containsKey(pointer)); GestureBinding.instance.pointerRouter.removeRoute(pointer, _handleEvent); _pointers.remove(pointer).dispose(); diff --git a/packages/flutter/lib/src/material/material.dart b/packages/flutter/lib/src/material/material.dart index aea80ff7e7f..36974ce9c68 100644 --- a/packages/flutter/lib/src/material/material.dart +++ b/packages/flutter/lib/src/material/material.dart @@ -63,7 +63,7 @@ abstract class InkSplash { /// Causes the reaction to propagate faster across the material. void confirm(); - /// The user input was cancelled. + /// The user input was canceled. /// /// Causes the reaction to gradually disappear. void cancel(); diff --git a/packages/flutter/lib/src/widgets/drag_target.dart b/packages/flutter/lib/src/widgets/drag_target.dart index 6f7b21ff788..1f679369ebc 100644 --- a/packages/flutter/lib/src/widgets/drag_target.dart +++ b/packages/flutter/lib/src/widgets/drag_target.dart @@ -134,6 +134,13 @@ class Draggable extends StatefulWidget { final VoidCallback onDragStarted; /// Called when the draggable is dropped without being accepted by a [DragTarget]. + /// + /// This function might be called after this widget has been removed from the + /// tree. For example, if a drag was in progress when this widget was removed + /// from the tree and the drag ended up being canceled, this callback will + /// still be called. For this reason, implementations of this callback might + /// need to check [State.mounted] to check whether the state receiving the + /// callback is still in the tree. final DraggableCanceledCallback onDraggableCanceled; /// Creates a gesture recognizer that recognizes the start of the drag. @@ -206,13 +213,29 @@ class _DraggableState extends State> { @override void dispose() { - _recognizer.dispose(); + _disposeRecognizerIfInactive(); super.dispose(); } + // This gesture recognizer has an unusual lifetime. We want to support the use + // case of removing the Draggable from the tree in the middle of a drag. That + // means we need to keep this recognizer alive after this state object has + // been disposed because it's the one listening to the pointer events that are + // driving the drag. + // + // We achieve that by keeping count of the number of active drags and only + // disposing the gesture recognizer after (a) this state object has been + // disposed and (b) there are no more active drags. GestureRecognizer _recognizer; int _activeCount = 0; + void _disposeRecognizerIfInactive() { + if (_activeCount > 0) + return; + _recognizer.dispose(); + _recognizer = null; + } + void _routePointer(PointerEvent event) { if (config.maxSimultaneousDrags != null && _activeCount >= config.maxSimultaneousDrags) return; @@ -243,11 +266,16 @@ class _DraggableState extends State> { feedback: config.feedback, feedbackOffset: config.feedbackOffset, onDragEnd: (Velocity velocity, Offset offset, bool wasAccepted) { - setState(() { + if (mounted) { + setState(() { + _activeCount -= 1; + }); + } else { _activeCount -= 1; - if (!wasAccepted && config.onDraggableCanceled != null) - config.onDraggableCanceled(velocity, offset); - }); + _disposeRecognizerIfInactive(); + } + if (!wasAccepted && config.onDraggableCanceled != null) + config.onDraggableCanceled(velocity, offset); } ); if (config.onDragStarted != null) @@ -366,9 +394,8 @@ typedef void _OnDragEnd(Velocity velocity, Offset offset, bool wasAccepted); // The lifetime of this object is a little dubious right now. Specifically, it // lives as long as the pointer is down. Arguably it should self-immolate if the -// overlay goes away, or maybe even if the Draggable that created goes away. -// This will probably need to be changed once we have more experience with using -// this widget. +// overlay goes away. _DraggableState has some delicate logic to continue +// eeding this object pointer events even after it has been disposed. class _DragAvatar extends Drag { _DragAvatar({ OverlayState overlay, @@ -400,7 +427,6 @@ class _DragAvatar extends Drag { Offset _lastOffset; OverlayEntry _entry; - // Drag API @override void update(DragUpdateDetails details) { _position += details.delta; diff --git a/packages/flutter/test/widget/draggable_test.dart b/packages/flutter/test/widget/draggable_test.dart index c8a8e2fe673..019290da00f 100644 --- a/packages/flutter/test/widget/draggable_test.dart +++ b/packages/flutter/test/widget/draggable_test.dart @@ -1124,6 +1124,82 @@ void main() { await gesture.up(); await tester.pump(); }); + + testWidgets('Drag and drop - remove draggable', (WidgetTester tester) async { + List accepted = []; + + await tester.pumpWidget(new MaterialApp( + home: new Column( + children: [ + new Draggable( + data: 1, + child: new Text('Source'), + feedback: new Text('Dragging') + ), + new DragTarget( + builder: (BuildContext context, List data, List rejects) { + return new Container(height: 100.0, child: new Text('Target')); + }, + onAccept: (int data) { + accepted.add(data); + } + ), + ] + ) + )); + + expect(accepted, isEmpty); + expect(find.text('Source'), findsOneWidget); + expect(find.text('Dragging'), findsNothing); + expect(find.text('Target'), findsOneWidget); + + Point firstLocation = tester.getCenter(find.text('Source')); + TestGesture gesture = await tester.startGesture(firstLocation, pointer: 7); + await tester.pump(); + + expect(accepted, isEmpty); + expect(find.text('Source'), findsOneWidget); + expect(find.text('Dragging'), findsOneWidget); + expect(find.text('Target'), findsOneWidget); + + await tester.pumpWidget(new MaterialApp( + home: new Column( + children: [ + new DragTarget( + builder: (BuildContext context, List data, List rejects) { + return new Container(height: 100.0, child: new Text('Target')); + }, + onAccept: (int data) { + accepted.add(data); + } + ), + ] + ) + )); + + expect(accepted, isEmpty); + expect(find.text('Source'), findsNothing); + expect(find.text('Dragging'), findsOneWidget); + expect(find.text('Target'), findsOneWidget); + + Point secondLocation = tester.getCenter(find.text('Target')); + await gesture.moveTo(secondLocation); + await tester.pump(); + + expect(accepted, isEmpty); + expect(find.text('Source'), findsNothing); + expect(find.text('Dragging'), findsOneWidget); + expect(find.text('Target'), findsOneWidget); + + await gesture.up(); + await tester.pump(); + + expect(accepted, equals([1])); + expect(find.text('Source'), findsNothing); + expect(find.text('Dragging'), findsNothing); + expect(find.text('Target'), findsOneWidget); + }); + } class DragTargetData { }