Fix QItemSelectionModel::selectionChanged emission

QItemSelectionModel has the property selectedIndexes with
the notification signal selectionChanged. When a row is deleted
or inserted above the current selection, the row number of the
current selection changes and thus the return value of
selectedIndexes changes. This should trigger its notification
signal.
This signal was not emitted. This patch fixes this and
adds a unit test to verify this.

[ChangeLog][Important Behavior Changes][QtCore]
QItemSelectionModel now emits the selectionChanged signal
if only the indexes of the selected items change.

Fixes: QTBUG-93305
Change-Id: Ia5fb5ca32d658c9c0e1d7093c57cc08a966b9402
Reviewed-by: Ivan Solovev <ivan.solovev@qt.io>
Reviewed-by: David Faure <david.faure@kdab.com>

2 files changed, 75 insertions, 8 deletions

diff --git a/src/corelib/itemmodels/qitemselectionmodel.cpp b/src/corelib/itemmodels/qitemselectionmodel.cpp
index bccf02bb0b..1b8b8f57ff 100644
--- a/src/corelib/itemmodels/qitemselectionmodel.cpp
+++ b/src/corelib/itemmodels/qitemselectionmodel.cpp
@@ -675,6 +675,7 @@ void QItemSelectionModelPrivate::_q_rowsAboutToBeRemoved(const QModelIndex &pare
                                                          int start, int end)
 {
     Q_Q(QItemSelectionModel);
+    Q_ASSERT(start <= end);
     finalize();
 
     // update current index
@@ -699,6 +700,7 @@ void QItemSelectionModelPrivate::_q_rowsAboutToBeRemoved(const QModelIndex &pare
 
     QItemSelection deselected;
     QItemSelection newParts;
+    bool indexesOfSelectionChanged = false;
     QItemSelection::iterator it = ranges.begin();
     while (it != ranges.end()) {
         if (it->topLeft().parent() != parent) {  // Check parents until reaching root or contained in range
@@ -710,6 +712,8 @@ void QItemSelectionModelPrivate::_q_rowsAboutToBeRemoved(const QModelIndex &pare
                 deselected.append(*it);
                 it = ranges.erase(it);
             } else {
+                if (itParent.isValid() && end < itParent.row())
+                    indexesOfSelectionChanged = true;
                 ++it;
             }
         } else if (start <= it->bottom() && it->bottom() <= end    // Full inclusion
@@ -734,12 +738,16 @@ void QItemSelectionModelPrivate::_q_rowsAboutToBeRemoved(const QModelIndex &pare
             deselected.append(removedRange);
             QItemSelection::split(*it, removedRange, &newParts);
             it = ranges.erase(it);
-        } else
+        } else if (end < it->top()) { // deleted row before selection
+            indexesOfSelectionChanged = true;
             ++it;
+        } else {
+            ++it;
+        }
     }
     ranges.append(newParts);
 
-    if (!deselected.isEmpty())
+    if (!deselected.isEmpty() || indexesOfSelectionChanged)
         emit q->selectionChanged(QItemSelection(), deselected);
 }
 
@@ -791,11 +799,12 @@ void QItemSelectionModelPrivate::_q_columnsAboutToBeInserted(const QModelIndex &
     QList<QItemSelectionRange> split;
     QList<QItemSelectionRange>::iterator it = ranges.begin();
     for (; it != ranges.end(); ) {
-        if ((*it).isValid() && (*it).parent() == parent
+        const QModelIndex &itParent = it->parent();
+        if ((*it).isValid() && itParent == parent
             && (*it).left() < start && (*it).right() >= start) {
-            QModelIndex bottomMiddle = model->index((*it).bottom(), start - 1, (*it).parent());
+            QModelIndex bottomMiddle = model->index((*it).bottom(), start - 1, itParent);
             QItemSelectionRange left((*it).topLeft(), bottomMiddle);
-            QModelIndex topMiddle = model->index((*it).top(), start, (*it).parent());
+            QModelIndex topMiddle = model->index((*it).top(), start, itParent);
             QItemSelectionRange right(topMiddle, (*it).bottomRight());
             it = ranges.erase(it);
             split.append(left);
@@ -815,25 +824,35 @@ void QItemSelectionModelPrivate::_q_columnsAboutToBeInserted(const QModelIndex &
 void QItemSelectionModelPrivate::_q_rowsAboutToBeInserted(const QModelIndex &parent,
                                                           int start, int end)
 {
+    Q_Q(QItemSelectionModel);
     Q_UNUSED(end);
     finalize();
     QList<QItemSelectionRange> split;
     QList<QItemSelectionRange>::iterator it = ranges.begin();
+    bool indexesOfSelectionChanged = false;
     for (; it != ranges.end(); ) {
-        if ((*it).isValid() && (*it).parent() == parent
+        const QModelIndex &itParent = it->parent();
+        if ((*it).isValid() && itParent == parent
             && (*it).top() < start && (*it).bottom() >= start) {
-            QModelIndex middleRight = model->index(start - 1, (*it).right(), (*it).parent());
+            QModelIndex middleRight = model->index(start - 1, (*it).right(), itParent);
             QItemSelectionRange top((*it).topLeft(), middleRight);
-            QModelIndex middleLeft = model->index(start, (*it).left(), (*it).parent());
+            QModelIndex middleLeft = model->index(start, (*it).left(), itParent);
             QItemSelectionRange bottom(middleLeft, (*it).bottomRight());
             it = ranges.erase(it);
             split.append(top);
             split.append(bottom);
+        } else if ((*it).isValid() && itParent == parent      // insertion before selection
+            && (*it).top() >= start) {
+            indexesOfSelectionChanged = true;
+            ++it;
         } else {
             ++it;
         }
     }
     ranges += split;
+
+    if (indexesOfSelectionChanged)
+        emit q->selectionChanged(QItemSelection(), QItemSelection());
 }
 
 /*!
@@ -1227,6 +1246,11 @@ void QItemSelectionModel::select(const QModelIndex &index, QItemSelectionModel::
     Note the that the current index changes independently from the selection.
     Also note that this signal will not be emitted when the item model is reset.
 
+    Items which stay selected but change their index are not included in
+    \a selected and \a deselected. Thus, this signal might be emitted with both
+    \a selected and \a deselected empty, if only the indices of selected items
+    change.
+
     \sa select(), currentChanged()
 */
 
diff --git a/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp b/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp
index ceca96f791..b7e13c8853 100644
--- a/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp
+++ b/tests/auto/corelib/itemmodels/qitemselectionmodel/tst_qitemselectionmodel.cpp
@@ -103,6 +103,8 @@ private slots:
     void QTBUG18001_data();
     void QTBUG18001();
 
+    void QTBUG93305();
+
 private:
     QAbstractItemModel *model;
     QItemSelectionModel *selection;
@@ -2897,5 +2899,46 @@ void tst_QItemSelectionModel::QTBUG18001()
 
 }
 
+void tst_QItemSelectionModel::QTBUG93305()
+{
+    // make sure the model is sane (5x5)
+    QCOMPARE(model->rowCount(QModelIndex()), 5);
+    QCOMPARE(model->columnCount(QModelIndex()), 5);
+
+    QSignalSpy spy(selection, &QItemSelectionModel::selectionChanged);
+
+    // select in row 1
+    QModelIndex index = model->index(1, 0, QModelIndex());
+    selection->select(index, QItemSelectionModel::ClearAndSelect);
+    QVERIFY(selection->hasSelection());
+    QCOMPARE(spy.count(), 1);
+
+    // removing row 0 does not change which cells are selected, but it
+    // does change the row number of the selected cells. Thus it changes
+    // what selectedIndexes() returns.
+    // The property selectedIndexes() has selectionChanged() as its
+    // NOTIFY signal, so selectionChanged() should be emitted.
+
+    // delete row 0
+    model->removeRows(0, 1, QModelIndex());
+    QVERIFY(selection->hasSelection());
+    QCOMPARE(spy.count(), 2);
+
+    // inserting a row before the first row again does not change which cells
+    // are selected, but does change the row number of the selected cells.
+    // This changes what selectedIndexes() returns and should thus trigger
+    // a selectionChanged() signal
+
+    // insert row 0 again
+    model->insertRows(0, 1, QModelIndex());
+    QVERIFY(selection->hasSelection());
+    QCOMPARE(spy.count(), 3);
+
+    // test for inserting multiple (6) rows
+    model->insertRows(0, 6, QModelIndex());
+    QVERIFY(selection->hasSelection());
+    QCOMPARE(spy.count(), 4);
+}
+
 QTEST_MAIN(tst_QItemSelectionModel)
 #include "tst_qitemselectionmodel.moc"