Major IMAP IDLE rework:

1) Actual message sync on MessagingController uses same connection as
   IDLE for faster and more efficient push operation.  Uses fewer
   connections to the server.

2) More aggressive handling of untagged responses should more reliably
   get flag changes and new messages when many events happen at once.

3) Simplification of new mail notification

4) Push mail now respects the folder visible limit

5) When multiple untagged FETCH responses arrive en bloc, the actual
   message flags and UIDs are fetched with a single request

src/com/android/email/MessagingController.java

@@ -52,6 +52,7 @@ import com.android.email.mail.FetchProfile;
 import com.android.email.mail.Flag;
 import com.android.email.mail.Folder;
 import com.android.email.mail.Message;
+import com.android.email.mail.MessageRemovalListener;
 import com.android.email.mail.MessageRetrievalListener;
 import com.android.email.mail.MessagingException;
 import com.android.email.mail.Part;
@@ -1061,7 +1062,7 @@ public class MessagingController implements Runnable
             /*
              * Now we download the actual content of messages.
              */
-            int newMessages = downloadMessages(account, remoteFolder, localFolder, remoteMessages);
+            int newMessages = downloadMessages(account, remoteFolder, localFolder, remoteMessages, false);
 
             setLocalUnreadCountToRemote(localFolder, remoteFolder,  newMessages);
 
@@ -1163,22 +1164,14 @@ public class MessagingController implements Runnable
     }
 
     private int downloadMessages(final Account account, final Folder remoteFolder,
-                                 final LocalFolder localFolder, List<Message> inputMessages) throws MessagingException
+                                 final LocalFolder localFolder, List<Message> inputMessages, boolean flagSyncOnly) throws MessagingException
     {
         final String folder = remoteFolder.getName();
 
         ArrayList<Message> syncFlagMessages = new ArrayList<Message>();
-        ArrayList<Message> unsyncedMessages = new ArrayList<Message>();
+        List<Message> unsyncedMessages = new ArrayList<Message>();
         final AtomicInteger newMessages = new AtomicInteger(0);
 
-        int visibleLimit = localFolder.getVisibleLimit();
-        int listSize = inputMessages.size();
-
-        if (listSize > visibleLimit)
-        {
-            inputMessages = inputMessages.subList(listSize - visibleLimit, listSize);
-        }
-
         List<Message> messages = new ArrayList<Message>(inputMessages);
 
         for (Message message : messages)
@@ -1188,6 +1181,8 @@ public class MessagingController implements Runnable
                 Message localMessage = localFolder.getMessage(message.getUid());
 
                 if (localMessage == null)
+                {
+                    if (!flagSyncOnly)
                     {
                         if (!message.isSet(Flag.X_DOWNLOADED_FULL) && !message.isSet(Flag.X_DOWNLOADED_PARTIAL))
                         {
@@ -1222,6 +1217,7 @@ public class MessagingController implements Runnable
                             }
                         }
                     }
+                }
                 else
                 {
                     if (Email.DEBUG)
@@ -1251,6 +1247,13 @@ public class MessagingController implements Runnable
              * fetch results for newest to oldest. If not, no harm done.
              */
             Collections.reverse(unsyncedMessages);
+            int visibleLimit = localFolder.getVisibleLimit();
+            int listSize = unsyncedMessages.size();
+
+            if (listSize > visibleLimit)
+            {
+                unsyncedMessages = unsyncedMessages.subList(listSize - visibleLimit, listSize);
+            }
 
             FetchProfile fp = new FetchProfile();
             if (remoteFolder.supportsFetchingFlags())
@@ -1556,6 +1559,19 @@ public class MessagingController implements Runnable
         }
         Log.i(Email.LOG_TAG, "SYNC: Synced remote messages for folder " + folder + ", " + newMessages.get() + " new messages");
 
+        localFolder.purgeToVisibleLimit(new MessageRemovalListener()
+        {
+            @Override
+            public void messageRemoved(Message message)
+            {
+                for (MessagingListener l : getListeners())
+                {
+                    l.synchronizeMailboxRemovedMessage(account, folder, message);
+                }
+            }
+
+        });
+
         return newMessages.get();
     }
 
@@ -2072,7 +2088,7 @@ public class MessagingController implements Runnable
             }
 
             String rootCauseMessage = getRootCauseMessage(t);
