Some fixes for online IMAP filtering:

- In the action scheduler, don't error out when the filtered message
  couldn't be moved back to the target folder. Instead, ignore the error
  (but don't delete the orginal message). This fixes filtering 
  stopping on GMail accounts once a message was encountered that was not
  moved to another folder, but stayed in the same folder (GMail prevents moving
  in this case, since it thinks it is a duplicate message)

- When moving the filtered message with the action scheduler from the
  temporary filter folder to the target folder, the original message wouldn't
  get deleted properly.
  The reason for this was that the move command thought the move failed because
  a message with another serial number arrived.
  Fix this by remembering the serial number (based on the message ID) when using
  the action scheduler for filtering.
  This fixes filters which move messages to other folders: Now the message is properly
  removed from the source folder again.

- add comments and kDebug output

This does _not_ solve the following problem:
When using GMail and online IMAP, filter actions which modify the message will not have
any effect, since GMails duplicate message prevention prevents the filtered message to
be moved back to the IMAP folder.
This will not be fixed, I see no way to work around this. GMail should fix their IMAP
server instead.

Please test, I'm not sure if this is safe to backport.

CCBUG: 166150

svn path=/trunk/KDE/kdepim/; revision=833475

actionscheduler.cpp

@@ -34,6 +34,7 @@
 #include "messageproperty.h"
 #include "kmfilter.h"
 #include "kmfolderindex.h"
+#include "kmfolderimap.h"
 #include "kmfoldermgr.h"
 #include "kmmsgdict.h"
 #include "kmcommands.h"
@@ -272,9 +273,16 @@ void ActionScheduler::execFilters(quint32 serNum)
         mFetchSerNums.pop_front();
       }
     }
-    else
-      return; // An error has already occurred don't even try to process this msg
+    else {
+      kDebug() << "Not starting filtering because of error. mExecuting=" << mExecuting
+               << "mExecutingLock=" << mExecutingLock
+               << "mFetchExecuting=" << mFetchExecuting;
+
+      // An error has already occurred don't even try to process this msg
+      return;
+    }
   }
+
   if (MessageProperty::filtering( serNum )) {
     // Not good someone else is already filtering this msg
     mResult = ResultError;
@@ -333,29 +341,36 @@ KMMessage *ActionScheduler::message(quint32 serNum)
 
 void ActionScheduler::finish()
 {
+  // FIXME: In case of errors, the locks don't get reset. Which means when filtering
+  //        one message errors out, and we will later filter another one, filtering
+  //        will not start because of this.
+  //        There is some cleanup code below, but this might not be the best thing
+  //        to do here: When a single message fails, we want to continue with the
+  //        rest and not completely reset everything...
   if (mResult != ResultOk) {
     // Must handle errors immediately
     emit result( mResult );
     return;
   }
+  kDebug() << "Filtering finished.";
 
   if (!mExecuting) {
 
-  if (!mFetchSerNums.isEmpty()) {
-    // Possibly if (mResult == ResultOk) should cancel job and start again.
-    // Believe smarter logic to bail out if an error has occurred is required.
-    // Perhaps should be testing for mFetchExecuting or at least set it to true
-    fetchMessageTimer->start( 0 ); // give it a bit of time at a test
-    return;
-  } else {
-    mFetchExecuting = false;
-  }
+    if (!mFetchSerNums.isEmpty()) {
+      // Possibly if (mResult == ResultOk) should cancel job and start again.
+      // Believe smarter logic to bail out if an error has occurred is required.
+      // Perhaps should be testing for mFetchExecuting or at least set it to true
+      fetchMessageTimer->start( 0 ); // give it a bit of time at a test
+      return;
+    } else {
+      mFetchExecuting = false;
+    }
 
-  if (mSerNums.begin() != mSerNums.end()) {
-    mExecuting = true;
-    processMessageTimer->start( 0 );
-    return;
-  }
+    if (mSerNums.begin() != mSerNums.end()) {
+      mExecuting = true;
+      processMessageTimer->start( 0 );
+      return;
+    }
 
     // If an error has occurred and a permanent source folder has
     // been set then move all the messages left in the source folder
@@ -510,7 +525,6 @@ void ActionScheduler::enqueue(quint32 serNum)
 
 void ActionScheduler::processMessage()
 {
-  kDebug(5006) << debug();
   QString statusMsg = i18n( "%1 messages waiting to be filtered",
                             mFetchSerNums.count() );
   KPIM::BroadcastStatus::instance()->setStatusMsg( statusMsg );
@@ -533,6 +547,7 @@ void ActionScheduler::processMessage()
   if ((mMessageIt == mSerNums.end()) || (mResult != ResultOk)) {
     mExecutingLock = false;
     mExecuting = false;
+    kDebug() << "Stopping filtering, error or end of message list.";
     finishTimer->start( 0 );
     return;
   }
@@ -678,6 +693,14 @@ void ActionScheduler::moveMessage()
     mOriginalSerNum = 0;
   MessageProperty::setFilterHandler( *mMessageIt, 0 );
   MessageProperty::setFiltering( *mMessageIt, false );
+
+  // If the target folder is an online IMAP folder, make sure to keep the same
+  // serial number (otherwise the move commands thinks the message wasn't moved
+  // correctly, which would trigger the error case in moveMessageFinished().
+  Q_ASSERT( folder );
+  if ( folder && folder->storage() && dynamic_cast<KMFolderImap*>( folder->storage() ) )
+    MessageProperty::setKeepSerialNumber( msg->getMsgSerNum(), true );
+
   mSerNums.removeAll( *mMessageIt );
 
   KMMessage *orgMsg = 0;
@@ -688,7 +711,7 @@ void ActionScheduler::moveMessage()
   if (!orgMsg || !orgMsg->parent()) {
     // Original message is gone, no point filtering it anymore
     mSrcFolder->removeMsg( mSrcFolder->find( msg ) );
-    kDebug(5006) <<"The original serial number is missing."
+    kDebug(5006) << "The original serial number is missing."
                  << "Cannot complete the filtering.";
     mExecutingLock = false;
     processMessageTimer->start( 0 );
@@ -710,7 +733,7 @@ void ActionScheduler::moveMessage()
   timeOutTime = QTime::currentTime();
   KMCommand *cmd = new KMMoveCommand( folder, msg );
   connect( cmd, SIGNAL( completed( KMCommand * ) ),
-	   this, SLOT( moveMessageFinished( KMCommand * ) ) );
+           this, SLOT( moveMessageFinished( KMCommand * ) ) );
   cmd->start();
   // sometimes the move command doesn't complete so time out after a minute
   // and move onto the next message
@@ -723,10 +746,23 @@ void ActionScheduler::moveMessageFinished( KMCommand *command )
   timeOutTimer->stop();
   bool movingFailed = false;
   if ( command->result() != KMCommand::OK ) {
-    mResult = ResultError;
+
+    // Simply ignore the error. Moving the message back might have failed because
+    // of GoogleMail preventing duplicates (see bug 166150).
+    // We ignore the error because otherwise filtering would not continue with
+    // the next message, filtering would just stop until KMail is restarted.
+    // Normally, finish() is supposed to clear the error state properly, but
+    // it doesn't do that and changing it can break all kind of logic.
+    //
+    // Note that ignoring the error does not mean dataloss, it just means that
+    // the filter actions will not work for the message. By setting movingFailed
+    // to false, we prevent the deletion of the original source message.
+    //mResult = ResultError;
+
     movingFailed = true;
     kWarning() << "Moving the message from the temporary filter folder to the "
                   "target folder failed. Message will stay unfiltered.";
+    kWarning() << "This might be because you're using GMail, see bug 166150.";
   }
 
   if (!mSrcFolder->count())
@@ -743,30 +779,36 @@ void ActionScheduler::moveMessageFinished( KMCommand *command )
   }
 
   mResult = mOldReturnCode; // ignore errors in deleting original message
+
+  // Delete the original message, because we have just moved the filtered message
+  // from the temporary filtering folder to the target folder, and don't need the
+  // old unfiltered message there anymore.
   KMCommand *cmd = 0;
   if ( msg && msg->parent() && !movingFailed ) {
     cmd = new KMMoveCommand( 0, msg );
-//    cmd->start(); // Note: sensitive logic here.
   }
 
-  if (mResult == ResultOk) {
-    mExecutingLock = false;
-    if (cmd)
-      connect( cmd, SIGNAL( completed( KMCommand * ) ),
-               this, SLOT( processMessage() ) );
-    else
-      processMessageTimer->start( 0 );
-  } else {
-    // Note: An alternative to consider is just calling
-    //       finishTimer->start and returning
-    if (cmd)
-      connect( cmd, SIGNAL( completed( KMCommand * ) ),
-               this, SLOT( finish() ) );
-    else
-      finishTimer->start( 0 );
-  }
-  if (cmd)
+  // When the above deleting is finished, filtering that single mail is complete.
+  // The next state is processMessage(), which will start filtering the next
+  // message if we still have mails in the temporary filter folder.
+  // If there are no unfiltered messages there or an error was encountered,
+  // processMessage() will stop filtering by setting mExecuting to false
+  // and then going to the finish() state.
+  //
+  // processMessage() will only do something if mExecutingLock if false.
+  //
+  // We can't advance to the finish() state just here, because that
+  // wouldn't reset mExecuting at all, so we make sure that the next stage is
+  // processMessage() in all cases.
+  mExecutingLock = false;
+
+  if ( cmd ) {
+    connect( cmd, SIGNAL( completed( KMCommand * ) ),
+             this, SLOT( processMessage() ) );
     cmd->start();
+  }
+  else
+    processMessageTimer->start( 0 );
 }
 
 void ActionScheduler::copyMessageFinished( KMCommand *command )

actionscheduler.h

@@ -139,10 +139,35 @@ private:
   static KMFolderMgr *tempFolderMgr;
   static int refCount, count;
   static bool sEnabled, sEnabledChecked;
+
+  // Iterates over the messages in mSerNums, describes which message is being
+  // filtered currently.
+  // In processMessage(), this iterator is set to the next message which is available
+  // for processing (thus processMessage() is called once for every message).
   QList<quint32>::Iterator mMessageIt;
+
+  // Iterates over all available filters. Used in filterMessage(), which is
+  // called once for every filter.
   QList<KMFilter*>::iterator mFilterIt;
+
+  // Iterates over all filter actions of the current filter. Used in
+  // actionMessage(), which is called once for every filter action.
   QList<KMFilterAction*>::iterator mFilterActionIt;
-  QList<quint32> mSerNums, mFetchSerNums;
+
+  // List of serial numbers of message that are in the temporary filter folder
+  // and await processing.
+  // Serial numbers are added as soon as new messages are added to the temp folder,
+  // and removed as soon as the message is moved back to the original
+  // source folder (or the target folder if specified in a filter action).
+  QList<quint32> mSerNums;
+
+  // List of serial numbers of messages that need to be fetched from the orginal
+  // source folder.
+  // Once they are fetched, the messages are copied into the temporary filter folder.
+  // Serial numbers are added when execFilters() is called by the user, and
+  // removed as soon as the message is fetched from the original source folder.
+  QList<quint32> mFetchSerNums;
+
   QList<QPointer<KMFolder> > mOpenFolders;
   QList<KMFilter*> mFilters, mQueuedFilters;
   KMFilterAction* mFilterAction;

imapjob.cpp

@@ -33,6 +33,7 @@
 #include "kmfolderimap.h"
 #include "kmfolder.h"
 #include "kmmsgpart.h"
+#include "messageproperty.h"
 #include "progressmanager.h"
 using KPIM::ProgressManager;
 #include "util.h"
@@ -106,7 +107,7 @@ void ImapJob::init( JobType jt, const QString &sets, KMFolderImap *folder,
     return;
   }
   account->registerJob( this );
-  //account->mJobList.append( this );
+
   if ( jt == tPutMessage )
   {
     // transfers the complete message to the server
@@ -131,7 +132,8 @@ void ImapJob::init( JobType jt, const QString &sets, KMFolderImap *folder,
       QByteArray cstr( curMsg->asString() );
       int a = cstr.indexOf("\nX-UID: ");
       int b = cstr.indexOf('\n', a);
-      if (a != -1 && b != -1 && cstr.indexOf("\n\n") > a) cstr.remove(a, b-a);
+      if (a != -1 && b != -1 && cstr.indexOf("\n\n") > a)
+        cstr.remove(a, b-a);
       jd.data.resize( cstr.length() + cstr.count( "\n" ) - cstr.count( "\r\n" ) );
       unsigned int i = 0;
       char prevChar = '\0';
@@ -167,6 +169,22 @@ void ImapJob::init( JobType jt, const QString &sets, KMFolderImap *folder,
           SLOT(slotPutMessageInfoData(KJob *, const QString &, const QString &)) );
       connect( job, SIGNAL( processedSize( KJob *, qulonglong ) ),
           SLOT( slotProcessedSize( KJob *, qulonglong ) ) );
+
+      // When moving the message, we want to keep the serial number if the
+      // message property is set (by the action scheduler).
+      // To do this, create metadata, which is later read by
+      // KMFolderImap::slotGetMessagesData().
+      if ( MessageProperty::keepSerialNumber( msg->getMsgSerNum() ) ) {
+
+        // Note that we can't rely on connecting infoMessage() to something like
+        // slotCopyMessageInfoData(), since that only works when the server supports
+        // UIDPLUS.
+        folder->rememberSerialNumber( curMsg );
+
+        // Forget that property again, so we don't end up keeping the serial number
+        // the next time we do something with that message
+        MessageProperty::setKeepSerialNumber( msg->getMsgSerNum(), false );
+      }
     }
   }
   else if ( jt == tCopyMessage || jt == tMoveMessage )

kmfolderimap.cpp

@@ -1554,6 +1554,13 @@ KMFolderImap::ignoreJobsForMessage( KMMessage* msg )
   account->ignoreJobsForMessage( msg );
 }
 
+//-----------------------------------------------------------------------------
+void KMFolderImap::rememberSerialNumber( const KMMessage *msg )
+{
+  mMetaDataMap.insert( msg->msgIdMD5(), new KMMsgMetaData( msg->status(),
+                                                           msg->getMsgSerNum() ) );
+}
+
 //-----------------------------------------------------------------------------
 void KMFolderImap::slotGetMessagesData( KIO::Job *job, const QByteArray &data )
 {

kmfolderimap.h

@@ -464,6 +464,14 @@ protected slots:
   void slotGetLastMessagesResult(KJob * job);
   void slotGetMessagesData(KIO::Job * job, const QByteArray & data);
 
+  /**
+   * Puts the serial number and other metadata of the message into a map.
+   * When the same message later is added to the folder, for example by calling
+   * addMsg(), slotGetMessagesData() then examines the metadata map and assigns
+   * the old serial number to the new message.
+   */
+  void rememberSerialNumber( const KMMessage *msg );
+
   /**
    * For creating a new subfolder
    */

kmmessage.h

@@ -756,7 +756,7 @@ public:
   void setUID(ulong uid);
 
   /** Status of the message. */
-  MessageStatus& status() { return mStatus; }
+  MessageStatus& status() const { return mStatus; }
   /** Set status and mark dirty. */
   void setStatus(const MessageStatus& status, int idx = -1);
   void setStatus(const char* s1, const char* s2=0) { KMMsgBase::setStatus(s1, s2); }

kmmsgbase.cpp

@@ -293,7 +293,7 @@ void KMMsgBase::setSignatureStateChar( QChar status, int idx )
 }
 
 //-----------------------------------------------------------------------------
-MessageStatus& KMMsgBase::status()
+MessageStatus& KMMsgBase::status() const
 {
   return mStatus;
 }

kmmsgbase.h

@@ -116,7 +116,7 @@ public:
   virtual bool isMessage(void) const;
 
   /** Status object of the message. */
-  virtual MessageStatus& status();
+  virtual MessageStatus& status() const;
 
   /** Const reference to a status object of a message. */
   const MessageStatus& messageStatus() const;

kmmsginfo.cpp

@@ -495,7 +495,7 @@ void KMMsgInfo::setMDNSentState( const KMMsgMDNSentState s, int idx )
 }
 
 //-----------------------------------------------------------------------------
