QComboBox: reset indexBeforeChange to -1 if index is invalidated

The member variable indexBeforeChange is member-initialized with -1 and
changed to the current row, if a valid index is set.
It is used to check, if the model has been reset to an empty model
and/or the index has been invalidated. The result is used to decide, if
the currentIndexChanged signal is emitted or not.

If a combo box had a valid index and it is invalidated afterwards
(e.g. because the combobox has no more entries), indexBeforeChange is
not reset to -1. The redundant signal emission is therefore prevented
only the first time.

This patch resets indexBeforeChange if the index is invalidated or the
last item is removed from the combo box.

It also adds a no-op check to tst_QComboBox::currentIndex.
The test data sets "check that setting the index to -1 works" and
"check that current index is invalid when removing the only item" check
the respective use cases.

As a drive-by, variable names and QObect::connect syntax have been
cleaned up in tst_QComboBox::currentIndex.

Fixes: QTBUG-108614
Pick-to: 6.5 6.2
Change-Id: Ib6dfa887d9247f2c47df065039d69ba57c32fa24
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

2 files changed, 55 insertions, 39 deletions

diff --git a/src/widgets/widgets/qcombobox.cpp b/src/widgets/widgets/qcombobox.cpp
index 7bfce05d68..80206b24ac 100644
--- a/src/widgets/widgets/qcombobox.cpp
+++ b/src/widgets/widgets/qcombobox.cpp
@@ -1121,6 +1121,12 @@ void QComboBoxPrivate::_q_rowsRemoved(const QModelIndex &parent, int /*start*/,
         q->updateGeometry();
     }
 
+    // model has removed the last row
+    if (model->rowCount(root) == 0) {
+        setCurrentIndex(QModelIndex());
+        return;
+    }
+
     // model has changed the currentIndex
     if (currentIndex.row() != indexBeforeChange) {
         if (!currentIndex.isValid() && q->count()) {
@@ -2127,10 +2133,15 @@ void QComboBoxPrivate::setCurrentIndex(const QModelIndex &mi)
         }
         updateLineEditGeometry();
     }
-    // If the model was reset to an empty, currentIndex will be invalidated
+    // If the model was reset to an empty one, currentIndex will be invalidated
     // (because it's a QPersistentModelIndex), but the index change will never
-    // be advertised. So we need an explicit check for such condition.
+    // be advertised. So an explicit check for this condition is needed.
+    // The variable used for that check has to be reset when a previously valid
+    // index becomes invalid.
     const bool modelResetToEmpty = !normalized.isValid() && indexBeforeChange != -1;
