Bug 99898

Elevating to grave.

SVN commit 632461 by dfaure:

It's just too easy to lose files when pressing Ctrl+z by mistake in konqueror or kdesktop, after copying a file.
E.g. when the file comes from a removable device, or has been removed meanwhile...
Let's ask for confirmation before Undo deletes any file.
BUG: 99898


 M  +13 -11    konq_operations.cc  
 M  +12 -3     konq_operations.h  
 M  +39 -12    konq_undo.cc  
 M  +9 -0      konq_undo.h  
 M  +26 -1     tests/konqundomanagertest.cpp  


--- trunk/KDE/kdebase/libkonq/konq_operations.cc #632460:632461
@@ -93,7 +93,7 @@
     }
 
     KonqOperations * op = new KonqOperations( parent );
-    int confirmation = DEFAULT_CONFIRMATION;
+    ConfirmationType confirmation = DEFAULT_CONFIRMATION;
     op->_del( method, selectedUrls, confirmation );
 }
 
@@ -171,7 +171,7 @@
         KonqUndoManager::self()->recordJob( method==MOVE?KonqUndoManager::MOVE:KonqUndoManager::LINK, selectedUrls, destUrl, job );
 }
 
-void KonqOperations::_del( Operation method, const KUrl::List & _selectedUrls, int confirmation )
+void KonqOperations::_del( Operation method, const KUrl::List & _selectedUrls, ConfirmationType confirmation )
 {
     KUrl::List selectedUrls;
     for (KUrl::List::ConstIterator it = _selectedUrls.begin(); it != _selectedUrls.end(); ++it)
@@ -182,11 +182,11 @@
         return;
     }
 
-    m_method = method;
-    if ( confirmation == SKIP_CONFIRMATION || askDeleteConfirmation( selectedUrls, confirmation ) )
+    if ( askDeleteConfirmation( selectedUrls, method, confirmation, parentWidget() ) )
     {
         //m_srcUrls = selectedUrls;
         KIO::Job *job;
+        m_method = method;
         switch( method )
         {
         case TRASH:
@@ -229,16 +229,18 @@
              SLOT( slotResult( KJob * ) ) );
 }
 
-bool KonqOperations::askDeleteConfirmation( const KUrl::List & selectedUrls, int confirmation )
+bool KonqOperations::askDeleteConfirmation( const KUrl::List & selectedUrls, int method, ConfirmationType confirmation, QWidget* widget )
 {
+    if ( confirmation == SKIP_CONFIRMATION )
+        return true;
     QString keyName;
     bool ask = ( confirmation == FORCE_CONFIRMATION );
     if ( !ask )
     {
         KConfig config("konquerorrc", true, false);
         config.setGroup( "Trash" );
-        keyName = ( m_method == DEL ? "ConfirmDelete" : "ConfirmTrash" );
-        bool defaultValue = ( m_method == DEL ? DEFAULT_CONFIRMDELETE : DEFAULT_CONFIRMTRASH );
+        keyName = ( method == DEL ? "ConfirmDelete" : "ConfirmTrash" );
+        bool defaultValue = ( method == DEL ? DEFAULT_CONFIRMDELETE : DEFAULT_CONFIRMTRASH );
         ask = config.readEntry( keyName, QVariant(defaultValue )).toBool();
     }
     if ( ask )
@@ -256,11 +258,11 @@
         }
 
         int result;