-MessageStatus& KMMsgInfo::status(void)
+MessageStatus& KMMsgInfo::status() const
 {
     if ( mStatus.isOfUnknownStatus() ) {
         mStatus.fromQInt32( getLongPart( MsgStatusPart ) );

kmmsginfo.h

@@ -82,7 +82,7 @@ public:
   virtual QString fileName(void) const;
   virtual QString tagString( void ) const; 
   virtual KMMessageTagList *tagList( void ) const; 
-  virtual MessageStatus& status();
+  virtual MessageStatus& status() const;
   virtual KMMsgEncryptionState encryptionState() const;
   virtual KMMsgSignatureState signatureState() const;
   virtual KMMsgMDNSentState mdnSentState() const;

messageproperty.cpp

@@ -33,6 +33,7 @@
 using namespace KMail;
 
 QMap<quint32, QPointer<KMFolder> > MessageProperty::sFolders;
+QMap<quint32, bool> MessageProperty::sKeepSerialNumber;
 QMap<quint32, QPointer<ActionScheduler> > MessageProperty::sHandlers;
 QMap<quint32, int > MessageProperty::sTransfers;
 QMap<const KMMsgBase*, long > MessageProperty::sSerialCache;
@@ -154,6 +155,24 @@ void MessageProperty::setSerialCache( const KMMsgBase *msgBase, quint32 serNum )
     sSerialCache.remove( msgBase );
 }
 
+void MessageProperty::setKeepSerialNumber( quint32 serialNumber, bool keepForMoving )
+{
+  if ( serialNumber ) {
+    if ( sKeepSerialNumber.contains( serialNumber ) )
+      sKeepSerialNumber[ serialNumber ] = keepForMoving;
+    else
+      sKeepSerialNumber.insert( serialNumber, keepForMoving );
+  }
+}
+
+bool MessageProperty::keepSerialNumber( quint32 serialNumber )
+{
+  if ( sKeepSerialNumber.contains( serialNumber ) )
+    return sKeepSerialNumber[ serialNumber ];
+  else
+    return false;
+}
+ 
 void MessageProperty::forget( const KMMsgBase *msgBase )
 {
   quint32 serNum = serialCache( msgBase );
@@ -161,6 +180,7 @@ void MessageProperty::forget( const KMMsgBase *msgBase )
     Q_ASSERT( !transferInProgress( serNum ) );
     sTransfers.remove( serNum );
     sSerialCache.remove( msgBase );
+    sKeepSerialNumber.remove( serNum );
   }
 }
 

