Commit 51707e71 authored by David Faure's avatar David Faure
Browse files

Fix stop() killing the list job even if another dirlister needs it.

Regression introduced by me in bef0bd3e.
Symptom: "dolphin $HOME" showed up empty.

In the case of concurrent listings, I made the use of the cached items job
conditional (only created if there's anything to emit) so that we can join
the current listjob without killing it (updateDirectory) if it hasn't emitted
anything yet.
The unittest also uncovered inconsistencies in the emission of the cancelled
signal, now cacheditemsjob behaves like the listjob in this respect.

FIXED-IN: 4.6.2
BUG: 267709
parent 7bb4e523
......@@ -194,7 +194,7 @@ bool KDirListerCache::listDir( KDirLister *lister, const KUrl& _u,
// List items from the cache in a delayed manner, just like things would happen
// if we were not using the cache.
new KDirLister::Private::CachedItemsJob(lister, itemU->lstItems, itemU->rootItem, _url, _reload);
new KDirLister::Private::CachedItemsJob(lister, _url, _reload);
} else {
// dir not in cache or _reload is true
......@@ -260,8 +260,13 @@ bool KDirListerCache::listDir( KDirLister *lister, const KUrl& _u,
// List existing items in a delayed manner, just like things would happen
// if we were not using the cache.
//kDebug() << "Listing" << itemU->lstItems.count() << "cached items soon";
new KDirLister::Private::CachedItemsJob(lister, itemU->lstItems, itemU->rootItem, _url, _reload);
if (!itemU->lstItems.isEmpty()) {
kDebug() << "Listing" << itemU->lstItems.count() << "cached items soon";
new KDirLister::Private::CachedItemsJob(lister, _url, _reload);
} else {
// The other lister hasn't emitted anything yet. Good, we'll just listen to it.
// One problem could be if we have _reload=true and the existing job doesn't, though.
}
#ifdef DEBUG_CACHE
printDebug();
......@@ -280,11 +285,9 @@ KDirLister::Private::CachedItemsJob* KDirLister::Private::cachedItemsJobForUrl(c
return 0;
}
KDirLister::Private::CachedItemsJob::CachedItemsJob(KDirLister* lister, const KFileItemList& items,
const KFileItem& rootItem, const KUrl& url, bool reload)
KDirLister::Private::CachedItemsJob::CachedItemsJob(KDirLister* lister, const KUrl& url, bool reload)
: KJob(lister),
m_lister(lister), m_url(url),
m_items(items), m_rootItem(rootItem),
m_reload(reload), m_emitCompleted(true)
{
//kDebug() << "Creating CachedItemsJob" << this << "for lister" << lister << url;
......@@ -301,40 +304,70 @@ void KDirLister::Private::CachedItemsJob::done()
{
if (!m_lister) // job was already killed, but waiting deletion due to deleteLater
return;
kDirListerCache->emitItemsFromCache(this, m_lister, m_items, m_rootItem, m_url, m_reload, m_emitCompleted);
kDirListerCache->emitItemsFromCache(this, m_lister, m_url, m_reload, m_emitCompleted);
emitResult();
}
bool KDirLister::Private::CachedItemsJob::doKill()
{
//kDebug() << this;
kDirListerCache->emitItemsFromCache(this, m_lister, KFileItemList(), KFileItem(), m_url, false, false);
//kDebug(7004) << this;
kDirListerCache->forgetCachedItemsJob(this, m_lister, m_url);
if (!property("_kdlc_silent").toBool()) {
emit m_lister->canceled(m_url);
emit m_lister->canceled();
}
m_lister = 0;
return true;
}
void KDirListerCache::emitItemsFromCache(KDirLister::Private::CachedItemsJob* cachedItemsJob, KDirLister* lister, const KFileItemList& items, const KFileItem& rootItem, const KUrl& _url, bool _reload, bool _emitCompleted)
void KDirListerCache::emitItemsFromCache(KDirLister::Private::CachedItemsJob* cachedItemsJob, KDirLister* lister, const KUrl& _url, bool _reload, bool _emitCompleted)
{
const QString urlStr = _url.url();
DirItem *itemU = kDirListerCache->itemsInUse.value(urlStr);
Q_ASSERT(itemU); // hey we're listing that dir, so this can't be 0, right?
KDirLister::Private* kdl = lister->d;
kdl->complete = false;
if (kdl->rootFileItem.isNull() && !rootItem.isNull() && kdl->url == _url) {
kdl->rootFileItem = rootItem;
DirItem *itemU = kDirListerCache->itemsInUse.value(urlStr);
if (!itemU) {
kWarning(7004) << "Can't find item for directory" << urlStr << "anymore";
} else {
const KFileItemList items = itemU->lstItems;
const KFileItem rootItem = itemU->rootItem;
_reload = _reload || !itemU->complete;
if (kdl->rootFileItem.isNull() && !rootItem.isNull() && kdl->url == _url) {
kdl->rootFileItem = rootItem;
}
if (!items.isEmpty()) {
//kDebug(7004) << "emitting" << items.count() << "for lister" << lister;
kdl->addNewItems(_url, items);
kdl->emitItems();
}
}
if (!items.isEmpty()) {
//kDebug(7004) << "emitting" << items.count() << "for lister" << lister;
kdl->addNewItems(_url, items);
kdl->emitItems();
forgetCachedItemsJob(cachedItemsJob, lister, _url);
// Emit completed, unless we were told not to,
// or if listDir() was called while another directory listing for this dir was happening,
// so we "joined" it. We detect that using jobForUrl to ensure it's a real ListJob,
// not just a lister-specific CachedItemsJob (which wouldn't emit completed for us).
if (_emitCompleted) {
kdl->complete = true;
emit lister->completed( _url );
emit lister->completed();
if ( _reload ) {
updateDirectory( _url );
}
}
}
void KDirListerCache::forgetCachedItemsJob(KDirLister::Private::CachedItemsJob* cachedItemsJob, KDirLister* lister, const KUrl& _url)
{
// Modifications to data structures only below this point;
// so that addNewItems is called with a consistent state
const QString urlStr = _url.url();
lister->d->m_cachedItemsJobs.removeAll(cachedItemsJob);
KDirListerCacheDirectoryData& dirData = directoryData[urlStr];
......@@ -343,27 +376,12 @@ void KDirListerCache::emitItemsFromCache(KDirLister::Private::CachedItemsJob* ca
KIO::ListJob *listJob = jobForUrl(urlStr);
if (!listJob) {
Q_ASSERT(!dirData.listersCurrentlyHolding.contains(lister));
kDebug(7004) << "Moving from listing to holding, because no more job" << lister << urlStr;
//kDebug(7004) << "Moving from listing to holding, because no more job" << lister << urlStr;
dirData.listersCurrentlyHolding.append( lister );
dirData.listersCurrentlyListing.removeAll( lister );
} else {
//kDebug(7004) << "Still having a listjob" << listJob << ", so not moving to currently-holding.";
}
// Emit completed, unless we were told not to,
// or if listDir() was called while another directory listing for this dir was happening,
// so we "joined" it. We detect that using jobForUrl to ensure it's a real ListJob,
// not just a lister-specific CachedItemsJob (which wouldn't emit completed for us).
if (_emitCompleted) {
kdl->complete = true;
emit lister->completed( _url );
emit lister->completed();
if ( _reload || !itemU->complete ) {
updateDirectory( _url );
}
}
}
bool KDirListerCache::validUrl( const KDirLister *lister, const KUrl& url ) const
......@@ -396,19 +414,13 @@ void KDirListerCache::stop( KDirLister *lister, bool silent )
#ifdef DEBUG_CACHE
//printDebug();
#endif
//kDebug(7004) << "lister: " << lister;
//kDebug(7004) << "lister:" << lister << "silent=" << silent;
const KUrl::List urls = lister->d->lstDirs;
Q_FOREACH(const KUrl& url, urls) {
//kDebug() << "Stopping any listjob for" << url.url();
stopListJob(url.url(), silent);
stopListingUrl(lister, url, silent);
}
Q_FOREACH(KDirLister::Private::CachedItemsJob* job, lister->d->m_cachedItemsJobs) {
//kDebug() << "Killing cached items job";
job->kill(); // removes job from list, too
}
#if 0 // test code
QHash<QString,KDirListerCacheDirectoryData>::iterator dirit = directoryData.begin();
const QHash<QString,KDirListerCacheDirectoryData>::iterator dirend = directoryData.end();
......@@ -416,6 +428,7 @@ void KDirListerCache::stop( KDirLister *lister, bool silent )
KDirListerCacheDirectoryData& dirData = dirit.value();
if (dirData.listersCurrentlyListing.contains(lister)) {
kDebug(7004) << "ERROR: found lister" << lister << "in list - for" << dirit.key();
Q_ASSERT(false);
}
}
#endif
......@@ -429,6 +442,9 @@ void KDirListerCache::stopListingUrl(KDirLister *lister, const KUrl& _u, bool si
KDirLister::Private::CachedItemsJob* cachedItemsJob = lister->d->cachedItemsJobForUrl(url);
if (cachedItemsJob) {
if (silent) {
cachedItemsJob->setProperty("_kdlc_silent", true);
}
cachedItemsJob->kill(); // removes job from list, too
}
......@@ -440,9 +456,18 @@ void KDirListerCache::stopListingUrl(KDirLister *lister, const KUrl& _u, bool si
return;
KDirListerCacheDirectoryData& dirData = dirit.value();
if (dirData.listersCurrentlyListing.contains(lister)) {
//kDebug(7004) << " found lister" << lister << "in list - for" << urlStr;
stopListJob(urlStr, silent);
if (dirData.listersCurrentlyListing.count() == 1) {
// This was the only dirlister interested in the list job -> kill the job
stopListJob(urlStr, silent);
} else {
// Leave the job running for the other dirlisters, just unsubscribe us.
dirData.listersCurrentlyListing.removeAll(lister);
if (!silent) {
emit lister->canceled();
emit lister->canceled(url);
}
}
}
}
......@@ -460,9 +485,10 @@ void KDirListerCache::stopListJob(const QString& url, bool silent)
KIO::ListJob *job = jobForUrl(url);
if (job) {
//kDebug() << "Killing list job" << job;
if (silent)
//kDebug() << "Killing list job" << job << "for" << url;
if (silent) {
job->setProperty("_kdlc_silent", true);
}
job->kill(KJob::EmitResult);
}
}
......
......@@ -209,10 +209,12 @@ public:
KFileItem *findByUrl(const KDirLister *lister, const KUrl &url) const;
// Called by CachedItemsJob:
// Emits those items, for this lister and this url
// Emits the cached items, for this lister and this url
void emitItemsFromCache(KDirLister::Private::CachedItemsJob* job, KDirLister* lister,
const KFileItemList& lst, const KFileItem& rootItem,
const KUrl& _url, bool _reload, bool _emitCompleted);
// Called by CachedItemsJob:
void forgetCachedItemsJob(KDirLister::Private::CachedItemsJob* job, KDirLister* lister,
const KUrl& url);
public Q_SLOTS:
/**
......@@ -464,8 +466,7 @@ struct KDirListerCacheDirectoryData
class KDirLister::Private::CachedItemsJob : public KJob {
Q_OBJECT
public:
CachedItemsJob(KDirLister* lister, const KFileItemList& items, const KFileItem& rootItem,
const KUrl& url, bool reload);
CachedItemsJob(KDirLister* lister, const KUrl& url, bool reload);
/*reimp*/ void start() { QMetaObject::invokeMethod(this, "done", Qt::QueuedConnection); }
......@@ -483,8 +484,6 @@ public Q_SLOTS:
private:
KDirLister* m_lister;
KUrl m_url;
KFileItemList m_items;
KFileItem m_rootItem;
bool m_reload;
bool m_emitCompleted;
};
......
......@@ -678,12 +678,83 @@ void KDirListerTest::testConcurrentHoldingListing()
QCOMPARE(m_dirLister.spyClear.count(), 1);
QCOMPARE(m_dirLister.spyClearKUrl.count(), 0);
QVERIFY(dirLister2.isFinished());
disconnect(&dirLister2, 0, this, 0);
QVERIFY(m_dirLister.isFinished());
disconnect(&m_dirLister, 0, this, 0);
QCOMPARE(m_items.count(), origItemCount);
}
void KDirListerTest::testConcurrentListingAndStop()
{
m_items.clear();
m_items2.clear();
MyDirLister dirLister2;
// Use a new tempdir for this test, so that we don't use the cache at all.
KTempDir tempDir;
const QString path = tempDir.name();
createTestFile(path+"file_1");
createTestFile(path+"file_2");
createTestFile(path+"file_3");
connect(&m_dirLister, SIGNAL(newItems(KFileItemList)), this, SLOT(slotNewItems(KFileItemList)));
connect(&dirLister2, SIGNAL(newItems(KFileItemList)), this, SLOT(slotNewItems2(KFileItemList)));
// Before m_dirLister has time to emit the items, let's make dirLister2 call stop().
// This should not stop the list job for m_dirLister (#267709).
dirLister2.openUrl(KUrl(path), KDirLister::Reload);
m_dirLister.openUrl(KUrl(path)/*, KDirLister::Reload*/);
QCOMPARE(m_dirLister.spyStarted.count(), 1);
QCOMPARE(m_dirLister.spyCompleted.count(), 0);
QCOMPARE(m_dirLister.spyCompletedKUrl.count(), 0);
QCOMPARE(m_dirLister.spyCanceled.count(), 0);
QCOMPARE(m_dirLister.spyCanceledKUrl.count(), 0);
QCOMPARE(m_dirLister.spyClear.count(), 1);
QCOMPARE(m_dirLister.spyClearKUrl.count(), 0);
QCOMPARE(m_items.count(), 0);
QCOMPARE(dirLister2.spyStarted.count(), 1);
QCOMPARE(dirLister2.spyCompleted.count(), 0);
QCOMPARE(dirLister2.spyCompletedKUrl.count(), 0);
QCOMPARE(dirLister2.spyCanceled.count(), 0);
QCOMPARE(dirLister2.spyCanceledKUrl.count(), 0);
QCOMPARE(dirLister2.spyClear.count(), 1);
QCOMPARE(dirLister2.spyClearKUrl.count(), 0);
QCOMPARE(m_items2.count(), 0);
QVERIFY(!m_dirLister.isFinished());
QVERIFY(!dirLister2.isFinished());
dirLister2.stop();
QCOMPARE(dirLister2.spyStarted.count(), 1);
QCOMPARE(dirLister2.spyCompleted.count(), 0);
QCOMPARE(dirLister2.spyCompletedKUrl.count(), 0);
QCOMPARE(dirLister2.spyCanceled.count(), 1);
QCOMPARE(dirLister2.spyCanceledKUrl.count(), 1);
QCOMPARE(dirLister2.spyClear.count(), 1);
QCOMPARE(dirLister2.spyClearKUrl.count(), 0);
QCOMPARE(m_items2.count(), 0);
// then wait for completed
qDebug("waiting for completed");
connect(&m_dirLister, SIGNAL(completed()), this, SLOT(exitLoop()));
enterLoop();
QCOMPARE(m_items.count(), 3);
QCOMPARE(m_items2.count(), 0);
//QCOMPARE(m_dirLister.spyStarted.count(), 1); // 2 when in cache
QCOMPARE(m_dirLister.spyCompleted.count(), 1);
QCOMPARE(m_dirLister.spyCompletedKUrl.count(), 1);
QCOMPARE(m_dirLister.spyCanceled.count(), 0);
QCOMPARE(m_dirLister.spyCanceledKUrl.count(), 0);
QCOMPARE(m_dirLister.spyClear.count(), 1);
QCOMPARE(m_dirLister.spyClearKUrl.count(), 0);
disconnect(&m_dirLister, 0, this, 0);
}
void KDirListerTest::testDeleteListerEarly()
{
// Do the same again, it should behave the same, even with the items in the cache
......
......@@ -101,6 +101,7 @@ private Q_SLOTS:
void testRenameAndOverwrite();
void testConcurrentListing();
void testConcurrentHoldingListing();
void testConcurrentListingAndStop();
void testDeleteListerEarly();
void testOpenUrlTwice();
void testOpenUrlTwiceWithKeep();
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment