Fix attachments in search results
b/11294681
We had some really broken logic about handling search
results.
In IMAP search, we would request, in a single pass,
FLAGS, ENVELOPE, STRUCTURE, and BODY_SANE. BODY_SANE means
the first N bytes of message content, whether it be from
the message text or attachments. This is different from how
sync works: In sync, we get FLAGS and ENVELOPE in one pass,
and in a later pass get STRUCTURE and first body part text
for each message.
If the total size of the message exceeded the maximum limit
for BODY_SANE, then we'd mark the message as partial, which
would cause us to create a dummy attachment in copyMessageToProvider().
This is a weird solution to the problem of POP messages not
being completely loaded, because in POP message body and
attachments can't be requested separately, so the dummy attachment
just signified that we needed to fetch more data.
This system fails completely on IMAP, because just fetching the
rest of the body will not get you the attachments.
But even if that code is disabled, attachments in search results
still didn't work properly. For reasons I don't yet understand,
if we requet both STRUCTURE and BODY_SANE at the same time, either
we don't received the full attachment metadata, or we ignore it, and
only use the attachments whose contents could actually fit in the
limit imposed by BODY_SANE. So attachments that didn't fit,
or didn't completely fit, would either be missing or corrupt
and unretriveable.
So, end result: It's not clear why we were trying to load
BODY_SANE all in one pass, unlike how it works for sync.
In fact, the way sync does it now makes a lot of sense: We
load FLAGS and ENVELOPE data (small) and put the in the DB
immediately so they can be displayed. In the second pass we
load the (potentially large) structure and message body. If this
is the right solution for sync, it's probably the right solution
for search. So now, that's what we do.
There is cleanup I'd like to do post MR1: Some code is duplicated
between sync and search that could be consolidated, but we're in
low risk mode now so I only changed search code.
Change-Id: I11475e290cda04b91f76d38ba952679e8e8964d5
diff --git a/src/com/android/email/mail/store/ImapFolder.java b/src/com/android/email/mail/store/ImapFolder.java
index 0dd79b5..935e09e 100644
--- a/src/com/android/email/mail/store/ImapFolder.java
+++ b/src/com/android/email/mail/store/ImapFolder.java
@@ -657,10 +657,13 @@
fetchFields.add(ImapConstants.FETCH_FIELD_BODY_PEEK);
}
+ // TODO Why are we only fetching the first part given?
final Part fetchPart = fp.getFirstPart();
if (fetchPart != null) {
final String[] partIds =
fetchPart.getHeader(MimeHeader.HEADER_ANDROID_ATTACHMENT_STORE_DATA);
+ // TODO Why can a single part have more than one Id? And why should we only fetch
+ // the first id if there are more than one?
if (partIds != null) {
fetchFields.add(ImapConstants.FETCH_FIELD_BODY_PEEK_BARE
+ "[" + partIds[0] + "]");
@@ -725,7 +728,7 @@
parseBodyStructure(bs, message, ImapConstants.TEXT);
} catch (MessagingException e) {
if (Logging.LOGD) {
- LogUtils.v(Logging.LOG_TAG, "Error handling message", e);
+ LogUtils.v(Logging.LOG_TAG, e, "Error handling message");
}
message.setBody(null);
}
diff --git a/src/com/android/email/provider/Utilities.java b/src/com/android/email/provider/Utilities.java
index d0372b4..d66f73d 100644
--- a/src/com/android/email/provider/Utilities.java
+++ b/src/com/android/email/provider/Utilities.java
@@ -98,7 +98,6 @@
public static void copyOneMessageToProvider(Context context, Message message,
EmailContent.Message localMessage, int loadStatus) {
try {
-
EmailContent.Body body = null;
if (localMessage.mId != EmailContent.Message.NO_MESSAGE) {
body = EmailContent.Body.restoreBodyWithMessageId(context, localMessage.mId);
@@ -143,12 +142,19 @@
// Since we haven't actually loaded the attachment, we're just putting
// a dummy placeholder here. When the user taps on it, we'll load the attachment
// for real.
- // TODO: This is not really a great way to model this.... could we at least get
- // the file names and mime types from the attachments? Then we could display
- // something sensible at least. This may be impossible in POP, but maybe
- // we could put a flag on the message indicating that there are undownloaded
- // attachments, rather than this dummy placeholder, which causes a lot of
- // special case handling in a lot of places.
+ // TODO: This is not a great way to model this. What we're saying is, we don't
+ // have the complete message, without paying any attention to what we do have.
+ // Did the main body exceed the maximum initial size? If so, we really might
+ // not have any attachments at all, and we just need a button somewhere that
+ // says "load the rest of the message".
+ // Or, what if we were able to load some, but not all of the attachments?
+ // Then we should ideally not be dropping the data we have on the floor.
+ // Also, what behavior we have here really should be based on what protocol
+ // we're dealing with. If it's POP, then we don't actually know how many
+ // attachments we have until we've loaded the complete message.
+ // If it's IMAP, we should know that, and we should load all attachment
+ // metadata we can get, regardless of whether or not we have the complete
+ // message body.
att.mFileName = "";
att.mSize = message.getSize();
att.mMimeType = "text/plain";
diff --git a/src/com/android/email/service/ImapService.java b/src/com/android/email/service/ImapService.java
index f8eef79..6009845 100644
--- a/src/com/android/email/service/ImapService.java
+++ b/src/com/android/email/service/ImapService.java
@@ -310,7 +310,6 @@
LogUtils.e(Logging.LOG_TAG,
"Error while copying downloaded message." + me);
}
-
}
}
catch (Exception e) {
@@ -592,8 +591,9 @@
localMessageMap, unseenMessages);
}
- // 11. Refresh the flags for any messages in the local store that we
- // didn't just download.
+ // 11. Refresh the flags for any messages in the local store that we didn't just download.
+ // TODO This is a bit wasteful because we're also updating any messages we already did get
+ // the flags and envelope for previously.
FetchProfile fp = new FetchProfile();
fp.add(FetchProfile.Item.FLAGS);
remoteFolder.fetch(remoteMessages, fp, null);
@@ -1461,21 +1461,25 @@
for (int i = searchParams.mOffset; i < numToLoad + searchParams.mOffset; i++) {
messageList.add(sortableMessages[i].mMessage);
}
- // Get everything in one pass, rather than two (as in sync); this starts getting us
- // usable results quickly.
- FetchProfile fp = new FetchProfile();
+ // First fetch FLAGS and ENVELOPE. In a second pass, we'll fetch STRUCTURE and
+ // the first body part.
+ final FetchProfile fp = new FetchProfile();
fp.add(FetchProfile.Item.FLAGS);
fp.add(FetchProfile.Item.ENVELOPE);
- fp.add(FetchProfile.Item.STRUCTURE);
- fp.add(FetchProfile.Item.BODY_SANE);
- remoteFolder.fetch(messageList.toArray(new Message[messageList.size()]), fp,
- new MessageRetrievalListener() {
+
+ Message[] messageArray = messageList.toArray(new Message[messageList.size()]);
+
+ // TODO: Why should we do this with a messageRetrievalListener? It updates the messages
+ // directly in the messageArray. After making this call, we could simply walk it
+ // and do all of these operations ourselves.
+ remoteFolder.fetch(messageArray, fp, new MessageRetrievalListener() {
@Override
public void messageRetrieved(Message message) {
+ // TODO: Why do we have two separate try/catch blocks here?
+ // After MR1, we should consolidate this.
try {
- // Determine if the new message was already known (e.g. partial)
- // And create or reload the full message info.
EmailContent.Message localMessage = new EmailContent.Message();
+
try {
// Copy the fields that are available into the message
LegacyConversions.updateMessageFields(localMessage,
@@ -1491,10 +1495,8 @@
// This will be used by loadMessageForView, etc. to use the proper remote
// folder
localMessage.mProtocolSearchInfo = mailbox.mServerId;
- if (message.getSize() > Store.FETCH_BODY_SANE_SUGGESTED_SIZE) {
- flag = EmailContent.Message.FLAG_LOADED_PARTIAL;
- }
- Utilities.copyOneMessageToProvider(context, message, localMessage, flag);
+ // Commit the message to the local store
+ Utilities.saveOrUpdate(localMessage, context);
} catch (MessagingException me) {
LogUtils.e(Logging.LOG_TAG,
"Error while copying downloaded message." + me);
@@ -1509,6 +1511,34 @@
public void loadAttachmentProgress(int progress) {
}
});
+
+ // Now load the structure for all of the messages:
+ fp.clear();
+ fp.add(FetchProfile.Item.STRUCTURE);
+ remoteFolder.fetch(messageArray, fp, null);
+
+ // Finally, load the first body part (i.e. message text).
+ // This means attachment contents are not yet loaded, but that's okay,
+ // we'll load them as needed, same as in synced messages.
+ Message [] oneMessageArray = new Message[1];
+ for (Message message : messageArray) {
+ // Build a list of parts we are interested in. Text parts will be downloaded
+ // right now, attachments will be left for later.
+ ArrayList<Part> viewables = new ArrayList<Part>();
+ ArrayList<Part> attachments = new ArrayList<Part>();
+ MimeUtility.collectParts(message, viewables, attachments);
+ // Download the viewables immediately
+ oneMessageArray[0] = message;
+ for (Part part : viewables) {
+ fp.clear();
+ fp.add(part);
+ remoteFolder.fetch(oneMessageArray, fp, null);
+ }
+ // Store the updated message locally and mark it fully loaded
+ Utilities.copyOneMessageToProvider(context, message, account, destMailbox,
+ EmailContent.Message.FLAG_LOADED_COMPLETE);
+ }
+
// Tell UI that we're done loading messages
statusValues.put(Mailbox.SYNC_TIME, System.currentTimeMillis());
statusValues.put(Mailbox.UI_SYNC_STATUS, UIProvider.SyncStatus.NO_SYNC);
