Version: 3.1.2 (using KDE KDE 3.1.2) Installed from: Gentoo Packages After choosing 'rename' from the context menu for a file, I renamed the file. Afterwards I wanted to undo this, but 'Edit' -> 'Undo Move' was not 'Undo Rename' like I expected. I understand that at the shell level, 'mv' is used for both actions, but since a file's context menu lists them seperately, the undo option should name them seperately.
This bug still exists in KDE 3.3 CVS.
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; };