-            log("Error" + "'" + rootCauseMessage + "'");
+            Log.e(Email.LOG_TAG, "Error " + "'" + rootCauseMessage + "'", t);
 
             Store localStore = Store.getInstance(account.getLocalStoreUri(), mApplication);
             LocalFolder localFolder = (LocalFolder)localStore.getFolder(account.getErrorFolderName());
@@ -3989,15 +4005,15 @@ public class MessagingController implements Runnable
                 }
             }
 
-            public void messagesFlagsChanged(String folderName,
+            public void messagesFlagsChanged(Folder folder,
                                              List<Message> messages)
             {
-                controller.messagesArrived(account, folderName, messages, false);
+                controller.messagesArrived(account, folder, messages, true);
 
             }
-            public void messagesArrived(String folderName, List<Message> messages)
+            public void messagesArrived(Folder folder, List<Message> messages)
             {
-                controller.messagesArrived(account, folderName, messages, true);
+                controller.messagesArrived(account, folder, messages, false);
             }
 
             public void sleep(long millis)
@@ -4152,44 +4168,37 @@ public class MessagingController implements Runnable
 
     }
 
-    public void messagesArrived(final Account account, final String folderName, final List<Message> messages, final boolean doNotify)
+    public void messagesArrived(final Account account, final Folder remoteFolder, final List<Message> messages, final boolean flagSyncOnly)
     {
         Log.i(Email.LOG_TAG, "Got new pushed email messages for account " + account.getDescription()
-              + ", folder " + folderName);
+              + ", folder " + remoteFolder.getName());
         final CountDownLatch latch = new CountDownLatch(1);
         putBackground("Push messageArrived of account " + account.getDescription()
-                      + ", folder " + folderName, null, new Runnable()
+                      + ", folder " + remoteFolder.getName(), null, new Runnable()
         {
             public void run()
             {
                 LocalFolder localFolder = null;
-                Folder remoteFolder = null;
                 try
                 {
                     LocalStore localStore = (LocalStore) Store.getInstance(account.getLocalStoreUri(), mApplication);
-                    Store remoteStore = Store.getInstance(account.getStoreUri(), mApplication);
+                    localFolder= (LocalFolder) localStore.getFolder(remoteFolder.getName());
-                    remoteFolder = remoteStore.getFolder(folderName);
-                    localFolder= (LocalFolder) localStore.getFolder(folderName);
                     localFolder.open(OpenMode.READ_WRITE);
                     remoteFolder.open(OpenMode.READ_WRITE);
 
-                    int newCount = downloadMessages(account, remoteFolder, localFolder, messages);
+                    int newCount = downloadMessages(account, remoteFolder, localFolder, messages, flagSyncOnly);
+                    setLocalUnreadCountToRemote(localFolder, remoteFolder,  messages.size());
+
                     localFolder.setLastPush(System.currentTimeMillis());
                     localFolder.setStatus(null);
 
                     int unreadMessageCount = account.getUnreadMessageCount(mApplication, mApplication);
-                    if (doNotify && newCount > 0 && unreadMessageCount > 0)
+                    Log.i(Email.LOG_TAG, "messagesArrived newCount = " + newCount + ", unreadMessageCount = " + unreadMessageCount);
-                    {
                     notifyAccount(mApplication, account, newCount, unreadMessageCount);
-                    }
-                    if (unreadMessageCount == 0)
-                    {
-                        notifyAccount(mApplication, account, newCount, unreadMessageCount);
-                    }
 
                     for (MessagingListener l : getListeners())
                     {
-                        l.folderStatusChanged(account, folderName);
+                        l.folderStatusChanged(account, remoteFolder.getName());
                         l.accountStatusChanged(account, unreadMessageCount);
                     }
 
@@ -4208,7 +4217,7 @@ public class MessagingController implements Runnable
                     }
                     for (MessagingListener l : getListeners())
                     {
-                        l.synchronizeMailboxFailed(account, folderName, errorMessage);
+                        l.synchronizeMailboxFailed(account, remoteFolder.getName(), errorMessage);
                     }
                     addErrorMessage(account, e);
                 }
@@ -4225,17 +4234,6 @@ public class MessagingController implements Runnable
                             Log.e(Email.LOG_TAG, "Unable to close localFolder", e);
                         }
                     }
