From: Peter Penz Date: Sun, 30 Oct 2011 19:57:50 +0000 (+0100) Subject: Fix sorting-issues when value of a sort-role has been changed X-Git-Url: https://cloud.milkyroute.net/gitweb/dolphin.git/commitdiff_plain/c872dcbda9b0fbcd945a7917ae9e4b3cb61f347c Fix sorting-issues when value of a sort-role has been changed If the value of a sort-role has been changed, emitting the signal itemsChanged() is not sufficient as from the KItemModelBase point of view an item has been moved or deleted/reinserted. Corresponding to the unit-test KFileItemModel::setData() respects this case correctly now, however there are some minor visual animation issues left that (hopefully) should not be tricky to solve. --- diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index 523fb619d..0a89068b4 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -51,9 +51,12 @@ KFileItemModel::KFileItemModel(KDirLister* dirLister, QObject* parent) : m_expandedUrls(), m_restoredExpandedUrls() { + // Apply default roles that should be determined resetRoles(); m_requestRole[NameRole] = true; m_requestRole[IsDirRole] = true; + m_roles.insert("name"); + m_roles.insert("isDir"); Q_ASSERT(dirLister); @@ -102,33 +105,109 @@ QHash KFileItemModel::data(int index) const bool KFileItemModel::setData(int index, const QHash& values) { - if (index >= 0 && index < count()) { - QHash currentValue = m_data.at(index); - - QSet changedRoles; - QHashIterator it(values); - while (it.hasNext()) { - it.next(); - const QByteArray role = it.key(); - const QVariant value = it.value(); - - if (currentValue[role] != value) { - currentValue[role] = value; - changedRoles.insert(role); - } - } + if (index < 0 || index >= count()) { + return false; + } - if (!changedRoles.isEmpty()) { - if (groupedSorting() && changedRoles.contains(sortRole())) { - m_groups.clear(); - } - m_data[index] = currentValue; - emit itemsChanged(KItemRangeList() << KItemRange(index, 1), changedRoles); + QHash currentValue = m_data.at(index); + + // Determine which roles have been changed + QSet changedRoles; + QHashIterator it(values); + while (it.hasNext()) { + it.next(); + const QByteArray role = it.key(); + const QVariant value = it.value(); + + if (currentValue[role] != value) { + currentValue[role] = value; + changedRoles.insert(role); } + } + + if (changedRoles.isEmpty()) { + return false; + } + + m_data[index] = currentValue; + if (!changedRoles.contains(sortRole())) { + emit itemsChanged(KItemRangeList() << KItemRange(index, 1), changedRoles); return true; } - return false; + + // The sort role has been changed which might result in a changed + // item index. In this case instead of emitting the itemsChanged() + // signal the following is done: + // 1. The item gets removed from the current position and the signal + // itemsRemoved() will be emitted. + // 2. The item gets inserted to the new position and the signal + // itemsInserted() will be emitted. + + const KFileItem& changedItem = m_sortedItems.at(index); + const bool sortOrderDecreased = (index > 0 && lessThan(changedItem, m_sortedItems.at(index - 1))); + const bool sortOrderIncreased = !sortOrderDecreased && + (index < count() - 1 && lessThan(m_sortedItems.at(index + 1), changedItem)); + + if (!sortOrderDecreased && !sortOrderIncreased) { + // Although the value of the sort-role has been changed it did not result + // into a changed position. + emit itemsChanged(KItemRangeList() << KItemRange(index, 1), changedRoles); + return true; + } + + m_groups.clear(); + + if (!m_pendingItemsToInsert.isEmpty()) { + insertItems(m_pendingItemsToInsert); + m_pendingItemsToInsert.clear(); + } + + // Do a binary search to find the new position where the item + // should get inserted. The result will be stored in 'mid'. + int min = sortOrderIncreased ? index + 1 : 0; + int max = sortOrderDecreased ? index - 1 : count() - 1; + int mid = 0; + do { + mid = (min + max) / 2; + if (lessThan(m_sortedItems.at(mid), changedItem)) { + min = mid + 1; + } else { + max = mid - 1; + } + } while (min <= max); + + if (sortOrderIncreased && mid == max && lessThan(m_sortedItems.at(max), changedItem)) { + ++mid; + } + + // Remove the item from the old position + const KFileItem removedItem = changedItem; + const QHash removedData = m_data[index]; + + m_items.remove(changedItem.url()); + m_sortedItems.removeAt(index); + m_data.removeAt(index); + for (int i = 0; i < m_sortedItems.count(); ++i) { + m_items.insert(m_sortedItems.at(i).url(), i); + } + + emit itemsRemoved(KItemRangeList() << KItemRange(index, 1)); + + // Insert the item to the new position + if (sortOrderIncreased) { + --mid; + } + + m_sortedItems.insert(mid, removedItem); + m_data.insert(mid, removedData); + for (int i = 0; i < m_sortedItems.count(); ++i) { + m_items.insert(m_sortedItems.at(i).url(), i); + } + + emit itemsInserted(KItemRangeList() << KItemRange(mid, 1)); + + return true; } void KFileItemModel::setSortFoldersFirst(bool foldersFirst) @@ -218,6 +297,9 @@ QString KFileItemModel::roleDescription(const QByteArray& role) const case TypeRole: descr = i18nc("@item:intable", "Type"); break; case DestinationRole: descr = i18nc("@item:intable", "Destination"); break; case PathRole: descr = i18nc("@item:intable", "Path"); break; + case CommentRole: descr = i18nc("@item:intable", "Comment"); break; + case TagsRole: descr = i18nc("@item:intable", "Tags"); break; + case RatingRole: descr = i18nc("@item:intable", "Rating"); break; case NoRole: break; case IsDirRole: break; case IsExpandedRole: break; @@ -245,6 +327,9 @@ QList > KFileItemModel::groups() const case TypeRole: m_groups = genericStringRoleGroups("type"); break; case DestinationRole: m_groups = genericStringRoleGroups("destination"); break; case PathRole: m_groups = genericStringRoleGroups("path"); break; + case CommentRole: m_groups = genericStringRoleGroups("comment"); break; + case TagsRole: m_groups = genericStringRoleGroups("tags"); break; + case RatingRole: m_groups = ratingRoleGroups(); break; case NoRole: break; case IsDirRole: break; case IsExpandedRole: break; @@ -831,6 +916,9 @@ KFileItemModel::Role KFileItemModel::roleIndex(const QByteArray& role) const rolesHash.insert("type", TypeRole); rolesHash.insert("destination", DestinationRole); rolesHash.insert("path", PathRole); + rolesHash.insert("comment", CommentRole); + rolesHash.insert("tags", TagsRole); + rolesHash.insert("rating", RatingRole); rolesHash.insert("isDir", IsDirRole); rolesHash.insert("isExpanded", IsExpandedRole); rolesHash.insert("expansionLevel", ExpansionLevelRole); @@ -968,9 +1056,6 @@ bool KFileItemModel::lessThan(const KFileItem& a, const KFileItem& b) const } case SizeRole: { - // TODO: Implement sorting folders by the number of items inside. - // This is more tricky to get right because this number is retrieved - // asynchronously by KFileItemModelRolesUpdater. const KIO::filesize_t sizeA = a.size(); const KIO::filesize_t sizeB = b.size(); if (sizeA < sizeB) { @@ -981,6 +1066,23 @@ bool KFileItemModel::lessThan(const KFileItem& a, const KFileItem& b) const break; } + case TypeRole: { + // Only compare the type if the MIME-type is known for performance reasons. + // If the MIME-type is unknown it will be resolved later by KFileItemModelRolesUpdater + // and a resorting will be triggered. + if (a.isMimeTypeKnown() && b.isMimeTypeKnown()) { + result = QString::compare(a.mimeComment(), b.mimeComment()); + } + break; + } + + case RatingRole: { + const int indexA = m_items.value(a.url(), -1); + const int indexB = m_items.value(b.url(), -1); + result = m_data.value(indexA).value("rating").toInt() - m_data.value(indexB).value("rating").toInt(); + break; + } + default: break; } @@ -1417,6 +1519,28 @@ QList > KFileItemModel::permissionRoleGroups() const return groups; } +QList > KFileItemModel::ratingRoleGroups() const +{ + Q_ASSERT(!m_data.isEmpty()); + + const int maxIndex = count() - 1; + QList > groups; + + int groupValue; + for (int i = 0; i <= maxIndex; ++i) { + if (isChildItem(i)) { + continue; + } + const int newGroupValue = m_data.at(i).value("rating").toInt(); + if (newGroupValue != groupValue) { + groupValue = newGroupValue; + groups.append(QPair(i, newGroupValue)); + } + } + + return groups; +} + QList > KFileItemModel::genericStringRoleGroups(const QByteArray& role) const { Q_ASSERT(!m_data.isEmpty()); diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h index e34503ebb..8bf299003 100644 --- a/src/kitemviews/kfileitemmodel.h +++ b/src/kitemviews/kfileitemmodel.h @@ -160,6 +160,9 @@ private: TypeRole, DestinationRole, PathRole, + CommentRole, + TagsRole, + RatingRole, IsDirRole, IsExpandedRole, ExpansionLevelRole, @@ -201,6 +204,7 @@ private: QList > sizeRoleGroups() const; QList > dateRoleGroups() const; QList > permissionRoleGroups() const; + QList > ratingRoleGroups() const; QList > genericStringRoleGroups(const QByteArray& role) const; /** diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp index 4f2f2f8ec..2dbefc6b6 100644 --- a/src/tests/kfileitemmodeltest.cpp +++ b/src/tests/kfileitemmodeltest.cpp @@ -24,6 +24,24 @@ #include "kitemviews/kfileitemmodel.h" #include "testdir.h" +void myMessageOutput(QtMsgType type, const char* msg) +{ + switch (type) { + case QtDebugMsg: + break; + case QtWarningMsg: + break; + case QtCriticalMsg: + fprintf(stderr, "Critical: %s\n", msg); + break; + case QtFatalMsg: + fprintf(stderr, "Fatal: %s\n", msg); + abort(); + default: + break; + } + } + namespace { const int DefaultTimeout = 5000; }; @@ -44,6 +62,9 @@ private slots: void testDefaultGroupedSorting(); void testNewItems(); void testRemoveItems(); + void testSetData(); + void testSetDataWithModifiedSortRole_data(); + void testSetDataWithModifiedSortRole(); void testModelConsistencyWhenInsertingItems(); void testItemRangeConsistencyWhenInsertingItems(); void testExpandItems(); @@ -66,6 +87,10 @@ private: void KFileItemModelTest::init() { + // The item-model tests result in a huge number of debugging + // output from kdelibs. Only show critical and fatal messages. + qInstallMsgHandler(myMessageOutput); + qRegisterMetaType("KItemRange"); qRegisterMetaType("KItemRangeList"); qRegisterMetaType("KFileItemList"); @@ -145,9 +170,105 @@ void KFileItemModelTest::testRemoveItems() QCOMPARE(m_model->count(), 0); } +void KFileItemModelTest::testSetData() +{ + m_testDir->createFile("a.txt"); + + m_dirLister->openUrl(m_testDir->url()); + QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout)); + + QHash values; + values.insert("customRole1", "Test1"); + values.insert("customRole2", "Test2"); + + QSignalSpy itemsChangedSpy(m_model, SIGNAL(itemsChanged(KItemRangeList,QSet))); + m_model->setData(0, values); + QCOMPARE(itemsChangedSpy.count(), 1); + + values = m_model->data(0); + QCOMPARE(values.value("customRole1").toString(), QString("Test1")); + QCOMPARE(values.value("customRole2").toString(), QString("Test2")); + QVERIFY(isModelConsistent()); +} + +void KFileItemModelTest::testSetDataWithModifiedSortRole_data() +{ + QTest::addColumn("changedIndex"); + QTest::addColumn("changedRating"); + QTest::addColumn("ratingIndex0"); + QTest::addColumn("ratingIndex1"); + QTest::addColumn("ratingIndex2"); + + // Default setup: + // Index 0 = rating 2 + // Index 1 = rating 4 + // Index 2 = rating 6 + + QTest::newRow("Index 0: Rating 3") << 0 << 3 << 3 << 4 << 6; + QTest::newRow("Index 0: Rating 5") << 0 << 5 << 4 << 5 << 6; + QTest::newRow("Index 0: Rating 8") << 0 << 8 << 4 << 6 << 8; + + QTest::newRow("Index 2: Rating 1") << 2 << 1 << 1 << 2 << 4; + QTest::newRow("Index 2: Rating 3") << 2 << 3 << 2 << 3 << 4; + QTest::newRow("Index 2: Rating 5") << 2 << 5 << 2 << 4 << 5; +} + +void KFileItemModelTest::testSetDataWithModifiedSortRole() +{ + QFETCH(int, changedIndex); + QFETCH(int, changedRating); + QFETCH(int, ratingIndex0); + QFETCH(int, ratingIndex1); + QFETCH(int, ratingIndex2); + + // Changing the value of a sort-role must result in + // a reordering of the items. + QCOMPARE(m_model->sortRole(), QByteArray("name")); + + QStringList files; + files << "a.txt" << "b.txt" << "c.txt"; + m_testDir->createFiles(files); + + m_dirLister->openUrl(m_testDir->url()); + QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout)); + + // Fill the "rating" role of each file: + // a.txt -> 2 + // b.txt -> 4 + // c.txt -> 6 + + QHash ratingA; + ratingA.insert("rating", 2); + m_model->setData(0, ratingA); + + QHash ratingB; + ratingB.insert("rating", 4); + m_model->setData(1, ratingB); + + QHash ratingC; + ratingC.insert("rating", 6); + m_model->setData(2, ratingC); + + m_model->setSortRole("rating"); + QCOMPARE(m_model->data(0).value("rating").toInt(), 2); + QCOMPARE(m_model->data(1).value("rating").toInt(), 4); + QCOMPARE(m_model->data(2).value("rating").toInt(), 6); + + // Now change the rating from a.txt. This usually results + // in reordering of the items. + QHash rating; + rating.insert("rating", changedRating); + m_model->setData(changedIndex, rating); + + QCOMPARE(m_model->data(0).value("rating").toInt(), ratingIndex0); + QCOMPARE(m_model->data(1).value("rating").toInt(), ratingIndex1); + QCOMPARE(m_model->data(2).value("rating").toInt(), ratingIndex2); + QVERIFY(isModelConsistent()); +} + void KFileItemModelTest::testModelConsistencyWhenInsertingItems() { - QSKIP("Temporary disabled", SkipSingle); + //QSKIP("Temporary disabled", SkipSingle); // KFileItemModel prevents that inserting a punch of items sequentially // results in an itemsInserted()-signal for each item. Instead internally