QListView: No-op, when a list item is dropped directly behind itself

QListView generates a move, when an item is dropped directly behind itself. This causes unexpected behavior, e.g. item widgets getting discarded. This patch prevents a move from being generated in that case. It adds an autotest to tst_QWidget, to verify item widgets and item data do not get discarded in case of a no-op drag&drop. Fixes: QTBUG-100128 Pick-to: 6.5 6.2 Change-Id: I02a755320a7b71dad218293c9c94c88da6507423 Reviewed-by: Jan Arve Sæther <[email protected]> Reviewed-by: Qt CI Bot <[email protected]>
leadtobetter · Mar 31, 2023 · a815c40 · a815c40
1 parent a94ba94
commit a815c40
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 1 deletion.
diff --git a/src/widgets/itemviews/qlistview.cpp b/src/widgets/itemviews/qlistview.cpp
@@ -910,7 +910,8 @@ void QListView::dropEvent(QDropEvent *event)
                 bool dataMoved = false;
                 for (int i = 0; i < persIndexes.size(); ++i) {
                     const QPersistentModelIndex &pIndex = persIndexes.at(i);
-                    if (r != pIndex.row()) {
+                    // only generate a move when not same row or behind itself
+                    if (r != pIndex.row() && r != pIndex.row() + 1) {
                         // try to move (preserves selection)
                         dataMoved |= model()->moveRow(QModelIndex(), pIndex.row(), QModelIndex(), r);
                         if (!dataMoved) // can't move - abort and let QAbstractItemView handle this

diff --git a/tests/auto/widgets/itemviews/qlistwidget/tst_qlistwidget.cpp b/tests/auto/widgets/itemviews/qlistwidget/tst_qlistwidget.cpp
@@ -8,6 +8,7 @@
 #include <QSignalSpy>
 #include <QStyledItemDelegate>
 #include <QTest>
+#include <QLabel>
 #include <private/qlistwidget_p.h>
 
 #include <QtWidgets/private/qapplication_p.h>
@@ -105,6 +106,7 @@ private slots:
     void moveRows();
     void moveRowsInvalid_data();
     void moveRowsInvalid();
+    void noopDragDrop();
 
 protected slots:
     void rowsAboutToBeInserted(const QModelIndex &parent, int first, int last)
@@ -1889,6 +1891,58 @@ void tst_QListWidget::createPersistentOnLayoutAboutToBeChangedAutoSort() // QTBU
     QCOMPARE(layoutChangedSpy.size(), 1);
 }
 
+// Test that dropping an item on or beneath itself remains a no-op
+void tst_QListWidget::noopDragDrop() // QTBUG-100128
+{
+    QListWidget listWidget;
+    QList<QListWidgetItem *> items;
+    for (int i = 0; i < 5; ++i) {
+        const QString number = QString::number(i);
+        QListWidgetItem *item = new QListWidgetItem(&listWidget);
+        item->setData(Qt::UserRole, number);
+        QLabel *label = new QLabel(number);
+        listWidget.setItemWidget(item, label);
+        items.append(item);
+    }
+
+    listWidget.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&listWidget));
+
+    const QRect &lastItemRect = listWidget.visualItemRect(items.at(4));
+    const QPoint &dragStart = lastItemRect.center();
+    const QPoint &dropPointNirvana = lastItemRect.center() + QPoint(20, 2 * lastItemRect.height());
+
+    // Implement check as a macro (not a method) to safely determine the error location.
+    // The macro checks that item data and item widget remain unchanged when drag&drop are executed.
+    // In order to verify that the assets do *not* change, we can't use QTRY*: These macros would
+    // spin the event loop only once, while 3/4 mouse events need to get processed.
+    // That's why we spin the event loop 13 times, to make sure other unexpected or pending events
+    // get processed.
+#define CHECK_ITEM {\
+        const QString number = QString::number(4);\
+        for (int i = 0; i < 13; ++i)\
+            QApplication::processEvents();\
+        QLabel *label = qobject_cast<QLabel *>(listWidget.itemWidget(items.at(4)));\
+        QVERIFY(label);\
+        QCOMPARE(label->text(), number);\
+        const QString &data = items.at(4)->data(Qt::UserRole).toString();\
+        QCOMPARE(data, number);\
+    }
+
+    // Test dropping last item beneath itself
+    QTest::mousePress(&listWidget, Qt::LeftButton, Qt::KeyboardModifiers(), dragStart);
+    QTest::mouseMove(&listWidget, dropPointNirvana);
+    QTest::mouseRelease(&listWidget, Qt::LeftButton);
+    CHECK_ITEM;
+
+    // Test dropping last item on itself
+    QTest::mousePress(&listWidget, Qt::LeftButton, Qt::KeyboardModifiers(), dragStart);
+    QTest::mouseMove(&listWidget, dropPointNirvana);
+    QTest::mouseMove(&listWidget, dragStart);
+    QTest::mouseRelease(&listWidget, Qt::LeftButton);
+    CHECK_ITEM;
+}
+
 #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
 void tst_QListWidget::clearItemData()
 {