-                    if (remoteFolder != null)
-                    {
-                        try
-                        {
-                            remoteFolder.close(false);
-                        }
-                        catch (Exception e)
-                        {
-                            Log.e(Email.LOG_TAG, "Unable to close remoteFolder", e);
-                        }
-                    }
                     latch.countDown();
                 }
 
@@ -4249,7 +4247,7 @@ public class MessagingController implements Runnable
         {
             Log.e(Email.LOG_TAG, "Interrupted while awaiting latch release", e);
         }
-        Log.i(Email.LOG_TAG, "Latch released");
+        Log.i(Email.LOG_TAG, "MessagingController.messagesArrivedLatch released");
     }
     enum MemorizingState { STARTED, FINISHED, FAILED };
 

src/com/android/email/mail/MessageRemovalListener.java

@@ -0,0 +1,6 @@
+package com.android.email.mail;
+
+public interface MessageRemovalListener
+{
+    public void messageRemoved(Message message);
+}

src/com/android/email/mail/PushReceiver.java

@@ -6,8 +6,8 @@ public interface PushReceiver
 {
     public void acquireWakeLock();
     public void releaseWakeLock();
-    public void messagesArrived(String folderName, List<Message> mess);
+    public void messagesArrived(Folder folder, List<Message> mess);
-    public void messagesFlagsChanged(String folderName, List<Message> mess);
+    public void messagesFlagsChanged(Folder folder, List<Message> mess);
     public String getPushState(String folderName);
     public void pushError(String errorMessage, Exception e);
     public void setPushActive(String folderName, boolean enabled);

src/com/android/email/mail/store/ImapStore.java

@@ -464,6 +464,12 @@ public class ImapStore extends Store
         public void open(OpenMode mode) throws MessagingException
         {
             internalOpen(mode);
+
+            if (mMessageCount == -1)
+            {
+                throw new MessagingException(
+                    "Did not find message count during open");
+            }
         }
 
         public List<ImapResponse> internalOpen(OpenMode mode) throws MessagingException
@@ -559,13 +565,8 @@ public class ImapStore extends Store
                     }
                 }
 
-                if (mMessageCount == -1)
-                {
-                    throw new MessagingException(
-                        "Did not find message count with command '" + command + "'");
-                }
                 mExists = true;
-                return null;
+                return responses;
             }
             catch (IOException ioe)
             {
@@ -788,12 +789,12 @@ public class ImapStore extends Store
         public Message[] getMessages(int start, int end, MessageRetrievalListener listener)
         throws MessagingException
         {
-
-
             return getMessages(start, end, false, listener);
         }
 
-        protected Message[] getMessages(int start, int end, boolean includeDeleted, MessageRetrievalListener listener)
+
+
+        protected Message[] getMessages(final int start, final int end, final boolean includeDeleted, final MessageRetrievalListener listener)
         throws MessagingException
         {
             if (start < 1 || end < 1 || end < start)
@@ -802,13 +803,39 @@ public class ImapStore extends Store
                     String.format("Invalid message set %d %d",
                                   start, end));
             }