messageproperty.h

@@ -89,20 +89,42 @@ public:
   static bool transferInProgress( const KMMsgBase* );
   static void setTransferInProgress( quint32, bool, bool = false );
   static bool transferInProgress( quint32 );
+
+  /**
+   * Set this property to true if you want to keep the serial number when moving
+   * a message from a local folder to an online IMAP folder.
+   * Setting this to true will cause the ImapJob to save the meta data, like the
+   * serial number, of the message in a map, which is later read when the
+   * message arrives in the new location. Then the serial number is restored.
+   */
+  static void setKeepSerialNumber( quint32 serialNumber, bool keepForMoving );
+  static bool keepSerialNumber( quint32 serialNumber );
+
   /** Some properties, namely complete, transferInProgress, and
       serialCache must be forgotten when a message class instance is
       destructed or assigned a new value */
   static void forget( const KMMsgBase* );
 
 private:
+
   // The folder a message is to be moved into once filtering is finished if any
   static QMap<quint32, QPointer<KMFolder> > sFolders;
+
+  // Whether the serial number of a message should be kept when moving it from
+  // a local folder to an online IMAP folder. This is currently only used by
+  // the action scheduler (in ActionScheduler::moveMessage()), to make the IMAP
+  // job aware that it should try to preserve the serial number when moving, see
+  // ImapJob::init().
+  static QMap<quint32, bool> sKeepSerialNumber;
+
   // The action scheduler currently processing a message if any
   static QMap<quint32, QPointer<ActionScheduler> > sHandlers;
+
   // The transferInProgres state of a message if any.
   static QMap<quint32, int > sTransfers;
+
   // The cached serial number of a message if any.
-  static QMap<const KMMsgBase*, long > sSerialCache;
+  static QMap<const KMMsgBase*, long> sSerialCache;
 };
 
 }