QAbstractProxyModel: fix canDropMimeData/dropMimeData implementations

The code in 4696e9dbaa4 was incorrect. It is perfectly valid to call
these methods with row=-1 column=1 parent=some_index, this is exactly
what happens in QListView and QTableView. Child row/column is only for
trees.

Move the coordinate mapping from QSortFilterProxyModel into a new
mapDropCoordinatesToSource internal method, used by QAbstractProxyModel.

Task-number: QTBUG-39549
Change-Id: I3312210473d84b639cbe4c01f70ea36437db3e91
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@theqtcompany.com>
Reviewed-by: Stephen Kelly <steveire@gmail.com>

src/corelib/itemmodels/qabstractproxymodel.cpp

@@ -384,6 +384,26 @@ QMimeData* QAbstractProxyModel::mimeData(const QModelIndexList &indexes) const
     return d->model->mimeData(list);
 }
 
+void QAbstractProxyModelPrivate::mapDropCoordinatesToSource(int row, int column, const QModelIndex &parent,
+                                                            int *sourceRow, int *sourceColumn, QModelIndex *sourceParent) const
+{
+    Q_Q(const QAbstractProxyModel);
+    *sourceRow = -1;
+    *sourceColumn = -1;
+    if (row == -1 && column == -1) {
+        *sourceParent = q->mapToSource(parent);
+    } else if (row == q->rowCount(parent)) {
+        *sourceParent = q->mapToSource(parent);
+        *sourceRow = model->rowCount(*sourceParent);
+    } else {
+        QModelIndex proxyIndex = q->index(row, column, parent);
+        QModelIndex sourceIndex = q->mapToSource(proxyIndex);
+        *sourceRow = sourceIndex.row();
+        *sourceColumn = sourceIndex.column();
+        *sourceParent = sourceIndex.parent();
+    }
+}
+
 /*!
     \reimp
     \since 5.4
@@ -392,8 +412,11 @@ bool QAbstractProxyModel::canDropMimeData(const QMimeData *data, Qt::DropAction
                                           int row, int column, const QModelIndex &parent) const
 {
     Q_D(const QAbstractProxyModel);
-    const QModelIndex source = mapToSource(index(row, column, parent));
+    int sourceDestinationRow;
-    return d->model->canDropMimeData(data, action, source.row(), source.column(), source.parent());
+    int sourceDestinationColumn;
+    QModelIndex sourceParent;
+    d->mapDropCoordinatesToSource(row, column, parent, &sourceDestinationRow, &sourceDestinationColumn, &sourceParent);
+    return d->model->canDropMimeData(data, action, sourceDestinationRow, sourceDestinationColumn, sourceParent);
 }
 
 /*!
@@ -404,8 +427,11 @@ bool QAbstractProxyModel::dropMimeData(const QMimeData *data, Qt::DropAction act
                                        int row, int column, const QModelIndex &parent)
 {
     Q_D(QAbstractProxyModel);
-    const QModelIndex source = mapToSource(index(row, column, parent));
+    int sourceDestinationRow;
-    return d->model->dropMimeData(data, action, source.row(), source.column(), source.parent());
+    int sourceDestinationColumn;
+    QModelIndex sourceParent;
+    d->mapDropCoordinatesToSource(row, column, parent, &sourceDestinationRow, &sourceDestinationColumn, &sourceParent);
+    return d->model->dropMimeData(data, action, sourceDestinationRow, sourceDestinationColumn, sourceParent);
 }
 
 /*!

src/corelib/itemmodels/qabstractproxymodel_p.h

@@ -59,6 +59,8 @@ public:
     QAbstractProxyModelPrivate() : QAbstractItemModelPrivate(), model(0) {}
     QAbstractItemModel *model;
     virtual void _q_sourceModelDestroyed();
+    void mapDropCoordinatesToSource(int row, int column, const QModelIndex &parent,
+                                    int *source_row, int *source_column, QModelIndex *source_parent) const;
 };
 
 QT_END_NAMESPACE

src/corelib/itemmodels/qsortfilterproxymodel.cpp

@@ -2030,30 +2030,14 @@ Qt::DropActions QSortFilterProxyModel::supportedDropActions() const
     return d->model->supportedDropActions();
 }
 
+// Qt6: remove unnecessary reimplementation
 /*!
   \reimp
 */
 bool QSortFilterProxyModel::dropMimeData(const QMimeData *data, Qt::DropAction action,
                                          int row, int column, const QModelIndex &parent)
 {
-    Q_D(QSortFilterProxyModel);
+    return QAbstractProxyModel::dropMimeData(data, action, row, column, parent);
-    if ((row == -1) && (column == -1))
-        return d->model->dropMimeData(data, action, -1, -1, mapToSource(parent));
-    int source_destination_row = -1;
-    int source_destination_column = -1;
-    QModelIndex source_parent;
-    if (row == rowCount(parent)) {
-        source_parent = mapToSource(parent);
-        source_destination_row = d->model->rowCount(source_parent);
-    } else {
-        QModelIndex proxy_index = index(row, column, parent);
-        QModelIndex source_index = mapToSource(proxy_index);
-        source_destination_row = source_index.row();
-        source_destination_column = source_index.column();
-        source_parent = source_index.parent();
-    }
-    return d->model->dropMimeData(data, action, source_destination_row,
-                                  source_destination_column, source_parent);
 }
 
 /*!

tests/auto/corelib/itemmodels/qsortfilterproxymodel/tst_qsortfilterproxymodel.cpp

@@ -144,6 +144,7 @@ private slots:
 
     void noMapAfterSourceDelete();
     void forwardDropApi();
+    void canDropMimeData();
 
 protected:
     void buildHierarchy(const QStringList &data, QAbstractItemModel *model);
@@ -3909,6 +3910,36 @@ void tst_QSortFilterProxyModel::chainedProxyModelRoleNames()
     QVERIFY(proxy2.roleNames().value(Qt::UserRole + 1) == "custom");
 }
 
+// A source model with ABABAB rows, where only A rows accept drops.
+// It will then be sorted by a QSFPM.
+class DropOnOddRows : public QAbstractListModel
+{
+    Q_OBJECT
+public:
+    DropOnOddRows(QObject *parent = 0) : QAbstractListModel(parent) {}
+
+    QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const
+    {
+        if (role == Qt::DisplayRole)
+            return (index.row() % 2 == 0) ? "A" : "B";
+        return QVariant();
+    }
+
+    int rowCount(const QModelIndex &parent = QModelIndex()) const
+    {
+        Q_UNUSED(parent);
+        return 10;
+    }
+
+    bool canDropMimeData(const QMimeData *, Qt::DropAction,
+                         int row, int column, const QModelIndex &parent) const Q_DECL_OVERRIDE
+    {
+        Q_UNUSED(row);
+        Q_UNUSED(column);
+        return parent.row() % 2 == 0;
+    }
+};
+
 class SourceAssertion : public QSortFilterProxyModel
 {
     Q_OBJECT
@@ -3973,5 +4004,30 @@ void tst_QSortFilterProxyModel::forwardDropApi()
     QVERIFY(model.dropMimeData(0, Qt::CopyAction, 0, 0, QModelIndex()));
 }
 
+static QString rowTexts(QAbstractItemModel *model) {
+    QString str;
+    for (int row = 0 ; row < model->rowCount(); ++row)
+        str += model->index(row, 0).data().toString();
+    return str;
+}
+
+void tst_QSortFilterProxyModel::canDropMimeData()
+{
+    // Given a source model which only supports dropping on even rows
+    DropOnOddRows sourceModel;
+    QCOMPARE(rowTexts(&sourceModel), QString("ABABABABAB"));
+
+    // and a proxy model that sorts the rows
+    QSortFilterProxyModel proxy;
+    proxy.setSourceModel(&sourceModel);
+    proxy.sort(0, Qt::AscendingOrder);
+    QCOMPARE(rowTexts(&proxy), QString("AAAAABBBBB"));
+
+    // the proxy should correctly map canDropMimeData to the source model,
+    // i.e. accept drops on the first 5 rows and refuse drops on the next 5.
+    for (int row = 0; row < proxy.rowCount(); ++row)
+        QCOMPARE(proxy.canDropMimeData(0, Qt::CopyAction, -1, -1, proxy.index(row, 0)), row < 5);
+}
+
 QTEST_MAIN(tst_QSortFilterProxyModel)
 #include "tst_qsortfilterproxymodel.moc"