+            ImapSearcher searcher = new ImapSearcher()
+            {
+                public List<ImapResponse> search() throws IOException, MessagingException
+                {
+                    return executeSimpleCommand(String.format("UID SEARCH %d:%d" + (includeDeleted ? "" : " NOT DELETED"), start, end));
+                }
+            };
+            return search(searcher, listener);
+
+        }
+        protected Message[] getMessages(final List<Integer> mesgSeqs, final boolean includeDeleted, final MessageRetrievalListener listener)
+        throws MessagingException
+        {
+            ImapSearcher searcher = new ImapSearcher()
+            {
+                public List<ImapResponse> search() throws IOException, MessagingException
+                {
+                    return executeSimpleCommand(String.format("UID SEARCH %s" + (includeDeleted ? "" : " NOT DELETED"), Utility.combine(mesgSeqs.toArray(), ',')));
+                }
+            };
+            return search(searcher, listener);
+        }
+
+        private Message[] search(ImapSearcher searcher, MessageRetrievalListener listener) throws MessagingException
+        {
+
             checkOpen();
             ArrayList<Message> messages = new ArrayList<Message>();
             try
             {
                 boolean gotSearchValues = false;
                 ArrayList<Integer> uids = new ArrayList<Integer>();
-                List<ImapResponse> responses = executeSimpleCommand(String.format("UID SEARCH %d:%d" + (includeDeleted ? "" : " NOT DELETED"), start, end));
+                List<ImapResponse> responses = searcher.search(); //
                 for (ImapResponse response : responses)
                 {
                     //          Log.d(Email.LOG_TAG, "Got search response: " + response.get(0) + ", size " + response.size());
@@ -975,7 +1002,6 @@ public class ImapStore extends Store
                 do
                 {
                     response = mConnection.readResponse();
-                    handleUntaggedResponse(response);
                     if (Email.DEBUG)
                     {
                         Log.v(Email.LOG_TAG, "response for fetch: " + response + " for " + getLogId());
@@ -988,7 +1014,8 @@ public class ImapStore extends Store
                         Message message = messageMap.get(uid);
                         if (message == null)
                         {
-                            Log.w(Email.LOG_TAG, "Do not have message in messageMap for UID " + uid + " for " + getLogId());
+                            Log.d(Email.LOG_TAG, "Do not have message in messageMap for UID " + uid + " for " + getLogId());
+                            handleUntaggedResponse(response);
                             continue;
                         }
                         if (listener != null)
@@ -1107,6 +1134,10 @@ public class ImapStore extends Store
                             listener.messageFinished(message, messageNumber, messageMap.size());
                         }
                     }
+                    else
+                    {
+                        handleUntaggedResponse(response);
+                    }
 
                     while (response.more());
 
@@ -1783,10 +1814,10 @@ public class ImapStore extends Store
                             {
                                 if (capability instanceof String)
                                 {
-                                    if (Email.DEBUG)
+//                                    if (Email.DEBUG)
-                                    {
+//                                    {
-                                        Log.v(Email.LOG_TAG, "Saving capability '" + capability + "' for " + getLogId());
+//                                        Log.v(Email.LOG_TAG, "Saving capability '" + capability + "' for " + getLogId());
-                                    }
+//                                    }
                                     capabilities.add((String)capability);
                                 }
                             }
@@ -1886,10 +1917,10 @@ public class ImapStore extends Store
             if (Email.DEBUG)
             {
                 Log.v(Email.LOG_TAG, "Connection " + getLogId() + " has " + capabilities.size() + " capabilities");
-                for (String capability : capabilities)
+//                for (String capability : capabilities)
-                {
+//                {
-                    Log.v(Email.LOG_TAG, "Have capability '" + capability + "' for " + getLogId());
+//                    Log.v(Email.LOG_TAG, "Have capability '" + capability + "' for " + getLogId());
-                }
+//                }
             }
             return capabilities.contains("IDLE");
         }
@@ -2223,6 +2254,7 @@ public class ImapStore extends Store
         final AtomicBoolean idling = new AtomicBoolean(false);
         final AtomicBoolean doneSent = new AtomicBoolean(false);
         final AtomicInteger delayTime = new AtomicInteger(NORMAL_DELAY_TIME);
+        List<ImapResponse> storedUntaggedResponses = new ArrayList<ImapResponse>();
 
         public ImapFolderPusher(ImapStore store, String name, PushReceiver nReceiver)
         {
@@ -2265,6 +2297,7 @@ public class ImapStore extends Store
                     Log.i(Email.LOG_TAG, "Pusher starting for " + getLogId());
                     while (stop.get() != true)
                     {
+
                         try
                         {
                             int oldUidNext = -1;
@@ -2327,15 +2360,26 @@ public class ImapStore extends Store
                             else
                             {
                                 if (stop.get() != true)
+                                {
+                                    List<ImapResponse> untaggedResponses = null;
+                                    if (storedUntaggedResponses.size() > 0)
+                                    {
+                                        Log.i(Email.LOG_TAG, "Processing " + storedUntaggedResponses.size() + " from previous commands for " + getLogId());
+                                        untaggedResponses = new ArrayList<ImapResponse>(storedUntaggedResponses);
+                                    }
+                                    else
                                     {
                                         Log.i(Email.LOG_TAG, "About to IDLE for " + getLogId());
 
                                         receiver.setPushActive(getName(), true);
                                         idling.set(true);
                                         doneSent.set(false);
-                                    executeSimpleCommand("IDLE", false, ImapFolderPusher.this);
+                                        untaggedResponses = executeSimpleCommand("IDLE", false, ImapFolderPusher.this);
                                         idling.set(false);
-                                    receiver.setPushActive(getName(), false);
+
+                                    }
+                                    storedUntaggedResponses.clear();
+                                    processUntaggedResponses(untaggedResponses);
                                     delayTime.set(NORMAL_DELAY_TIME);
                                 }
                             }
@@ -2343,6 +2387,7 @@ public class ImapStore extends Store
                         catch (Exception e)
                         {
                             receiver.acquireWakeLock();
+                            storedUntaggedResponses.clear();
                             idling.set(false);
                             receiver.setPushActive(getName(), false);
                             try
@@ -2372,6 +2417,7 @@ public class ImapStore extends Store
                             }
                         }
                     }
+                    receiver.setPushActive(getName(), false);
                     try
                     {
                         Log.i(Email.LOG_TAG, "Pusher for " + getLogId() + " is exiting");
@@ -2391,39 +2437,50 @@ public class ImapStore extends Store
             listeningThread.start();
         }
 
-        List<Integer> flagSyncMsgSeqs = new ArrayList<Integer>();
+        @Override
-
+        protected void handleUntaggedResponse(ImapResponse response)
-        protected List<ImapResponse> handleUntaggedResponses(List<ImapResponse> responses)
+        {
+            if (response.mTag == null && response.size() > 1)
+            {
+                Object responseType = response.get(1);
+                if ("FETCH".equals(responseType)
+                        || "EXPUNGE".equals(responseType)
+                        || "EXISTS".equals(responseType))
                 {
-            flagSyncMsgSeqs.clear();
-            int oldMessageCount = mMessageCount;
-
-            super.handleUntaggedResponses(responses);
-
-            List<Integer> flagSyncMsgSeqsCopy = new ArrayList<Integer>();
-            flagSyncMsgSeqsCopy.addAll(flagSyncMsgSeqs);
-
-
                     if (Email.DEBUG)
                     {
-                Log.d(Email.LOG_TAG, "oldMessageCount = " + oldMessageCount + ", new mMessageCount = " + mMessageCount
+                        Log.d(Email.LOG_TAG, "Storing response " + response + " for later processing");
-                      + " for " + getLogId());
                     }
-            if (oldMessageCount > 0 && mMessageCount > oldMessageCount)
+                    storedUntaggedResponses.add(response);
+                }
+            }
+        }
+
+        protected void processUntaggedResponses(List<ImapResponse> responses)
+        {
+            int oldMessageCount = mMessageCount;
+            List<Integer> flagSyncMsgSeqs = new ArrayList<Integer>();
+
+            for (ImapResponse response : responses)
+            {
+                oldMessageCount += processUntaggedResponse(oldMessageCount, response, flagSyncMsgSeqs);
+            }
+            if (oldMessageCount < 0)
+            {
+                oldMessageCount = 0;
+            }
+            if (mMessageCount > oldMessageCount)
             {
                 syncMessages(oldMessageCount + 1, mMessageCount, true);
             }
             if (Email.DEBUG)
             {
-                Log.d(Email.LOG_TAG, "There are " + flagSyncMsgSeqsCopy + " messages needing flag sync for " + getLogId());
+                Log.d(Email.LOG_TAG, "There are " + flagSyncMsgSeqs + " messages needing flag sync for " + getLogId());
             }
-            // TODO: Identify ranges and call syncMessages on said identified ranges
+            if (flagSyncMsgSeqs.size() > 0)
-            for (Integer msgSeq : flagSyncMsgSeqsCopy)
             {
-                syncMessages(msgSeq, msgSeq, false);
+                syncMessages(flagSyncMsgSeqs);
             }
-
-            return responses;
         }
 
         private void syncMessages(int start, int end, boolean newArrivals)
@@ -2448,9 +2505,32 @@ public class ImapStore extends Store
             }
         }
 
-        protected void handleUntaggedResponse(ImapResponse response)
+        private void syncMessages(List<Integer> flagSyncMsgSeqs)
+        {
+            try
+            {
+                Message[] messageArray = null;
+
+                messageArray = getMessages(flagSyncMsgSeqs, true, null);
+
+                List<Message> messages = new ArrayList<Message>();
+                for (Message message : messageArray)
+                {
+                    messages.add(message);
+                }
+                pushMessages(messages, false);
+
+            }
+            catch (Exception e)
+            {
+                receiver.pushError("Exception while processing Push untagged responses", e);
+            }
+        }
+
+        protected int processUntaggedResponse(int oldMessageCount, ImapResponse response, List<Integer> flagSyncMsgSeqs)
         {
             super.handleUntaggedResponse(response);
+            int messageCountDelta = 0;
             if (response.mTag == null && response.size() > 1)
             {
                 try
@@ -2463,14 +2543,45 @@ public class ImapStore extends Store
                         {
                             Log.d(Email.LOG_TAG, "Got untagged FETCH for msgseq " + msgSeq + " for " + getLogId());
                         }
+                        if (flagSyncMsgSeqs.contains(msgSeq) == false)
+                        {
                             flagSyncMsgSeqs.add(msgSeq);
                         }
                     }
+                    if ("EXPUNGE".equals(responseType))
+                    {
+                        int msgSeq = response.getNumber(0);
+                        if (msgSeq <= oldMessageCount)
+                        {
+                            messageCountDelta = -1;
+                        }
+                        if (Email.DEBUG)
+                        {
+                            Log.d(Email.LOG_TAG, "Got untagged EXPUNGE for msgseq " + msgSeq + " for " + getLogId());
+                        }
+                        List<Integer> newSeqs = new ArrayList<Integer>();
+                        Iterator<Integer> flagIter = flagSyncMsgSeqs.iterator();
+                        while (flagIter.hasNext())
+                        {
+                            Integer flagMsg = flagIter.next();
+                            if (flagMsg >= msgSeq)
+                            {
+                                flagIter.remove();
+                                if (flagMsg > msgSeq)
+                                {
+                                    newSeqs.add(flagMsg--);
+                                }
+                            }
+                        }
+                        flagSyncMsgSeqs.addAll(newSeqs);
+                    }
+                }
                 catch (Exception e)
                 {
                     Log.e(Email.LOG_TAG, "Could not handle untagged FETCH for " + getLogId(), e);
                 }
             }
