QItemSelectionModel: More fixes for is(Column/Row)Selected

Replace the code for isRowSelected and isColumnSelected with
a much simpler algorithm for deciding if a row/column is selected.

In a model with a cross-hatch of unselectable indexes, the return values
of is(Column/Row)Selected would depend on the order in which the
selections were done.

Task-number: QTBUG-18001
Change-Id: I6aa4b1df7c07fae469a686041927fa8c42bc9b16
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@qt.io>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

2 files changed, 142 insertions, 27 deletions

diff --git a/src/corelib/itemmodels/qitemselectionmodel.cpp b/src/corelib/itemmodels/qitemselectionmodel.cpp
index ceec0e4c94..edb9bb9098 100644
--- a/src/corelib/itemmodels/qitemselectionmodel.cpp
+++ b/src/corelib/itemmodels/qitemselectionmodel.cpp
@@ -1502,30 +1502,40 @@ bool QItemSelectionModel::isRowSelected(int row, const QModelIndex &parent) cons
                         && d->currentSelection.at(i).intersected(d->ranges.at(j)).isValid())
                         return false;
     }
+
+    auto isSelectable = [&](int row, int column) {
+        Qt::ItemFlags flags = d->model->index(row, column, parent).flags();
+        return (flags & Qt::ItemIsSelectable);
+    };
+
+    const int colCount = d->model->columnCount(parent);
+    int unselectable = 0;
     // add ranges and currentSelection and check through them all
     QList<QItemSelectionRange>::const_iterator it;
     QList<QItemSelectionRange> joined = d->ranges;
     if (d->currentSelection.count())
         joined += d->currentSelection;
