Improved error handling during dimap sync: don't always abort the whole thing.

Give the user the choice, for non-critical errors.
Currently only done for get/delete/expunge/put, more code should be ported
to handleJobError().
Added error() and signal result() to FolderJob to allow KMFolderCachedImap
to skip the expunge step if deleting failed.
Earlier versions of this patch were posted to kmail-devel, but Till told me
to go ahead :-)

svn path=/trunk/kdepim/; revision=305206

cachedimapjob.cpp

@@ -193,11 +193,13 @@ void CachedImapJob::slotDeleteResult( KIO::Job * job )
     delete this;
     return;
   }
-  mAccount->removeJob(it);
 
-  if (job->error())
-    mAccount->slotSlaveError( mAccount->slave(), job->error(),
-                              job->errorText() );
+  if (job->error()) {
+    mErrorCode = job->error();
+    mAccount->handleJobError( mErrorCode, job->errorText(), job, i18n( "Error while deleting messages on the server: " ) + '\n' );
+  }
+  else
+    mAccount->removeJob(it);
 
   delete this;
 }
@@ -212,9 +214,8 @@ void CachedImapJob::slotGetNextMessage(KIO::Job * job)
     }
 
     if (job->error()) {
-      mAccount->removeJob(it);
-      mAccount->slotSlaveError( mAccount->slave(), job->error(),
-                                job->errorText() );
+      mErrorCode = job->error();
+      mAccount->handleJobError( mErrorCode, job->errorText(), job, i18n( "Error while retrieving message on the server: " ) + '\n' );
       delete this;
       return;
     }
@@ -389,12 +390,10 @@ void CachedImapJob::slotPutMessageResult(KIO::Job *job)
   }
 
   if ( job->error() ) {
-    QStringList errors = job->detailedErrorStrings();
-    QString myError = "<qt><p><b>" + i18n("Error while uploading message")
+    QString myError = "<p><b>" + i18n("Error while uploading message")
       + "</b></p><p>" + i18n("Could not upload the message %1 on the server from folder %2 with URL %3.").arg((*it).items[0]).arg(mFolder->name()).arg((*it).htmlURL())
       + "</p><p>" + i18n("This could be because you do not have permission to do this; the error message from the server communication is here:") + "</p>";
-    KMessageBox::error( 0, myError + errors[1] + '\n' + errors[2], errors[0] );
-    mAccount->removeJob(it);
+    mAccount->handleJobError( job->error(), job->errorText(), job, myError );
     delete this;
     return;
   }

folderjob.cpp

@@ -42,6 +42,7 @@ namespace KMail {
 FolderJob::FolderJob( KMMessage *msg, JobType jt, KMFolder* folder,
                           QString partSpecifier )
   : mType( jt ), mSrcFolder( 0 ), mDestFolder( folder ), mPartSpecifier( partSpecifier ),
+    mErrorCode( 0 ),
     mPassiveDestructor( false ), mStarted( false )
 {
   if ( msg ) {
@@ -55,13 +56,16 @@ FolderJob::FolderJob( const QPtrList<KMMessage>& msgList, const QString& sets,
                           JobType jt, KMFolder *folder )
   : mMsgList( msgList ),mType( jt ),
     mSets( sets ), mSrcFolder( 0 ), mDestFolder( folder ),
+    mErrorCode( 0 ),
     mPassiveDestructor( false ), mStarted( false )
 {
 }
 
 //----------------------------------------------------------------------------
 FolderJob::FolderJob( JobType jt )
-  : mType( jt ), mPassiveDestructor( false ), mStarted( false )
+  : mType( jt ),
+    mErrorCode( 0 ),
+    mPassiveDestructor( false ), mStarted( false )
 {
 }
 
@@ -69,6 +73,7 @@ FolderJob::FolderJob( JobType jt )
 FolderJob::~FolderJob()
 {
   if( !mPassiveDestructor ) {
+    emit result( this );
     emit finished();
   }
 }

folderjob.h

@@ -75,6 +75,12 @@ public:
   QPtrList<KMMessage> msgList() const;
   void start();
 
+  /**
+   * @return the error code of the job. This must only be called from
+   * the slot connected to the finished() signal.
+   */
+  int error() const { return mErrorCode; }
+
 signals:
   /**
    * Emitted whenever a KMMessage has been completely
@@ -85,7 +91,7 @@ signals:
   /**
    * Emitted whenever a KMMessage was updated
    */
-  void messageUpdated( KMMessage *, QString ); 
+  void messageUpdated( KMMessage *, QString );
 
   /**
    * Emitted whenever a message has been stored in
@@ -112,6 +118,14 @@ signals:
    */
   void finished();
 
+  /**
+   * Emitted when the job finishes all processing.
+   * More convenient signal than finished(), since it provides a pointer to the job.
+   * This signal is emitted by the FolderJob destructor => do NOT downcast
+   * the job to a subclass!
+   */
+  void result( KMail::FolderJob* job );
+
   /**
    * This progress signal contains the "done" and the "total" numbers so
    * that the caller can either make a % out of it, or combine it into
@@ -137,6 +151,7 @@ protected:
   KMFolder*           mSrcFolder;
   KMFolder*           mDestFolder;
   QString             mPartSpecifier;
+  int                 mErrorCode;
 
   //finished() won't be emitted when this is set
   bool                mPassiveDestructor;

kmacctcachedimap.cpp

@@ -128,34 +128,70 @@ void KMAcctCachedImap::setAutoExpunge( bool /*aAutoExpunge*/ )
 
 //-----------------------------------------------------------------------------
 void KMAcctCachedImap::slotSlaveError(KIO::Slave *aSlave, int errorCode,
-  const QString &errorMsg)
+                                      const QString &errorMsg )
 {
   if (aSlave != mSlave) return;
-  if (errorCode == KIO::ERR_SLAVE_DIED) slaveDied();
-  if (errorCode == KIO::ERR_COULD_NOT_LOGIN) mAskAgain = TRUE;
-
+  handleJobError( errorCode, errorMsg, 0, QString::null, true );
+}
 
-  if (errorCode == KIO::ERR_CONNECTION_BROKEN ) {
+//-----------------------------------------------------------------------------
+void KMAcctCachedImap::handleJobError( int errorCode, const QString &errorMsg, KIO::Job* job, const QString& context, bool abortSync )
+{
+  // Copy job's data before a possible killAllJobs
+  QStringList errors;
+  if ( job )
+    errors = job->detailedErrorStrings();
+
+  bool jobsKilled = true;
+  switch( errorCode ) {
+  case KIO::ERR_SLAVE_DIED: slaveDied(); break;
+  case KIO::ERR_COULD_NOT_LOGIN: mAskAgain = TRUE; break;
+  case KIO::ERR_CONNECTION_BROKEN:
     if ( slave() ) {
       KIO::Scheduler::disconnectSlave( slave() );
       mSlave = 0;
-      // TODO reset all syncs, killall jobs?
+      // TODO reset all syncs?
     }
+    killAllJobs( true );
+    break;
+  default:
+    if ( abortSync )
+      killAllJobs( false );
+    else
+      jobsKilled = false;
+    break;
   }
 
-  killAllJobs( errorCode == KIO::ERR_CONNECTION_BROKEN );
-
   // check if we still display an error
   if ( !mErrorDialogIsActive )
   {
     mErrorDialogIsActive = true;
-    KMessageBox::messageBox(kapp->activeWindow(), KMessageBox::Error,
-          KIO::buildErrorString(errorCode, errorMsg),
-          i18n("Error"));
+    QString msg;
+    QString caption;
+    if ( errors.count() >= 3 ) {
+      msg = QString( "<qt>") + context + errors[1] + '\n' + errors[2];
+      caption = errors[0];
+    } else {
+      msg = context + '\n' + KIO::buildErrorString( errorCode, errorMsg );
+      caption = i18n("Error");
+    }
+
+    if ( jobsKilled )
+      KMessageBox::error( kapp->activeWindow(), msg, caption );
+    else // i.e. we have a chance to continue, ask the user about it
+    {
+      int ret = KMessageBox::warningContinueCancel( kapp->activeWindow(), msg, caption );
+      if ( ret == KMessageBox::Cancel ) {
+        jobsKilled = true;
+        killAllJobs( false );
+      }
+    }
     mErrorDialogIsActive = false;
   } else
     kdDebug(5006) << "suppressing error:" << errorMsg << endl;
 
+  if ( job && !jobsKilled )
+    removeJob( job );
   mSyncActive = false;
 }
 
@@ -251,11 +287,16 @@ void KMAcctCachedImap::killAllJobs( bool disconnectSlave )
       fld->setSubfolderState(KMFolderCachedImap::imapNoInformation);
       fld->sendFolderComplete(FALSE);
     }
+#if 0
+  // Steffen Hansen doesn't remember why he wrote this.
+  // But we don't need to kill the slave upon the slightest error (e.g. permission denied)
+  // For big errors we have disconnectSlave anyway.
   if (mSlave && mapJobData.begin() != mapJobData.end())
   {
     mSlave->kill();
     mSlave = 0;
   }
+#endif
   mapJobData.clear();
 
   // Clear the joblist. Make SURE to stop the job emitting "finished"

kmacctcachedimap.h

@@ -127,18 +127,38 @@ public:
    * connects to slotListResult and slotListEntries
    */
   void listDirectory(QString path, ListType subscription,
-      bool secondStep = FALSE, KMFolder* parent = NULL, bool reset = false);  
+      bool secondStep = FALSE, KMFolder* parent = NULL, bool reset = false);
 
-  /** 
+  /**
    * Starts the folderlisting for the root folder
-   */   
+   */
   virtual void listDirectory();
 