+    if (modelResetToEmpty)
+        indexBeforeChange = -1;
+
     if (indexChanged || modelResetToEmpty) {
         q->update();
         _q_emitCurrentIndexChanged(currentIndex);
diff --git a/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp b/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
index 337a7fecaa..245c4c8ca4 100644
--- a/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
+++ b/tests/auto/widgets/widgets/qcombobox/tst_qcombobox.cpp
@@ -1018,7 +1018,7 @@ void tst_QComboBox::currentIndex_data()
         expectedCurrentIndex = -1;
         expectedCurrentText = "";
         expectedSignalCount = 2;
-        QTest::newRow("check that isetting the index to -1 works")
+        QTest::newRow("check that setting the index to -1 works")
             << initialItems << setCurrentIndex << removeIndex
             << insertIndex << insertText << expectedCurrentIndex << expectedCurrentText
             << expectedSignalCount;
@@ -1161,66 +1161,71 @@ void tst_QComboBox::currentIndex()
     TestWidget topLevel;
     topLevel.show();
     QVERIFY(QTest::qWaitForWindowExposed(&topLevel));
-    QComboBox *testWidget = topLevel.comboBox();
+    QComboBox *comboBox = topLevel.comboBox();
     // test both editable/non-editable combobox
     for (int edit = 0; edit < 2; ++edit) {
-        testWidget->clear();
-        testWidget->setEditable(edit ? true : false);
+        comboBox->clear();
+        comboBox->setEditable(edit ? true : false);
         if (edit)
-            QVERIFY(testWidget->lineEdit());
+            QVERIFY(comboBox->lineEdit());
 
         // verify it is empty, has no current index and no current text
-        QCOMPARE(testWidget->count(), 0);
-        QCOMPARE(testWidget->currentIndex(), -1);
-        QVERIFY(testWidget->currentText().isEmpty());
+        QCOMPARE(comboBox->count(), 0);
+        QCOMPARE(comboBox->currentIndex(), -1);
+        QVERIFY(comboBox->currentText().isEmpty());
 
         // spy on currentIndexChanged
-        QSignalSpy indexChangedInt(testWidget, SIGNAL(currentIndexChanged(int)));
+        QSignalSpy indexChangedSpy(comboBox, &QComboBox::currentIndexChanged);
 
         // stuff items into it
-        foreach(QString text, initialItems) {
-            testWidget->addItem(text);
-        }
-        QCOMPARE(testWidget->count(), initialItems.size());
+        for (const QString &text : initialItems)
+            comboBox->addItem(text);
+
+        QCOMPARE(comboBox->count(), initialItems.size());
 
         // set current index, remove and/or insert
         if (setCurrentIndex >= -1) {
-            testWidget->setCurrentIndex(setCurrentIndex);
-            QCOMPARE(testWidget->currentIndex(), setCurrentIndex);
+            comboBox->setCurrentIndex(setCurrentIndex);
+            QCOMPARE(comboBox->currentIndex(), setCurrentIndex);
         }
 
         if (removeIndex >= 0)
-            testWidget->removeItem(removeIndex);
+            comboBox->removeItem(removeIndex);
         if (insertIndex >= 0)
-            testWidget->insertItem(insertIndex, insertText);
+            comboBox->insertItem(insertIndex, insertText);
 
         // compare with expected index and text
-        QCOMPARE(testWidget->currentIndex(), expectedCurrentIndex);
-        QCOMPARE(testWidget->currentText(), expectedCurrentText);
+        QCOMPARE(comboBox->currentIndex(), expectedCurrentIndex);
+        QCOMPARE(comboBox->currentText(), expectedCurrentText);
 
         // check that signal count is correct
-        QCOMPARE(indexChangedInt.size(), expectedSignalCount);
+        QCOMPARE(indexChangedSpy.size(), expectedSignalCount);
 
         // compare with last sent signal values
-        if (indexChangedInt.size())
-            QCOMPARE(indexChangedInt.at(indexChangedInt.size() - 1).at(0).toInt(),
-                    testWidget->currentIndex());
+        if (indexChangedSpy.size())
+            QCOMPARE(indexChangedSpy.at(indexChangedSpy.size() - 1).at(0).toInt(),
+                    comboBox->currentIndex());
+
+        // Test a no-op index change
+        const int index = comboBox->currentIndex();
+        comboBox->setCurrentIndex(index);
+        QCOMPARE(indexChangedSpy.size(), expectedSignalCount);
 
         if (edit) {
-            testWidget->setCurrentIndex(-1);
-            testWidget->setInsertPolicy(QComboBox::InsertAtBottom);
-            QTest::keyPress(testWidget, 'a');
-            QTest::keyPress(testWidget, 'b');
-            QCOMPARE(testWidget->currentText(), QString("ab"));
-            QCOMPARE(testWidget->currentIndex(), -1);
-            int numItems = testWidget->count();
-            QTest::keyPress(testWidget, Qt::Key_Return);
-            QCOMPARE(testWidget->count(), numItems + 1);
-            QCOMPARE(testWidget->currentIndex(), numItems);
-            testWidget->setCurrentIndex(-1);
-            QTest::keyPress(testWidget, 'a');
-            QTest::keyPress(testWidget, 'b');
-            QCOMPARE(testWidget->currentIndex(), -1);
+            comboBox->setCurrentIndex(-1);
+            comboBox->setInsertPolicy(QComboBox::InsertAtBottom);
+            QTest::keyPress(comboBox, 'a');
+            QTest::keyPress(comboBox, 'b');
+            QCOMPARE(comboBox->currentText(), QString("ab"));
+            QCOMPARE(comboBox->currentIndex(), -1);
+            int numItems = comboBox->count();
+            QTest::keyPress(comboBox, Qt::Key_Return);
+            QCOMPARE(comboBox->count(), numItems + 1);
+            QCOMPARE(comboBox->currentIndex(), numItems);
+            comboBox->setCurrentIndex(-1);
+            QTest::keyPress(comboBox, 'a');
+            QTest::keyPress(comboBox, 'b');
+            QCOMPARE(comboBox->currentIndex(), -1);
         }
     }
 }