Bug 61442

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