From 4416377eae06e70f5f841e94347f2d0b31113524 Mon Sep 17 00:00:00 2001 From: Frank Reininghaus Date: Mon, 9 Sep 2013 21:38:47 +0200 Subject: [PATCH] Always sort items correctly when the refreshItems() signal is received When sorting by, e.g., "Size", and the name is used as a fallback because there are multiple files with the same size, the refreshItems signal that is received when a file's name is changed either with the dialog or outside the current view did not cause the view to be resorted after commit d70a4811807776966c3241a72121242f4d1eaee8. This patch fixes it. BUG: 324713 FIXED-IN: 4.11.2 REVIEW: 112561 --- src/kitemviews/kfileitemmodel.cpp | 86 +++++++++++++++++++------------ src/kitemviews/kfileitemmodel.h | 7 +++ src/tests/kfileitemmodeltest.cpp | 12 +++++ 3 files changed, 73 insertions(+), 32 deletions(-) diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index 7bbc1d17f..b6b6ee0e2 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -190,33 +190,7 @@ bool KFileItemModel::setData(int index, const QHash& value m_itemData[index]->item.setUrl(url); } - emit itemsChanged(KItemRangeList() << KItemRange(index, 1), changedRoles); - - // Trigger a resorting if the item's correct position has changed. Note - // that this can happen even if the sort role has not changed at all - // because the file name can be used as a fallback. - if (changedRoles.contains(sortRole()) || changedRoles.contains(roleForType(NameRole))) { - // Compare the changed item with its neighbors to see - // if an expensive resorting is needed at all. - const ItemData* changedItem = m_itemData.at(index); - const ItemData* previousItem = (index == 0) ? 0 : m_itemData.at(index - 1); - const ItemData* nextItem = (index == m_itemData.count() - 1) ? 0 : m_itemData.at(index + 1); - - if ((previousItem && lessThan(changedItem, previousItem)) - || (nextItem && lessThan(nextItem, changedItem))) { - m_resortAllItemsTimer->start(); - } else if (groupedSorting() && changedRoles.contains(sortRole())) { - // The position is still correct, but the groups might have changed - // if the changed item is either the first or the last item in a - // group. - // In principle, we could try to find out if the item really is the - // first or last one in its group and then update the groups - // (possibly with a delayed timer to make sure that we don't - // re-calculate the groups very often if items are updated one by - // one), but starting m_resortAllItemsTimer is easier. - m_resortAllItemsTimer->start(); - } - } + emitItemsChangedAndTriggerResorting(KItemRangeList() << KItemRange(index, 1), changedRoles); return true; } @@ -945,11 +919,7 @@ void KFileItemModel::slotRefreshItems(const QList >& itemRangeList.append(KItemRange(rangeIndex, rangeCount)); } - emit itemsChanged(itemRangeList, changedRoles); - - if (changedRoles.contains(sortRole())) { - m_resortAllItemsTimer->start(); - } + emitItemsChangedAndTriggerResorting(itemRangeList, changedRoles); } void KFileItemModel::slotClear() @@ -1229,6 +1199,58 @@ void KFileItemModel::removeExpandedItems() m_expandedDirs.clear(); } +void KFileItemModel::emitItemsChangedAndTriggerResorting(const KItemRangeList& itemRanges, const QSet& changedRoles) +{ + emit itemsChanged(itemRanges, changedRoles); + + // Trigger a resorting if necessary. Note that this can happen even if the sort + // role has not changed at all because the file name can be used as a fallback. + if (changedRoles.contains(sortRole()) || changedRoles.contains(roleForType(NameRole))) { + foreach (const KItemRange& range, itemRanges) { + bool needsResorting = false; + + const int first = range.index; + const int last = range.index + range.count - 1; + + // Resorting the model is necessary if + // (a) The first item in the range is "lessThan" its predecessor, + // (b) the successor of the last item is "lessThan" the last item, or + // (c) the internal order of the items in the range is incorrect. + if (first > 0 + && lessThan(m_itemData.at(first), m_itemData.at(first - 1))) { + needsResorting = true; + } else if (last < count() - 1 + && lessThan(m_itemData.at(last + 1), m_itemData.at(last))) { + needsResorting = true; + } else { + for (int index = first; index < last; ++index) { + if (lessThan(m_itemData.at(index + 1), m_itemData.at(index))) { + needsResorting = true; + break; + } + } + } + + if (needsResorting) { + m_resortAllItemsTimer->start(); + return; + } + } + } + + if (groupedSorting() && changedRoles.contains(sortRole())) { + // The position is still correct, but the groups might have changed + // if the changed item is either the first or the last item in a + // group. + // In principle, we could try to find out if the item really is the + // first or last one in its group and then update the groups + // (possibly with a delayed timer to make sure that we don't + // re-calculate the groups very often if items are updated one by + // one), but starting m_resortAllItemsTimer is easier. + m_resortAllItemsTimer->start(); + } +} + void KFileItemModel::resetRoles() { for (int i = 0; i < RolesCount; ++i) { diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h index 5917e6818..c87ee9736 100644 --- a/src/kitemviews/kfileitemmodel.h +++ b/src/kitemviews/kfileitemmodel.h @@ -321,6 +321,13 @@ private: void removeExpandedItems(); + /** + * This function is called by setData() and slotRefreshItems(). It emits + * the itemsChanged() signal, checks if the sort order is still correct, + * and starts m_resortAllItemsTimer if that is not the case. + */ + void emitItemsChangedAndTriggerResorting(const KItemRangeList& itemRanges, const QSet& changedRoles); + /** * Resets all values from m_requestRole to false. */ diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp index b0a27cd01..391fe5be5 100644 --- a/src/tests/kfileitemmodeltest.cpp +++ b/src/tests/kfileitemmodeltest.cpp @@ -391,6 +391,18 @@ void KFileItemModelTest::testResortAfterChangingName() QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsMoved(KItemRange,QList)), DefaultTimeout)); QCOMPARE(itemsInModel(), QStringList() << "b.txt" << "c.txt" << "d.txt"); + + // We rename d.txt back to a.txt using the dir lister's refreshItems() signal. + const KFileItem fileItemD = m_model->fileItem(2); + KFileItem fileItemA = fileItemD; + KUrl urlA = fileItemA.url(); + urlA.setFileName("a.txt"); + fileItemA.setUrl(urlA); + + m_model->slotRefreshItems(QList >() << qMakePair(fileItemD, fileItemA)); + + QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsMoved(KItemRange,QList)), DefaultTimeout)); + QCOMPARE(itemsInModel(), QStringList() << "a.txt" << "b.txt" << "c.txt"); } void KFileItemModelTest::testModelConsistencyWhenInsertingItems() -- 2.47.3