QAbstractItemView: don't lose items if model only allows MoveAction

If a model only allows MoveAction, then calls in the view/widget subclasses'
dropEvent implementation to set the event's drop action to CopyAction
will fail. QAbstractItemView will then remove the item when QDrag::exec
returns.

Instead of abusing the event actions for this, store explicitly that the
dropEvent implementation already moved the item. If the flag is set,
don't remove the item.

In QListView, which uses moveRow to move items in the dropEvent handler,
handle the case that the model might not implement moveRows. In that
case, or when dropping an item onto another item (to overwrite data),
fall back to the default implementation of QAbstractItemView. Sadly, it
is impossible to know whether a model doesn't implement moveRows, or
whether the move failed for other reasons, so this requires a bit of
extra special case handling. QListView in IconMode is particularly odd
in that it moves the item in the view, but not in the model.

This follows up on fd894fd68e and fixes
additional issues discovered during debugging. Extend the existing unit
test; since drag'n'drop runs a modal, native event loop on most systems,
it still only runs on the Xcb platform.

Change-Id: I6c5377e2b097c8080001afe904d6d3e4aed33df4
Pick-to: 5.15
Fixes: QTBUG-87057
Reviewed-by: Jan Arve Sæther <jan-arve.saether@qt.io>

src/widgets/itemviews/qabstractitemview.cpp

@@ -100,6 +100,7 @@ QAbstractItemViewPrivate::QAbstractItemViewPrivate()
         dragEnabled(false),
         dragDropMode(QAbstractItemView::NoDragDrop),
         overwrite(false),
+        dropEventMoved(false),
         dropIndicatorPosition(QAbstractItemView::OnItem),
         defaultDropAction(Qt::IgnoreAction),
 #endif
@@ -3699,8 +3700,10 @@ void QAbstractItemView::startDrag(Qt::DropActions supportedActions)
             defaultDropAction = d->defaultDropAction;
         else if (supportedActions & Qt::CopyAction && dragDropMode() != QAbstractItemView::InternalMove)
             defaultDropAction = Qt::CopyAction;
-        if (drag->exec(supportedActions, defaultDropAction) == Qt::MoveAction)
+        d->dropEventMoved = false;
+        if (drag->exec(supportedActions, defaultDropAction) == Qt::MoveAction && !d->dropEventMoved)
             d->clearOrRemove();
+        d->dropEventMoved = false;
         // Reset the drop indicator
         d->dropIndicatorRect = QRect();
         d->dropIndicatorPosition = OnItem;

src/widgets/itemviews/qabstractitemview_p.h

@@ -392,6 +392,7 @@ public:
     bool dragEnabled;
     QAbstractItemView::DragDropMode dragDropMode;
     bool overwrite;
+    bool dropEventMoved;
     QAbstractItemView::DropIndicatorPosition dropIndicatorPosition;
     Qt::DropAction defaultDropAction;
 #endif

src/widgets/itemviews/qlistview.cpp

@@ -934,7 +934,7 @@ void QListView::dropEvent(QDropEvent *event)
                 }
             }
 
-            if (!topIndexDropped) {
+            if (!topIndexDropped && !topIndex.isValid()) {
                 std::sort(persIndexes.begin(), persIndexes.end()); // The dropped items will remain in the same visual order.
 
                 QPersistentModelIndex dropRow = model()->index(row, col, topIndex);
@@ -942,19 +942,29 @@ void QListView::dropEvent(QDropEvent *event)
                 int r = row == -1 ? model()->rowCount() : (dropRow.row() >= 0 ? dropRow.row() : row);
                 for (int i = 0; i < persIndexes.count(); ++i) {
                     const QPersistentModelIndex &pIndex = persIndexes.at(i);
-                    model()->moveRow(QModelIndex(), pIndex.row(), QModelIndex(), r);
+                    if (r != pIndex.row()) {
+                        // try to move (preserves selection)
+                        d->dropEventMoved |= model()->moveRow(QModelIndex(), pIndex.row(), QModelIndex(), r);
+                        if (!d->dropEventMoved) // can't move - abort and let QAbstractItemView handle this
+                            break;
+                    } else {
+                        // move onto itself is blocked, don't delete anything
+                        d->dropEventMoved = true;
+                    }
                     r = pIndex.row() + 1;   // Dropped items are inserted contiguously and in the right order.
                 }
-
-                event->accept();
-                // Don't want QAbstractItemView to delete it because it was "moved" we already did it
-                event->setDropAction(Qt::CopyAction);
+                if (d->dropEventMoved)
+                    event->accept(); // data moved, nothing to be done in QAbstractItemView::dropEvent
             }
         }
     }
 
-    if (!d->commonListView->filterDropEvent(event))
+    if (!d->commonListView->filterDropEvent(event) || !d->dropEventMoved) {
+        // icon view didn't move the data, and moveRows not implemented, so fall back to default
+        if (!d->dropEventMoved)
+            event->ignore();
         QAbstractItemView::dropEvent(event);
+    }
 }
 
 /*!
@@ -2887,12 +2897,13 @@ bool QIconModeViewBase::filterStartDrag(Qt::DropActions supportedActions)
         drag->setMimeData(dd->model->mimeData(indexes));
         drag->setPixmap(pixmap);
         drag->setHotSpot(dd->pressedPosition - rect.topLeft());
+        dd->dropEventMoved = false;
         Qt::DropAction action = drag->exec(supportedActions, dd->defaultDropAction);
         draggedItems.clear();
-        // for internal moves the action was set to Qt::CopyAction in
-        // filterDropEvent() to avoid the deletion here
-        if (action == Qt::MoveAction)
+        // delete item, unless it has already been moved internally (see filterDropEvent)
+        if (action == Qt::MoveAction && !dd->dropEventMoved)
             dd->clearOrRemove();
+        dd->dropEventMoved = false;
     }
     return true;
 }
@@ -2927,8 +2938,6 @@ bool QIconModeViewBase::filterDropEvent(QDropEvent *e)
     dd->stopAutoScroll();
     draggedItems.clear();
     dd->emitIndexesMoved(indexes);
-    // do not delete item on internal move, see filterStartDrag()
-    e->setDropAction(Qt::CopyAction);
     e->accept(); // we have handled the event
     // if the size has not grown, we need to check if it has shrinked
     if (contentsSize != contents) {

src/widgets/itemviews/qtablewidget.cpp

@@ -2702,7 +2702,7 @@ void QTableWidget::dropEvent(QDropEvent *event) {
 
             event->accept();
             // Don't want QAbstractItemView to delete it because it was "moved" we already did it
-            event->setDropAction(Qt::CopyAction);
+            d->dropEventMoved = true;
         }
     }
 

src/widgets/itemviews/qtreewidget.cpp

@@ -3392,7 +3392,7 @@ void QTreeWidget::dropEvent(QDropEvent *event) {
 
             event->accept();
             // Don't want QAbstractItemView to delete it because it was "moved" we already did it
-            event->setDropAction(Qt::CopyAction);
+            d->dropEventMoved = true;
         }
     }
 

tests/auto/widgets/itemviews/qlistview/tst_qlistview.cpp

@@ -167,6 +167,7 @@ private slots:
     void taskQTBUG_51086_skippingIndexesInSelectedIndexes();
     void taskQTBUG_47694_indexOutOfBoundBatchLayout();
     void itemAlignment();
+    void internalDragDropMove_data();
     void internalDragDropMove();
 };
 
@@ -2526,46 +2527,185 @@ void tst_QListView::itemAlignment()
     QVERIFY(w.visualRect(item1->index()).width() < w.visualRect(item2->index()).width());
 }
 
+void tst_QListView::internalDragDropMove_data()
+{
+    QTest::addColumn<QListView::ViewMode>("viewMode");
+    QTest::addColumn<QAbstractItemView::DragDropMode>("dragDropMode");
+    QTest::addColumn<Qt::DropActions>("supportedDropActions");
+    QTest::addColumn<Qt::DropAction>("defaultDropAction");
+    QTest::addColumn<Qt::ItemFlags>("itemFlags");
+    QTest::addColumn<bool>("modelMoves");
+    QTest::addColumn<QStringList>("expectedData");
+
+    const Qt::ItemFlags defaultFlags = Qt::ItemIsSelectable
+                                     | Qt::ItemIsEnabled
+                                     | Qt::ItemIsEditable
+                                     | Qt::ItemIsDragEnabled;
+
+    const QStringList reordered = QStringList{"0", "2", "3", "4", "5", "6", "7", "8", "9", "1"};
+    const QStringList replaced = QStringList{"0", "2", "3", "4", "1", "6", "7", "8", "9"};
+
+    for (auto viewMode : { QListView::IconMode, QListView::ListMode }) {
+        for (auto modelMoves : { true, false } ) {
+            QByteArray rowName = viewMode == QListView::IconMode ? "icon" : "list" ;
+            rowName += modelMoves ? ", model moves" : ", model doesn't move";
+            QTest::newRow((rowName + ", copy&move").constData())
+                << viewMode
+                << QAbstractItemView::InternalMove
+                << (Qt::CopyAction|Qt::MoveAction)
+                << Qt::MoveAction
+                << defaultFlags
+                << modelMoves
+                << reordered;
+
+            QTest::newRow((rowName + ", only move").constData())
+                << viewMode
+                << QAbstractItemView::InternalMove
+                << (Qt::IgnoreAction|Qt::MoveAction)
+                << Qt::MoveAction
+                << defaultFlags
+                << modelMoves
+                << reordered;
+
+            QTest::newRow((rowName + ", replace item").constData())
+                << viewMode
+                << QAbstractItemView::InternalMove
+                << (Qt::IgnoreAction|Qt::MoveAction)
+                << Qt::MoveAction
+                << (defaultFlags | Qt::ItemIsDropEnabled)
+                << modelMoves
+                << replaced;
+        }
+    }
+}
+
+/*
+    Test moving of items items via drag'n'drop.
+
+    This should reorder items when an item is dropped in between two items,
+    or - if items can be dropped on - replace the content of the drop target.
+
+    Test QListView in both icon and list view modes.
+
+    See QTBUG-67440, QTBUG-83084, QTBUG-87057
+*/
 void tst_QListView::internalDragDropMove()
 {
     const QString platform(QGuiApplication::platformName().toLower());
     if (platform != QLatin1String("xcb"))
         QSKIP("Need a window system with proper DnD support via injected mouse events");
 
-    // on an internal move, the item was deleted which should not happen
-    // see QTBUG-67440
+    QFETCH(QListView::ViewMode, viewMode);
+    QFETCH(QAbstractItemView::DragDropMode, dragDropMode);
+    QFETCH(Qt::DropActions, supportedDropActions);
+    QFETCH(Qt::DropAction, defaultDropAction);
+    QFETCH(Qt::ItemFlags, itemFlags);
+    QFETCH(bool, modelMoves);
+    QFETCH(QStringList, expectedData);
+
+    class ItemModel : public QStringListModel
+    {
+    public:
+        ItemModel()
+        {
+            QStringList list;
+            for (int i = 0; i < 10; ++i) {
+                list << QString::number(i);
+            }
+            setStringList(list);
+        }
+
+        Qt::DropActions supportedDropActions() const override { return m_supportedDropActions; }
+        Qt::ItemFlags flags(const QModelIndex &index) const override
+        {
+            if (!index.isValid())
+                return QStringListModel::flags(index);
+            return m_itemFlags;
+        }
+        bool moveRows(const QModelIndex &sourceParent, int sourceRow, int count,
+                      const QModelIndex &destinationParent, int destinationChild) override
+        {
+            if (!m_modelMoves) // many models don't implement moveRows
+                return false;
+            return QStringListModel::moveRows(sourceParent, sourceRow, count,
+                                              destinationParent, destinationChild);
+        }
+        bool setItemData(const QModelIndex &index, const QMap<int, QVariant> &values) override
+        {
+            return QStringListModel::setData(index, values.value(Qt::DisplayRole), Qt::DisplayRole);
+        }
+        QVariant data(const QModelIndex &index, int role) const override
+        {
+            if (role == Qt::DecorationRole)
+                return QColor(Qt::GlobalColor(index.row() + 1));
+            return QStringListModel::data(index, role);
+        }
+        QMap<int, QVariant> itemData(const QModelIndex &index) const override
+        {
+            auto item = QStringListModel::itemData(index);
+            item[Qt::DecorationRole] = data(index, Qt::DecorationRole);
+            return item;
+        }
+
+        Qt::DropActions m_supportedDropActions;
+        Qt::ItemFlags m_itemFlags;
+        bool m_modelMoves;
+    };
+
+    ItemModel data;
+    data.m_supportedDropActions = supportedDropActions;
+    data.m_itemFlags = itemFlags;
+    data.m_modelMoves = modelMoves;
 
-    QStandardItemModel data(0, 1);
-    QPixmap pixmap(32, 32);
-    for (int i = 0; i < 10; ++i) {
-        pixmap.fill(Qt::GlobalColor(i + 1));
-        data.appendRow(new QStandardItem(QIcon(pixmap), QString::number(i)));
-    }
     QItemSelectionModel selections(&data);
     PublicListView list;
     list.setWindowTitle(QTest::currentTestFunction());
-    list.setViewMode(QListView::IconMode);
-    list.setDefaultDropAction(Qt::MoveAction);
+    list.setViewMode(viewMode);
+    list.setDragDropMode(dragDropMode);
+    list.setDefaultDropAction(defaultDropAction);
     list.setModel(&data);
     list.setSelectionModel(&selections);
-    list.resize(300, 300);
+    int itemHeight = list.sizeHintForIndex(data.index(1, 0)).height();
+    list.resize(300, 15 * itemHeight);
     list.show();
     selections.select(data.index(1, 0), QItemSelectionModel::Select);
+    auto getSelectedTexts = [&]() -> QStringList {
+        QStringList selectedTexts;
+        for (auto index : selections.selectedIndexes())
+            selectedTexts << data.itemData(index).value(Qt::DisplayRole).toString();
+        return selectedTexts;
+    };
     QVERIFY(QTest::qWaitForWindowExposed(&list));
-
     // execute as soon as the eventloop is running again
     // which is the case inside list.startDrag()
-    QTimer::singleShot(0, [&list]()
+    QTimer::singleShot(0, [&]()
     {
-        const QPoint pos = list.rect().center();
-        QMouseEvent mouseMove(QEvent::MouseMove, pos, list.mapToGlobal(pos), Qt::NoButton, {}, {});
+        QPoint droppos;
+        // take into account subtle differences between icon and list mode in QListView's drop placement
+        if (itemFlags & Qt::ItemIsDropEnabled)
+            droppos = list.rectForIndex(data.index(5, 0)).center();
+        else if (viewMode == QListView::IconMode)
+            droppos = list.rectForIndex(data.index(9, 0)).bottomRight() + QPoint(30, 30);
+        else
+            droppos = list.rectForIndex(data.index(9, 0)).bottomRight();
+
+        QMouseEvent mouseMove(QEvent::MouseMove, droppos, list.mapToGlobal(droppos), Qt::NoButton, {}, {});
         QCoreApplication::sendEvent(&list, &mouseMove);
-        QMouseEvent mouseRelease(QEvent::MouseButtonRelease, pos, list.mapToGlobal(pos), Qt::LeftButton, {}, {});
+        QMouseEvent mouseRelease(QEvent::MouseButtonRelease, droppos, list.mapToGlobal(droppos), Qt::LeftButton, {}, {});
         QCoreApplication::sendEvent(&list, &mouseRelease);
     });
-    const int expectedCount = data.rowCount();
-    list.startDrag(Qt::MoveAction|Qt::CopyAction);
-    QCOMPARE(expectedCount, data.rowCount());
+
+    const QStringList expectedSelected = getSelectedTexts();
+
+    list.startDrag(Qt::MoveAction);
+
+    QCOMPARE(data.stringList(), expectedData);
+
+     // if the model doesn't implement moveRows, or if items are replaced, then selection is lost
+    if (modelMoves && !(itemFlags & Qt::ItemIsDropEnabled)) {
+        const QStringList actualSelected = getSelectedTexts();
+        QCOMPARE(actualSelected, expectedSelected);
+    }
 }