From be1aa37dc516a9c3dd4af65b11f92a5951f5c5c3 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Fri, 22 Oct 2010 15:29:26 -0700 Subject: [PATCH] Don't use sendMailCallback() -- don't track outbox status There're differences between how SMTP and EAS use this callback. We should eventually unify the behavior, but till then let's not use sendMailCallback(). Bug 3116377 Change-Id: Ic5ecf16251c11ab2bd2e16e29bd417f1ece67f14 --- src/com/android/email/Controller.java | 20 ++++++ src/com/android/email/RefreshManager.java | 66 +++++++------------ .../android/email/activity/MessageListXL.java | 16 +++-- .../com/android/email/RefreshManagerTest.java | 66 +------------------ 4 files changed, 54 insertions(+), 114 deletions(-) diff --git a/src/com/android/email/Controller.java b/src/com/android/email/Controller.java index af19eb24a..a9ea48718 100644 --- a/src/com/android/email/Controller.java +++ b/src/com/android/email/Controller.java @@ -1072,6 +1072,26 @@ public class Controller { * Callback for sending pending messages. This will be called once to start the * group, multiple times for messages, and once to complete the group. * + * Unfortunately this callback works differently on SMTP and EAS. + * + * On SMTP: + * + * First, we get this. + * result == null, messageId == -1, progress == 0: start batch send + * + * Then we get these callbacks per message. + * (Exchange backend may skip "start sending one message".) + * result == null, messageId == xx, progress == 0: start sending one message + * result == xxxx, messageId == xx, progress == 0; failed sending one message + * + * Finally we get this. + * result == null, messageId == -1, progres == 100; finish sending batch + * + * On EAS: Almost same as above, except: + * + * - There's no first ("start batch send") callback. + * - accountId is always -1. + * * @param result If null, the operation completed without error * @param accountId The account being operated on * @param messageId The being sent (may be unknown at start) diff --git a/src/com/android/email/RefreshManager.java b/src/com/android/email/RefreshManager.java index 2e3d5ad0e..456a802b5 100644 --- a/src/com/android/email/RefreshManager.java +++ b/src/com/android/email/RefreshManager.java @@ -62,7 +62,22 @@ public class RefreshManager { private String mErrorMessage; public interface Listener { + /** + * Refresh status of a mailbox list or a message list has changed. + * + * @param accountId ID of the account. + * @param mailboxId -1 if it's about the mailbox list, or the ID of the mailbox list in + * question. + */ public void onRefreshStatusChanged(long accountId, long mailboxId); + + /** + * Error callback. + * + * @param accountId ID of the account, or -1 if unknown. + * @param mailboxId ID of the mailbox, or -1 if unknown. + * @param message error message which can be shown to the user. + */ public void onMessagingError(long accountId, long mailboxId, String message); } @@ -145,7 +160,6 @@ public class RefreshManager { private final RefreshStatusMap mMailboxListStatus = new RefreshStatusMap(); private final RefreshStatusMap mMessageListStatus = new RefreshStatusMap(); - private final RefreshStatusMap mOutboxStatus = new RefreshStatusMap(); /** * @return the singleton instance. @@ -248,11 +262,7 @@ public class RefreshManager { * Send pending messages. */ public boolean sendPendingMessages(long accountId) { - final Status status = mOutboxStatus.get(accountId); - if (!status.canRefresh()) return false; - Log.i(Email.LOG_TAG, "sendPendingMessages " + accountId); - status.onRefreshRequested(); notifyRefreshStatusChanged(accountId, -1); mController.sendPendingMessages(accountId); return true; @@ -285,10 +295,6 @@ public class RefreshManager { return mMessageListStatus.get(mailboxId).getLastRefreshTime(); } - public long getLastSendMessageTime(long accountId) { - return mOutboxStatus.get(accountId).getLastRefreshTime(); - } - public boolean isMailboxListRefreshing(long accountId) { return mMailboxListStatus.get(accountId).isRefreshing(); } @@ -297,10 +303,6 @@ public class RefreshManager { return mMessageListStatus.get(mailboxId).isRefreshing(); } - public boolean isSendingMessage(long accountId) { - return mOutboxStatus.get(accountId).isRefreshing(); - } - public boolean isRefreshingAnyMailboxList() { return mMailboxListStatus.isRefreshingAny(); } @@ -309,13 +311,8 @@ public class RefreshManager { return mMessageListStatus.isRefreshingAny(); } - public boolean isSendingAnyMessage() { - return mOutboxStatus.isRefreshingAny(); - } - - public boolean isRefreshingOrSendingAny() { - return isRefreshingAnyMailboxList() || isRefreshingAnyMessageList() - || isSendingAnyMessage(); + public boolean isRefreshingAny() { + return isRefreshingAnyMailboxList() || isRefreshingAnyMessageList(); } public String getErrorMessage() { @@ -347,10 +344,6 @@ public class RefreshManager { return mMessageListStatus.get(mailboxId); } - /* package */ Status getOutboxStatusForTest(long acountId) { - return mOutboxStatus.get(acountId); - } - private class ControllerResult extends Controller.Result { private boolean mSendMailExceptionReported = false; @@ -425,20 +418,8 @@ public class RefreshManager { /** * Send message progress callback. * - * This callback is overly overloaded: - * - * First, we get this. - * result == null, messageId == -1, progress == 0: start batch send - * - * Then we get these callbacks per message. - * (Exchange backend may skip "start sending one message".) - * result == null, messageId == xx, progress == 0: start sending one message - * result == xxxx, messageId == xx, progress == 0; failed sending one message - * - * Finally we get this. - * result == null, messageId == -1, progres == 100; finish sending batch - * - * So, let's just report the first exception we get, and ignore the rest. + * We don't keep track of the status of outboxes, but we monitor this to catch + * errors. */ @Override public void sendMailCallback(MessagingException exception, long accountId, long messageId, @@ -450,17 +431,14 @@ public class RefreshManager { if (progress == 0 && messageId == -1) { mSendMailExceptionReported = false; } - if (messageId == -1) { - // Update the status only for the batch start/end. - // (i.e. don't report for each message.) - mOutboxStatus.get(accountId).onCallback(exception, progress, mClock); - notifyRefreshStatusChanged(accountId, -1); - } if (exception != null && !mSendMailExceptionReported) { // Only the first error in a batch will be reported. mSendMailExceptionReported = true; reportError(accountId, messageId, exception.getUiErrorMessage(mContext)); } + if (progress == 100) { + mSendMailExceptionReported = false; + } } } } diff --git a/src/com/android/email/activity/MessageListXL.java b/src/com/android/email/activity/MessageListXL.java index c727084ad..86dcc33fc 100644 --- a/src/com/android/email/activity/MessageListXL.java +++ b/src/com/android/email/activity/MessageListXL.java @@ -534,13 +534,15 @@ public class MessageListXL extends Activity implements Utility.runAsync(new Runnable() { @Override public void run() { - Account account = Account.restoreAccountWithId(mContext, accountId); - String msg = message; - if (account != null) { - msg = account.mDisplayName + ": " + msg; - } - Utility.showToast(MessageListXL.this, msg); - }}); + String msg = message; + if (accountId != -1) { + Account account = Account.restoreAccountWithId(mContext, accountId); + if (account != null) { + msg = account.mDisplayName + ": " + msg; + } + } + Utility.showToast(MessageListXL.this, msg); + }}); // END STOPSHIP updateProgressIcon(); } diff --git a/tests/src/com/android/email/RefreshManagerTest.java b/tests/src/com/android/email/RefreshManagerTest.java index 123ad973a..7d002eab0 100644 --- a/tests/src/com/android/email/RefreshManagerTest.java +++ b/tests/src/com/android/email/RefreshManagerTest.java @@ -329,22 +329,10 @@ public class RefreshManagerTest extends InstrumentationTestCase { assertEquals(ACCOUNT_1, mController.mAccountId); assertEquals(-1, mController.mMailboxId); mController.reset(); - assertTrue(mTarget.isSendingMessage(ACCOUNT_1)); - assertTrue(mTarget.isSendingAnyMessage()); - - // Request again -- shouldn't be accepted. - assertFalse(mTarget.sendPendingMessages(ACCOUNT_1)); - - assertFalse(mListener.mCalledOnRefreshStatusChanged); - assertFalse(mListener.mCalledOnConnectionError); - mListener.reset(); - assertFalse(mController.mCalledSendPendingMessages); - mController.reset(); // request sending for account 2 assertTrue(mTarget.sendPendingMessages(ACCOUNT_2)); - assertTrue(mListener.mCalledOnRefreshStatusChanged); assertFalse(mListener.mCalledOnConnectionError); assertEquals(ACCOUNT_2, mListener.mAccountId); assertEquals(-1, mListener.mMailboxId); @@ -353,33 +341,24 @@ public class RefreshManagerTest extends InstrumentationTestCase { assertEquals(ACCOUNT_2, mController.mAccountId); assertEquals(-1, mController.mMailboxId); mController.reset(); - assertTrue(mTarget.isSendingMessage(ACCOUNT_2)); - assertTrue(mTarget.isSendingAnyMessage()); - // sending for account 1... + // Sending start for account 1... + // batch send start. (message id == -1, progress == 0) mController.mListener.sendMailCallback(null, ACCOUNT_1, -1, 0); - assertTrue(mListener.mCalledOnRefreshStatusChanged); assertFalse(mListener.mCalledOnConnectionError); - assertEquals(ACCOUNT_1, mListener.mAccountId); - assertEquals(-1, mListener.mMailboxId); mListener.reset(); - assertTrue(mTarget.isSendingMessage(ACCOUNT_1)); - assertEquals(0, mTarget.getOutboxStatusForTest(ACCOUNT_1).getLastRefreshTime()); - // Per message callback (1) + // Per message callback mController.mListener.sendMailCallback(null, ACCOUNT_1, 100, 0); mController.mListener.sendMailCallback(null, ACCOUNT_1, 101, 0); - // No callback per message - assertFalse(mListener.mCalledOnRefreshStatusChanged); assertFalse(mListener.mCalledOnConnectionError); mListener.reset(); // Exception -- first error will be reported. mController.mListener.sendMailCallback(EXCEPTION, ACCOUNT_1, 102, 0); - assertFalse(mListener.mCalledOnRefreshStatusChanged); assertTrue(mListener.mCalledOnConnectionError); assertEquals(EXCEPTION.getUiErrorMessage(mContext), mListener.mMessage); mListener.reset(); @@ -388,7 +367,6 @@ public class RefreshManagerTest extends InstrumentationTestCase { mController.mListener.sendMailCallback(null, ACCOUNT_1, 103, 0); mController.mListener.sendMailCallback(EXCEPTION, ACCOUNT_1, 104, 0); - assertFalse(mListener.mCalledOnRefreshStatusChanged); assertFalse(mListener.mCalledOnConnectionError); mListener.reset(); @@ -396,46 +374,8 @@ public class RefreshManagerTest extends InstrumentationTestCase { Log.w(Email.LOG_TAG, "" + mController.mListener.getClass()); mController.mListener.sendMailCallback(null, ACCOUNT_1, -1, 100); - assertTrue(mListener.mCalledOnRefreshStatusChanged); assertFalse(mListener.mCalledOnConnectionError); - assertEquals(ACCOUNT_1, mListener.mAccountId); - assertEquals(-1, mListener.mMailboxId); mListener.reset(); - assertFalse(mTarget.isSendingMessage(ACCOUNT_1)); - assertEquals(mClock.mTime, mTarget.getOutboxStatusForTest(ACCOUNT_1) - .getLastRefreshTime()); - - // Check "any" method. - assertTrue(mTarget.isSendingAnyMessage()); // still sending for account 2 - - // sending for account 2... - mClock.advance(); - - mController.mListener.sendMailCallback(null, ACCOUNT_2, -1, 0); - - assertTrue(mListener.mCalledOnRefreshStatusChanged); - assertFalse(mListener.mCalledOnConnectionError); - assertEquals(ACCOUNT_2, mListener.mAccountId); - assertEquals(-1, mListener.mMailboxId); - mListener.reset(); - assertTrue(mTarget.isSendingMessage(ACCOUNT_2)); - assertEquals(0, mTarget.getOutboxStatusForTest(ACCOUNT_2).getLastRefreshTime()); - - // Done with exception. - mController.mListener.sendMailCallback(EXCEPTION, ACCOUNT_2, -1, 0); - - assertTrue(mListener.mCalledOnRefreshStatusChanged); - assertTrue(mListener.mCalledOnConnectionError); - assertEquals(ACCOUNT_2, mListener.mAccountId); - assertEquals(-1, mListener.mMailboxId); - assertEquals(EXCEPTION.getUiErrorMessage(mContext), mListener.mMessage); - mListener.reset(); - assertFalse(mTarget.isSendingMessage(ACCOUNT_2)); - assertEquals(mClock.mTime, mTarget.getOutboxStatusForTest(ACCOUNT_2) - .getLastRefreshTime()); - - // Check "any" method. - assertFalse(mTarget.isSendingAnyMessage()); } public void testSendPendingMessagesForAllAccounts() throws Throwable {