From 6524bf701a04e09ce31a7793ccccbedefc11e514 Mon Sep 17 00:00:00 2001 From: Frank Reininghaus Date: Sun, 4 Aug 2013 22:20:37 +0200 Subject: [PATCH] Introduce a new signal "groupsChanged" Sometimes when items are renamed, the order of the items in the directory is not affected, but the groups still change (simple example: with files a, b, c, e, rename "c" to "d"). At the moment, we always emit the itemsMoved signal in such a case to make sure that the view is updated. However, it would be preferable if this signal was not emitted because it can trigger some quite expensive operations which are not needed at all. This commit introduces a new signal groupsChanged and modifies KFileItemModel and KItemListView such that these classes make use of it. Some unit tests for the new functionality are included as well. Thanks to Emmanuel Pescosta for finding a latent bug in the code which was triggered by this change and fixed in 998954db6d53999dfa75d380cbb4ca3111589f66. REVIEW: 111808 --- src/kitemviews/kfileitemmodel.cpp | 50 +++++++++++++++++++++++-------- src/kitemviews/kitemlistview.cpp | 11 +++++++ src/kitemviews/kitemlistview.h | 1 + src/kitemviews/kitemmodelbase.h | 9 ++++++ src/tests/kfileitemmodeltest.cpp | 35 ++++++++++++---------- 5 files changed, 78 insertions(+), 28 deletions(-) diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index 1b4911dece..c7e1c86004 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -689,7 +689,6 @@ void KFileItemModel::resortAllItems() oldUrls.append(itemData->item.url()); } - m_groups.clear(); m_items.clear(); // Resort the items @@ -698,20 +697,45 @@ void KFileItemModel::resortAllItems() m_items.insert(m_itemData.at(i)->item.url(), i); } - // Determine the indexes that have been moved - QList movedToIndexes; - movedToIndexes.reserve(itemCount); - for (int i = 0; i < itemCount; i++) { - const int newIndex = m_items.value(oldUrls.at(i)); - movedToIndexes.append(newIndex); + // Determine the first index that has been moved. + int firstMovedIndex = 0; + while (firstMovedIndex < itemCount + && firstMovedIndex == m_items.value(oldUrls.at(firstMovedIndex))) { + ++firstMovedIndex; } - // Don't check whether items have really been moved and always emit a - // itemsMoved() signal after resorting: In case of grouped items - // the groups might change even if the items themselves don't change their - // position. Let the receiver of the signal decide whether a check for moved - // items makes sense. - emit itemsMoved(KItemRange(0, itemCount), movedToIndexes); + const bool itemsHaveMoved = firstMovedIndex < itemCount; + if (itemsHaveMoved) { + m_groups.clear(); + + int lastMovedIndex = itemCount - 1; + while (lastMovedIndex > firstMovedIndex + && lastMovedIndex == m_items.value(oldUrls.at(lastMovedIndex))) { + --lastMovedIndex; + } + + Q_ASSERT(firstMovedIndex <= lastMovedIndex); + + // Create a list movedToIndexes, which has the property that + // movedToIndexes[i] is the new index of the item with the old index + // firstMovedIndex + i. + const int movedItemsCount = lastMovedIndex - firstMovedIndex + 1; + QList movedToIndexes; + movedToIndexes.reserve(movedItemsCount); + for (int i = firstMovedIndex; i <= lastMovedIndex; ++i) { + const int newIndex = m_items.value(oldUrls.at(i)); + movedToIndexes.append(newIndex); + } + + emit itemsMoved(KItemRange(firstMovedIndex, movedItemsCount), movedToIndexes); + } else if (groupedSorting()) { + // The groups might have changed even if the order of the items has not. + const QList > oldGroups = m_groups; + m_groups.clear(); + if (groups() != oldGroups) { + emit groupsChanged(); + } + } #ifdef KFILEITEMMODEL_DEBUG kDebug() << "[TIME] Resorting of" << itemCount << "items:" << timer.elapsed(); diff --git a/src/kitemviews/kitemlistview.cpp b/src/kitemviews/kitemlistview.cpp index 0c3183cd57..80a4ba7e3a 100644 --- a/src/kitemviews/kitemlistview.cpp +++ b/src/kitemviews/kitemlistview.cpp @@ -1229,6 +1229,13 @@ void KItemListView::slotItemsChanged(const KItemRangeList& itemRanges, QAccessible::updateAccessibility(this, 0, QAccessible::TableModelChanged); } +void KItemListView::slotGroupsChanged() +{ + updateVisibleGroupHeaders(); + doLayout(NoAnimation); + updateSiblingsInformation(); +} + void KItemListView::slotGroupedSortingChanged(bool current) { m_grouped = current; @@ -1521,6 +1528,8 @@ void KItemListView::setModel(KItemModelBase* model) this, SLOT(slotItemsRemoved(KItemRangeList))); disconnect(m_model, SIGNAL(itemsMoved(KItemRange,QList)), this, SLOT(slotItemsMoved(KItemRange,QList))); + disconnect(m_model, SIGNAL(groupsChanged()), + this, SLOT(slotGroupsChanged())); disconnect(m_model, SIGNAL(groupedSortingChanged(bool)), this, SLOT(slotGroupedSortingChanged(bool))); disconnect(m_model, SIGNAL(sortOrderChanged(Qt::SortOrder,Qt::SortOrder)), @@ -1544,6 +1553,8 @@ void KItemListView::setModel(KItemModelBase* model) this, SLOT(slotItemsRemoved(KItemRangeList))); connect(m_model, SIGNAL(itemsMoved(KItemRange,QList)), this, SLOT(slotItemsMoved(KItemRange,QList))); + connect(m_model, SIGNAL(groupsChanged()), + this, SLOT(slotGroupsChanged())); connect(m_model, SIGNAL(groupedSortingChanged(bool)), this, SLOT(slotGroupedSortingChanged(bool))); connect(m_model, SIGNAL(sortOrderChanged(Qt::SortOrder,Qt::SortOrder)), diff --git a/src/kitemviews/kitemlistview.h b/src/kitemviews/kitemlistview.h index 6467b8c915..14360b02bb 100644 --- a/src/kitemviews/kitemlistview.h +++ b/src/kitemviews/kitemlistview.h @@ -394,6 +394,7 @@ protected slots: virtual void slotItemsMoved(const KItemRange& itemRange, const QList& movedToIndexes); virtual void slotItemsChanged(const KItemRangeList& itemRanges, const QSet& roles); + virtual void slotGroupsChanged(); virtual void slotGroupedSortingChanged(bool current); virtual void slotSortOrderChanged(Qt::SortOrder current, Qt::SortOrder previous); diff --git a/src/kitemviews/kitemmodelbase.h b/src/kitemviews/kitemmodelbase.h index 70f6883909..7545192da4 100644 --- a/src/kitemviews/kitemmodelbase.h +++ b/src/kitemviews/kitemmodelbase.h @@ -218,11 +218,20 @@ signals: * with the items 5 and 6 then the parameters look like this: * - itemRange: has the index 0 and a count of 7. * - movedToIndexes: Contains the seven values 5, 6, 2, 3, 4, 0, 1 + * + * This signal implies that the groups might have changed. Therefore, + * gropusChanged() is not emitted if this signal is emitted. */ void itemsMoved(const KItemRange& itemRange, const QList& movedToIndexes); void itemsChanged(const KItemRangeList& itemRanges, const QSet& roles); + /** + * Is emitted if the groups have changed, even though the order of the + * items has not been modified. + */ + void groupsChanged(); + void groupedSortingChanged(bool current); void sortRoleChanged(const QByteArray& current, const QByteArray& previous); void sortOrderChanged(Qt::SortOrder current, Qt::SortOrder previous); diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp index 513ecef5a0..6d5d4b77cf 100644 --- a/src/tests/kfileitemmodeltest.cpp +++ b/src/tests/kfileitemmodeltest.cpp @@ -49,6 +49,7 @@ namespace { const int DefaultTimeout = 5000; }; +Q_DECLARE_METATYPE(KItemRange) Q_DECLARE_METATYPE(KItemRangeList) Q_DECLARE_METATYPE(QList) @@ -719,7 +720,8 @@ void KFileItemModelTest::testSorting() QCOMPARE(m_model->sortOrder(), Qt::AscendingOrder); QCOMPARE(itemsInModel(), QStringList() << "a" << "b" << "c" << "c-1" << "c-2" << "c-3" << "d" << "e"); QCOMPARE(spyItemsMoved.count(), 1); - QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 2 << 4 << 5 << 3 << 0 << 1 << 6 << 7); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(0, 6)); + QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 2 << 4 << 5 << 3 << 0 << 1); // Sort by Name, descending m_model->setSortDirectoriesFirst(true); @@ -728,8 +730,10 @@ void KFileItemModelTest::testSorting() QCOMPARE(m_model->sortOrder(), Qt::DescendingOrder); QCOMPARE(itemsInModel(), QStringList() << "c" << "c-2" << "c-3" << "c-1" << "e" << "d" << "b" << "a"); QCOMPARE(spyItemsMoved.count(), 2); - QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 4 << 5 << 0 << 3 << 1 << 2 << 6 << 7); - QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 0 << 1 << 2 << 3 << 7 << 6 << 5 << 4); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(0, 6)); + QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 4 << 5 << 0 << 3 << 1 << 2); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(4, 4)); + QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 7 << 6 << 5 << 4); // Sort by Date, descending m_model->setSortDirectoriesFirst(true); @@ -738,7 +742,8 @@ void KFileItemModelTest::testSorting() QCOMPARE(m_model->sortOrder(), Qt::DescendingOrder); QCOMPARE(itemsInModel(), QStringList() << "c" << "c-2" << "c-3" << "c-1" << "b" << "d" << "a" << "e"); QCOMPARE(spyItemsMoved.count(), 1); - QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 0 << 1 << 2 << 3 << 7 << 5 << 4 << 6); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(4, 4)); + QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 7 << 5 << 4 << 6); // Sort by Date, ascending m_model->setSortOrder(Qt::AscendingOrder); @@ -746,7 +751,8 @@ void KFileItemModelTest::testSorting() QCOMPARE(m_model->sortOrder(), Qt::AscendingOrder); QCOMPARE(itemsInModel(), QStringList() << "c" << "c-2" << "c-3" << "c-1" << "e" << "a" << "d" << "b"); QCOMPARE(spyItemsMoved.count(), 1); - QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 0 << 1 << 2 << 3 << 7 << 6 << 5 << 4); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(4, 4)); + QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 7 << 6 << 5 << 4); // Sort by Date, ascending, 'Sort Folders First' disabled m_model->setSortDirectoriesFirst(false); @@ -755,7 +761,8 @@ void KFileItemModelTest::testSorting() QVERIFY(!m_model->sortDirectoriesFirst()); QCOMPARE(itemsInModel(), QStringList() << "e" << "a" << "c" << "c-1" << "c-2" << "c-3" << "d" << "b"); QCOMPARE(spyItemsMoved.count(), 1); - QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 2 << 4 << 5 << 3 << 0 << 1 << 6 << 7); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(0, 6)); + QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 2 << 4 << 5 << 3 << 0 << 1); // Sort by Name, ascending, 'Sort Folders First' disabled m_model->setSortRole("text"); @@ -763,6 +770,7 @@ void KFileItemModelTest::testSorting() QVERIFY(!m_model->sortDirectoriesFirst()); QCOMPARE(itemsInModel(), QStringList() << "a" << "b" << "c" << "c-1" << "c-2" << "c-3" << "d" << "e"); QCOMPARE(spyItemsMoved.count(), 1); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(0, 8)); QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 7 << 0 << 2 << 3 << 4 << 5 << 6 << 1); // Sort by Size, ascending, 'Sort Folders First' disabled @@ -772,19 +780,15 @@ void KFileItemModelTest::testSorting() QVERIFY(!m_model->sortDirectoriesFirst()); QCOMPARE(itemsInModel(), QStringList() << "c" << "c-2" << "c-3" << "c-1" << "a" << "b" << "e" << "d"); QCOMPARE(spyItemsMoved.count(), 1); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(0, 8)); QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 4 << 5 << 0 << 3 << 1 << 2 << 7 << 6); - QSKIP("2 tests of testSorting() are temporary deactivated as in KFileItemModel resortAllItems() " - "always emits a itemsMoved() signal. Before adjusting the tests think about probably introducing " - "another signal", SkipSingle); - // Internal note: Check comment in KFileItemModel::resortAllItems() for details. - // In 'Sort by Size' mode, folders are always first -> changing 'Sort Folders First' does not resort the model m_model->setSortDirectoriesFirst(true); QCOMPARE(m_model->sortRole(), QByteArray("size")); QCOMPARE(m_model->sortOrder(), Qt::AscendingOrder); QVERIFY(m_model->sortDirectoriesFirst()); - QCOMPARE(itemsInModel(), QStringList() << "c" << "a" << "b" << "e" << "d"); + QCOMPARE(itemsInModel(), QStringList() << "c" << "c-2" << "c-3" << "c-1" << "a" << "b" << "e" << "d"); QCOMPARE(spyItemsMoved.count(), 0); // Sort by Size, descending, 'Sort Folders First' enabled @@ -792,9 +796,10 @@ void KFileItemModelTest::testSorting() QCOMPARE(m_model->sortRole(), QByteArray("size")); QCOMPARE(m_model->sortOrder(), Qt::DescendingOrder); QVERIFY(m_model->sortDirectoriesFirst()); - QCOMPARE(itemsInModel(), QStringList() << "c" << "d" << "e" << "b" << "a"); + QCOMPARE(itemsInModel(), QStringList() << "c" << "c-2" << "c-3" << "c-1" << "d" << "e" << "b" << "a"); QCOMPARE(spyItemsMoved.count(), 1); - QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 0 << 4 << 3 << 2 << 1); + QCOMPARE(spyItemsMoved.first().at(0).value(), KItemRange(4, 4)); + QCOMPARE(spyItemsMoved.takeFirst().at(1).value >(), QList() << 7 << 6 << 5 << 4); // TODO: Sort by other roles; show/hide hidden files } @@ -1212,7 +1217,7 @@ void KFileItemModelTest::testNameRoleGroups() // Rename c.txt to d.txt. data.insert("text", "d.txt"); m_model->setData(2, data); - QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsMoved(KItemRange,QList)), DefaultTimeout)); + QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(groupsChanged()), DefaultTimeout)); QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "b.txt" << "d.txt" << "e.txt"); expectedGroups.clear();