[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-commits
Subject: KDE/kdelibs/kio
From: David Faure <faure () kde ! org>
Date: 2010-10-08 19:28:38
Message-ID: 20101008192838.18577AC892 () svn ! kde ! org
[Download RAW message or body]
SVN commit 1183937 by dfaure:
Fix assertion "listers.isEmpty() || killed" (or "HUH? Lister ... is supposed to be \
listing" in debug mode) after a dirlister was put into the wrong list, in the \
following situation (happening at k3b startup) 1) A flat dirlister lists the contents \
of /a/b/, so this goes into the cache 2) A tree dirmodel for '/' tries to drill down \
to /a/b: it lists /a, then /a/b, finds items in the cache, creates a job to emit \
them, and then goes back to "Directory listing finished for /a, are we done with that \
dir?", but the code was not comparing urls, so because of the pending \
cached-item-job, it thought it was still listing that dir, and didn't mark it as \
done. Then the job for /a/b would be processed, but that didn't fix the status of \
/a. 3) And only when doing something else later on (e.g. deleting /a/foo, which \
triggers an update of /a), the inconsistency would show up and assert (lister marked \
as "still listing /a" but no associated job).
This fixes bug 193364, I'll close on backport.
M +14 -7 kio/kdirlister.cpp
M +2 -1 kio/kdirlister_p.h
M +69 -18 tests/kdirmodeltest.cpp
M +1 -0 tests/kdirmodeltest.h
--- trunk/KDE/kdelibs/kio/kio/kdirlister.cpp #1183936:1183937
@@ -235,6 +235,7 @@
emit lister->started(_url);
}
+ //kDebug(7004) << "Entry now being listed by" << \
dirData.listersCurrentlyListing; }
} else {
@@ -307,6 +308,7 @@
if (_emitCompleted && jobForUrl( urlStr ) == 0) {
Q_ASSERT(!dirData.listersCurrentlyHolding.contains(lister));
+ //kDebug(7004) << "Moving from listing to holding, because emitCompleted is \
true and no job" << lister << urlStr; dirData.listersCurrentlyHolding.append( lister \
); dirData.listersCurrentlyListing.removeAll( lister );
@@ -361,7 +363,7 @@
// lister is listing url
const QString url = dirit.key();
- //kDebug(7004) << " found lister in list - for " << url;
+ //kDebug(7004) << " found lister" << lister << "in list - for" << url;
stopLister(lister, url, dirData, silent);
stopped = true;
}
@@ -404,6 +406,7 @@
KDirListerCacheDirectoryData& dirData = dirit.value();
if ( dirData.listersCurrentlyListing.removeAll(lister) ) { // contains + \
removeAll in one go
+ //kDebug(7004) << " found lister" << lister << "in list - for" << urlStr;
stopLister(lister, urlStr, dirData, silent);
if ( lister->d->numJobs() == 0 ) {
@@ -412,6 +415,7 @@
if (!silent) {
emit lister->canceled();
}
+ //kDebug(7004) << "Entry now being listed by" << \
dirData.listersCurrentlyListing; }
}
}
@@ -587,6 +591,8 @@
QList<KDirLister *> listers = dirData.listersCurrentlyListing;
QList<KDirLister *> holders = dirData.listersCurrentlyHolding;
+ //kDebug(7004) << urlStr << "listers=" << listers << "holders=" << holders;
+
// restart the job for _dir if it is running already
bool killed = false;
QWidget *window = 0;
@@ -615,9 +621,7 @@
}
}
}
- //if (killed) {
// kDebug(7004) << "Killed=" << killed;
- //}
// we don't need to emit canceled signals since we only replaced the job,
// the listing is continuing.
@@ -1181,6 +1185,7 @@
Q_ASSERT(dit != directoryData.end());
KDirListerCacheDirectoryData& dirData = *dit;
if ( dirData.listersCurrentlyListing.isEmpty() ) {
+ kError() << "OOOOPS, nothing in directoryData.listersCurrentlyListing for" << \
jobUrlStr; // We're about to assert; dump the current state...
#ifndef NDEBUG
printDebug();
@@ -1193,7 +1198,7 @@
// the signals to make sure it exists in KDirListerCache in case someone
// calls listDir during the signal emission
Q_ASSERT( dirData.listersCurrentlyHolding.isEmpty() );
- dirData.moveListersWithoutCachedItemsJob();
+ dirData.moveListersWithoutCachedItemsJob(jobUrl);
if ( job->error() )
{
@@ -1578,7 +1583,7 @@
KDirListerCacheDirectoryData& dirData = directoryData[jobUrlStr];
// Collect the dirlisters which were listing the URL using that ListJob
// plus those that were already holding that URL - they all get updated.
- dirData.moveListersWithoutCachedItemsJob();
+ dirData.moveListersWithoutCachedItemsJob(jobUrl);
QList<KDirLister *> listers = dirData.listersCurrentlyHolding;
listers += dirData.listersCurrentlyListing;
@@ -2619,7 +2624,7 @@
emit m_parent->redirection( oldUrl, newUrl );
}
-void KDirListerCacheDirectoryData::moveListersWithoutCachedItemsJob()
+void KDirListerCacheDirectoryData::moveListersWithoutCachedItemsJob(const KUrl& url)
{
// Move dirlisters from listersCurrentlyListing to listersCurrentlyHolding,
// but not those that are still waiting on a CachedItemsJob...
@@ -2629,7 +2634,7 @@
QMutableListIterator<KDirLister *> lister_it(listersCurrentlyListing);
while (lister_it.hasNext()) {
KDirLister* kdl = lister_it.next();
- if (!kdl->d->m_cachedItemsJob) {
+ if (!kdl->d->m_cachedItemsJob || kdl->d->m_cachedItemsJob->url() != url) {
// OK, move this lister from "currently listing" to "currently holding".
// Huh? The KDirLister was present twice in listersCurrentlyListing, or \
was in both lists? @@ -2638,6 +2643,8 @@
listersCurrentlyHolding.append(kdl);
}
lister_it.remove();
+ } else {
+ //kDebug(7004) << "Not moving" << kdl << "to listersCurrentlyHolding \
because it still has job" << kdl->d->m_cachedItemsJob; }
}
}
--- trunk/KDE/kdelibs/kio/kio/kdirlister_p.h #1183936:1183937
@@ -30,6 +30,7 @@
#include <QtGui/QWidget>
#include <kurl.h>
+#include <kdebug.h>
#include <kio/global.h>
#include <kdirwatch.h>
@@ -450,7 +451,7 @@
// Listers that are currently holding this url
QList<KDirLister *> listersCurrentlyHolding;
- void moveListersWithoutCachedItemsJob();
+ void moveListersWithoutCachedItemsJob(const KUrl& url);
};
//const unsigned short KDirListerCache::MAX_JOBS_PER_LISTER = 5;
--- trunk/KDE/kdelibs/kio/tests/kdirmodeltest.cpp #1183936:1183937
@@ -25,6 +25,7 @@
#include "kdirmodeltest.moc"
#include <kdirmodel.h>
#include <kdirlister.h>
+//TODO #include "../../kdeui/tests/proxymodeltestsuite/modelspy.h"
#include <qtest_kde.h>
@@ -76,9 +77,13 @@
void KDirModelTest::recreateTestData()
{
- if (m_tempDir)
+ if (m_tempDir) {
kDebug() << "Deleting old tempdir" << m_tempDir->name();
delete m_tempDir;
+ qApp->processEvents(); // process inotify events so they don't pollute us \
later on + }
+
+
m_tempDir = new KTempDir;
kDebug() << "new tmp dir:" << m_tempDir->name();
// Create test data:
@@ -389,7 +394,11 @@
const QString file = m_tempDir->name() + "toplevelfile_2";
const KUrl url(file);
+#if 1
QSignalSpy spyDataChanged(m_dirModel, SIGNAL(dataChanged(QModelIndex, \
QModelIndex))); +#else
+ ModelSpy modelSpy(m_dirModel);
+#endif
connect( m_dirModel, SIGNAL(dataChanged(QModelIndex,QModelIndex)),
&m_eventLoop, SLOT(exitLoop()) );
@@ -405,10 +414,15 @@
enterLoop();
// If we come here, then dataChanged() was emitted - all good.
- QCOMPARE(spyDataChanged.count(), 1);
- QModelIndex receivedIndex = spyDataChanged[0][0].value<QModelIndex>();
+#if 0
+ QCOMPARE(modelSpy.count(), 1);
+ const QVariantList dataChanged = modelSpy.first();
+#else
+ const QVariantList dataChanged = spyDataChanged[0];
+#endif
+ QModelIndex receivedIndex = dataChanged[0].value<QModelIndex>();
COMPARE_INDEXES(receivedIndex, m_secondFileIndex);
- receivedIndex = spyDataChanged[0][1].value<QModelIndex>();
+ receivedIndex = dataChanged[1].value<QModelIndex>();
QCOMPARE(receivedIndex.row(), m_secondFileIndex.row()); // only compare row; \
column is count-1
disconnect( m_dirModel, SIGNAL(dataChanged(QModelIndex,QModelIndex)),
@@ -651,59 +665,78 @@
&m_eventLoop, SLOT(exitLoop()) );
}
+enum {
+ NoFlag = 0,
+ NewDir = 1, // whether to re-create a new KTempDir completely, to avoid cached \
fileitems + ListFinalDir = 2, // whether to list the target dir at the same time, \
like k3b, for #193364 + Recreate = 4
+ // flags, next item is 8!
+};
+
void KDirModelTest::testExpandToUrl_data()
{
- QTest::addColumn<bool>("newdir"); // whether to re-create a new KTempDir \
completely, to avoid cached fileitems + QTest::addColumn<int>("flags"); // see \
enum above QTest::addColumn<QString>("expandToPath"); // relative path
QTest::addColumn<QStringList>("expectedExpandSignals");
QTest::newRow("the root, nothing to do")
- << false << QString() << QStringList();
+ << int(NoFlag) << QString() << QStringList();
QTest::newRow(".")
- << false << "." << (QStringList());
+ << int(NoFlag) << "." << (QStringList());
QTest::newRow("subdir")
- << false << "subdir" << (QStringList()<<"subdir");
+ << int(NoFlag) << "subdir" << (QStringList()<<"subdir");
QTest::newRow("subdir/.")
- << false << "subdir/." << (QStringList()<<"subdir");
+ << int(NoFlag) << "subdir/." << (QStringList()<<"subdir");
const QString subsubdir = "subdir/subsubdir";
// Must list root, emit expand for subdir, list subdir, emit expand for \
subsubdir. QTest::newRow("subdir/subsubdir")
- << false << subsubdir << (QStringList()<<"subdir"<<subsubdir);
+ << int(NoFlag) << subsubdir << (QStringList()<<"subdir"<<subsubdir);
// Must list root, emit expand for subdir, list subdir, emit expand for \
subsubdir, list subsubdir. const QString subsubdirfile = subsubdir + "/testfile";
QTest::newRow("subdir/subsubdir/testfile sync")
- << false << subsubdirfile << \
(QStringList()<<"subdir"<<subsubdir<<subsubdirfile); + << int(NoFlag) << \
subsubdirfile << (QStringList()<<"subdir"<<subsubdir<<subsubdirfile);
#ifndef Q_WS_WIN
// Expand a symlink to a directory (#219547)
const QString dirlink = m_tempDir->name() + "dirlink";
createTestSymlink(dirlink, "/");
QTest::newRow("dirlink")
- << false << "dirlink/tmp" << (QStringList()<<"dirlink"<<"dirlink/tmp");
+ << int(NoFlag) << "dirlink/tmp" << \
(QStringList()<<"dirlink"<<"dirlink/tmp"); #endif
// Do a cold-cache test too, but nowadays it doesn't change anything anymore,
// apart from testing different code paths inside KDirLister.
QTest::newRow("subdir/subsubdir/testfile with reload")
- << true << subsubdirfile << \
(QStringList()<<"subdir"<<subsubdir<<subsubdirfile); + << int(NewDir) << \
subsubdirfile << (QStringList()<<"subdir"<<subsubdir<<subsubdirfile); +
+ QTest::newRow("hold dest dir") // #193364
+ << int(NewDir|ListFinalDir|Recreate) << subsubdirfile << \
(QStringList()<<"subdir"<<subsubdir<<subsubdirfile); +
+ // Make sure the last test has the Recreate option set, for the subsequent test \
methods. }
void KDirModelTest::testExpandToUrl()
{
- QFETCH(bool, newdir);
+ QFETCH(int, flags);
QFETCH(QString, expandToPath); // relative
QFETCH(QStringList, expectedExpandSignals);
- if (newdir) {
+ if (flags & NewDir) {
recreateTestData();
// WARNING! m_dirIndex, m_fileIndex, m_secondFileIndex etc. are not valid \
anymore after this point!
}
const QString path = m_tempDir->name();
- if (!m_dirModelForExpand || newdir) {
+ if (flags & ListFinalDir) {
+ // This way, the last listDir will find items in cache, and will schedule a \
CachedItemsJob + m_dirModel->dirLister()->openUrl(path + "subdir/subsubdir");
+ QTest::kWaitForSignal(m_dirModel->dirLister(), SIGNAL(completed()), 2000);
+ }
+
+ if (!m_dirModelForExpand || (flags & NewDir)) {
delete m_dirModelForExpand;
m_dirModelForExpand = new KDirModel;
connect(m_dirModelForExpand, SIGNAL(expand(QModelIndex)),
@@ -740,10 +773,16 @@
QVERIFY(m_dirModelForExpand->indexForUrl(m_urlToExpandTo).isValid());
}
- // recreateTestData was called -> fill again, for the next tests
- if (newdir)
+ if (flags & ListFinalDir) {
+ testUpdateParentAfterExpand();
+ }
+
+ if (flags & Recreate) {
+ // Clean up, for the next tests
+ recreateTestData();
fillModel(false);
}
+}
void KDirModelTest::slotExpand(const QModelIndex& index)
{
@@ -770,6 +809,18 @@
m_rowsInsertedEmitted = true;
}
+// This code is called by testExpandToUrl
+void KDirModelTest::testUpdateParentAfterExpand() // #193364
+{
+ const QString path = m_tempDir->name();
+ const QString file = path + "subdir/aNewFile";
+ kDebug() << "Creating" << file;
+ QVERIFY(!QFile::exists(file));
+ createTestFile(file);
+ //QSignalSpy spyRowsInserted(m_dirModelForExpand, \
SIGNAL(rowsInserted(QModelIndex,int,int))); + \
QTest::kWaitForSignal(m_dirModelForExpand, \
SIGNAL(rowsInserted(QModelIndex,int,int))); +}
+
void KDirModelTest::testFilter()
{
QVERIFY(m_dirIndex.isValid());
--- trunk/KDE/kdelibs/kio/tests/kdirmodeltest.h #1183936:1183937
@@ -84,6 +84,7 @@
void fillModel(bool reload, bool expectAllIndexes = true);
void collectKnownIndexes();
void testMoveDirectory(const QString& srcdir);
+ void testUpdateParentAfterExpand();
private:
#ifdef USE_QTESTEVENTLOOP
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic