kde/dolphin
kde/dolphin
1
0
Fork 0
mirror of https://invent.kde.org/system/dolphin synced 2024-07-15 18:01:11 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Re-organize the code that compares expanded items

Browse source 
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
This commit is contained in:
Frank Reininghaus 2013-02-10 18:09:07 +01:00
parent 45450959ea
commit dc6322dc09
3 changed files with 81 additions and 103 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

116
src/kitemviews/kfileitemmodel.cpp
View file

@ -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();

16
src/kitemviews/kfileitemmodel.h
View file

@ -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;

52
src/tests/kfileitemmodeltest.cpp
View file

@ -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