-        switch(m_method)
+        switch(method)
         {
         case DEL:
             result = KMessageBox::warningContinueCancelList(
-                parentWidget(),
+                widget,
              	i18np( "Do you really want to delete this item?", "Do you really want to delete these %n items?", prettyList.count()),
              	prettyList,
 		i18n( "Delete Files" ),
@@ -271,7 +273,7 @@
         case MOVE:
         default:
             result = KMessageBox::warningContinueCancelList(
-                parentWidget(),
+                widget,
                 i18np( "Do you really want to move this item to the trash?", "Do you really want to move these %n items to the trash?", prettyList.count()),
                 prettyList,
 		i18n( "Move to Trash" ),
@@ -492,7 +494,7 @@
         }
 
         m_method = TRASH;
-        if ( askDeleteConfirmation( mlst, DEFAULT_CONFIRMATION ) )
+        if ( askDeleteConfirmation( mlst, TRASH, DEFAULT_CONFIRMATION, parentWidget() ) )
             action = Qt::MoveAction;
         else
         {
--- trunk/KDE/kdebase/libkonq/konq_operations.h #632460:632461
@@ -132,15 +132,24 @@
      */
     static void newDir( QWidget * parent, const KUrl & baseUrl );
 
+    enum ConfirmationType { DEFAULT_CONFIRMATION, SKIP_CONFIRMATION, FORCE_CONFIRMATION };
+    /**
+     * Ask for confirmation before deleting/trashing @p selectedUrls.
+     * @param selectedUrls the urls about to be deleted
+     * @param method the type of deletion (DEL for real deletion, anything else for trash)
+     * @param confirmation default (based on config file), skip (no confirmation) or force (always confirm)
+     * @param widget parent widget for message boxes
+     * @return true if confirmed
+     */
+    static bool askDeleteConfirmation( const KUrl::List & selectedUrls, int method, ConfirmationType confirmation, QWidget* widget );
+
 Q_SIGNALS:
     void statFinished( const KFileItem * item );
     void aboutToCreate(const QPoint &pos, const QList<KIO::CopyInfo> &files);
 
 private:
     QWidget* parentWidget() const;
-    enum { DEFAULT_CONFIRMATION, SKIP_CONFIRMATION, FORCE_CONFIRMATION };
-    bool askDeleteConfirmation( const KUrl::List & selectedUrls, int confirmation );
-    void _del( Operation method, const KUrl::List & selectedUrls, int confirmation );
+    void _del( Operation method, const KUrl::List & selectedUrls, ConfirmationType confirmation );
     void _restoreTrashedItems( const KUrl::List& urls );
     void _statUrl( const KUrl & url, const QObject *receiver, const char *member );
 
--- trunk/KDE/kdebase/libkonq/konq_undo.cc #632460:632461
@@ -21,8 +21,9 @@
 #include "konq_undo.h"
 #include "konq_undo_p.h"
 #include "undomanageradaptor.h"
+#include "konq_operations.h"
 
-#include "kio/observer.h"
+#include <kio/observer.h>
 #include <kio/job.h>
 #include <kio/jobuidelegate.h>
 #include <QtDBus/QtDBus>
@@ -277,24 +278,40 @@
 {
     // Make a copy of the command to undo before broadcastPop() pops it.
     KonqCommand cmd = d->m_commands.top();
+    assert( cmd.m_valid );
+    d->m_current = cmd;
+
+    KonqBasicOperation::Stack& opStack = d->m_current.m_opStack;
+    assert( !opStack.isEmpty() );
+
+    // Let's first ask for confirmation if we need to delete any file (#99898)
+    KUrl::List fileCleanupStack;
+    QStack<KonqBasicOperation>::Iterator it = opStack.begin();
+    for ( ; it != opStack.end() ; ++it ) {
+        KonqBasicOperation::Type type = (*it).m_type;
+        if ( type == KonqBasicOperation::File && d->m_current.m_type == KonqUndoManager::COPY ) {
+            fileCleanupStack.append( (*it).m_dst );
+        }
+    }
+    if ( !fileCleanupStack.isEmpty() ) {
+        if ( !d->m_uiInterface->confirmDeletion( fileCleanupStack ) ) {
+            return;
+        }
+    }
+
     broadcastPop();
     broadcastLock();
 
-    assert( cmd.m_valid );
-
-    d->m_current = cmd;
     d->m_dirCleanupStack.clear();
     d->m_dirStack.clear();
     d->m_dirsToUpdate.clear();
 
     d->m_undoState = MOVINGFILES;
+
     // Let's have a look at the basic operations we need to undo.
     // While we're at it, collect all links that should be deleted.
 
-    KonqBasicOperation::Stack& opStack = d->m_current.m_opStack;
-    assert( !opStack.isEmpty() );
-
-    QStack<KonqBasicOperation>::Iterator it = opStack.begin();
+    it = opStack.begin();
     while ( it != opStack.end() ) // don't cache end() here, erase modifies it
     {
         bool removeBasicOperation = false;
@@ -396,9 +413,11 @@
     if ( d->m_undoState == REMOVINGDIRS )
         stepRemovingDirectories();
 
-    if ( d->m_currentJob )
+    if ( d->m_currentJob ) {
+        //TODO d->m_currentJob->setWindow(...);
         connect( d->m_currentJob, SIGNAL( result( KJob * ) ),
                  this, SLOT( slotResult( KJob * ) ) );
+    }
 }
 
 void KonqUndoManager::stepMakingDirectories()
@@ -658,9 +677,9 @@
     d->m_uiInterface = ui;
 }
 
-KonqUndoManager::UiInterface::UiInterface( QWidget* )
+KonqUndoManager::UiInterface::UiInterface( QWidget* w )
+    : m_parentWidget( w )
 {
-    // TODO store widget
 }
 
 void KonqUndoManager::UiInterface::jobError( KIO::Job* job )
@@ -675,7 +694,7 @@
     // Possible improvement: only show the time if date is today
     const QString timeStr = KGlobal::locale()->formatDateTime( destDt, true /*short*/ );
     return KMessageBox::warningContinueCancel(
-        0 /*TODO parent*/,
+        m_parentWidget,
         i18n( "The file %1 was copied from %2, but since then it has apparently been modified at %3.\n"
               "Undoing the copy will delete the file, and all modifications will be lost.\n"
               "Are you sure you want to delete %4?", dest.pathOrUrl(), src.pathOrUrl(), timeStr, dest.pathOrUrl() ),
@@ -685,5 +704,13 @@
         KMessageBox::Notify | KMessageBox::Dangerous ) == KMessageBox::Continue;
 }
 
+bool KonqUndoManager::UiInterface::confirmDeletion( const KUrl::List& files )
+{
+    // Because undo can happen with an accidental Ctrl-Z, we want to always confirm.
+    return KonqOperations::askDeleteConfirmation( files, KonqOperations::DEL,
+                                                  KonqOperations::FORCE_CONFIRMATION,
+                                                  m_parentWidget );
+}
+
 #include "konq_undo.moc"
 #include "konq_undo_p.moc"
--- trunk/KDE/kdebase/libkonq/konq_undo.h #632460:632461
@@ -63,10 +63,19 @@
     virtual void jobError( KIO::Job* job );
 
     /**
+     * Called when we are about to remove those files.
+     * Return true if we should proceed with deleting them.
+     */
+    virtual bool confirmDeletion( const KUrl::List& files );
+
+    /**
      * Called when dest was modified since it was copied from src.
+     * Note that this is called after confirmDeletion.
      * Return true if we should proceed with deleting dest.
      */
     virtual bool copiedFileWasModified( const KUrl& src, const KUrl& dest, time_t srcTime, time_t destTime );
+  private:
+    QWidget* m_parentWidget;
   };
 
   /**
--- trunk/KDE/kdebase/libkonq/tests/konqundomanagertest.cpp #632460:632461
@@ -118,7 +118,7 @@
 class TestUiInterface : public KonqUndoManager::UiInterface
 {
 public:
-    TestUiInterface() : KonqUndoManager::UiInterface(0) {}
+    TestUiInterface() : KonqUndoManager::UiInterface(0), m_nextReplyToConfirmDeletion(true) {}
     virtual void jobError( KIO::Job* job ) {
         kFatal() << job->errorString() << endl;
     }
@@ -129,9 +129,23 @@
         Q_UNUSED( destTime );
         return true;
     }
+    virtual bool confirmDeletion( const KUrl::List& files ) {
+        m_files = files;
+        return m_nextReplyToConfirmDeletion;
+    }
+    void setNextReplyToConfirmDeletion( bool b ) {
+        m_nextReplyToConfirmDeletion = b;
+    }
+    KUrl::List files() const { return m_files; }
     KUrl dest() const { return m_dest; }
+    void clear() {
+        m_dest = KUrl();
+        m_files.clear();
+    }
 private:
+    bool m_nextReplyToConfirmDeletion;
     KUrl m_dest;
+    KUrl::List m_files;
 };
 
 void KonqUndoManagerTest::initTestCase()
@@ -211,12 +225,23 @@
     QVERIFY( KonqUndoManager::self()->undoAvailable() );
     QCOMPARE( spyUndoAvailable.count(), 1 );
     QCOMPARE( spyTextChanged.count(), 1 );
+    m_uiInterface->clear();
 
+    m_uiInterface->setNextReplyToConfirmDeletion( false ); // act like the user didn't confirm
+    KonqUndoManager::self()->undo();
+    QCOMPARE( m_uiInterface->files().count(), 1 ); // confirmDeletion was called
+    QCOMPARE( m_uiInterface->files()[0].url(), KUrl(destFile()).url() );
+    QVERIFY( QFile::exists( destFile() ) ); // nothing happened yet
+
+    // OK, now do it
+    m_uiInterface->setNextReplyToConfirmDeletion( true );
     doUndo();
 
     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 );
+    QCOMPARE( m_uiInterface->files().count(), 1 ); // confirmDeletion was called
+    QCOMPARE( m_uiInterface->files()[0].url(), KUrl(destFile()).url() );
 
     // Check that undo worked
     QVERIFY( !QFile::exists( destFile() ) );

Backport to 3.x branch now (commit 632792)
Will be in kde-3.5.7.

Bug closed. Kdesktop is no more mantained.