+            return messageCountDelta;
         }
 
 
@@ -2481,11 +2592,11 @@ public class ImapStore extends Store
             {
                 if (newArrivals)
                 {
-                    receiver.messagesArrived(getName(), messages);
+                    receiver.messagesArrived(this, messages);
                 }
                 else
                 {
-                    receiver.messagesFlagsChanged(getName(), messages);
+                    receiver.messagesFlagsChanged(this, messages);
                 }
             }
             catch (RuntimeException e)
@@ -2684,4 +2795,8 @@ public class ImapStore extends Store
         }
 
     }
+    private interface ImapSearcher
+    {
+        List<ImapResponse> search() throws IOException, MessagingException;
+    }
 }

src/com/android/email/mail/store/LocalStore.java

@@ -46,6 +46,7 @@ import com.android.email.mail.FetchProfile;
 import com.android.email.mail.Flag;
 import com.android.email.mail.Folder;
 import com.android.email.mail.Message;
+import com.android.email.mail.MessageRemovalListener;
 import com.android.email.mail.MessageRetrievalListener;
 import com.android.email.mail.MessagingException;
 import com.android.email.mail.Part;
@@ -799,6 +800,23 @@ public class LocalStore extends Store implements Serializable
             return mVisibleLimit;
         }
 
+        public void purgeToVisibleLimit(MessageRemovalListener listener) throws MessagingException
+        {
+            open(OpenMode.READ_WRITE);
+            Message[] messages = getMessages(null);
+            for (int i = 0; i < messages.length; i++)
+            {
+                if (i >= mVisibleLimit)
+                {
+                    if (listener != null)
+                    {
+                        listener.messageRemoved(messages[i]);
+                    }
+                    messages[i].setFlag(Flag.X_DESTROYED, true);
+                }
+            }
+        }
+
 
         public void setVisibleLimit(int visibleLimit) throws MessagingException
         {