Merge branch '1585-check-attachment-content-type' into 'master'

Improve handling of missing attachments in UI

See merge request briar/briar!1142

briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentCreator.java

@@ -98,7 +98,7 @@ public class AttachmentCreator {
 		// get and cache AttachmentItem for ImagePreview
 		try {
 			Attachment a = retriever.getMessageAttachment(h);
-			AttachmentItem item = retriever.getAttachmentItem(h, a, needsSize);
+			AttachmentItem item = retriever.getAttachmentItem(a, needsSize);
 			if (item.hasError()) throw new IOException();
 			AttachmentItemResult itemResult =
 					new AttachmentItemResult(uri, item);

briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java

@@ -68,7 +68,7 @@ public class AttachmentItem implements Parcelable {
 		header = new AttachmentHeader(messageId, mimeType);
 	}
 
-	AttachmentHeader getHeader() {
+	public AttachmentHeader getHeader() {
 		return header;
 	}
 

briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java

@@ -9,8 +9,6 @@ import android.webkit.MimeTypeMap;
 
 import com.bumptech.glide.util.MarkEnforcingInputStream;
 
-import org.briarproject.bramble.api.Pair;
-import org.briarproject.bramble.api.db.DatabaseExecutor;
 import org.briarproject.bramble.api.db.DbException;
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
 import org.briarproject.bramble.api.sync.MessageId;
@@ -22,7 +20,6 @@ import org.briarproject.briar.api.messaging.MessagingManager;
 import java.io.BufferedInputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
@@ -38,9 +35,7 @@ import static android.support.media.ExifInterface.TAG_ORIENTATION;
 import static java.util.logging.Level.WARNING;
 import static java.util.logging.Logger.getLogger;
 import static org.briarproject.bramble.util.IoUtils.tryToClose;
-import static org.briarproject.bramble.util.LogUtils.logDuration;
 import static org.briarproject.bramble.util.LogUtils.logException;
-import static org.briarproject.bramble.util.LogUtils.now;
 
 @NotNullByDefault
 public class AttachmentRetriever {
@@ -93,7 +88,8 @@ public class AttachmentRetriever {
 		});
 	}
 
-	public void cachePut(MessageId messageId, List<AttachmentItem> attachments) {
+	public void cachePut(MessageId messageId,
+			List<AttachmentItem> attachments) {
 		attachmentCache.put(messageId, attachments);
 	}
 
@@ -102,48 +98,17 @@ public class AttachmentRetriever {
 		return attachmentCache.get(messageId);
 	}
 
-	@DatabaseExecutor
-	public List<Pair<AttachmentHeader, Attachment>> getMessageAttachments(
-			List<AttachmentHeader> headers) throws DbException {
-		long start = now();
-		List<Pair<AttachmentHeader, Attachment>> attachments =
-				new ArrayList<>(headers.size());
-		for (AttachmentHeader h : headers) {
-			Attachment a = messagingManager.getAttachment(h.getMessageId());
-			attachments.add(new Pair<>(h, a));
-		}
-		logDuration(LOG, "Loading attachments", start);
-		return attachments;
-	}
-
-	@DatabaseExecutor
-	Attachment getMessageAttachment(AttachmentHeader h) throws DbException {
-		return messagingManager.getAttachment(h.getMessageId());
-	}
-
-	/**
-	 * Creates {@link AttachmentItem}s from the passed headers and Attachments.
-	 * <p>
-	 * Note: This closes the {@link Attachment}'s {@link InputStream}.
-	 */
-	public List<AttachmentItem> getAttachmentItems(
-			List<Pair<AttachmentHeader, Attachment>> attachments) {
-		boolean needsSize = attachments.size() == 1;
-		List<AttachmentItem> items = new ArrayList<>(attachments.size());
-		for (Pair<AttachmentHeader, Attachment> a : attachments) {
-			AttachmentItem item =
-					getAttachmentItem(a.getFirst(), a.getSecond(), needsSize);
-			items.add(item);
-		}
-		return items;
+	public Attachment getMessageAttachment(AttachmentHeader h)
+			throws DbException {
+		return messagingManager.getAttachment(h);
 	}
 
 	/**
 	 * Creates an {@link AttachmentItem} from the {@link Attachment}'s
 	 * {@link InputStream} which will be closed when this method returns.
 	 */
-	AttachmentItem getAttachmentItem(AttachmentHeader h, Attachment a,
-			boolean needsSize) {
+	public AttachmentItem getAttachmentItem(Attachment a, boolean needsSize) {
+		AttachmentHeader h = a.getHeader();
 		if (!needsSize) {
 			String extension =
 					imageHelper.getExtensionFromMimeType(h.getContentType());

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java

@@ -37,6 +37,7 @@ import org.briarproject.bramble.api.contact.event.ContactRemovedEvent;
 import org.briarproject.bramble.api.db.DatabaseExecutor;
 import org.briarproject.bramble.api.db.DbException;
 import org.briarproject.bramble.api.db.NoSuchContactException;
+import org.briarproject.bramble.api.db.NoSuchMessageException;
 import org.briarproject.bramble.api.event.Event;
 import org.briarproject.bramble.api.event.EventBus;
 import org.briarproject.bramble.api.event.EventListener;
@@ -81,6 +82,7 @@ import org.briarproject.briar.api.messaging.Attachment;
 import org.briarproject.briar.api.messaging.AttachmentHeader;
 import org.briarproject.briar.api.messaging.MessagingManager;
 import org.briarproject.briar.api.messaging.PrivateMessageHeader;
+import org.briarproject.briar.api.messaging.event.AttachmentReceivedEvent;
 import org.briarproject.briar.api.privategroup.invitation.GroupInvitationManager;
 
 import java.util.ArrayList;
@@ -107,10 +109,12 @@ import static android.support.v7.util.SortedList.INVALID_POSITION;
 import static android.view.Gravity.RIGHT;
 import static android.widget.Toast.LENGTH_SHORT;
 import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
 import static java.util.Collections.sort;
 import static java.util.Objects.requireNonNull;
 import static java.util.logging.Level.INFO;
 import static java.util.logging.Level.WARNING;
+import static java.util.logging.Logger.getLogger;
 import static org.briarproject.bramble.util.LogUtils.logDuration;
 import static org.briarproject.bramble.util.LogUtils.logException;
 import static org.briarproject.bramble.util.LogUtils.now;
@@ -138,7 +142,7 @@ public class ConversationActivity extends BriarActivity
 	public static final String CONTACT_ID = "briar.CONTACT_ID";
 
 	private static final Logger LOG =
-			Logger.getLogger(ConversationActivity.class.getName());
+			getLogger(ConversationActivity.class.getName());
 
 	private static final int TRANSITION_DURATION_MS = 500;
 	private static final int ONBOARDING_DELAY_MS = 250;
@@ -171,6 +175,8 @@ public class ConversationActivity extends BriarActivity
 	volatile GroupInvitationManager groupInvitationManager;
 
 	private final Map<MessageId, String> textCache = new ConcurrentHashMap<>();
+	private final Map<MessageId, PrivateMessageHeader> missingAttachments =
+			new ConcurrentHashMap<>();
 	private final Observer<String> contactNameObserver = name -> {
 		requireNonNull(name);
 		loadMessages();
@@ -434,29 +440,40 @@ public class ConversationActivity extends BriarActivity
 		});
 	}
 
-	private void eagerlyLoadMessageSize(PrivateMessageHeader h)
-			throws DbException {
-		MessageId id = h.getId();
-		// If the message has text, load it
-		if (h.hasText()) {
-			String text = textCache.get(id);
-			if (text == null) {
-				LOG.info("Eagerly loading text for latest message");
-				text = messagingManager.getMessageText(id);
-				textCache.put(id, requireNonNull(text));
+	private void eagerlyLoadMessageSize(PrivateMessageHeader h) {
+		try {
+			MessageId id = h.getId();
+			// If the message has text, load it
+			if (h.hasText()) {
+				String text = textCache.get(id);
+				if (text == null) {
+					LOG.info("Eagerly loading text for latest message");
+					text = messagingManager.getMessageText(id);
+					textCache.put(id, requireNonNull(text));
+				}
 			}
-		}
-		// If the message has a single image, load its size - for multiple
-		// images we use a grid so the size is fixed
-		if (h.getAttachmentHeaders().size() == 1) {
-			List<AttachmentItem> items = attachmentRetriever.cacheGet(id);
-			if (items == null) {
-				LOG.info("Eagerly loading image size for latest message");
-				items = attachmentRetriever.getAttachmentItems(
-						attachmentRetriever.getMessageAttachments(
-								h.getAttachmentHeaders()));
-				attachmentRetriever.cachePut(id, items);
+			// If the message has a single image, load its size - for multiple
+			// images we use a grid so the size is fixed
+			List<AttachmentHeader> headers = h.getAttachmentHeaders();
+			if (headers.size() == 1) {
+				List<AttachmentItem> items = attachmentRetriever.cacheGet(id);
+				if (items == null) {
+					LOG.info("Eagerly loading image size for latest message");
+					AttachmentHeader header = headers.get(0);
+					try {
+						Attachment a = attachmentRetriever
+								.getMessageAttachment(header);
+						AttachmentItem item =
+								attachmentRetriever.getAttachmentItem(a, true);
+						attachmentRetriever.cachePut(id, singletonList(item));
+					} catch (NoSuchMessageException e) {
+						LOG.info("Attachment not received yet");
+						missingAttachments.put(header.getMessageId(), h);
+					}
+				}
 			}
+		} catch (DbException e) {
+			logException(LOG, WARNING, e);
 		}
 	}
 
@@ -534,16 +551,30 @@ public class ConversationActivity extends BriarActivity
 				&& adapter.isScrolledToBottom(layoutManager);
 	}
 
-	private void loadMessageAttachments(MessageId messageId,
-			List<AttachmentHeader> headers) {
+	private void loadMessageAttachments(PrivateMessageHeader h) {
+		// TODO: Use placeholders for missing/invalid attachments
 		runOnDbThread(() -> {
 			try {
-				List<Pair<AttachmentHeader, Attachment>> attachments =
-						attachmentRetriever.getMessageAttachments(headers);
 				// TODO move getting the items off to IoExecutor, if size == 1
-				List<AttachmentItem> items =
-						attachmentRetriever.getAttachmentItems(attachments);
-				displayMessageAttachments(messageId, items);
+				List<AttachmentHeader> headers = h.getAttachmentHeaders();
+				boolean needsSize = headers.size() == 1;
+				List<AttachmentItem> items = new ArrayList<>(headers.size());
+				for (AttachmentHeader header : headers) {
+					try {
+						Attachment a = attachmentRetriever
+								.getMessageAttachment(header);
+						AttachmentItem item = attachmentRetriever
+								.getAttachmentItem(a, needsSize);
+						items.add(item);
+					} catch (NoSuchMessageException e) {
+						LOG.info("Attachment not received yet");
+						missingAttachments.put(header.getMessageId(), h);
+						return;
+					}
+				}
+				// Don't cache items unless all are present and valid
+				attachmentRetriever.cachePut(h.getId(), items);
+				displayMessageAttachments(h.getId(), items);
 			} catch (DbException e) {
 				logException(LOG, WARNING, e);
 			}
@@ -553,7 +584,6 @@ public class ConversationActivity extends BriarActivity
 	private void displayMessageAttachments(MessageId m,
 			List<AttachmentItem> items) {
 		runOnUiThreadUnlessDestroyed(() -> {
-			attachmentRetriever.cachePut(m, items);
 			Pair<Integer, ConversationMessageItem> pair =
 					adapter.getMessageItem(m);
 			if (pair != null) {
@@ -567,6 +597,13 @@ public class ConversationActivity extends BriarActivity
 
 	@Override
 	public void eventOccurred(Event e) {
+		if (e instanceof AttachmentReceivedEvent) {
+			AttachmentReceivedEvent a = (AttachmentReceivedEvent) e;
+			if (a.getContactId().equals(contactId)) {
+				LOG.info("Attachment received");
+				onAttachmentReceived(a.getMessageId());
+			}
+		}
 		if (e instanceof ContactRemovedEvent) {
 			ContactRemovedEvent c = (ContactRemovedEvent) e;
 			if (c.getContactId().equals(contactId)) {
@@ -617,6 +654,15 @@ public class ConversationActivity extends BriarActivity
 			scrollToBottom();
 	}
 
+	@UiThread
+	private void onAttachmentReceived(MessageId attachmentId) {
+		PrivateMessageHeader h = missingAttachments.remove(attachmentId);
+		if (h != null) {
+			LOG.info("Missing attachment received");
+			loadMessageAttachments(h);
+		}
+	}
+
 	@UiThread
 	private void onNewConversationMessage(ConversationMessageHeader h) {
 		if (h instanceof ConversationRequest ||
@@ -903,11 +949,11 @@ public class ConversationActivity extends BriarActivity
 	}
 
 	@Override
-	public List<AttachmentItem> getAttachmentItems(MessageId m,
-			List<AttachmentHeader> headers) {
-		List<AttachmentItem> attachments = attachmentRetriever.cacheGet(m);
+	public List<AttachmentItem> getAttachmentItems(PrivateMessageHeader h) {
+		List<AttachmentItem> attachments =
+				attachmentRetriever.cacheGet(h.getId());
 		if (attachments == null) {
-			loadMessageAttachments(m, headers);
+			loadMessageAttachments(h);
 			return emptyList();
 		}
 		return attachments;

briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationVisitor.java

@@ -15,7 +15,6 @@ import org.briarproject.briar.api.forum.ForumInvitationRequest;
 import org.briarproject.briar.api.forum.ForumInvitationResponse;
 import org.briarproject.briar.api.introduction.IntroductionRequest;
 import org.briarproject.briar.api.introduction.IntroductionResponse;
-import org.briarproject.briar.api.messaging.AttachmentHeader;
 import org.briarproject.briar.api.messaging.PrivateMessageHeader;
 import org.briarproject.briar.api.privategroup.invitation.GroupInvitationRequest;
 import org.briarproject.briar.api.privategroup.invitation.GroupInvitationResponse;
@@ -56,8 +55,7 @@ class ConversationVisitor implements
 		if (h.getAttachmentHeaders().isEmpty()) {
 			attachments = emptyList();
 		} else {
-			attachments = attachmentCache
-					.getAttachmentItems(h.getId(), h.getAttachmentHeaders());
+			attachments = attachmentCache.getAttachmentItems(h);
 		}
 		if (h.isLocal()) {
 			item = new ConversationMessageItem(
@@ -295,7 +293,6 @@ class ConversationVisitor implements
 	}
 
 	interface AttachmentCache {
-		List<AttachmentItem> getAttachmentItems(MessageId m,
-				List<AttachmentHeader> headers);
+		List<AttachmentItem> getAttachmentItems(PrivateMessageHeader h);
 	}
 }

briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java

@@ -12,7 +12,6 @@ import org.briarproject.bramble.api.db.DatabaseExecutor;
 import org.briarproject.bramble.api.db.DbException;
 import org.briarproject.bramble.api.lifecycle.IoExecutor;
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
-import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.briar.android.attachment.AttachmentItem;
 import org.briarproject.briar.android.viewmodel.LiveEvent;
 import org.briarproject.briar.android.viewmodel.MutableLiveEvent;
@@ -135,10 +134,10 @@ public class ImageViewModel extends AndroidViewModel {
 
 	private void saveImage(AttachmentItem attachment, OutputStreamProvider osp,
 			@Nullable Runnable afterCopy) {
-		MessageId messageId = attachment.getMessageId();
 		dbExecutor.execute(() -> {
 			try {
-				Attachment a = messagingManager.getAttachment(messageId);
+				Attachment a =
+						messagingManager.getAttachment(attachment.getHeader());
 				copyImageFromDb(a, osp, afterCopy);
 			} catch (DbException e) {
 				logException(LOG, WARNING, e);

briar-android/src/main/java/org/briarproject/briar/android/conversation/glide/BriarDataFetcher.java

@@ -9,8 +9,8 @@ import com.bumptech.glide.load.data.DataFetcher;
 import org.briarproject.bramble.api.db.DatabaseExecutor;
 import org.briarproject.bramble.api.db.DbException;
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
-import org.briarproject.bramble.api.sync.MessageId;
 import org.briarproject.briar.android.attachment.AttachmentItem;
+import org.briarproject.briar.api.messaging.Attachment;
 import org.briarproject.briar.api.messaging.MessagingManager;
 
 import java.io.InputStream;
@@ -50,11 +50,12 @@ class BriarDataFetcher implements DataFetcher<InputStream> {
 	@Override
 	public void loadData(Priority priority,
 			DataCallback<? super InputStream> callback) {
-		MessageId id = attachment.getMessageId();
 		dbExecutor.execute(() -> {
 			if (cancel) return;
 			try {
-				inputStream = messagingManager.getAttachment(id).getStream();
+				Attachment a =
+						messagingManager.getAttachment(attachment.getHeader());
+				inputStream = a.getStream();
 				callback.onDataReady(inputStream);
 			} catch (DbException e) {
 				callback.onLoadFailed(e);