[android] move unsent attachment cache logic into AttachmentController

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

@@ -29,7 +29,6 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.logging.Logger;
 
 import static android.support.media.ExifInterface.ORIENTATION_ROTATE_270;
@@ -59,7 +58,8 @@ class AttachmentController {
 	private final int minWidth, maxWidth;
 	private final int minHeight, maxHeight;
 
-	private final List<AttachmentHeader> unsent = new CopyOnWriteArrayList<>();
+	private final Map<Uri, AttachmentItem> unsentItems =
+			new ConcurrentHashMap<>();
 	private final Map<MessageId, List<AttachmentItem>> attachmentCache =
 			new ConcurrentHashMap<>();
 
@@ -121,9 +121,15 @@ class AttachmentController {
 	}
 
 	@DatabaseExecutor
-	AttachmentHeader createAttachmentHeader(ContentResolver contentResolver,
+	void createAttachmentHeader(ContentResolver contentResolver,
-			GroupId groupId, Uri uri)
+			GroupId groupId, Uri uri, boolean needsSize)
 			throws IOException, DbException {
+		if (unsentItems.containsKey(uri)) {
+			// This can happen due to configuration (screen orientation) change.
+			// So don't create a new attachment, if we have one already.
+			return;
+		}
+		long start = now();
 		InputStream is = contentResolver.openInputStream(uri);
 		if (is == null) throw new IOException();
 		String contentType = contentResolver.getType(uri);
@@ -132,32 +138,47 @@ class AttachmentController {
 		AttachmentHeader h = messagingManager
 				.addLocalAttachment(groupId, timestamp, contentType, is);
 		tryToClose(is, LOG, WARNING);
-		unsent.add(h);
+		logDuration(LOG, "Storing attachment", start);
-		return h;
+		// get and store AttachmentItem for ImagePreview
+		AttachmentItem item =
+				getAttachmentItem(contentResolver, uri, h, needsSize);
+		if (item.hasError()) throw new IOException();
+		unsentItems.put(uri, item);
 	}
 
 	@DatabaseExecutor
 	void deleteUnsentAttachments() {
-		for (AttachmentHeader h : unsent) {
+		for (AttachmentItem item : unsentItems.values()) {
 			try {
-				messagingManager.removeAttachment(h);
+				messagingManager.removeAttachment(item.getHeader());
 			} catch (DbException e) {
 				logException(LOG, WARNING, e);
 			}
 		}
+		unsentItems.clear();
 	}
 
-	List<AttachmentHeader> getUnsentAttachments() {
+	List<AttachmentHeader> getUnsentAttachmentHeaders() {
-		return new ArrayList<>(unsent);
+		List<AttachmentHeader> headers =
+				new ArrayList<>(unsentItems.values().size());
+		for (AttachmentItem item : unsentItems.values()) {
+			headers.add(item.getHeader());
+		}
+		return headers;
 	}
 
-	void markAttachmentsSent() {
+	/**
-		unsent.clear();
+	 * Marks the attachments as sent and adds the items to the cache for display
+	 * @param id The MessageId of the sent message.
+	 */
+	void onAttachmentsSent(MessageId id) {
+		attachmentCache.put(id, new ArrayList<>(unsentItems.values()));
+		unsentItems.clear();
 	}
 
 	@DatabaseExecutor
-	AttachmentItem getAttachmentItem(ContentResolver contentResolver, Uri uri,
+	private AttachmentItem getAttachmentItem(ContentResolver contentResolver,
-			AttachmentHeader h, boolean needsSize) throws IOException {
+			Uri uri, AttachmentHeader h, boolean needsSize) throws IOException {
 		InputStream is = null;
 		try {
 			is = contentResolver.openInputStream(uri);
@@ -192,17 +213,15 @@ class AttachmentController {
 	@VisibleForTesting
 	AttachmentItem getAttachmentItem(AttachmentHeader h, Attachment a,
 			boolean needsSize) {
-		MessageId messageId = h.getMessageId();
 		if (!needsSize) {
-			String mimeType = h.getContentType();
+			String extension =
-			String extension = imageHelper.getExtensionFromMimeType(mimeType);
+					imageHelper.getExtensionFromMimeType(h.getContentType());
 			boolean hasError = false;
 			if (extension == null) {
 				extension = "";
 				hasError = true;
 			}
-			return new AttachmentItem(messageId, 0, 0, mimeType, extension, 0,
+			return new AttachmentItem(h, 0, 0, extension, 0, 0, hasError);
-					0, hasError);
 		}
 
 		Size size = new Size();
@@ -240,10 +259,17 @@ class AttachmentController {
 		// get file extension
 		String extension = imageHelper.getExtensionFromMimeType(size.mimeType);
 		boolean hasError = extension == null || size.error;
+		if (!h.getContentType().equals(size.mimeType)) {
+			if (LOG.isLoggable(WARNING)) {
+				LOG.warning("Header has different mime type (" +
+						h.getContentType() + ") than image (" + size.mimeType +
+						").");
+			}
+			hasError = true;
+		}
 		if (extension == null) extension = "";
-		return new AttachmentItem(messageId, size.width, size.height,
+		return new AttachmentItem(h, size.width, size.height, extension,
-				size.mimeType, extension, thumbnailSize.width,
+				thumbnailSize.width, thumbnailSize.height, hasError);
-				thumbnailSize.height, hasError);
 	}
 
 	/**

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

@@ -6,6 +6,7 @@ import android.support.annotation.Nullable;
 
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
 import org.briarproject.bramble.api.sync.MessageId;
+import org.briarproject.briar.api.messaging.AttachmentHeader;
 
 import java.util.concurrent.atomic.AtomicLong;
 
@@ -15,9 +16,9 @@ import javax.annotation.concurrent.Immutable;
 @NotNullByDefault
 public class AttachmentItem implements Parcelable {
 
-	private final MessageId messageId;
+	private final AttachmentHeader header;
 	private final int width, height;
-	private final String mimeType, extension;
+	private final String extension;
 	private final int thumbnailWidth, thumbnailHeight;
 	private final boolean hasError;
 	private final long instanceId;
@@ -37,13 +38,12 @@ public class AttachmentItem implements Parcelable {
 
 	private static final AtomicLong NEXT_INSTANCE_ID = new AtomicLong(0);
 
-	AttachmentItem(MessageId messageId, int width, int height, String mimeType,
+	AttachmentItem(AttachmentHeader header, int width, int height,
 			String extension, int thumbnailWidth, int thumbnailHeight,
 			boolean hasError) {
-		this.messageId = messageId;
+		this.header = header;
 		this.width = width;
 		this.height = height;
-		this.mimeType = mimeType;
 		this.extension = extension;
 		this.thumbnailWidth = thumbnailWidth;
 		this.thumbnailHeight = thumbnailHeight;
@@ -54,19 +54,24 @@ public class AttachmentItem implements Parcelable {
 	protected AttachmentItem(Parcel in) {
 		byte[] messageIdByte = new byte[MessageId.LENGTH];
 		in.readByteArray(messageIdByte);
-		messageId = new MessageId(messageIdByte);
+		MessageId messageId = new MessageId(messageIdByte);
 		width = in.readInt();
 		height = in.readInt();
-		mimeType = in.readString();
+		String mimeType = in.readString();
 		extension = in.readString();
 		thumbnailWidth = in.readInt();
 		thumbnailHeight = in.readInt();
 		hasError = in.readByte() != 0;
 		instanceId = in.readLong();
+		header = new AttachmentHeader(messageId, mimeType);
+	}
+
+	AttachmentHeader getHeader() {
+		return header;
 	}
 
 	public MessageId getMessageId() {
-		return messageId;
+		return header.getMessageId();
 	}
 
 	int getWidth() {
@@ -78,7 +83,7 @@ public class AttachmentItem implements Parcelable {
 	}
 
 	String getMimeType() {
-		return mimeType;
+		return header.getContentType();
 	}
 
 	String getExtension() {
@@ -108,10 +113,10 @@ public class AttachmentItem implements Parcelable {
 
 	@Override
 	public void writeToParcel(Parcel dest, int flags) {
-		dest.writeByteArray(messageId.getBytes());
+		dest.writeByteArray(header.getMessageId().getBytes());
 		dest.writeInt(width);
 		dest.writeInt(height);
-		dest.writeString(mimeType);
+		dest.writeString(header.getContentType());
 		dest.writeString(extension);
 		dest.writeInt(thumbnailWidth);
 		dest.writeInt(thumbnailHeight);

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

@@ -38,11 +38,8 @@ import org.briarproject.briar.api.messaging.PrivateMessageFactory;
 import org.briarproject.briar.api.messaging.PrivateMessageHeader;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.logging.Logger;
 
@@ -102,8 +99,6 @@ public class ConversationViewModel extends AndroidViewModel implements
 			new MutableLiveData<>();
 	private final MutableLiveData<PrivateMessageHeader> addedHeader =
 			new MutableLiveData<>();
-	// TODO move to AttachmentController
-	private final Map<Uri, AttachmentItem> unsentItems = new HashMap<>();
 
 	@Inject
 	ConversationViewModel(Application application,
@@ -199,12 +194,6 @@ public class ConversationViewModel extends AndroidViewModel implements
 	@Override
 	public void storeAttachment(Uri uri, boolean needsSize, Runnable onSuccess,
 			Runnable onError) {
-		if (unsentItems.containsKey(uri)) {
-			// This can happen due to configuration (screen orientation) change.
-			// So don't create a new attachment, if we have one already.
-			androidExecutor.runOnUiThread(onSuccess);
-			return;
-		}
 		if (messagingGroupId.getValue() == null) loadGroupId();
 		observeForeverOnce(messagingGroupId, groupId -> dbExecutor.execute(()
 				-> {
@@ -213,10 +202,8 @@ public class ConversationViewModel extends AndroidViewModel implements
 			try {
 				ContentResolver contentResolver =
 						getApplication().getContentResolver();
-				AttachmentHeader h = attachmentController
+				attachmentController.createAttachmentHeader(contentResolver,
-						.createAttachmentHeader(contentResolver, groupId, uri);
+						groupId, uri, needsSize);
-				unsentItems.put(uri, attachmentController
-						.getAttachmentItem(contentResolver, uri, h, needsSize));
 				androidExecutor.runOnUiThread(onSuccess);
 			} catch (DbException | IOException e) {
 				logException(LOG, WARNING, e);
@@ -227,13 +214,12 @@ public class ConversationViewModel extends AndroidViewModel implements
 	}
 
 	@Override
-	public List<AttachmentHeader> getAttachments() {
+	public List<AttachmentHeader> getAttachmentHeaders() {
-		return attachmentController.getUnsentAttachments();
+		return attachmentController.getUnsentAttachmentHeaders();
 	}
 
 	@Override
 	public void removeAttachments() {
-		unsentItems.clear();
 		dbExecutor.execute(attachmentController::deleteUnsentAttachments);
 	}
 
@@ -332,10 +318,7 @@ public class ConversationViewModel extends AndroidViewModel implements
 						message.getId(), message.getGroupId(),
 						message.getTimestamp(), true, true, false, false,
 						text != null, attachments);
-				attachmentController.put(m.getMessage().getId(),
+				attachmentController.onAttachmentsSent(m.getMessage().getId());
-						new ArrayList<>(unsentItems.values()));
-				unsentItems.clear();
-				attachmentController.markAttachmentsSent();
 				// TODO add text to cache when available here
 				addedHeader.postValue(h);
 			} catch (DbException e) {

briar-android/src/main/java/org/briarproject/briar/android/view/TextAttachmentController.java

@@ -89,7 +89,7 @@ public class TextAttachmentController extends TextSendController
 	public void onSendEvent() {
 		if (canSend()) {
 			listener.onSendClick(textInput.getText(),
-					attachmentManager.getAttachments());
+					attachmentManager.getAttachmentHeaders());
 			reset();
 		}
 	}
@@ -283,7 +283,7 @@ public class TextAttachmentController extends TextSendController
 		void storeAttachment(Uri uri, boolean needsSize, Runnable onSuccess,
 				Runnable onError);
 
-		List<AttachmentHeader> getAttachments();
+		List<AttachmentHeader> getAttachmentHeaders();
 
 		void removeAttachments();
 	}

briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java

@@ -163,7 +163,7 @@ class MessagingManagerImpl extends ConversationClientImpl
 		if (is.available() == 0) throw new IOException();
 		byte[] b = new byte[MessageId.LENGTH];
 		new Random().nextBytes(b);
-		return new AttachmentHeader(new MessageId(b), "image/png");
+		return new AttachmentHeader(new MessageId(b), contentType);
 	}
 
 	@Override