QAbstractProxyModel::sibling: treat row/column as offsets within the proxy

Qt5 allows QAIM subclasses to reimplement the sibling() method. Unfortunately,
the default QAbstractProxyModel's reimplementation differs in behavior to what
the Qt4 version was doing. In particular, the Qt4 version used to use the row
and column as positions within the proxy model, while the Qt5 version mistakenly
does this at the level of source model. This is arguably broken; the caller asks
for a sibling of the proxy index, not for a sibling within the proxy model.

This change makes the QAPM::sibling work explicitly in the same way as the Qt4
code behaved.

The reimplementation of QAbstractProxyModel::sibling was introduced in
9dfba89c28. It was subsequently fixed with commit
999109866d not to return indexes from the source
model, but the logic was still different from the Qt4 version.

[ChangeLog][QtCore][QAbstractProxyModel] Fixed QAbstractProxyModel::sibling to
work in the same manner as the Qt4 code used to behave. Previously, Qt5's
implementation would treat the row and column as positions in the source model
instead of a position in the proxy itself.

Followup-to 9dfba89c28 and
999109866d
Change-Id: Ia25027b2ad9e4777ba28de2d2226d48f8cccf587
Reviewed-by: Olivier Goffart <ogoffart@woboq.com>
Reviewed-by: Mark Brand <mabrand@mabrand.nl>
Reviewed-by: Stephen Kelly <stephen.kelly@kdab.com>

src/corelib/itemmodels/qabstractproxymodel.cpp

@@ -377,8 +377,7 @@ bool QAbstractProxyModel::hasChildren(const QModelIndex &parent) const
  */
 QModelIndex QAbstractProxyModel::sibling(int row, int column, const QModelIndex &idx) const
 {
-    Q_D(const QAbstractProxyModel);
-    return mapFromSource(d->model->sibling(row, column, mapToSource(idx)));
+    return index(row, column, idx.parent());
 }
 
 /*!

tests/auto/corelib/itemmodels/qabstractproxymodel/tst_qabstractproxymodel.cpp

@@ -71,6 +71,7 @@ private slots:
     void submit_data();
     void submit();
     void testRoleNames();
+    void testSwappingRowsProxy();
 };
 
 // Subclass that exposes the protected functions.
@@ -397,6 +398,103 @@ void tst_QAbstractProxyModel::testRoleNames()
     QVERIFY( proxy2RoleNames.value(StandardItemModelWithCustomRoleNames::CustomRole2) == "custom2" );
 }
 
+// This class only supports very simple table models
+class SwappingProxy : public QAbstractProxyModel
+{
+    static int swapRow(const int row)
+    {
+        if (row == 2) {
+            return 3;
+        } else if (row == 3) {
+            return 2;
+        } else {
+            return row;
+        }
+    }
+public:
+    virtual QModelIndex index(int row, int column, const QModelIndex &parentIdx) const
+    {
+        if (!sourceModel())
+            return QModelIndex();
+        if (row < 0 || column < 0)
+            return QModelIndex();
+        if (row >= sourceModel()->rowCount())
+            return QModelIndex();
+        if (column >= sourceModel()->columnCount())
+            return QModelIndex();
+        return createIndex(row, column, parentIdx.internalPointer());
+    }
+
+    virtual QModelIndex parent(const QModelIndex &parentIdx) const
+    {
+        // well, we're a 2D model
+        Q_UNUSED(parentIdx);
+        return QModelIndex();
+    }
+
+    virtual int rowCount(const QModelIndex &parentIdx) const
+    {
+        if (parentIdx.isValid() || !sourceModel())
+            return 0;
+        return sourceModel()->rowCount();
+    }
+
+    virtual int columnCount(const QModelIndex &parentIdx) const
+    {
+        if (parentIdx.isValid() || !sourceModel())
+            return 0;
+        return sourceModel()->rowCount();
+    }
+
+    virtual QModelIndex mapToSource(const QModelIndex &proxyIndex) const
+    {
+        if (!proxyIndex.isValid())
+            return QModelIndex();
+        if (!sourceModel())
+            return QModelIndex();
+        Q_ASSERT(!proxyIndex.parent().isValid());
+        return sourceModel()->index(swapRow(proxyIndex.row()), proxyIndex.column(), QModelIndex());
+    }
+
+    virtual QModelIndex mapFromSource(const QModelIndex &sourceIndex) const
+    {
+        if (!sourceIndex.isValid())
+            return QModelIndex();
+        if (!sourceModel())
+            return QModelIndex();
+        Q_ASSERT(!sourceIndex.parent().isValid());
+        return index(swapRow(sourceIndex.row()), sourceIndex.column(), QModelIndex());
+    }
+};
+
+void tst_QAbstractProxyModel::testSwappingRowsProxy()
+{
+    QStandardItemModel defaultModel;
+    defaultModel.setRowCount(4);
+    defaultModel.setColumnCount(2);
+    for (int row = 0; row < defaultModel.rowCount(); ++row) {
+        defaultModel.setItem(row, 0, new QStandardItem(QString::number(row) + QLatin1Char('A')));
+        defaultModel.setItem(row, 1, new QStandardItem(QString::number(row) + QLatin1Char('B')));
+    }
+    SwappingProxy proxy;
+    proxy.setSourceModel(&defaultModel);
+    QCOMPARE(proxy.data(proxy.index(0, 0, QModelIndex())), QVariant("0A"));
+    QCOMPARE(proxy.data(proxy.index(0, 1, QModelIndex())), QVariant("0B"));
+    QCOMPARE(proxy.data(proxy.index(1, 0, QModelIndex())), QVariant("1A"));
+    QCOMPARE(proxy.data(proxy.index(1, 1, QModelIndex())), QVariant("1B"));
+    QCOMPARE(proxy.data(proxy.index(2, 0, QModelIndex())), QVariant("3A"));
+    QCOMPARE(proxy.data(proxy.index(2, 1, QModelIndex())), QVariant("3B"));
+    QCOMPARE(proxy.data(proxy.index(3, 0, QModelIndex())), QVariant("2A"));
+    QCOMPARE(proxy.data(proxy.index(3, 1, QModelIndex())), QVariant("2B"));
+
+    for (int row = 0; row < defaultModel.rowCount(); ++row) {
+        QModelIndex left = proxy.index(row, 0, QModelIndex());
+        QModelIndex right = proxy.index(row, 1, QModelIndex());
+        QCOMPARE(left.sibling(left.row(), 1), right);
+        QCOMPARE(right.sibling(right.row(), 0), left);
+    }
+}
+
 QTEST_MAIN(tst_QAbstractProxyModel)
 #include "tst_qabstractproxymodel.moc"