Fixes: NB#159396 - DuiComboBox emits an extraneous currentIndexChanged(-1) signal the first time an item is selected from the GUI

RevBy: Daniel d'Andrada
Details: Cleaned up selection handling in DuiComboBox and DuiPopupList.
Unit tests extended.

6 files changed, 59 insertions, 26 deletions

diff --git a/src/widgets/duicombobox.cpp b/src/widgets/duicombobox.cpp
index 11878bdc..c12dc417 100644
--- a/src/widgets/duicombobox.cpp
+++ b/src/widgets/duicombobox.cpp
@@ -47,19 +47,15 @@ void DuiComboBoxPrivate::_q_modelDestroyed()
     q->model()->setItemModel(duiEmptyModel());
 }
 
-void DuiComboBoxPrivate::_q_selectionModelCurrentChanged(const QModelIndex &current, const QModelIndex &previous)
-{
-    Q_UNUSED(previous);
-    Q_Q(DuiComboBox);
-    emit q->currentIndexChanged(current.row());
-    emit q->currentIndexChanged(q->itemText(current.row()));
-}
-
 void DuiComboBoxPrivate::_q_selectionModelSelectionChanged(const QItemSelection &selected, const QItemSelection &deselected)
 {
     Q_UNUSED(deselected);
     Q_Q(DuiComboBox);
-    if (selected.isEmpty()) {
+    if (!selected.isEmpty()) {
+        int currentRow = selected.indexes().first().row();
+        emit q->currentIndexChanged(currentRow);
+        emit q->currentIndexChanged(q->itemText(currentRow));
+    } else if (q->currentIndex() == -1) {
         emit q->currentIndexChanged(-1);
         emit q->currentIndexChanged(QString());
     }
@@ -69,10 +65,7 @@ void DuiComboBoxPrivate::_q_itemModelDataChanged(const QModelIndex &topLeft, con
 {
     Q_Q(DuiComboBox);
     int curRow = q->currentIndex();
-    if (curRow == -1) {
-        emit q->currentIndexChanged(-1);
-        emit q->currentIndexChanged(QString());
-    } else if (curRow >= topLeft.row() && curRow <= bottomRight.row()) {
+    if (curRow >= topLeft.row() && curRow <= bottomRight.row()) {
         emit q->currentIndexChanged(curRow);
         emit q->currentIndexChanged(q->itemText(curRow));
     }
@@ -146,7 +139,6 @@ void DuiComboBox::setItemModel(QAbstractItemModel *itemModel)
     connect(itemModel, SIGNAL(destroyed()), this, SLOT(_q_modelDestroyed()));
     connect(itemModel, SIGNAL(dataChanged(QModelIndex, QModelIndex)), this, SLOT(_q_itemModelDataChanged(QModelIndex, QModelIndex)));
     connect(itemModel, SIGNAL(modelReset()), this, SLOT(_q_itemModelReset()));
-    connect(itemSelectionModel, SIGNAL(currentChanged(QModelIndex, QModelIndex)), this, SLOT(_q_selectionModelCurrentChanged(QModelIndex, QModelIndex)));
     connect(itemSelectionModel, SIGNAL(selectionChanged(QItemSelection, QItemSelection)), this, SLOT(_q_selectionModelSelectionChanged(QItemSelection, QItemSelection)));
 }
 
@@ -381,10 +373,9 @@ void DuiComboBox::setCurrentIndex(int index)
 
     QModelIndex item = model()->itemModel()->index(index, 0);
 
-    if (item.isValid()) {
-        itemSelectionModel->select(item, QItemSelectionModel::ClearAndSelect);
-        itemSelectionModel->setCurrentIndex(item, QItemSelectionModel::Current);
-    } else
+    if (item.isValid())
+        itemSelectionModel->setCurrentIndex(item, QItemSelectionModel::ClearAndSelect);
+    else
         itemSelectionModel->clear();
 }
 
diff --git a/src/widgets/duicombobox.h b/src/widgets/duicombobox.h
index 00a6d34c..3de24ac1 100644
--- a/src/widgets/duicombobox.h
+++ b/src/widgets/duicombobox.h
@@ -380,7 +380,6 @@ private:
     Q_PRIVATE_SLOT(d_func(), void _q_modelDestroyed())
     Q_PRIVATE_SLOT(d_func(), void _q_itemModelDataChanged(QModelIndex, QModelIndex))
     Q_PRIVATE_SLOT(d_func(), void _q_itemModelReset())
-    Q_PRIVATE_SLOT(d_func(), void _q_selectionModelCurrentChanged(QModelIndex, QModelIndex))
     Q_PRIVATE_SLOT(d_func(), void _q_selectionModelSelectionChanged(QItemSelection, QItemSelection))
     Q_PRIVATE_SLOT(d_func(), void _q_itemClicked(QModelIndex))
 
diff --git a/src/widgets/duicombobox_p.h b/src/widgets/duicombobox_p.h
index 93a22f67..a79f68ad 100644
--- a/src/widgets/duicombobox_p.h
+++ b/src/widgets/duicombobox_p.h
@@ -34,7 +34,6 @@ public:
 
     void init();
     void _q_modelDestroyed();
-    void _q_selectionModelCurrentChanged(const QModelIndex &current, const QModelIndex &previous);
     void _q_selectionModelSelectionChanged(const QItemSelection &selected, const QItemSelection &deselected);
     void _q_itemModelDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight);
     void _q_itemModelReset();
diff --git a/src/widgets/duipopuplist.cpp b/src/widgets/duipopuplist.cpp
index 7c2923ad..cefc0ed9 100644
--- a/src/widgets/duipopuplist.cpp
+++ b/src/widgets/duipopuplist.cpp
@@ -131,8 +131,7 @@ void DuiPopupList::click(const QModelIndex &index)
     if (index == currentIndex()) {
         emit clicked(index);
     } else {
-        d->selectionModel->select(index, QItemSelectionModel::ClearAndSelect);
-        d->selectionModel->setCurrentIndex(index, QItemSelectionModel::Current);
+        d->selectionModel->setCurrentIndex(index, QItemSelectionModel::ClearAndSelect);
 
         emit clicked(index);
         emit currentIndexChanged(index);
@@ -157,10 +156,9 @@ void DuiPopupList::setCurrentIndex(const QModelIndex &index)
     // Since we didn't change the current index, no new signals should not send
     if (index == currentIndex()) return;
 
-    if (index.isValid()) {
-        d->selectionModel->select(index, QItemSelectionModel::ClearAndSelect);
-        d->selectionModel->setCurrentIndex(index, QItemSelectionModel::Current);
-    } else
+    if (index.isValid())
+        d->selectionModel->setCurrentIndex(index, QItemSelectionModel::ClearAndSelect);
+    else
         d->selectionModel->clear();
 
     emit currentIndexChanged(index);
diff --git a/tests/ut_duicombobox/ut_duicombobox.cpp b/tests/ut_duicombobox/ut_duicombobox.cpp
index 74644b76..ac098a48 100644
--- a/tests/ut_duicombobox/ut_duicombobox.cpp
+++ b/tests/ut_duicombobox/ut_duicombobox.cpp
@@ -263,6 +263,7 @@ void Ut_DuiComboBox::testBuiltinModel()
 {
     DuiComboBoxView *view = (DuiComboBoxView *)m_combobox->view();
     DuiComboBoxViewPrivate *viewPrivate = view->d_func();
+    QSignalSpy spy(m_combobox, SIGNAL(currentIndexChanged(int)));
 
     m_combobox->addItems(QStringList() << "item0" << "item1" << "item2" << "item3");
 
@@ -270,6 +271,8 @@ void Ut_DuiComboBox::testBuiltinModel()
     m_combobox->setCurrentIndex(2);
     QCOMPARE(m_combobox->currentIndex(), 2);
     QCOMPARE(viewPrivate->contentItem->subtitle(), QString("item2"));
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeLast()[0], QVariant(2));
     // remove item1, index should change
     m_combobox->removeItem(1);
     QCOMPARE(m_combobox->currentIndex(), 1);
@@ -278,15 +281,20 @@ void Ut_DuiComboBox::testBuiltinModel()
     m_combobox->removeItem(1);
     QCOMPARE(m_combobox->currentIndex(), -1);
     QCOMPARE(viewPrivate->contentItem->subtitle(), qtTrId("xx_ComboBoxSubtitle"));
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeLast()[0], QVariant(-1));
     // check that after removing items everything looks ok
     QCOMPARE(m_combobox->count(), 2);
     m_combobox->setCurrentIndex(1);
     QCOMPARE(m_combobox->currentIndex(), 1);
     QCOMPARE(viewPrivate->contentItem->subtitle(), QString("item3"));
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeLast()[0], QVariant(1));
     // add one item
     m_combobox->insertItem(0, QString("itemX"));
     QCOMPARE(m_combobox->currentIndex(), 2);
     QCOMPARE(viewPrivate->contentItem->subtitle(), QString("item3"));
+    QCOMPARE(spy.count(), 0);
 
     // change current item text
     m_combobox->setItemText(2, QString("beef"));
@@ -298,12 +306,15 @@ void Ut_DuiComboBox::testModelSwitching()
 {
     DuiComboBoxView *view = (DuiComboBoxView *)m_combobox->view();
     DuiComboBoxViewPrivate *viewPrivate = view->d_func();
+    QSignalSpy spy(m_combobox, SIGNAL(currentIndexChanged(int)));
 
     m_combobox->addItems(QStringList() << "item0" << "item1" << "item2" << "item3");
 
     m_combobox->setCurrentIndex(2);
     QCOMPARE(m_combobox->currentIndex(), 2);
     QCOMPARE(viewPrivate->contentItem->subtitle(), QString("item2"));
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeLast()[0], QVariant(2));
 
     QStringListModel *strListModel = new QStringListModel();
     strListModel->setStringList(QStringList() << "foo0" << "foo1" << "foo2" << "foo3");
@@ -312,6 +323,12 @@ void Ut_DuiComboBox::testModelSwitching()
     // model changed selection should reset
     QCOMPARE(m_combobox->currentIndex(), -1);
     QCOMPARE(viewPrivate->contentItem->subtitle(), qtTrId("xx_ComboBoxSubtitle"));
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeLast()[0], QVariant(-1));
+
+    // currentIndexChanged signals should not be emitted
+    m_combobox->insertItems(0, QStringList() << "bar1" << "bar2" << "bar3");
+    QCOMPARE(spy.count(), 0);
 }
 
 void Ut_DuiComboBox::testStringListModel()
@@ -355,6 +372,7 @@ void Ut_DuiComboBox::testStringListModelSetStringList()
 {
     DuiComboBoxView *view = (DuiComboBoxView *)m_combobox->view();
     DuiComboBoxViewPrivate *viewPrivate = view->d_func();
+    QSignalSpy spy(m_combobox, SIGNAL(currentIndexChanged(int)));
 
     QStringListModel *strListModel = new QStringListModel();
     strListModel->setStringList(QStringList() << "foo0" << "foo1" << "foo2" << "foo3");
@@ -362,10 +380,14 @@ void Ut_DuiComboBox::testStringListModelSetStringList()
 
     // select foo2
     m_combobox->setCurrentIndex(2);
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeFirst()[0], QVariant(2));
     // change string list completely
     strListModel->setStringList(QStringList() << "bar0" << "bar1" << "bar2" << "bar3");
     QCOMPARE(m_combobox->currentIndex(), -1);
     QCOMPARE(viewPrivate->contentItem->subtitle(), qtTrId("xx_ComboBoxSubtitle"));
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeFirst()[0], QVariant(-1));
 }
 
 void Ut_DuiComboBox::testActivatedSignal()
@@ -418,5 +440,28 @@ void Ut_DuiComboBox::testActivatedSignal()
     QCOMPARE(currentItemChangedQStringSpy.last()[0].toString(), QString("foo2"));
 }
 
+void Ut_DuiComboBox::testSetCurrentIndex()
+{
+    win->scene()->addItem(m_combobox);
+
+    DuiComboBoxView *view = (DuiComboBoxView *)m_combobox->view();
+    DuiComboBoxViewPrivate *viewPrivate = view->d_func();
+
+    QSignalSpy spy(m_combobox, SIGNAL(currentIndexChanged(int)));
+
+    m_combobox->addItem("text1");
+    m_combobox->setCurrentIndex(0);
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeFirst()[0].toInt(), 0);
+
+    m_combobox->addItem("text2");
+    m_combobox->click();
+    viewPrivate->popuplist->click(m_combobox->itemModel()->index(1, 0));
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(spy.takeFirst()[0].toInt(), 1);
+
+    win->scene()->removeItem(m_combobox);
+}
+
 QTEST_APPLESS_MAIN(Ut_DuiComboBox)
 
diff --git a/tests/ut_duicombobox/ut_duicombobox.h b/tests/ut_duicombobox/ut_duicombobox.h
index 270d096c..4552367f 100644
--- a/tests/ut_duicombobox/ut_duicombobox.h
+++ b/tests/ut_duicombobox/ut_duicombobox.h
@@ -52,6 +52,7 @@ private slots:
     void testStringListModel();
     void testStringListModelSetStringList();
     void testActivatedSignal();
+    void testSetCurrentIndex();
 
 private:
     DuiComboBox *m_combobox;