-    int colCount = d->model->columnCount(parent);
     for (int column = 0; column < colCount; ++column) {
+        if (!isSelectable(row, column)) {
+            ++unselectable;
+            continue;
+        }
+
         for (it = joined.constBegin(); it != joined.constEnd(); ++it) {
             if ((*it).contains(row, column, parent)) {
-                bool selectable = false;
-                for (int i = column; !selectable && i <= (*it).right(); ++i) {
-                    Qt::ItemFlags flags = d->model->index(row, i, parent).flags();
-                    selectable = flags & Qt::ItemIsSelectable;
-                }
-                if (selectable){
-                    column = qMax(column, (*it).right());
-                    break;
+                for (int i = column; i <= (*it).right(); ++i) {
+                    if (!isSelectable(row, i))
+                        ++unselectable;
                 }
+
+                column = qMax(column, (*it).right());
+                break;
             }
         }
         if (it == joined.constEnd())
             return false;
     }
-    return colCount > 0; // no columns means no selected items
+    return unselectable < colCount;
 }
 
 /*!
@@ -1568,30 +1578,39 @@ bool QItemSelectionModel::isColumnSelected(int column, const QModelIndex &parent
             }
         }
     }
+
+    auto isSelectable = [&](int row, int column) {
+        Qt::ItemFlags flags = d->model->index(row, column, parent).flags();
+        return (flags & Qt::ItemIsSelectable);
+    };
+    const int rowCount = d->model->rowCount(parent);
+    int unselectable = 0;
+
     // add ranges and currentSelection and check through them all
     QList<QItemSelectionRange>::const_iterator it;
     QList<QItemSelectionRange> joined = d->ranges;
     if (d->currentSelection.count())
         joined += d->currentSelection;
-    int rowCount = d->model->rowCount(parent);
     for (int row = 0; row < rowCount; ++row) {
-         for (it = joined.constBegin(); it != joined.constEnd(); ++it) {
-             if ((*it).contains(row, column, parent)) {
-                 bool selectable = false;
-                 for (int i = row; !selectable && i <= (*it).bottom(); ++i) {
-                     Qt::ItemFlags flags = d->model->index(i, column, parent).flags();
-                     selectable = flags & Qt::ItemIsSelectable;
-                 }
-                 if (selectable){
-                     row = qMax(row, (*it).bottom());
-                     break;
-                 }
-             }
-         }
-         if (it == joined.constEnd())
-             return false;
+        if (!isSelectable(row, column)) {
+            ++unselectable;
+            continue;
+        }
+        for (it = joined.constBegin(); it != joined.constEnd(); ++it) {
+            if ((*it).contains(row, column, parent)) {
+                for (int i = row; i <= (*it).bottom(); ++i) {
+                    if (!isSelectable(i, column)) {
+                        ++unselectable;
+                    }
+                }
+                row = qMax(row, (*it).bottom());
+                break;
+            }
+        }
+        if (it == joined.constEnd())
+            return false;
     }
-    return rowCount > 0; // no rows means no selected items
+    return unselectable < rowCount;
 }
 
 /*!
diff --git a/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp b/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp
index b05e3968ea..6fbaa28d69 100644
--- a/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp
+++ b/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp
@@ -95,6 +95,9 @@ private slots:
     void QTBUG58851_data();
     void QTBUG58851();
 
+    void QTBUG18001_data();
+    void QTBUG18001();
+
 private:
     QAbstractItemModel *model;
     QItemSelectionModel *selection;
@@ -2922,5 +2925,98 @@ void tst_QItemSelectionModel::QTBUG58851()
     }
 }
 
+void tst_QItemSelectionModel::QTBUG18001_data()
+{
+    using IntPair = std::pair<int, int>;
+    using IntPairList = QList<IntPair>;
+    using IntList = QList<int>;
+    using BoolList = QList<bool>;
+
+    QTest::addColumn<IntPairList>("indexesToSelect");
+    QTest::addColumn<IntList>("selectionCommands");
+    QTest::addColumn<BoolList>("expectedSelectedRows");
+    QTest::addColumn<BoolList>("expectedSelectedColums");
+
+    int colSelect = QItemSelectionModel::Select | QItemSelectionModel::Columns;
+    int rowSelect = QItemSelectionModel::Select | QItemSelectionModel::Rows;
+
+    QTest::newRow("Select column 1")
+          << IntPairList { {0, 1} }
+          << IntList{ colSelect }
+          << BoolList{ false, false, false, false, false }
+          << BoolList{ false, true, false, false, false };
+
+    QTest::newRow("Select row 1")
+          << IntPairList { {1, 0} }
+          << IntList{ rowSelect }
+          << BoolList{ false, true, false, false, false }
+          << BoolList{ false, false, false, false, false };
+
+    QTest::newRow("Select column 1+2, row 1+2")
+          << IntPairList { {0, 1}, {0, 2}, {1, 0}, {2, 0} }
+          << IntList{ colSelect, colSelect, rowSelect, rowSelect }
+          << BoolList{ false, true, true, false, false }
+          << BoolList{ false, true, true, false, false };
+
+    QTest::newRow("Select row 1+2, col 1+2")
+          << IntPairList { {1, 0}, {2, 0}, {0, 1}, {0, 2} }
+          << IntList{ rowSelect, rowSelect, colSelect, colSelect }
+          << BoolList{ false, true, true, false, false }
+          << BoolList{ false, true, true, false, false };
+}
+
+void tst_QItemSelectionModel::QTBUG18001()
+{
+    using IntPair = std::pair<int, int>;
+    using IntPairList = QList<IntPair>;
+    using IntList = QList<int>;
+    using BoolList = QList<bool>;
+
+    QFETCH(IntPairList, indexesToSelect);
+    QFETCH(IntList, selectionCommands);
+    QFETCH(BoolList, expectedSelectedRows);
+    QFETCH(BoolList, expectedSelectedColums);
+
+    QStandardItemModel model(5, 5);
+    for (int row = 0; row < model.rowCount(); ++row) {
+        for (int column = 0; column < model.columnCount(); ++column) {
+            QStandardItem *item = new QStandardItem(QString("%0x%1").arg(row).arg(column));
+            model.setItem(row, column, item);
+
+            const bool oddRow = row % 2;
+            const bool oddCol = column % 2;
+
+            if (oddRow == oddCol)
+               item->setSelectable(false);
+        }
+    }
+
+    QItemSelectionModel selectionModel(&model);
+
+    for (int i = 0; i < indexesToSelect.count(); ++i) {
+       QModelIndex idx = model.index( indexesToSelect.at(i).first, indexesToSelect.at(i).second );
+       selectionModel.select(idx, QItemSelectionModel::SelectionFlag(selectionCommands.at(i)));
+    }
+
+    for (int i = 0; i < expectedSelectedRows.count(); ++i) {
+       const bool expected = expectedSelectedRows.at(i);
+       const bool actual = selectionModel.isRowSelected(i, QModelIndex());
+       QByteArray description =  QByteArray("Row ") + QByteArray::number(i)
+             + " Expected " + QByteArray::number(expected)
+             + " Actual " + QByteArray::number(actual);
+       QVERIFY2(expected == actual, description.data());
+    }
+
+    for (int i = 0; i < expectedSelectedColums.count(); ++i) {
+       const bool expected = expectedSelectedColums.at(i);
+       const bool actual =  selectionModel.isColumnSelected(i, QModelIndex());
+       QByteArray description =  QByteArray("Col ") + QByteArray::number(i)
+             + " Expected " + QByteArray::number(expected)
+             + " Actual " + QByteArray::number(actual);
+       QVERIFY2(expected == actual, description.data());
+    }
+
+}
+
 QTEST_MAIN(tst_QItemSelectionModel)
 #include "tst_qitemselectionmodel.moc"