Removing a Draggable during a drag should work (#6341)

Previously we would maroon the feedback in the overlay. Now we let the
drag proceed and only tear down the gesture recognizer when all the
drags it spawns have been resolved.

Fixes #6151

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<T extends MultiDragPointerState> 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<T extends MultiDragPointerState> 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();

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();

packages/flutter/lib/src/widgets/drag_target.dart

@@ -134,6 +134,13 @@ class Draggable<T> 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<T> extends State<Draggable<T>> {
 
   @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<T> extends State<Draggable<T>> {
       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<T> extends Drag {
   _DragAvatar({
     OverlayState overlay,
@@ -400,7 +427,6 @@ class _DragAvatar<T> extends Drag {
   Offset _lastOffset;
   OverlayEntry _entry;
 
-  // Drag API
   @override
   void update(DragUpdateDetails details) {
     _position += details.delta;

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<int> accepted = <int>[];
+
+    await tester.pumpWidget(new MaterialApp(
+        home: new Column(
+            children: <Widget>[
+              new Draggable<int>(
+                  data: 1,
+                  child: new Text('Source'),
+                  feedback: new Text('Dragging')
+              ),
+              new DragTarget<int>(
+                  builder: (BuildContext context, List<int> data, List<dynamic> 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: <Widget>[
+              new DragTarget<int>(
+                  builder: (BuildContext context, List<int> data, List<dynamic> 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(<int>[1]));
+    expect(find.text('Source'), findsNothing);
+    expect(find.text('Dragging'), findsNothing);
+    expect(find.text('Target'), findsOneWidget);
+  });
+
 }
 
 class DragTargetData { }