+  /**
+   * Handle an error coming from a KIO job
+   * and abort everything (in all cases) if abortSync is true.
+   * Otherwise (abortSync==false), we only abort in case of severe errors (connection broken),
+   * but not a "normal" errors (no permission to delete, etc.)
+   * It would be good to port more and more code to abortSync==false, i.e. better error recovery.
+   *
+   * @param error the error code, usually job->error())
+   * @param errorMsg the error message, usually job->errorText()
+   * @param job the kio job (can be 0). If set, removeJob will be called automatically.
+   * This is important! It means you should not call removeJob yourself in case of errors.
+   * We can't let the caller do that, since it should only be done afterwards, and only if we didn't abort.
+   *
+   * @param context a sentence that gives some context to the error, e.g. i18n("Error while uploading message [...]")
+   * @param abortSync if true, abort sync in all cases (see above). If false, ask the user (when possible).
+   */
+  void handleJobError( int error, const QString &errorMsg, KIO::Job* job, const QString& context, bool abortSync = false );
+
 public slots:
   void processNewMail() { processNewMail( mFolder, true ); }
 
   /**
-   * Display an error message
+   * Handle an error coming from the KIO scheduler
+   * This slot has been abused for job error handling, but this should be changed
+   * to use handleJobError instead.
    */
   void slotSlaveError(KIO::Slave *aSlave, int, const QString &errorMsg);
 

kmfoldercachedimap.cpp

@@ -514,7 +514,7 @@ QString KMFolderCachedImap::state2String( int state ) const
 // the state that should be executed next
 void KMFolderCachedImap::serverSyncInternal()
 {
-  //kdDebug() << state2String( mSyncState ) << endl;
+  //kdDebug(5006) << label() << ": " << state2String( mSyncState ) << endl;
   switch( mSyncState ) {
   case SYNC_STATE_INITIAL:
   {
@@ -972,10 +972,11 @@ bool KMFolderCachedImap::deleteMessages()
       // Rerun the sync until the messages are all deleted
       mResync = true;
     }
-    //kdDebug(5006) << "Deleting " << sets.front() << " from sever folder " << imapPath() << endl;
+    //kdDebug(5006) << "Deleting " << sets.front() << " from server folder " << imapPath() << endl;
     CachedImapJob *job = new CachedImapJob( sets.front(), CachedImapJob::tDeleteMessage,
                                                 this );
-    connect( job, SIGNAL( finished() ), this, SLOT( serverSyncInternal() ) );
+    connect( job, SIGNAL( result(KMail::FolderJob *) ),
+             this, SLOT( slotDeleteMessagesResult(KMail::FolderJob *) ) );
     job->start();
     return true;
   } else {
@@ -1446,4 +1447,21 @@ KMFolderCachedImap::slotACLChanged( const QString& userId, int permissions )
   }
 }
 
+void KMFolderCachedImap::slotDeleteMessagesResult( KMail::FolderJob* job )
+{
+  if ( job->error() ) {
+    // Skip the EXPUNGE state if deleting didn't work, no need to show two error messages
+    mSyncState = SYNC_STATE_GET_MESSAGES;
+  }
+  serverSyncInternal();
+}
+
+// called by KMAcctCachedImap::killAllJobs
+void KMFolderCachedImap::resetSyncState()
+{
+  mSyncState = SYNC_STATE_INITIAL;
+  emit newState( label(), mProgress, i18n("Aborted"));
+  emit statusMsg( i18n("%1: Aborted").arg(label()) );
+}
+
 #include "kmfoldercachedimap.moc"

kmfoldercachedimap.h

@@ -98,7 +98,7 @@ public:
   virtual void serverSync( bool suppressDialog );
 
   /** Force the sync state to be done. */
-  void resetSyncState() { mSyncState = SYNC_STATE_INITIAL; }
+  void resetSyncState();
 
   virtual void checkUidValidity();
 
@@ -224,13 +224,6 @@ protected slots:
   void slotProgress(unsigned long done, unsigned long total);
 
   //virtual void slotCheckValidityResult(KIO::Job * job);
-  virtual void listMessages();
-  virtual void uploadNewMessages();
-  virtual void uploadFlags();
-  /* returns true if there were messages to delete
-     on the server */
-  virtual bool deleteMessages();
-  virtual void createNewFolders();
 
   // Connected to the imap account
   void slotConnectionResult( int errorCode );
@@ -240,8 +233,17 @@ protected slots:
 
   void slotMultiSetACLResult(KIO::Job *);
   void slotACLChanged( const QString&, int );
+  void slotDeleteMessagesResult(KMail::FolderJob *);
 
 protected:
+  /* returns true if there were messages to delete
+     on the server */
+  bool deleteMessages();
+  void listMessages();
+  void uploadNewMessages();
+  void uploadFlags();
+  void createNewFolders();
+
   void listDirectory2();