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 523fb619d766c1ede5bab4f7af9cd363e3fefc97..0a89068b4b602945d9efef323275a33953b9d2c4 100644 (file)
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -51,9 +51,12 @@ KFileItemModel::KFileItemModel(KDirLister* dirLister, QObject* parent) :
     m_expandedUrls(),
     m_restoredExpandedUrls()
 {
     m_expandedUrls(),
     m_restoredExpandedUrls()
 {

+    // Apply default roles that should be determined

     resetRoles();
     m_requestRole[NameRole] = true;
     m_requestRole[IsDirRole] = true;
     resetRoles();
     m_requestRole[NameRole] = true;
     m_requestRole[IsDirRole] = true;

+    m_roles.insert("name");
+    m_roles.insert("isDir");

 
     Q_ASSERT(dirLister);
 
 
     Q_ASSERT(dirLister);
 


@@ -102,33 +105,109 @@ QHash<QByteArray, QVariant> KFileItemModel::data(int index) const
 
 bool KFileItemModel::setData(int index, const QHash<QByteArray, QVariant>& values)
 {
 
 bool KFileItemModel::setData(int index, const QHash<QByteArray, QVariant>& values)
 {

-    if (index >= 0 && index < count()) {
-        QHash<QByteArray, QVariant> currentValue = m_data.at(index);
-
-        QSet<QByteArray> changedRoles;
-        QHashIterator<QByteArray, QVariant> 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<QByteArray, QVariant> currentValue = m_data.at(index);
+
+    // Determine which roles have been changed
+    QSet<QByteArray> changedRoles;
+    QHashIterator<QByteArray, QVariant> 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 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<QByteArray, QVariant> 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)
 }
 
 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 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;
     case NoRole:             break;
     case IsDirRole:          break;
     case IsExpandedRole:     break;


@@ -245,6 +327,9 @@ QList<QPair<int, QVariant> > 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 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;
         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("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);
         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: {
     }
 
     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) {
         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;
     }
 
         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;
     }
     default:
         break;
     }


@@ -1417,6 +1519,28 @@ QList<QPair<int, QVariant> > KFileItemModel::permissionRoleGroups() const
     return groups;
 }
 
     return groups;
 }
 

+QList<QPair<int, QVariant> > KFileItemModel::ratingRoleGroups() const
+{
+    Q_ASSERT(!m_data.isEmpty());
+
+    const int maxIndex = count() - 1;
+    QList<QPair<int, QVariant> > 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<int, QVariant>(i, newGroupValue));
+        }
+    }
+
+    return groups;
+}
+

 QList<QPair<int, QVariant> > KFileItemModel::genericStringRoleGroups(const QByteArray& role) const
 {
     Q_ASSERT(!m_data.isEmpty());
 QList<QPair<int, QVariant> > KFileItemModel::genericStringRoleGroups(const QByteArray& role) const
 {
     Q_ASSERT(!m_data.isEmpty());

diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index e34503ebb270c41d8f3976cda0eeebd193d7bb42..8bf299003f37c984529eb2286349e14df059f90e 100644 (file)
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -160,6 +160,9 @@ private:
         TypeRole,
         DestinationRole,
         PathRole,
         TypeRole,
         DestinationRole,
         PathRole,

+        CommentRole,
+        TagsRole,
+        RatingRole,

         IsDirRole,
         IsExpandedRole,
         ExpansionLevelRole,
         IsDirRole,
         IsExpandedRole,
         ExpansionLevelRole,


@@ -201,6 +204,7 @@ private:
     QList<QPair<int, QVariant> > sizeRoleGroups() const;
     QList<QPair<int, QVariant> > dateRoleGroups() const;
     QList<QPair<int, QVariant> > permissionRoleGroups() const;
     QList<QPair<int, QVariant> > sizeRoleGroups() const;
     QList<QPair<int, QVariant> > dateRoleGroups() const;
     QList<QPair<int, QVariant> > permissionRoleGroups() const;

+    QList<QPair<int, QVariant> > ratingRoleGroups() const;

     QList<QPair<int, QVariant> > genericStringRoleGroups(const QByteArray& role) const;
 
     /**
     QList<QPair<int, QVariant> > genericStringRoleGroups(const QByteArray& role) const;
 
     /**

diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index 4f2f2f8ec8efb4895f4a8cbca787fd622ddb971e..2dbefc6b6f51eeb0666813d8b2eea1f2b71101e8 100644 (file)
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -24,6 +24,24 @@
 #include "kitemviews/kfileitemmodel.h"
 #include "testdir.h"
 
 #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;
 };
 namespace {
     const int DefaultTimeout = 5000;
 };


@@ -44,6 +62,9 @@ private slots:
     void testDefaultGroupedSorting();
     void testNewItems();
     void testRemoveItems();
     void testDefaultGroupedSorting();
     void testNewItems();
     void testRemoveItems();

+    void testSetData();
+    void testSetDataWithModifiedSortRole_data();
+    void testSetDataWithModifiedSortRole();

     void testModelConsistencyWhenInsertingItems();
     void testItemRangeConsistencyWhenInsertingItems();
     void testExpandItems();
     void testModelConsistencyWhenInsertingItems();
     void testItemRangeConsistencyWhenInsertingItems();
     void testExpandItems();


@@ -66,6 +87,10 @@ private:
 
 void KFileItemModelTest::init()
 {
 
 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>("KItemRange");
     qRegisterMetaType<KItemRangeList>("KItemRangeList");
     qRegisterMetaType<KFileItemList>("KFileItemList");
     qRegisterMetaType<KItemRange>("KItemRange");
     qRegisterMetaType<KItemRangeList>("KItemRangeList");
     qRegisterMetaType<KFileItemList>("KFileItemList");


@@ -145,9 +170,105 @@ void KFileItemModelTest::testRemoveItems()
     QCOMPARE(m_model->count(), 0);
 }
 
     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<QByteArray, QVariant> values;
+    values.insert("customRole1", "Test1");
+    values.insert("customRole2", "Test2");
+
+    QSignalSpy itemsChangedSpy(m_model, SIGNAL(itemsChanged(KItemRangeList,QSet<QByteArray>)));
+    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<int>("changedIndex");
+    QTest::addColumn<int>("changedRating");
+    QTest::addColumn<int>("ratingIndex0");
+    QTest::addColumn<int>("ratingIndex1");
+    QTest::addColumn<int>("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<QByteArray, QVariant> ratingA;
+    ratingA.insert("rating", 2);
+    m_model->setData(0, ratingA);
+
+    QHash<QByteArray, QVariant> ratingB;
+    ratingB.insert("rating", 4);
+    m_model->setData(1, ratingB);
+
+    QHash<QByteArray, QVariant> 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<QByteArray, QVariant> 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()
 {
 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
 
     // KFileItemModel prevents that inserting a punch of items sequentially
     // results in an itemsInserted()-signal for each item. Instead internally