[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-commits
Subject: KDE/kdebase/libkonq
From: David Faure <faure () kde ! org>
Date: 2007-01-20 10:37:26
Message-ID: 1169289446.375533.9353.nullmailer () svn ! kde ! org
[Download RAW message or body]
SVN commit 625536 by dfaure:
Added unit tests for undo after moving files and for copying a directory.
The latter crashed; found a nice porting bug in konq_undo:
QVector::iterator end = v.end(); while (it!=end) { if (...) { it = v.erase(it); } \
else ++it; } This is invalid because erasing elements changing the end, so in such a \
case we can't cache end().
Also fixed 61442: after renaming a file, undo says "Undo Rename" now instead of "Undo \
Move".
BUG: 61442
M +1 -1 konq_operations.cc
M +25 -12 konq_undo.cc
M +1 -1 konq_undo.h
M +6 -0 konq_undo_p.h
M +138 -34 tests/konqundomanagertest.cpp
M +6 -0 tests/konqundomanagertest.h
--- trunk/KDE/kdebase/libkonq/konq_operations.cc #625535:625536
@@ -628,7 +628,7 @@
KIO::Job * job = KIO::moveAs( oldurl, newurl, !oldurl.isLocalFile() );
KonqOperations * op = new KonqOperations( parent );
op->setOperation( job, MOVE, newurl );
- KonqUndoManager::self()->recordJob( KonqUndoManager::MOVE, lst, newurl, job );
+ KonqUndoManager::self()->recordJob( KonqUndoManager::RENAME, lst, newurl, job );
// if moving the desktop then update config file and emit
if ( oldurl.isLocalFile() && oldurl.path( KUrl::AddTrailingSlash ) == \
KGlobalSettings::desktopPath() ) {
--- trunk/KDE/kdebase/libkonq/konq_undo.cc #625535:625536
@@ -49,11 +49,18 @@
* copy files -> works
* move files -> works (TODO: optimize (change FileCopyJob to use the renamed arg \
for copyingDone)
*
- * copy files -> overwrite -> works
- * move files -> overwrite -> works
+ * copy files -> overwrite -> works (sorry for your overwritten file...)
+ * move files -> overwrite -> works (sorry for your overwritten file...)
*
* copy files -> rename -> works
* move files -> rename -> works
+ *
+ * -> see also konqundomanagertest, which aims at testing all the above.
+ *
+ * TODO: fix http://bugs.kde.org/show_bug.cgi?id=20532
+ * (Undoing a copy operation might delete a modified file causing loss of data)
+ * by storing+comparing modification time.
+ *
*/
class KonqUndoJob : public KIO::Job
@@ -246,6 +253,9 @@
return i18n( "Und&o: Link" );
else if ( t == KonqUndoManager::MOVE )
return i18n( "Und&o: Move" );
+ // distinguish renaming from moving - #61442
+ else if ( t == KonqUndoManager::RENAME )
+ return i18n( "Und&o: Rename" );
else if ( t == KonqUndoManager::TRASH )
return i18n( "Und&o: Trash" );
else if ( t == KonqUndoManager::MKDIR )
@@ -272,15 +282,18 @@
d->m_undoState = MOVINGFILES;
kDebug(1203) << "KonqUndoManager::undo MOVINGFILES" << endl;
- QStack<KonqBasicOperation>::Iterator it = d->m_current.m_opStack.begin();
- QStack<KonqBasicOperation>::Iterator end = d->m_current.m_opStack.end();
- while ( it != end )
+ KonqBasicOperation::Stack& opStack = d->m_current.m_opStack;
+ assert( !opStack.isEmpty() );
+
+ QStack<KonqBasicOperation>::Iterator it = opStack.begin();
+ while ( it != opStack.end() )
{
+ bool removeBasicOperation = false;
if ( (*it).m_directory && !(*it).m_renamed )
{
d->m_dirStack.push( (*it).m_src );
d->m_dirCleanupStack.prepend( (*it).m_dst );
- it = d->m_current.m_opStack.erase( it );
+ removeBasicOperation = true;
d->m_undoState = MAKINGDIRS;
kDebug(1203) << "KonqUndoManager::undo MAKINGDIRS" << endl;
}
@@ -289,11 +302,11 @@
if ( !d->m_fileCleanupStack.contains( (*it).m_dst ) )
d->m_fileCleanupStack.prepend( (*it).m_dst );
- if ( d->m_current.m_type != KonqUndoManager::MOVE )
- it = d->m_current.m_opStack.erase( it );
- else
- ++it;
+ removeBasicOperation = !d->m_current.isMoveCommand();
}
+
+ if ( removeBasicOperation )
+ it = opStack.erase( it );
else
++it;
}
@@ -313,7 +326,7 @@
}
*/
- if ( d->m_current.m_type != KonqUndoManager::MOVE )
+ if ( !d->m_current.isMoveCommand() )
d->m_dirStack.clear();
d->m_undoJob = new KonqUndoJob;
@@ -422,7 +435,7 @@
d->m_currentJob = KIO::file_delete( op.m_dst );
Observer::self()->slotDeleting( d->m_undoJob, op.m_dst );
}
- else if ( d->m_current.m_type == KonqUndoManager::MOVE
+ else if ( d->m_current.isMoveCommand()
|| d->m_current.m_type == KonqUndoManager::TRASH )
{
kDebug(1203) << "KonqUndoManager::undoStep file_move " << \
op.m_dst.prettyUrl() << " " << op.m_src.prettyUrl() << endl;
--- trunk/KDE/kdebase/libkonq/konq_undo.h #625535:625536
@@ -49,7 +49,7 @@
static void decRef();
static KonqUndoManager *self();
- enum CommandType { COPY, MOVE, LINK, MKDIR, TRASH };
+ enum CommandType { COPY, MOVE, RENAME, LINK, MKDIR, TRASH };
/**
* Record this job while it's happening and add a command for it so that the user \
can undo it.
--- trunk/KDE/kdebase/libkonq/konq_undo_p.h #625535:625536
@@ -42,6 +42,9 @@
// ### I considered inheriting this from QUndoCommand.
// ### but since it is being copied by value in the code, we can't use that.
+// Alternatively the data here should be contained into the QUndoCommand-subclass.
+// This way we could copy the data in the manager code.
+//
// ### also it would need to implement undo() itself (well, it can call the \
undomanager for it) class KonqCommand
{
@@ -61,6 +64,9 @@
//virtual void undo() {} // TODO
//virtual void redo() {} // TODO
+ // TODO: is ::TRASH missing?
+ bool isMoveCommand() const { return m_type == KonqUndoManager::MOVE || m_type == \
KonqUndoManager::RENAME; } +
bool m_valid;
KonqUndoManager::CommandType m_type;
--- trunk/KDE/kdebase/libkonq/tests/konqundomanagertest.cpp #625535:625536
@@ -30,17 +30,36 @@
QTEST_KDEMAIN( KonqUndomanagerTest, NoGUI )
-static QString homeTmpDir()
+static QString homeTmpDir() { return QFile::decodeName( getenv( "KDEHOME" ) ) + \
"/jobtest/"; } +static QString destDir() { return homeTmpDir() + "destdir/"; }
+
+static QString srcFile() { return homeTmpDir() + "testfile"; }
+static QString destFile() { return destDir() + "testfile"; }
+
+#ifndef Q_WS_WIN
+static QString srcLink() { return homeTmpDir() + "symlink"; }
+static QString destLink() { return destDir() + "symlink"; }
+#endif
+
+static QString srcSubDir() { return homeTmpDir() + "subdir"; }
+static QString destSubDir() { return destDir() + "subdir"; }
+
+static KUrl::List sourceList()
{
- return QFile::decodeName( getenv( "KDEHOME" ) ) + "/jobtest/";
+ KUrl::List lst;
+ lst << KUrl( srcFile() );
+#ifndef Q_WS_WIN
+ lst << KUrl( srcLink() );
+#endif
+ return lst;
}
-static void createTestFile( const QString& path )
+static void createTestFile( const QString& path, const char* contents )
{
QFile f( path );
if ( !f.open( QIODevice::WriteOnly ) )
kFatal() << "Can't create " << path << endl;
- f.write( QByteArray( "Hello world" ) );
+ f.write( QByteArray( contents ) );
f.close();
}
@@ -61,17 +80,32 @@
QVERIFY( QFileInfo( path ).isSymLink() );
}
+static void checkTestDirectory( const QString& path )
+{
+ QVERIFY( QFileInfo( path ).isDir() );
+ QVERIFY( QFileInfo( path + "/fileindir" ).isFile() );
+#ifndef Q_WS_WIN
+ QVERIFY( QFileInfo( path + "/testlink" ).isSymLink() );
+#endif
+ QVERIFY( QFileInfo( path + "/dirindir" ).isDir() );
+ QVERIFY( QFileInfo( path + "/dirindir/nested" ).isFile() );
+}
+
static void createTestDirectory( const QString& path )
{
QDir dir;
bool ok = dir.mkdir( path );
- if ( !ok && !dir.exists() )
+ if ( !ok )
kFatal() << "couldn't create " << path << endl;
- createTestFile( path + "/testfile" );
+ createTestFile( path + "/fileindir", "File in dir" );
#ifndef Q_WS_WIN
createTestSymlink( path + "/testlink" );
- QVERIFY( QFileInfo( path + "/testlink" ).isSymLink() );
#endif
+ ok = dir.mkdir( path + "/dirindir" );
+ if ( !ok )
+ kFatal() << "couldn't create " << path << endl;
+ createTestFile( path + "/dirindir/nested", "Nested" );
+ checkTestDirectory( path );
}
void KonqUndomanagerTest::initTestCase()
@@ -87,6 +121,15 @@
kFatal() << "Couldn't create " << homeTmpDir() << endl;
}
+ createTestFile( srcFile(), "Hello world" );
+#ifndef Q_WS_WIN
+ createTestSymlink( srcLink() );
+#endif
+ createTestDirectory( srcSubDir() );
+
+ QDir().mkdir( destDir() );
+ QVERIFY( QFileInfo( destDir() ).isDir() );
+
QVERIFY( !KonqUndoManager::self()->undoAvailable() );
}
@@ -95,25 +138,25 @@
KIO::NetAccess::del( KUrl::fromPath( homeTmpDir() ), 0 );
}
+void KonqUndomanagerTest::doUndo()
+{
+ bool ok = connect( KonqUndoManager::self(), SIGNAL( undoJobFinished() ),
+ &m_eventLoop, SLOT( quit() ) );
+ QVERIFY( ok );
+
+ KonqUndoManager::self()->undo();
+ m_eventLoop.exec(QEventLoop::ExcludeUserInputEvents); // wait for undo job to \
finish +}
+
void KonqUndomanagerTest::testCopyFiles()
{
kDebug() << k_funcinfo << endl;
- // See JobTest::copyFileToSamePartition()
- const QString filePath = homeTmpDir() + "fileFromHome";
- createTestFile( filePath );
-#ifndef Q_WS_WIN
- const QString linkPath = homeTmpDir() + "symlink";
- createTestSymlink( linkPath );
-#endif
- const QString destdir = homeTmpDir() + "destdir";
- QDir().mkdir( destdir );
- KUrl::List lst;
- lst << KUrl( filePath );
-#ifndef Q_WS_WIN
- lst << KUrl( linkPath );
-#endif
+ // Initially inspired from JobTest::copyFileToSamePartition()
+ const QString destdir = destDir();
+ KUrl::List lst = sourceList();
const KUrl d( destdir );
KIO::CopyJob* job = KIO::copy( lst, d, 0 );
+ job->setUiDelegate( 0 );
KonqUndoManager::self()->recordJob( KonqUndoManager::COPY, lst, d, job );
QSignalSpy spyUndoAvailable( KonqUndoManager::self(), \
SIGNAL(undoAvailable(bool)) ); @@ -123,35 +166,96 @@
bool ok = KIO::NetAccess::synchronousRun( job, 0 );
QVERIFY( ok );
- QVERIFY( QFile::exists( destdir + "/fileFromHome" ) );
+
+ QVERIFY( QFile::exists( destFile() ) );
#ifndef Q_WS_WIN
// Don't use QFile::exists, it's a broken symlink...
- QVERIFY( QFileInfo( destdir + "/symlink" ).isSymLink() );
+ QVERIFY( QFileInfo( destLink() ).isSymLink() );
#endif
// might have to wait for dbus signal here... but this is currently disabled.
//QTest::qWait( 20 );
-
QVERIFY( KonqUndoManager::self()->undoAvailable() );
QCOMPARE( spyUndoAvailable.count(), 1 );
QCOMPARE( spyTextChanged.count(), 1 );
- ok = connect( KonqUndoManager::self(), SIGNAL( undoJobFinished() ),
- &m_eventLoop, SLOT( quit() ) );
- QVERIFY( ok );
+ doUndo();
- // Now undo
- KonqUndoManager::self()->undo();
- m_eventLoop.exec(QEventLoop::ExcludeUserInputEvents); // wait for undo job to \
finish
-
QVERIFY( !KonqUndoManager::self()->undoAvailable() );
QVERIFY( spyUndoAvailable.count() >= 2 ); // it's in fact 3, due to lock/unlock \
emitting it as well QCOMPARE( spyTextChanged.count(), 2 );
// Check that undo worked
- QVERIFY( !QFile::exists( destdir + "/fileFromHome" ) );
+ QVERIFY( !QFile::exists( destFile() ) );
#ifndef Q_WS_WIN
- QVERIFY( !QFile::exists( destdir + "/symlink" ) );
- QVERIFY( !QFileInfo( destdir + "/symlink" ).isSymLink() );
+ QVERIFY( !QFile::exists( destLink() ) );
+ QVERIFY( !QFileInfo( destLink() ).isSymLink() );
#endif
}
+
+void KonqUndomanagerTest::testMoveFiles()
+{
+ kDebug() << k_funcinfo << endl;
+ const QString destdir = destDir();
+ KUrl::List lst = sourceList();
+ const KUrl d( destdir );
+ KIO::CopyJob* job = KIO::move( lst, d, 0 );
+ job->setUiDelegate( 0 );
+ KonqUndoManager::self()->recordJob( KonqUndoManager::MOVE, lst, d, job );
+
+ bool ok = KIO::NetAccess::synchronousRun( job, 0 );
+ QVERIFY( ok );
+
+ QVERIFY( !QFile::exists( srcFile() ) ); // the source moved
+ QVERIFY( QFile::exists( destFile() ) );
+#ifndef Q_WS_WIN
+ QVERIFY( !QFileInfo( srcLink() ).isSymLink() );
+ // Don't use QFile::exists, it's a broken symlink...
+ QVERIFY( QFileInfo( destLink() ).isSymLink() );
+#endif
+
+ doUndo();
+
+ QVERIFY( QFile::exists( srcFile() ) ); // the source is back
+ QVERIFY( !QFile::exists( destFile() ) );
+#ifndef Q_WS_WIN
+ QVERIFY( QFileInfo( srcLink() ).isSymLink() );
+ QVERIFY( !QFileInfo( destLink() ).isSymLink() );
+#endif
+}
+
+// Testing for overwrite isn't possible, because non-interactive jobs never \
overwrite. +// And nothing different happens anyway, the dest is removed...
+#if 0
+void KonqUndomanagerTest::testCopyFilesOverwrite()
+{
+ kDebug() << k_funcinfo << endl;
+ // Create a different file in the destdir
+ createTestFile( destFile(), "An old file already in the destdir" );
+
+ testCopyFiles();
+}
+#endif
+
+void KonqUndomanagerTest::testCopyDirectory()
+{
+ const QString destdir = destDir();
+ KUrl::List lst; lst << srcSubDir();
+ const KUrl d( destdir );
+ KIO::CopyJob* job = KIO::copy( lst, d, 0 );
+ job->setUiDelegate( 0 );
+ KonqUndoManager::self()->recordJob( KonqUndoManager::COPY, lst, d, job );
+
+ bool ok = KIO::NetAccess::synchronousRun( job, 0 );
+ QVERIFY( ok );
+
+ checkTestDirectory( srcSubDir() ); // src untouched
+ checkTestDirectory( destSubDir() );
+
+ doUndo();
+
+ checkTestDirectory( srcSubDir() );
+ QVERIFY( !QFile::exists( destSubDir() ) );
+}
+
+// TODO: add test for undoing after a partial move \
(http://bugs.kde.org/show_bug.cgi?id=91579)
--- trunk/KDE/kdebase/libkonq/tests/konqundomanagertest.h #625535:625536
@@ -29,7 +29,13 @@
void initTestCase();
void cleanupTestCase();
void testCopyFiles();
+ void testMoveFiles();
+ //void testCopyFilesOverwrite();
+ void testCopyDirectory();
+ // TODO testTrashFiles
+
private:
+ void doUndo();
QEventLoop m_eventLoop;
};
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic