From 854b0acd1a259fab40e42c8470bb144c955dcc5a Mon Sep 17 00:00:00 2001 From: Peter Penz Date: Sun, 4 Dec 2011 18:16:39 +0100 Subject: [PATCH] Fix crash #1 when filtering items When filtering items it was possible that the current index got an invalid value which resulted in accessing the URL of a null-KFileItem. There is still one (general) open issue in KFileItemModelRolesUpdater (crash #2) where a KFileItem that is already null gets read. It is not really related to filtering but can be triggered quite easy when filtering huge directories with enabled previews. CCBUG: 287642 --- src/kitemviews/kfileitemmodel.cpp | 73 +++++++++++++------- src/kitemviews/kfileitemmodel.h | 6 +- src/kitemviews/kitemlistselectionmanager.cpp | 6 +- src/tests/kfileitemmodeltest.cpp | 11 ++- src/tests/kitemlistselectionmanagertest.cpp | 55 +++++++++++++-- src/views/dolphinview.cpp | 1 + 6 files changed, 115 insertions(+), 37 deletions(-) diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index bd83bf621..6ed10a602 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -480,46 +480,37 @@ void KFileItemModel::setExpanded(const QSet& urls) void KFileItemModel::setNameFilter(const QString& nameFilter) { if (m_nameFilter != nameFilter) { - // TODO #1: Assure that expanded items only can get hidden - // if no child item is visible - - // TODO #2: If the user entered a '*' use a regular expression + dispatchPendingItemsToInsert(); m_nameFilter = nameFilter; - const QString filter = nameFilter.toLower(); - // Check which shown items from m_itemData must get // hidden and hence moved to m_filteredItems. KFileItemList newFilteredItems; foreach (ItemData* itemData, m_itemData) { - if (!matchesNameFilter(itemData->item, filter)) { - m_filteredItems.append(itemData->item); + if (!matchesNameFilter(itemData->item)) { newFilteredItems.append(itemData->item); + m_filteredItems.insert(itemData->item); } } - if (!newFilteredItems.isEmpty()) { - slotItemsDeleted(newFilteredItems); - } + removeItems(newFilteredItems); // Check which hidden items from m_filteredItems should // get visible again and hence removed from m_filteredItems. KFileItemList newVisibleItems; - for (int i = m_filteredItems.count() - 1; i >= 0; --i) { - const KFileItem item = m_filteredItems.at(i); - if (matchesNameFilter(item, filter)) { + QMutableSetIterator it(m_filteredItems); + while (it.hasNext()) { + const KFileItem item = it.next(); + if (matchesNameFilter(item)) { newVisibleItems.append(item); - m_filteredItems.removeAt(i); + m_filteredItems.remove(item); } } - if (!newVisibleItems.isEmpty()) { - slotNewItems(newVisibleItems); - dispatchPendingItemsToInsert(); - } + insertItems(newVisibleItems); } } @@ -657,7 +648,23 @@ void KFileItemModel::slotCanceled() void KFileItemModel::slotNewItems(const KFileItemList& items) { - m_pendingItemsToInsert.append(items); + if (m_nameFilter.isEmpty()) { + m_pendingItemsToInsert.append(items); + } else { + // The name-filter is active. Hide filtered items + // before inserting them into the model and remember + // the filtered items in m_filteredItems. + KFileItemList filteredItems; + foreach (const KFileItem& item, items) { + if (matchesNameFilter(item)) { + filteredItems.append(item); + } else { + m_filteredItems.insert(item); + } + } + + m_pendingItemsToInsert.append(filteredItems); + } if (useMaximumUpdateInterval() && !m_maximumUpdateIntervalTimer->isActive()) { // Assure that items get dispatched if no completed() or canceled() signal is @@ -668,10 +675,14 @@ void KFileItemModel::slotNewItems(const KFileItemList& items) void KFileItemModel::slotItemsDeleted(const KFileItemList& items) { - if (!m_pendingItemsToInsert.isEmpty()) { - insertItems(m_pendingItemsToInsert); - m_pendingItemsToInsert.clear(); + dispatchPendingItemsToInsert(); + + if (!m_filteredItems.isEmpty()) { + foreach (const KFileItem& item, items) { + m_filteredItems.remove(item); + } } + removeItems(items); } @@ -738,6 +749,7 @@ void KFileItemModel::slotClear() kDebug() << "Clearing all items"; #endif + m_filteredItems.clear(); m_groups.clear(); m_minimumUpdateIntervalTimer->stop(); @@ -907,7 +919,11 @@ void KFileItemModel::removeItems(const KFileItemList& items) // Delete the items for (int i = indexesToRemove.count() - 1; i >= 0; --i) { const int indexToRemove = indexesToRemove.at(i); - delete m_itemData.at(indexToRemove); + ItemData* data = m_itemData.at(indexToRemove); + + m_items.remove(data->item.url()); + + delete data; m_itemData.removeAt(indexToRemove); } @@ -1707,10 +1723,15 @@ QList > KFileItemModel::genericStringRoleGroups(const QByte return groups; } -bool KFileItemModel::matchesNameFilter(const KFileItem& item, const QString& nameFilter) +bool KFileItemModel::matchesNameFilter(const KFileItem& item) const { + // TODO #1: A performance improvement would be possible by caching m_nameFilter.toLower(). + // Before adding yet-another-member it should be checked whether it brings a noticable + // improvement at all. + + // TODO #2: If the user entered a '*' use a regular expression const QString itemText = item.text().toLower(); - return itemText.contains(nameFilter); + return itemText.contains(m_nameFilter.toLower()); } #include "kfileitemmodel.moc" diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h index a83d57ba8..5d0aa4203 100644 --- a/src/kitemviews/kfileitemmodel.h +++ b/src/kitemviews/kfileitemmodel.h @@ -275,9 +275,9 @@ private: bool isChildItem(int index) const; /** - * @return True if the given item matches with the name filter. + * @return True if the given item matches with the current set name filter. */ - static bool matchesNameFilter(const KFileItem& item, const QString& nameFilter); + bool matchesNameFilter(const KFileItem& item) const; private: QWeakPointer m_dirLister; @@ -293,7 +293,7 @@ private: QHash m_items; // Allows O(1) access for KFileItemModel::index(const KFileItem& item) QString m_nameFilter; - KFileItemList m_filteredItems; // Items that got hidden by KFileItemModel::setNameFilter() + QSet m_filteredItems; // Items that got hidden by KFileItemModel::setNameFilter() bool m_requestRole[RolesCount]; diff --git a/src/kitemviews/kitemlistselectionmanager.cpp b/src/kitemviews/kitemlistselectionmanager.cpp index fb279796f..448e79293 100644 --- a/src/kitemviews/kitemlistselectionmanager.cpp +++ b/src/kitemviews/kitemlistselectionmanager.cpp @@ -293,15 +293,19 @@ void KItemListSelectionManager::itemsRemoved(const KItemRangeList& itemRanges) if (currentItem < itemRange.index) { break; } + if (currentItem >= itemRange.index + itemRange.count) { currentItem -= itemRange.count; - } else if (currentItem >= m_model->count()) { + } + + if (currentItem >= m_model->count()) { currentItem = m_model->count() - 1; } } // 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; + Q_ASSERT(m_currentItem < m_model->count()); emit currentChanged(m_currentItem, previousCurrent); } diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp index 23b899136..18580c24b 100644 --- a/src/tests/kfileitemmodeltest.cpp +++ b/src/tests/kfileitemmodeltest.cpp @@ -162,14 +162,17 @@ void KFileItemModelTest::testNewItems() void KFileItemModelTest::testRemoveItems() { m_testDir->createFile("a.txt"); + m_testDir->createFile("b.txt"); m_dirLister->openUrl(m_testDir->url()); QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout)); - QCOMPARE(m_model->count(), 1); + QCOMPARE(m_model->count(), 2); + QVERIFY(isModelConsistent()); m_testDir->removeFile("a.txt"); m_dirLister->updateDirectory(m_testDir->url()); QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsRemoved(KItemRangeList)), DefaultTimeout)); - QCOMPARE(m_model->count(), 0); + QCOMPARE(m_model->count(), 1); + QVERIFY(isModelConsistent()); } void KFileItemModelTest::testSetData() @@ -675,6 +678,10 @@ void KFileItemModelTest::testNameFilter() bool KFileItemModelTest::isModelConsistent() const { + if (m_model->m_items.count() != m_model->m_itemData.count()) { + return false; + } + for (int i = 0; i < m_model->count(); ++i) { const KFileItem item = m_model->fileItem(i); if (item.isNull()) { diff --git a/src/tests/kitemlistselectionmanagertest.cpp b/src/tests/kitemlistselectionmanagertest.cpp index 2fa6f677b..7f79197a9 100644 --- a/src/tests/kitemlistselectionmanagertest.cpp +++ b/src/tests/kitemlistselectionmanagertest.cpp @@ -27,18 +27,28 @@ class DummyModel : public KItemModelBase { public: DummyModel(); + void setCount(int count); virtual int count() const; virtual QHash data(int index) const; + +private: + int m_count; }; DummyModel::DummyModel() : - KItemModelBase() + KItemModelBase(), + m_count(100) +{ +} + +void DummyModel::setCount(int count) { + m_count = count; } int DummyModel::count() const { - return 100; + return m_count; } QHash DummyModel::data(int index) const @@ -48,7 +58,6 @@ QHash DummyModel::data(int index) const } - class KItemListSelectionManagerTest : public QObject { Q_OBJECT @@ -67,24 +76,30 @@ private slots: void testAnchoredSelection(); void testChangeSelection_data(); void testChangeSelection(); + void testDeleteCurrentItem_data(); + void testDeleteCurrentItem(); private: void verifySelectionChange(QSignalSpy& spy, const QSet& currentSelection, const QSet& previousSelection) const; KItemListSelectionManager* m_selectionManager; + DummyModel* m_model; }; void KItemListSelectionManagerTest::init() { + m_model = new DummyModel(); m_selectionManager = new KItemListSelectionManager(); - m_selectionManager->setModel(new DummyModel()); + m_selectionManager->setModel(m_model); } void KItemListSelectionManagerTest::cleanup() { - delete m_selectionManager->model(); delete m_selectionManager; m_selectionManager = 0; + + delete m_model; + m_model = 0; } void KItemListSelectionManagerTest::testConstructor() @@ -474,6 +489,36 @@ void KItemListSelectionManagerTest::testChangeSelection() verifySelectionChange(spySelectionChanged, QSet(), finalSelection); } +void KItemListSelectionManagerTest::testDeleteCurrentItem_data() +{ + QTest::addColumn("oldCurrentItemIndex"); + QTest::addColumn("removeIndex"); + QTest::addColumn("removeCount"); + QTest::addColumn("newCurrentItemIndex"); + + QTest::newRow("Remove before") << 50 << 0 << 10 << 40; + QTest::newRow("Remove after") << 50 << 51 << 10 << 50; + QTest::newRow("Remove exactly current item") << 50 << 50 << 1 << 50; + QTest::newRow("Remove around current item") << 50 << 45 << 10 << 50; + QTest::newRow("Remove all except one item") << 50 << 1 << 99 << 0; +} + +void KItemListSelectionManagerTest::testDeleteCurrentItem() +{ + QFETCH(int, oldCurrentItemIndex); + QFETCH(int, removeIndex); + QFETCH(int, removeCount); + QFETCH(int, newCurrentItemIndex); + + m_selectionManager->setCurrentItem(oldCurrentItemIndex); + + const int newCount = m_model->count() - removeCount; + m_model->setCount(newCount); + m_selectionManager->itemsRemoved(KItemRangeList() << KItemRange(removeIndex, removeCount)); + + QCOMPARE(m_selectionManager->currentItem(), newCurrentItemIndex); +} + void KItemListSelectionManagerTest::verifySelectionChange(QSignalSpy& spy, const QSet& currentSelection, const QSet& previousSelection) const diff --git a/src/views/dolphinview.cpp b/src/views/dolphinview.cpp index 857aae395..455905f6a 100644 --- a/src/views/dolphinview.cpp +++ b/src/views/dolphinview.cpp @@ -950,6 +950,7 @@ void DolphinView::saveState(QDataStream& stream) const int currentIndex = m_container->controller()->selectionManager()->currentItem(); if (currentIndex != -1) { KFileItem item = fileItemModel()->fileItem(currentIndex); + Q_ASSERT(!item.isNull()); // If the current index is valid a item must exist KUrl currentItemUrl = item.url(); stream << currentItemUrl; } else { -- 2.47.3