Re-organize the code that compares expanded items

The previous approach, which was based on comparing the URLs as
strings, was not only very complex, but also could lead to
inconsistencies in the model, namely, that not all children were
removed from the model when the dir lister reported the parent as
deleted. Later on, this could even lead to a crash.

BUG: 311947
FIXED-IN: 4.11
REVIEW: 108766

diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp
index d0fd3d77ca18181f6648144866924eb6d485427f..60fc27546bb435e5d73f7378be41334cd50eec49 100644 (file)
--- a/src/kitemviews/kfileitemmodel.cpp
+++ b/src/kitemviews/kfileitemmodel.cpp
@@ -1,5 +1,6 @@
 /***************************************************************************
  *   Copyright (C) 2011 by Peter Penz <peter.penz19@gmail.com>             *
+ *   Copyright (C) 2013 by Frank Reininghaus <frank78ac@googlemail.com>    *
  *                                                                         *
  *   This program is free software; you can redistribute it and/or modify  *
  *   it under the terms of the GNU General Public License as published by  *
@@ -1338,11 +1339,34 @@ bool KFileItemModel::lessThan(const ItemData* a, const ItemData* b) const
 {
     int result = 0;
 
-    if (m_expandedParentsCountRoot >= 0) {
-        result = expandedParentsCountCompare(a, b);
-        if (result != 0) {
-            // The items have parents with different expansion levels
-            return (sortOrder() == Qt::AscendingOrder) ? result < 0 : result > 0;
+    if (m_expandedParentsCountRoot >= 0 && a->parent != b->parent) {
+        const int expansionLevelA = a->values.value("expandedParentsCount").toInt();
+        const int expansionLevelB = b->values.value("expandedParentsCount").toInt();
+
+        // If b has a higher expansion level than a, check if a is a parent
+        // of b, and make sure that both expansion levels are equal otherwise.
+        for (int i = expansionLevelB; i > expansionLevelA; --i) {
+            if (b->parent == a) {
+                return true;
+            }
+            b = b->parent;
+        }
+
+        // If a has a higher expansion level than a, check if b is a parent
+        // of a, and make sure that both expansion levels are equal otherwise.
+        for (int i = expansionLevelA; i > expansionLevelB; --i) {
+            if (a->parent == b) {
+                return false;
+            }
+            a = a->parent;
+        }
+
+        Q_ASSERT(a->values.value("expandedParentsCount").toInt() == b->values.value("expandedParentsCount").toInt());
+
+        // Compare the last parents of a and b which are different.
+        while (a->parent != b->parent) {
+            a = a->parent;
+            b = b->parent;
         }
     }
 
@@ -1523,88 +1547,6 @@ int KFileItemModel::stringCompare(const QString& a, const QString& b) const
                             : QString::compare(a, b, Qt::CaseSensitive);
 }
 
-int KFileItemModel::expandedParentsCountCompare(const ItemData* a, const ItemData* b) const
-{
-    const KUrl urlA = a->item.url();
-    const KUrl urlB = b->item.url();
-    if (urlA.directory() == urlB.directory()) {
-        // Both items have the same directory as parent
-        return 0;
-    }
-
-    // Check whether one item is the parent of the other item
-    if (urlA.isParentOf(urlB)) {
-        return (sortOrder() == Qt::AscendingOrder) ? -1 : +1;
-    } else if (urlB.isParentOf(urlA)) {
-        return (sortOrder() == Qt::AscendingOrder) ? +1 : -1;
-    }
-
-    // Determine the maximum common path of both items and
-    // remember the index in 'index'
-    const QString pathA = urlA.path();
-    const QString pathB = urlB.path();
-
-    const int maxIndex = qMin(pathA.length(), pathB.length()) - 1;
-    int index = 0;
-    while (index <= maxIndex && pathA.at(index) == pathB.at(index)) {
-        ++index;
-    }
-    if (index > maxIndex) {
-        index = maxIndex;
-    }
-    while (index > 0 && (pathA.at(index) != QLatin1Char('/') || pathB.at(index) != QLatin1Char('/'))) {
-        --index;
-    }
-
-    // Determine the first sub-path after the common path and
-    // check whether it represents a directory or already a file
-    bool isDirA = true;
-    const QString subPathA = subPath(a->item, pathA, index, &isDirA);
-    bool isDirB = true;
-    const QString subPathB = subPath(b->item, pathB, index, &isDirB);
-
-    if (m_sortDirsFirst || m_sortRole == SizeRole) {
-        if (isDirA && !isDirB) {
-            return (sortOrder() == Qt::AscendingOrder) ? -1 : +1;
-        } else if (!isDirA && isDirB) {
-            return (sortOrder() == Qt::AscendingOrder) ? +1 : -1;
-        }
-    }
-
-    // Compare the items of the parents that represent the first
-    // different path after the common path.
-    const QString parentPathA = pathA.left(index) + subPathA;
-    const QString parentPathB = pathB.left(index) + subPathB;
-
-    const ItemData* parentA = a;
-    while (parentA && parentA->item.url().path() != parentPathA) {
-        parentA = parentA->parent;
-    }
-
-    const ItemData* parentB = b;
-    while (parentB && parentB->item.url().path() != parentPathB) {
-        parentB = parentB->parent;
-    }
-
-    if (parentA && parentB) {
-        return sortRoleCompare(parentA, parentB);
-    }
-
-    kWarning() << "Child items without parent detected:" << a->item.url() << b->item.url();
-    return QString::compare(urlA.url(), urlB.url(), Qt::CaseSensitive);
-}
-
-QString KFileItemModel::subPath(const KFileItem& item,
-                                const QString& itemPath,
-                                int start,
-                                bool* isDir) const
-{
-    Q_ASSERT(isDir);
-    const int pathIndex = itemPath.indexOf('/', start + 1);
-    *isDir = (pathIndex > 0) || item.isDir();
-    return itemPath.mid(start, pathIndex - start);
-}
-
 bool KFileItemModel::useMaximumUpdateInterval() const
 {
     return !m_dirLister->url().isLocalFile();

diff --git a/src/kitemviews/kfileitemmodel.h b/src/kitemviews/kfileitemmodel.h
index 903291a4c7f004307c542080d6d1348ce0955c85..a05d1f9d8d10b4970abfbffe494f668adadcd441 100644 (file)
--- a/src/kitemviews/kfileitemmodel.h
+++ b/src/kitemviews/kfileitemmodel.h
@@ -356,22 +356,6 @@ private:
 
     int stringCompare(const QString& a, const QString& b) const;
 
-    /**
-     * Compares the expansion level of both items. The "expansion level" is defined
-     * by the number of parent directories. However simply comparing just the numbers
-     * is not sufficient, it is also important to check the hierarchy for having
-     * a correct order like shown in a tree.
-     */
-    int expandedParentsCountCompare(const ItemData* a, const ItemData* b) const;
-
-    /**
-     * Helper method for expandedParentsCountCompare().
-     */
-    QString subPath(const KFileItem& item,
-                    const QString& itemPath,
-                    int start,
-                    bool* isDir) const;
-
     bool useMaximumUpdateInterval() const;
 
     QList<QPair<int, QVariant> > nameRoleGroups() const;

diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp
index 85a46488f51505d85b7882511fe6b716b249bac0..c9f8a3468460b462f7b0fde835e4626db863ecdc 100644 (file)
--- a/src/tests/kfileitemmodeltest.cpp
+++ b/src/tests/kfileitemmodeltest.cpp
@@ -21,6 +21,8 @@
 #include <qtest_kde.h>
 
 #include <KDirLister>
+#include <kio/job.h>
+
 #include "kitemviews/kfileitemmodel.h"
 #include "kitemviews/private/kfileitemmodeldirlister.h"
 #include "testdir.h"
@@ -71,6 +73,7 @@ private slots:
     void testItemRangeConsistencyWhenInsertingItems();
     void testExpandItems();
     void testExpandParentItems();
+    void testMakeExpandedItemHidden();
     void testSorting();
     void testIndexForKeyboardSearch();
     void testNameFilter();
@@ -569,6 +572,55 @@ void KFileItemModelTest::testExpandParentItems()
     QVERIFY(m_model->isConsistent());
 }
 
+/**
+ * Renaming an expanded folder by prepending its name with a dot makes it
+ * hidden. Verify that this does not cause an inconsistent model state and
+ * a crash later on, see https://bugs.kde.org/show_bug.cgi?id=311947
+ */
+void KFileItemModelTest::testMakeExpandedItemHidden()
+{
+    QSet<QByteArray> modelRoles = m_model->roles();
+    modelRoles << "isExpanded" << "isExpandable" << "expandedParentsCount";
+    m_model->setRoles(modelRoles);
+
+    QStringList files;
+    m_testDir->createFile("1a/2a/3a");
+    m_testDir->createFile("1a/2a/3b");
+    m_testDir->createFile("1a/2b");
+    m_testDir->createFile("1b");
+
+    m_model->loadDirectory(m_testDir->url());
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+
+    // So far, the model contains only "1a/" and "1b".
+    QCOMPARE(m_model->count(), 2);
+    m_model->setExpanded(0, true);
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+
+    // Now "1a/2a" and "1a/2b" have appeared.
+    QCOMPARE(m_model->count(), 4);
+    m_model->setExpanded(1, true);
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsInserted(KItemRangeList)), DefaultTimeout));
+    QCOMPARE(m_model->count(), 6);
+
+    // Rename "1a/2" and make it hidden.
+    const QString oldPath = m_model->fileItem(0).url().path() + "/2a";
+    const QString newPath = m_model->fileItem(0).url().path() + "/.2a";
+
+    KIO::SimpleJob* job = KIO::rename(oldPath, newPath, KIO::HideProgressInfo);
+    bool ok = job->exec();
+    QVERIFY(ok);
+    QVERIFY(QTest::kWaitForSignal(m_model, SIGNAL(itemsRemoved(KItemRangeList)), DefaultTimeout));
+
+    // "1a/2" and its subfolders have disappeared now.
+    QVERIFY(m_model->isConsistent());
+    QCOMPARE(m_model->count(), 3);
+
+    m_model->setExpanded(0, false);
+    QCOMPARE(m_model->count(), 2);
+
+}
+
 void KFileItemModelTest::testSorting()
 {
     // Create some files with different sizes and modification times to check the different sorting options