Unverified Commit 650b2406 authored by Darren Austin's avatar Darren Austin Committed by GitHub

Fix a bug with duplicate keys being used in the ReorderableListView. (#74842)

parent 164671e2
...@@ -477,14 +477,13 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -477,14 +477,13 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
// to be inserted. // to be inserted.
final Map<int, _ReorderableItemState> _items = <int, _ReorderableItemState>{}; final Map<int, _ReorderableItemState> _items = <int, _ReorderableItemState>{};
bool _reorderingDrag = false;
bool _autoScrolling = false;
OverlayEntry? _overlayEntry; OverlayEntry? _overlayEntry;
_ReorderableItemState? _dragItem; int? _dragIndex;
_DragInfo? _dragInfo; _DragInfo? _dragInfo;
int? _insertIndex; int? _insertIndex;
Offset? _finalDropPosition; Offset? _finalDropPosition;
MultiDragGestureRecognizer<MultiDragPointerState>? _recognizer; MultiDragGestureRecognizer<MultiDragPointerState>? _recognizer;
bool _autoScrolling = false;
late ScrollableState _scrollable; late ScrollableState _scrollable;
Axis get _scrollDirection => axisDirectionToAxis(_scrollable.axisDirection); Axis get _scrollDirection => axisDirectionToAxis(_scrollable.axisDirection);
...@@ -530,11 +529,11 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -530,11 +529,11 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
}) { }) {
assert(0 <= index && index < widget.itemCount); assert(0 <= index && index < widget.itemCount);
setState(() { setState(() {
if (_reorderingDrag) { if (_dragInfo != null) {
cancelReorder(); cancelReorder();
} }
if (_items.containsKey(index)) { if (_items.containsKey(index)) {
_dragItem = _items[index]!; _dragIndex = index;
_recognizer = recognizer _recognizer = recognizer
..onStart = _dragStart ..onStart = _dragStart
..addPointer(event); ..addPointer(event);
...@@ -561,6 +560,10 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -561,6 +560,10 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
void _registerItem(_ReorderableItemState item) { void _registerItem(_ReorderableItemState item) {
_items[item.index] = item; _items[item.index] = item;
if (item.index == _dragInfo?.index) {
item.dragging = true;
item.rebuild();
}
} }
void _unregisterItem(int index, _ReorderableItemState item) { void _unregisterItem(int index, _ReorderableItemState item) {
...@@ -571,11 +574,12 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -571,11 +574,12 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
} }
Drag? _dragStart(Offset position) { Drag? _dragStart(Offset position) {
assert(_reorderingDrag == false); assert(_dragInfo == null);
final _ReorderableItemState item = _dragItem!; final _ReorderableItemState item = _items[_dragIndex!]!;
item.dragging = true;
item.rebuild();
_insertIndex = item.index; _insertIndex = item.index;
_reorderingDrag = true;
_dragInfo = _DragInfo( _dragInfo = _DragInfo(
item: item, item: item,
initialPosition: position, initialPosition: position,
...@@ -587,15 +591,13 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -587,15 +591,13 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
proxyDecorator: widget.proxyDecorator, proxyDecorator: widget.proxyDecorator,
tickerProvider: this, tickerProvider: this,
); );
_dragInfo!.startDrag();
final OverlayState overlay = Overlay.of(context)!; final OverlayState overlay = Overlay.of(context)!;
assert(_overlayEntry == null); assert(_overlayEntry == null);
_overlayEntry = OverlayEntry(builder: _dragInfo!.createProxy); _overlayEntry = OverlayEntry(builder: _dragInfo!.createProxy);
overlay.insert(_overlayEntry!); overlay.insert(_overlayEntry!);
_dragInfo!.startDrag();
item.dragging = true;
for (final _ReorderableItemState childItem in _items.values) { for (final _ReorderableItemState childItem in _items.values) {
if (childItem == item || !childItem.mounted) if (childItem == item || !childItem.mounted)
continue; continue;
...@@ -636,7 +638,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -636,7 +638,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
} }
void _dropCompleted() { void _dropCompleted() {
final int fromIndex = _dragItem!.index; final int fromIndex = _dragIndex!;
final int toIndex = _insertIndex!; final int toIndex = _insertIndex!;
if (fromIndex != toIndex) { if (fromIndex != toIndex) {
widget.onReorder.call(fromIndex, toIndex); widget.onReorder.call(fromIndex, toIndex);
...@@ -646,10 +648,13 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -646,10 +648,13 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
void _dragReset() { void _dragReset() {
setState(() { setState(() {
if (_reorderingDrag) { if (_dragInfo != null) {
_reorderingDrag = false; if (_dragIndex != null && _items.containsKey(_dragIndex)) {
_dragItem!.dragging = false; final _ReorderableItemState dragItem = _items[_dragIndex!]!;
_dragItem = null; dragItem._dragging = false;
dragItem.rebuild();
_dragIndex = null;
}
_dragInfo?.dispose(); _dragInfo?.dispose();
_dragInfo = null; _dragInfo = null;
_resetItemGap(); _resetItemGap();
...@@ -669,10 +674,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -669,10 +674,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
} }
void _dragUpdateItems() { void _dragUpdateItems() {
assert(_reorderingDrag);
assert(_dragItem != null);
assert(_dragInfo != null); assert(_dragInfo != null);
final _ReorderableItemState gapItem = _dragItem!;
final double gapExtent = _dragInfo!.itemExtent; final double gapExtent = _dragInfo!.itemExtent;
final double proxyItemStart = _offsetExtent(_dragInfo!.dragPosition - _dragInfo!.dragOffset, _scrollDirection); final double proxyItemStart = _offsetExtent(_dragInfo!.dragPosition - _dragInfo!.dragOffset, _scrollDirection);
final double proxyItemEnd = proxyItemStart + gapExtent; final double proxyItemEnd = proxyItemStart + gapExtent;
...@@ -680,7 +682,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -680,7 +682,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
// Find the new index for inserting the item being dragged. // Find the new index for inserting the item being dragged.
int newIndex = _insertIndex!; int newIndex = _insertIndex!;
for (final _ReorderableItemState item in _items.values) { for (final _ReorderableItemState item in _items.values) {
if (item == gapItem || !item.mounted) if (item.index == _dragIndex! || !item.mounted)
continue; continue;
final Rect geometry = item.targetGeometry(); final Rect geometry = item.targetGeometry();
...@@ -735,7 +737,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -735,7 +737,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
if (newIndex != _insertIndex) { if (newIndex != _insertIndex) {
_insertIndex = newIndex; _insertIndex = newIndex;
for (final _ReorderableItemState item in _items.values) { for (final _ReorderableItemState item in _items.values) {
if (item == gapItem || !item.mounted) if (item.index == _dragIndex! || !item.mounted)
continue; continue;
item.updateForGap(newIndex, gapExtent, true, _reverse); item.updateForGap(newIndex, gapExtent, true, _reverse);
} }
...@@ -784,7 +786,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -784,7 +786,7 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
curve: Curves.linear curve: Curves.linear
); );
_autoScrolling = false; _autoScrolling = false;
if (_dragItem != null) { if (_dragInfo != null) {
_dragUpdateItems(); _dragUpdateItems();
_autoScrollIfNecessary(); _autoScrollIfNecessary();
} }
...@@ -824,7 +826,8 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke ...@@ -824,7 +826,8 @@ class SliverReorderableListState extends State<SliverReorderableList> with Ticke
// When dragging, the dragged item is still in the list but has been replaced // When dragging, the dragged item is still in the list but has been replaced
// by a zero height SizedBox, so that the gap can move around. To make the // by a zero height SizedBox, so that the gap can move around. To make the
// list extent stable we add a dummy entry to the end. // list extent stable we add a dummy entry to the end.
delegate: SliverChildBuilderDelegate(_itemBuilder, childCount: widget.itemCount + (_reorderingDrag ? 1 : 0)), delegate: SliverChildBuilderDelegate(_itemBuilder,
childCount: widget.itemCount + (_dragInfo != null ? 1 : 0)),
); );
} }
} }
...@@ -1071,7 +1074,7 @@ typedef _DragItemCallback = void Function(_DragInfo item); ...@@ -1071,7 +1074,7 @@ typedef _DragItemCallback = void Function(_DragInfo item);
class _DragInfo extends Drag { class _DragInfo extends Drag {
_DragInfo({ _DragInfo({
required this.item, required _ReorderableItemState item,
Offset initialPosition = Offset.zero, Offset initialPosition = Offset.zero,
this.scrollDirection = Axis.vertical, this.scrollDirection = Axis.vertical,
this.onUpdate, this.onUpdate,
...@@ -1082,6 +1085,10 @@ class _DragInfo extends Drag { ...@@ -1082,6 +1085,10 @@ class _DragInfo extends Drag {
required this.tickerProvider, required this.tickerProvider,
}) { }) {
final RenderBox itemRenderBox = item.context.findRenderObject()! as RenderBox; final RenderBox itemRenderBox = item.context.findRenderObject()! as RenderBox;
listState = item._listState;
index = item.index;
child = item.widget.child;
capturedThemes = item.widget.capturedThemes;
dragPosition = initialPosition; dragPosition = initialPosition;
dragOffset = itemRenderBox.globalToLocal(initialPosition); dragOffset = itemRenderBox.globalToLocal(initialPosition);
itemSize = item.context.size!; itemSize = item.context.size!;
...@@ -1089,7 +1096,6 @@ class _DragInfo extends Drag { ...@@ -1089,7 +1096,6 @@ class _DragInfo extends Drag {
scrollable = Scrollable.of(item.context); scrollable = Scrollable.of(item.context);
} }
final _ReorderableItemState item;
final Axis scrollDirection; final Axis scrollDirection;
final _DragItemUpdate? onUpdate; final _DragItemUpdate? onUpdate;
final _DragItemCallback? onEnd; final _DragItemCallback? onEnd;
...@@ -1098,10 +1104,14 @@ class _DragInfo extends Drag { ...@@ -1098,10 +1104,14 @@ class _DragInfo extends Drag {
final ReorderItemProxyDecorator? proxyDecorator; final ReorderItemProxyDecorator? proxyDecorator;
final TickerProvider tickerProvider; final TickerProvider tickerProvider;
late SliverReorderableListState listState;
late int index;
late Widget child;
late Offset dragPosition; late Offset dragPosition;
late Offset dragOffset; late Offset dragOffset;
late Size itemSize; late Size itemSize;
late double itemExtent; late double itemExtent;
late CapturedThemes capturedThemes;
ScrollableState? scrollable; ScrollableState? scrollable;
AnimationController? _proxyAnimation; AnimationController? _proxyAnimation;
...@@ -1149,14 +1159,16 @@ class _DragInfo extends Drag { ...@@ -1149,14 +1159,16 @@ class _DragInfo extends Drag {
} }
Widget createProxy(BuildContext context) { Widget createProxy(BuildContext context) {
return item.widget.capturedThemes.wrap( return capturedThemes.wrap(
_DragItemProxy( _DragItemProxy(
item: item, listState: listState,
index: index,
child: child,
size: itemSize, size: itemSize,
animation: _proxyAnimation!, animation: _proxyAnimation!,
position: dragPosition - dragOffset - _overlayOrigin(context), position: dragPosition - dragOffset - _overlayOrigin(context),
proxyDecorator: proxyDecorator, proxyDecorator: proxyDecorator,
) ),
); );
} }
} }
...@@ -1170,14 +1182,18 @@ Offset _overlayOrigin(BuildContext context) { ...@@ -1170,14 +1182,18 @@ Offset _overlayOrigin(BuildContext context) {
class _DragItemProxy extends StatelessWidget { class _DragItemProxy extends StatelessWidget {
const _DragItemProxy({ const _DragItemProxy({
Key? key, Key? key,
required this.item, required this.listState,
required this.index,
required this.child,
required this.position, required this.position,
required this.size, required this.size,
required this.animation, required this.animation,
required this.proxyDecorator, required this.proxyDecorator,
}) : super(key: key); }) : super(key: key);
final _ReorderableItemState item; final SliverReorderableListState listState;
final int index;
final Widget child;
final Offset position; final Offset position;
final Size size; final Size size;
final AnimationController animation; final AnimationController animation;
...@@ -1185,15 +1201,14 @@ class _DragItemProxy extends StatelessWidget { ...@@ -1185,15 +1201,14 @@ class _DragItemProxy extends StatelessWidget {
@override @override
Widget build(BuildContext context) { Widget build(BuildContext context) {
final Widget child = item.widget.child; final Widget proxyChild = proxyDecorator?.call(child, index, animation.view) ?? child;
final Widget proxyChild = proxyDecorator?.call(child, item.index, animation.view) ?? child;
final Offset overlayOrigin = _overlayOrigin(context); final Offset overlayOrigin = _overlayOrigin(context);
return AnimatedBuilder( return AnimatedBuilder(
animation: animation, animation: animation,
builder: (BuildContext context, Widget? child) { builder: (BuildContext context, Widget? child) {
Offset effectivePosition = position; Offset effectivePosition = position;
final Offset? dropPosition = item._listState._finalDropPosition; final Offset? dropPosition = listState._finalDropPosition;
if (dropPosition != null) { if (dropPosition != null) {
effectivePosition = Offset.lerp(dropPosition - overlayOrigin, effectivePosition, Curves.easeOut.transform(animation.value))!; effectivePosition = Offset.lerp(dropPosition - overlayOrigin, effectivePosition, Curves.easeOut.transform(animation.value))!;
} }
......
...@@ -1259,6 +1259,74 @@ void main() { ...@@ -1259,6 +1259,74 @@ void main() {
}); });
// TODO(djshuckerow): figure out how to write a test for scrolling the list. // TODO(djshuckerow): figure out how to write a test for scrolling the list.
}); });
testWidgets('ReorderableListView, can deal with the dragged item getting unmounted and rebuilt during drag', (WidgetTester tester) async {
// See https://github.com/flutter/flutter/issues/74840 for more details.
final List<int> items = List<int>.generate(100, (int index) => index);
void handleReorder(int fromIndex, int toIndex) {
if (toIndex > fromIndex) {
toIndex -= 1;
}
items.insert(toIndex, items.removeAt(fromIndex));
}
// The list is 800x600, 8 items, each item is 800x100 with
// an "item $index" text widget at the item's origin. Drags are initiated by
// a simple press on the text widget.
await tester.pumpWidget(MaterialApp(
home: ReorderableListView.builder(
itemBuilder: (BuildContext context, int index) {
return Container(
key: ValueKey<int>(items[index]),
height: 100,
child: ReorderableDragStartListener(
child: Row(
crossAxisAlignment: CrossAxisAlignment.start,
children: <Widget>[
Text('item ${items[index]}'),
],
),
index: index,
),
);
},
itemCount: items.length,
onReorder: handleReorder,
),
));
// Drag item 0 downwards and force an auto scroll off the end of the list
// far enough that item zeros original entry in the list is unmounted.
final TestGesture drag = await tester.startGesture(tester.getCenter(find.text('item 0')));
await tester.pump(kPressTimeout);
// Off the bottom of the screen, which should autoscroll until we hit the
// end of the list
await drag.moveBy(const Offset(0, 700));
await tester.pump(const Duration(seconds: 30));
await tester.pumpAndSettle();
// Ensure we made it to the bottom (only 4 should be showing as there should
// be a gap at the end for the drop area of the dragged item.
for (final int i in <int>[95, 96, 97, 98, 99]) {
expect(find.text('item $i'), findsOneWidget);
}
// Drag back to off the top of the list, which should autoscroll until
// we hit the beginning of the list. This should cause the first item's
// entry to be rebuilt. However, the contents should not be in both places.
await drag.moveBy(const Offset(0, -1400));
await tester.pump(const Duration(seconds: 30));
await tester.pumpAndSettle();
// Release back at the top so item 0 should drop where it was
await drag.up();
await tester.pumpAndSettle();
// Should not have changed anything
for (final int i in <int>[0, 1, 2, 3, 4, 5]) {
expect(find.text('item $i'), findsOneWidget);
}
expect(items.take(8), orderedEquals(<int>[0, 1, 2, 3, 4, 5, 6, 7]));
});
} }
Future<void> longPressDrag(WidgetTester tester, Offset start, Offset end) async { Future<void> longPressDrag(WidgetTester tester, Offset start, Offset end) async {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment