Emit KitemListSelectionManager's selectionChanged signal correctly

This commit makes sure that the signal is emitted with the correct
current and previous selection after a selection change, and
that the signal is emitted exactly once in
KItemListSelectionManager::itemsInserted and
KItemListSelectionManager::itemsRemoved.

2 files changed, 57 insertions, 33 deletions

diff --git a/src/kitemviews/kitemlistselectionmanager.cpp b/src/kitemviews/kitemlistselectionmanager.cpp
index 18fa2f660..cfe847b69 100644
--- a/src/kitemviews/kitemlistselectionmanager.cpp
+++ b/src/kitemviews/kitemlistselectionmanager.cpp
@@ -43,6 +43,8 @@ KItemListSelectionManager::~KItemListSelectionManager()
 void KItemListSelectionManager::setCurrentItem(int current)
 {
     const int previous = m_currentItem;
+    const QSet<int> previousSelection = selectedItems();
+
     if (m_model && current >= 0 && current < m_model->count()) {
         m_currentItem = current;
     } else {
@@ -51,6 +53,13 @@ void KItemListSelectionManager::setCurrentItem(int current)
 
     if (m_currentItem != previous) {
         emit currentChanged(m_currentItem, previous);
+
+        if (m_isAnchoredSelectionActive) {
+            const QSet<int> selection = selectedItems();
+            if (selection != previousSelection) {
+                emit selectionChanged(selection, previousSelection);
+            }
+        }
     }
 }
 
@@ -95,7 +104,7 @@ void KItemListSelectionManager::setSelected(int index, int count, SelectionMode
         return;
     }
 
-    const QSet<int> previous = m_selectedItems;
+    const QSet<int> previous = selectedItems();
 
     count = qMin(count, m_model->count() - index);
 
@@ -128,23 +137,19 @@ void KItemListSelectionManager::setSelected(int index, int count, SelectionMode
         break;
     }
 
-    if (m_selectedItems != previous) {
-        emit selectionChanged(m_selectedItems, previous);
+    const QSet<int> selection = selectedItems();
+    if (selection != previous) {
+        emit selectionChanged(selection, previous);
     }
 }
 
 void KItemListSelectionManager::clearSelection()
 {
-    if (!m_selectedItems.isEmpty()) {
-        const QSet<int> previous = m_selectedItems;
+    const QSet<int> previous = selectedItems();
+    if (!previous.isEmpty()) {
         m_selectedItems.clear();
         m_isAnchoredSelectionActive = false;
-        emit selectionChanged(m_selectedItems, previous);
-    }
-    else if (m_isAnchoredSelectionActive) {
-        m_isAnchoredSelectionActive = false;
-        // TODO: the 'previous' parameter of the signal has to be set correctly, but do we actually need it?
-        emit selectionChanged(m_selectedItems, m_selectedItems);
+        emit selectionChanged(QSet<int>(), previous);
     }
 }
 
@@ -212,10 +217,14 @@ void KItemListSelectionManager::setModel(KItemModelBase* model)
 
 void KItemListSelectionManager::itemsInserted(const KItemRangeList& itemRanges)
 {
+    // Store the current selection (needed in the selectionChanged() signal)
+    const QSet<int> previousSelection = selectedItems();
+
     // Update the current item
     if (m_currentItem < 0) {
         setCurrentItem(0);
     } else {
+        const int previousCurrent = m_currentItem;
         int inc = 0;
         foreach (const KItemRange& itemRange, itemRanges) {
             if (m_currentItem < itemRange.index) {
@@ -223,13 +232,17 @@ void KItemListSelectionManager::itemsInserted(const KItemRangeList& itemRanges)
             }
             inc += itemRange.count;
         }
-        setCurrentItem(m_currentItem + inc);
+        // Calling setCurrentItem would trigger the selectionChanged signal, but we want to
+        // emit it only once in this function -> change the current item manually and emit currentChanged
+        m_currentItem += inc;
+        emit currentChanged(m_currentItem, previousCurrent);
     }
 
     // Update the anchor item
     if (m_anchorItem < 0) {
         setAnchorItem(0);
     } else {
+        const int previousAnchor = m_anchorItem;
         int inc = 0;
         foreach (const KItemRange& itemRange, itemRanges) {
             if (m_anchorItem < itemRange.index) {
@@ -237,16 +250,15 @@ void KItemListSelectionManager::itemsInserted(const KItemRangeList& itemRanges)
             }
             inc += itemRange.count;
         }
-        setAnchorItem(m_anchorItem + inc);
+        m_anchorItem += inc;
+        emit anchorChanged(m_anchorItem, previousAnchor);
     }
 
     // Update the selections
     if (!m_selectedItems.isEmpty()) {
         const QSet<int> previous = m_selectedItems;
-
-        QSet<int> current;
-        current.reserve(m_selectedItems.count());
-        QSetIterator<int> it(m_selectedItems);
+        m_selectedItems.clear();
+        QSetIterator<int> it(previous);
         while (it.hasNext()) {
             const int index = it.next();
             int inc = 0;
@@ -256,20 +268,24 @@ void KItemListSelectionManager::itemsInserted(const KItemRangeList& itemRanges)
                 }
                 inc += itemRange.count;
             }
-            current.insert(index + inc);
+            m_selectedItems.insert(index + inc);
         }
+    }
 
-        if (current != previous) {
-            m_selectedItems = current;
-            emit selectionChanged(current, previous);
-        }
+    const QSet<int> selection = selectedItems();
+    if (selection != previousSelection) {
+        emit selectionChanged(selection, previousSelection);
     }
 }
 
 void KItemListSelectionManager::itemsRemoved(const KItemRangeList& itemRanges)
 {
+    // Store the current selection (needed in the selectionChanged() signal)
+    const QSet<int> previousSelection = selectedItems();
+
     // Update the current item
     if (m_currentItem >= 0) {
+        const int previousCurrent = m_currentItem;
         int currentItem = m_currentItem;
         foreach (const KItemRange& itemRange, itemRanges) {
             if (currentItem < itemRange.index) {
@@ -281,11 +297,15 @@ void KItemListSelectionManager::itemsRemoved(const KItemRangeList& itemRanges)
                 currentItem = m_model->count() - 1;
             }
         }
-        setCurrentItem(currentItem);
+        // Calling setCurrentItem would trigger the selectionChanged signal, but we want to
+        // emit it only once in this function -> change the current item manually and emit currentChanged
+        m_currentItem = currentItem;
+        emit currentChanged(m_currentItem, previousCurrent);
     }
 
     // Update the anchor item
     if (m_anchorItem >= 0) {
+        const int previousAnchor = m_anchorItem;
         int anchorItem = m_anchorItem;
         foreach (const KItemRange& itemRange, itemRanges) {
             if (anchorItem < itemRange.index) {
@@ -297,16 +317,15 @@ void KItemListSelectionManager::itemsRemoved(const KItemRangeList& itemRanges)
                 anchorItem = m_model->count() - 1;
             }
         }
-        setAnchorItem(anchorItem);
+        m_anchorItem = anchorItem;
+        emit anchorChanged(m_anchorItem, previousAnchor);
     }
 
     // Update the selections
     if (!m_selectedItems.isEmpty()) {
         const QSet<int> previous = m_selectedItems;
-
-        QSet<int> current;
-        current.reserve(m_selectedItems.count());
-        QSetIterator<int> it(m_selectedItems);
+        m_selectedItems.clear();
+        QSetIterator<int> it(previous);
         while (it.hasNext()) {
             int index = it.next();
             int dec = 0;
@@ -326,14 +345,14 @@ void KItemListSelectionManager::itemsRemoved(const KItemRangeList& itemRanges)
             }
             index -= dec;
             if (index >= 0)  {
-                current.insert(index);
+                m_selectedItems.insert(index);
             }
         }
+    }
 
-        if (current != previous) {
-            m_selectedItems = current;
-            emit selectionChanged(current, previous);
-        }
+    const QSet<int> selection = selectedItems();
+    if (selection != previousSelection) {
+        emit selectionChanged(selection, previousSelection);
     }
 }
 
diff --git a/src/tests/kitemlistselectionmanagertest.cpp b/src/tests/kitemlistselectionmanagertest.cpp
index 4c45fd1b8..a79b9e6f3 100644
--- a/src/tests/kitemlistselectionmanagertest.cpp
+++ b/src/tests/kitemlistselectionmanagertest.cpp
@@ -212,6 +212,7 @@ void KItemListSelectionManagerTest::testItemsInserted()
     // Select items 10 to 12
     m_selectionManager->setSelected(10, 3);
     QSet<int> selectedItems = m_selectionManager->selectedItems();
+    QCOMPARE(selectedItems.count(), 3);
     QVERIFY(selectedItems.contains(10));
     QVERIFY(selectedItems.contains(11));
     QVERIFY(selectedItems.contains(12));
@@ -219,6 +220,7 @@ void KItemListSelectionManagerTest::testItemsInserted()
     // Insert items 0 to 4 -> selection must be 15 to 17
     m_selectionManager->itemsInserted(KItemRangeList() << KItemRange(0, 5));
     selectedItems = m_selectionManager->selectedItems();
+    QCOMPARE(selectedItems.count(), 3);
     QVERIFY(selectedItems.contains(15));
     QVERIFY(selectedItems.contains(16));
     QVERIFY(selectedItems.contains(17));
@@ -229,6 +231,7 @@ void KItemListSelectionManagerTest::testItemsInserted()
                                       KItemRange(16, 1) <<
                                       KItemRange(17, 1));
     selectedItems = m_selectionManager->selectedItems();
+    QCOMPARE(selectedItems.count(), 3);
     QVERIFY(selectedItems.contains(16));
     QVERIFY(selectedItems.contains(18));
     QVERIFY(selectedItems.contains(20));
@@ -239,6 +242,7 @@ void KItemListSelectionManagerTest::testItemsRemoved()
     // Select items 10 to 15
     m_selectionManager->setSelected(10, 6);
     QSet<int> selectedItems = m_selectionManager->selectedItems();
+    QCOMPARE(selectedItems.count(), 6);
     for (int i = 10; i <= 15; ++i) {
         QVERIFY(selectedItems.contains(i));
     }
@@ -246,6 +250,7 @@ void KItemListSelectionManagerTest::testItemsRemoved()
     // Remove items 0 to 4 -> selection must be 5 to 10
     m_selectionManager->itemsRemoved(KItemRangeList() << KItemRange(0, 5));
     selectedItems = m_selectionManager->selectedItems();
+    QCOMPARE(selectedItems.count(), 6);
     for (int i = 5; i <= 10; ++i) {
         QVERIFY(selectedItems.contains(i));
     }