From aef9515ee70f1f0b6cc4fa601078597b55831331 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Fri, 10 Dec 2010 13:36:18 -0800 Subject: [PATCH] Controller.Result callbacks should all have accountId - Added accountId to loadAttachmentCallback/loadMessageForViewCallback - Cleaned up LegacyListener/MessagingListener. Removed the constructors which take messageId and attachmentId, which are used to bridge loadAttachmentProgress, which the callsite doesn't know these IDs. The inconsistency (only loadAttachmentProgress() uses the member messageId) doesn't look too good, so extracted this into a separate class, MessageRetrievalListenerBridge. Change-Id: I46303e50df2b0e1fe8616e7c9cef632ac14f23aa --- src/com/android/email/Controller.java | 83 ++++++++++++------- .../ControllerResultUiThreadWrapper.java | 9 +- .../android/email/GroupMessagingListener.java | 10 +-- .../android/email/MessagingController.java | 3 +- src/com/android/email/MessagingListener.java | 10 --- .../android/email/activity/MessageListXL.java | 50 ++--------- .../activity/MessageViewFragmentBase.java | 8 +- .../android/email/provider/EmailContent.java | 14 +++- .../android/email/provider/ProviderTests.java | 16 ++++ 9 files changed, 97 insertions(+), 106 deletions(-) diff --git a/src/com/android/email/Controller.java b/src/com/android/email/Controller.java index 5ab0036ab..7a3b8022e 100644 --- a/src/com/android/email/Controller.java +++ b/src/com/android/email/Controller.java @@ -395,9 +395,10 @@ public class Controller { cv.put(MessageColumns.FLAG_LOADED, Message.FLAG_LOADED_COMPLETE); mProviderContext.getContentResolver().update(uri, cv, null, null); Log.d(Email.LOG_TAG, "Unexpected loadMessageForView() for service-based message."); + final long accountId = Account.getAccountIdForMessageId(mProviderContext, messageId); synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadMessageForViewCallback(null, messageId, 100); + listener.loadMessageForViewCallback(null, accountId, messageId, 100); } } } else { @@ -871,10 +872,10 @@ public class Controller { // This presumably is for POP3 messages synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadAttachmentCallback(null, messageId, attachmentId, 0); + listener.loadAttachmentCallback(null, accountId, messageId, attachmentId, 0); } for (Result listener : mListeners) { - listener.loadAttachmentCallback(null, messageId, attachmentId, 100); + listener.loadAttachmentCallback(null, accountId, messageId, attachmentId, 100); } } return; @@ -1080,8 +1081,8 @@ public class Controller { * @param messageId the message which contains the attachment * @param progress 0 for "starting", 1..99 for updates (if needed in UI), 100 for complete */ - public void loadMessageForViewCallback(MessagingException result, long messageId, - int progress) { + public void loadMessageForViewCallback(MessagingException result, long accountId, + long messageId, int progress) { } /** @@ -1092,8 +1093,8 @@ public class Controller { * @param attachmentId the attachment being loaded * @param progress 0 for "starting", 1..99 for updates (if needed in UI), 100 for complete */ - public void loadAttachmentCallback(MessagingException result, long messageId, - long attachmentId, int progress) { + public void loadAttachmentCallback(MessagingException result, long accountId, + long messageId, long attachmentId, int progress) { } /** @@ -1151,15 +1152,41 @@ public class Controller { } } + /** + * Bridge to intercept {@link MessageRetrievalListener#loadAttachmentProgress} and + * pass down to {@link Result}. + */ + public class MessageRetrievalListenerBridge implements MessageRetrievalListener { + private final long mMessageId; + private final long mAttachmentId; + private final long mAccountId; + + public MessageRetrievalListenerBridge(long messageId, long attachmentId) { + mMessageId = messageId; + mAttachmentId = attachmentId; + mAccountId = Account.getAccountIdForMessageId(mProviderContext, mMessageId); + } + + @Override + public void loadAttachmentProgress(int progress) { + synchronized (mListeners) { + for (Result listener : mListeners) { + listener.loadAttachmentCallback(null, mAccountId, mMessageId, mAttachmentId, + progress); + } + } + } + + @Override + public void messageRetrieved(com.android.email.mail.Message message) { + } + } + /** * Support for receiving callbacks from MessagingController and dealing with UI going * out of scope. */ - public class LegacyListener extends MessagingListener implements MessageRetrievalListener { - public LegacyListener(long messageId, long attachmentId) { - super(messageId, attachmentId); - } - + public class LegacyListener extends MessagingListener { public LegacyListener() { } @@ -1244,28 +1271,31 @@ public class Controller { @Override public void loadMessageForViewStarted(long messageId) { + final long accountId = Account.getAccountIdForMessageId(mProviderContext, messageId); synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadMessageForViewCallback(null, messageId, 0); + listener.loadMessageForViewCallback(null, accountId, messageId, 0); } } } @Override public void loadMessageForViewFinished(long messageId) { + final long accountId = Account.getAccountIdForMessageId(mProviderContext, messageId); synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadMessageForViewCallback(null, messageId, 100); + listener.loadMessageForViewCallback(null, accountId, messageId, 100); } } } @Override public void loadMessageForViewFailed(long messageId, String message) { + final long accountId = Account.getAccountIdForMessageId(mProviderContext, messageId); synchronized (mListeners) { for (Result listener : mListeners) { listener.loadMessageForViewCallback(new MessagingException(message), - messageId, 0); + accountId, messageId, 0); } } } @@ -1280,7 +1310,7 @@ public class Controller { } synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadAttachmentCallback(null, messageId, attachmentId, 0); + listener.loadAttachmentCallback(null, accountId, messageId, attachmentId, 0); } } } @@ -1294,20 +1324,11 @@ public class Controller { } synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadAttachmentCallback(null, messageId, attachmentId, 100); + listener.loadAttachmentCallback(null, accountId, messageId, attachmentId, 100); } } } - @Override - public void loadAttachmentProgress(int progress) { - synchronized (mListeners) { - for (Result listener : mListeners) { - listener.loadAttachmentCallback(null, messageId, attachmentId, progress); - } - } - } - @Override public void loadAttachmentFailed(long accountId, long messageId, long attachmentId, MessagingException me) { @@ -1324,7 +1345,7 @@ public class Controller { } synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadAttachmentCallback(me, messageId, attachmentId, 0); + listener.loadAttachmentCallback(me, accountId, messageId, attachmentId, 0); } } } @@ -1362,10 +1383,6 @@ public class Controller { } } } - - @Override - public void messageRetrieved(com.android.email.mail.Message message) { - } } /** @@ -1393,9 +1410,11 @@ public class Controller { } break; } + final long accountId = Account.getAccountIdForMessageId(mProviderContext, messageId); synchronized (mListeners) { for (Result listener : mListeners) { - listener.loadAttachmentCallback(result, messageId, attachmentId, progress); + listener.loadAttachmentCallback(result, accountId, messageId, attachmentId, + progress); } } } diff --git a/src/com/android/email/ControllerResultUiThreadWrapper.java b/src/com/android/email/ControllerResultUiThreadWrapper.java index c50e6572a..8c9d855cb 100644 --- a/src/com/android/email/ControllerResultUiThreadWrapper.java +++ b/src/com/android/email/ControllerResultUiThreadWrapper.java @@ -57,7 +57,7 @@ public class ControllerResultUiThreadWrapper extends Result { @Override public void loadAttachmentCallback(final MessagingException result, final long messageId, - final long attachmentId, final int progress) { + final long accountId, final long attachmentId, final int progress) { run(new Runnable() { public void run() { /* It's possible this callback is unregistered after this Runnable was posted and @@ -65,18 +65,19 @@ public class ControllerResultUiThreadWrapper extends Result { * on the UI thread. */ if (!isRegistered()) return; - mWrappee.loadAttachmentCallback(result, messageId, attachmentId, progress); + mWrappee.loadAttachmentCallback(result, accountId, messageId, attachmentId, + progress); } }); } @Override - public void loadMessageForViewCallback(final MessagingException result, + public void loadMessageForViewCallback(final MessagingException result, final long accountId, final long messageId, final int progress) { run(new Runnable() { public void run() { if (!isRegistered()) return; - mWrappee.loadMessageForViewCallback(result, messageId, progress); + mWrappee.loadMessageForViewCallback(result, accountId, messageId, progress); } }); } diff --git a/src/com/android/email/GroupMessagingListener.java b/src/com/android/email/GroupMessagingListener.java index 8ee609c82..45924593c 100644 --- a/src/com/android/email/GroupMessagingListener.java +++ b/src/com/android/email/GroupMessagingListener.java @@ -26,7 +26,7 @@ import java.util.concurrent.ConcurrentHashMap; public class GroupMessagingListener extends MessagingListener { /* The synchronization of the methods in this class is not needed because we use ConcurrentHashMap. - + Nevertheless, let's keep the "synchronized" for a while in the case we may want to change the implementation to use something else than ConcurrentHashMap. @@ -171,14 +171,6 @@ public class GroupMessagingListener extends MessagingListener { } } - @Override - synchronized public void loadAttachmentProgress( - int progress) { - for (MessagingListener l : mListeners) { - l.loadAttachmentProgress(progress); - } - } - @Override synchronized public void loadAttachmentFinished( long accountId, diff --git a/src/com/android/email/MessagingController.java b/src/com/android/email/MessagingController.java index 44ec5db0a..38ad539ac 100644 --- a/src/com/android/email/MessagingController.java +++ b/src/com/android/email/MessagingController.java @@ -1936,7 +1936,8 @@ public class MessagingController implements Runnable { FetchProfile fp = new FetchProfile(); fp.add(storePart); remoteFolder.fetch(new Message[] { storeMessage }, fp, - mController.new LegacyListener(messageId, attachmentId)); + mController.new MessageRetrievalListenerBridge( + messageId, attachmentId)); // If we failed to load the attachment, throw an Exception here, so that // AttachmentDownloadService knows that we failed diff --git a/src/com/android/email/MessagingListener.java b/src/com/android/email/MessagingListener.java index 9c5ee5f83..3d12a945e 100644 --- a/src/com/android/email/MessagingListener.java +++ b/src/com/android/email/MessagingListener.java @@ -28,14 +28,6 @@ import android.content.Context; * changes in this class. */ public class MessagingListener { - long messageId = -1; - long attachmentId = -1; - - public MessagingListener(long _messageId, long _attachmentId) { - messageId = _messageId; - attachmentId = _attachmentId; - } - public MessagingListener() { } @@ -94,8 +86,6 @@ public class MessagingListener { boolean requiresDownload) { } - public void loadAttachmentProgress(int progress) {} - public void loadAttachmentFinished( long accountId, long messageId, diff --git a/src/com/android/email/activity/MessageListXL.java b/src/com/android/email/activity/MessageListXL.java index 0fbe89dba..188cdcef9 100644 --- a/src/com/android/email/activity/MessageListXL.java +++ b/src/com/android/email/activity/MessageListXL.java @@ -847,56 +847,18 @@ public class MessageListXL extends Activity implements } @Override - public void loadAttachmentCallback( - MessagingException result, long messageId, long attachmentId, int progress) { - new AccountFinder(result, messageId, progress).execute(); + public void loadAttachmentCallback(MessagingException result, long accountId, + long messageId, long attachmentId, int progress) { + handleError(result, accountId, progress); } @Override - public void loadMessageForViewCallback( - MessagingException result, long messageId, int progress) { - new AccountFinder(result, messageId, progress).execute(); - } - - /** - * AsyncTask to determine the account id from a message id. Used for - * {@link #loadAttachmentCallback} and {@link #loadMessageForViewCallback}, which don't - * report the underlying account ID. - */ - private class AccountFinder extends AsyncTask { - private final MessagingException mException; - private final long mMessageId; - private final int mProgress; - - public AccountFinder(MessagingException exception, long messageId, int progress) { - mException = exception; - mMessageId = messageId; - mProgress = progress; - } - - @Override - protected Long doInBackground(Void... params) { - if (mMessageId == -1) { - return null; // Message ID unknown - } - Message m = Message.restoreMessageWithId(MessageListXL.this, mMessageId); - return m != null ? m.mAccountKey : null; - } - - @Override - protected void onPostExecute(Long accountId) { - if ((accountId == null) || isCancelled()) { - return; - } - handleError(mException, accountId, mProgress); - } + public void loadMessageForViewCallback(MessagingException result, long accountId, + long messageId, int progress) { + handleError(result, accountId, progress); } private void handleError(MessagingException result, long accountId, int progress) { - if (!isRegistered()) { - // This ControllerResult may be already unregistered, because of the asynctask. - return; - } if (accountId == -1) { return; } diff --git a/src/com/android/email/activity/MessageViewFragmentBase.java b/src/com/android/email/activity/MessageViewFragmentBase.java index 9dc20e1c5..eab6d1b3a 100644 --- a/src/com/android/email/activity/MessageViewFragmentBase.java +++ b/src/com/android/email/activity/MessageViewFragmentBase.java @@ -1374,8 +1374,8 @@ public abstract class MessageViewFragmentBase extends Fragment implements View.O } @Override - public void loadMessageForViewCallback(MessagingException result, long messageId, - int progress) { + public void loadMessageForViewCallback(MessagingException result, long accountId, + long messageId, int progress) { if (messageId != mWaitForLoadMessageId) { // We are not waiting for this message to load, so exit quickly return; @@ -1414,8 +1414,8 @@ public abstract class MessageViewFragmentBase extends Fragment implements View.O } @Override - public void loadAttachmentCallback(MessagingException result, long messageId, - long attachmentId, int progress) { + public void loadAttachmentCallback(MessagingException result, long accountId, + long messageId, long attachmentId, int progress) { if (messageId == mMessageId) { if (result == null) { showAttachmentProgress(attachmentId, progress); diff --git a/src/com/android/email/provider/EmailContent.java b/src/com/android/email/provider/EmailContent.java index 5e7ddf907..67f8ac9bf 100644 --- a/src/com/android/email/provider/EmailContent.java +++ b/src/com/android/email/provider/EmailContent.java @@ -1507,6 +1507,17 @@ public abstract class EmailContent { return null; } + /** + * Return the account ID for a message with a given id + * + * @param context the caller's context + * @param messageId the id of the message + * @return the account ID, or -1 if the account doesn't exist + */ + public static long getAccountIdForMessageId(Context context, long messageId) { + return Message.getKeyColumnLong(context, messageId, MessageColumns.ACCOUNT_KEY); + } + /** * Return the account for a message with a given id * @param context the caller's context @@ -1514,8 +1525,7 @@ public abstract class EmailContent { * @return the account, or null if the account doesn't exist */ public static Account getAccountForMessageId(Context context, long messageId) { - long accountId = Message.getKeyColumnLong(context, messageId, - MessageColumns.ACCOUNT_KEY); + long accountId = getAccountIdForMessageId(context, messageId); if (accountId != -1) { return Account.restoreAccountWithId(context, accountId); } diff --git a/tests/src/com/android/email/provider/ProviderTests.java b/tests/src/com/android/email/provider/ProviderTests.java index 74e81b371..c612c7080 100644 --- a/tests/src/com/android/email/provider/ProviderTests.java +++ b/tests/src/com/android/email/provider/ProviderTests.java @@ -2069,6 +2069,22 @@ public class ProviderTests extends ProviderTestCase2 { assertEquals(b2.mId, Message.getKeyColumnLong(c, m2.mId, MessageColumns.MAILBOX_KEY)); } + public void testGetAccountIdForMessageId() { + final Context c = mMockContext; + Account a1 = ProviderTestUtils.setupAccount("acct1", true, c); + Account a2 = ProviderTestUtils.setupAccount("acct2", true, c); + Mailbox b1 = ProviderTestUtils.setupMailbox("box1", a1.mId, true, c, Mailbox.TYPE_MAIL); + Mailbox b2 = ProviderTestUtils.setupMailbox("box2", a2.mId, true, c, Mailbox.TYPE_MAIL); + Message m1 = createMessage(c, b1, false, false); + Message m2 = createMessage(c, b2, false, false); + + assertEquals(a1.mId, Account.getAccountIdForMessageId(c, m1.mId)); + assertEquals(a2.mId, Account.getAccountIdForMessageId(c, m2.mId)); + + // message desn't exist + assertEquals(-1, Account.getAccountIdForMessageId(c, 12345)); + } + public void testGetAccountMailboxFromMessageId() { final Context c = mMockContext; Account a = ProviderTestUtils.setupAccount("acct", true, c);