From dc6322dc090bcaec40d75522debad1edfb25b27a Mon Sep 17 00:00:00 2001 From: Frank Reininghaus Date: Sun, 10 Feb 2013 18:09:07 +0100 Subject: [PATCH] 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 --- src/kitemviews/kfileitemmodel.cpp | 116 ++++++++---------------------- src/kitemviews/kfileitemmodel.h | 16 ----- src/tests/kfileitemmodeltest.cpp | 52 ++++++++++++++ 3 files changed, 81 insertions(+), 103 deletions(-) diff --git a/src/kitemviews/kfileitemmodel.cpp b/src/kitemviews/kfileitemmodel.cpp index d0fd3d77c..60fc27546 100644 --- a/src/kitemviews/kfileitemmodel.cpp +++ b/src/kitemviews/kfileitemmodel.cpp @@ -1,5 +1,6 @@ /*************************************************************************** * Copyright (C) 2011 by Peter Penz * + * Copyright (C) 2013 by Frank Reininghaus * * * * 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 903291a4c..a05d1f9d8 100644 --- 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 > nameRoleGroups() const; diff --git a/src/tests/kfileitemmodeltest.cpp b/src/tests/kfileitemmodeltest.cpp index 85a46488f..c9f8a3468 100644 --- a/src/tests/kfileitemmodeltest.cpp +++ b/src/tests/kfileitemmodeltest.cpp @@ -21,6 +21,8 @@ #include #include +#include + #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 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