[android] refactor AttachmentCreator to return a single LiveData

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

@@ -14,7 +14,7 @@ import org.jsoup.UnsupportedMimeTypeException;
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.List;
+import java.util.Collection;
 import java.util.logging.Logger;
 
 import static java.util.logging.Level.WARNING;
@@ -34,7 +34,7 @@ class AttachmentCreationTask {
 	private final MessagingManager messagingManager;
 	private final ContentResolver contentResolver;
 	private final GroupId groupId;
-	private final List<Uri> uris;
+	private final Collection<Uri> uris;
 	private final boolean needsSize;
 	@Nullable
 	private volatile AttachmentCreator attachmentCreator;
@@ -44,7 +44,7 @@ class AttachmentCreationTask {
 	AttachmentCreationTask(MessagingManager messagingManager,
 			ContentResolver contentResolver,
 			AttachmentCreator attachmentCreator, GroupId groupId,
-			List<Uri> uris, boolean needsSize) {
+			Collection<Uri> uris, boolean needsSize) {
 		this.messagingManager = messagingManager;
 		this.contentResolver = contentResolver;
 		this.groupId = groupId;

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

@@ -24,14 +24,14 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.Executor;
 import java.util.logging.Logger;
 
 import static java.util.logging.Level.WARNING;
 import static java.util.logging.Logger.getLogger;
 import static org.briarproject.bramble.util.LogUtils.logException;
+import static org.briarproject.briar.android.util.UiUtils.observeForeverOnce;
 import static org.briarproject.briar.api.messaging.MessagingConstants.MAX_IMAGE_SIZE;
 
 @NotNullByDefault
@@ -45,14 +45,12 @@ public class AttachmentCreator {
 	private final MessagingManager messagingManager;
 	private final AttachmentRetriever retriever;
 
-	// store unsent items separately, as LiveData might not return latest value
-	private final Map<Uri, AttachmentItem> unsentItems =
-			new ConcurrentHashMap<>();
-	private final Map<Uri, MutableLiveData<AttachmentItemResult>>
-			liveDataResult = new ConcurrentHashMap<>();
+	private final CopyOnWriteArrayList<Uri> uris = new CopyOnWriteArrayList<>();
+	private final CopyOnWriteArrayList<AttachmentItemResult> itemResults =
+			new CopyOnWriteArrayList<>();
 
 	@Nullable
-	private MutableLiveData<Boolean> liveDataFinished = null;
+	private volatile MutableLiveData<AttachmentResult> result = null;
 	@Nullable
 	private AttachmentCreationTask task;
 
@@ -65,56 +63,49 @@ public class AttachmentCreator {
 	}
 
 	@UiThread
-	public AttachmentResult storeAttachments(GroupId groupId,
-			Collection<Uri> uris, boolean restart) {
-		List<LiveData<AttachmentItemResult>> itemResults = new ArrayList<>();
+	public LiveData<AttachmentResult> storeAttachments(
+			LiveData<GroupId> groupId, Collection<Uri> newUris, boolean restart) {
+		MutableLiveData<AttachmentResult> result;
 		if (restart) {
 			// This can happen due to configuration changes.
-			// So don't create new attachments, if we have (or creating) them.
-			// Instead, re-subscribe to the existing LiveData.
-			if (task == null || isNotStoring()) throw new AssertionError();
-			for (Uri uri : uris) {
-				// We don't want to expose mutable(!) LiveData
-				LiveData<AttachmentItemResult> liveData =
-						liveDataResult.get(uri);
-				if (liveData == null) throw new IllegalStateException();
-				itemResults.add(liveData);
-			}
-			if (liveDataFinished == null) throw new IllegalStateException();
+			// So don't create new attachments. They are already being created
+			// and returned by the existing LiveData.
+			result = this.result;
+			if (task == null || uris.isEmpty() || result == null)
+				throw new IllegalStateException();
+			// A task is already running. It will update the result LiveData.
+			// So nothing more to do here.
 		} else {
-			if (task != null && isNotStoring()) throw new AssertionError();
-			List<Uri> urisToStore = new ArrayList<>();
-			for (Uri uri : uris) {
-				urisToStore.add(uri);
-				MutableLiveData<AttachmentItemResult> liveData =
-						new MutableLiveData<>();
-				liveDataResult.put(uri, liveData);
-				itemResults.add(liveData);
-			}
-			boolean needsSize = uris.size() == 1;
-			task = new AttachmentCreationTask(messagingManager,
-					app.getContentResolver(), this, groupId, urisToStore,
-					needsSize);
-			ioExecutor.execute(() -> task.storeAttachments());
-			liveDataFinished = new MutableLiveData<>();
+			if (this.result != null || !uris.isEmpty())
+				throw new IllegalStateException();
+			result = new MutableLiveData<>();
+			this.result = result;
+			uris.addAll(newUris);
+			observeForeverOnce(groupId, id -> {
+				if (id == null) throw new IllegalStateException();
+				boolean needsSize = uris.size() == 1;
+				task = new AttachmentCreationTask(messagingManager,
+						app.getContentResolver(), this, id, uris, needsSize);
+				ioExecutor.execute(() -> task.storeAttachments());
+			});
 		}
-		return new AttachmentResult(itemResults, liveDataFinished);
+		return result;
 	}
 
 	@IoExecutor
 	void onAttachmentHeaderReceived(Uri uri, AttachmentHeader h,
 			boolean needsSize) {
+		MutableLiveData<AttachmentResult> result = this.result;
+		if (result == null) return;
 		// get and cache AttachmentItem for ImagePreview
 		try {
 			Attachment a = retriever.getMessageAttachment(h);
 			AttachmentItem item = retriever.getAttachmentItem(h, a, needsSize);
 			if (item.hasError()) throw new IOException();
-			unsentItems.put(uri, item);
-			MutableLiveData<AttachmentItemResult> result =
-					liveDataResult.get(uri);
-			if (result != null) {  // might have been cleared on UiThread
-				result.postValue(new AttachmentItemResult(uri, item));
-			}
+			AttachmentItemResult itemResult =
+					new AttachmentItemResult(uri, item);
+			itemResults.add(itemResult);
+			result.postValue(new AttachmentResult(itemResults, false));
 		} catch (IOException | DbException e) {
 			logException(LOG, WARNING, e);
 			onAttachmentError(uri, e);
@@ -123,6 +114,9 @@ public class AttachmentCreator {
 
 	@IoExecutor
 	void onAttachmentError(Uri uri, Throwable t) {
+		MutableLiveData<AttachmentResult> result = this.result;
+		if (result == null) return;
+		// get error message
 		String errorMsg;
 		if (t instanceof UnsupportedMimeTypeException) {
 			String mimeType = ((UnsupportedMimeTypeException) t).getMimeType();
@@ -134,23 +128,29 @@ public class AttachmentCreator {
 		} else {
 			errorMsg = null; // generic error
 		}
-		MutableLiveData<AttachmentItemResult> result = liveDataResult.get(uri);
-		if (result != null)
-			result.postValue(new AttachmentItemResult(errorMsg));
+		AttachmentItemResult itemResult =
+				new AttachmentItemResult(uri, errorMsg);
+		itemResults.add(itemResult);
+		result.postValue(new AttachmentResult(itemResults, false));
 		// expect to receive a cancel from the UI
 	}
 
 	@IoExecutor
 	void onAttachmentCreationFinished() {
-		if (liveDataFinished != null) liveDataFinished.postValue(true);
+		if (uris.size() != itemResults.size())
+			throw new IllegalStateException();
+		MutableLiveData<AttachmentResult> result = this.result;
+		if (result == null) return;
+		result.postValue(new AttachmentResult(itemResults, true));
 	}
 
 	@UiThread
 	public List<AttachmentHeader> getAttachmentHeadersForSending() {
-		List<AttachmentHeader> headers =
-				new ArrayList<>(unsentItems.values().size());
-		for (AttachmentItem item : unsentItems.values()) {
-			headers.add(item.getHeader());
+		List<AttachmentHeader> headers = new ArrayList<>(itemResults.size());
+		for (AttachmentItemResult itemResult : itemResults) {
+			// check if we are trying to send attachment items with errors
+			if (itemResult.getItem() == null) throw new IllegalStateException();
+			headers.add(itemResult.getItem().getHeader());
 		}
 		return headers;
 	}
@@ -161,22 +161,28 @@ public class AttachmentCreator {
 	 * @param id The MessageId of the sent message.
 	 */
 	public void onAttachmentsSent(MessageId id) {
-		retriever.cachePut(id, new ArrayList<>(unsentItems.values()));
+		List<AttachmentItem> items = new ArrayList<>(itemResults.size());
+		for (AttachmentItemResult itemResult : itemResults) {
+			// check if we are trying to send attachment items with errors
+			if (itemResult.getItem() == null) throw new IllegalStateException();
+			items.add(itemResult.getItem());
+		}
+		retriever.cachePut(id, items);
 		resetState();
 	}
 
+	/**
+	 * Needs to be called when created attachments will not be sent anymore.
+	 */
 	@UiThread
 	public void cancel() {
 		if (task == null) throw new AssertionError();
 		task.cancel();
 		// let observers know that they can remove themselves
-		for (MutableLiveData<AttachmentItemResult> liveData : liveDataResult
-				.values()) {
-			if (liveData.getValue() == null) {
-				liveData.setValue(null);
-			}
+		MutableLiveData<AttachmentResult> result = this.result;
+		if (result != null) {
+			result.setValue(null);
 		}
-		if (liveDataFinished != null) liveDataFinished.setValue(false);
 		deleteUnsentAttachments();
 		resetState();
 	}
@@ -184,20 +190,24 @@ public class AttachmentCreator {
 	@UiThread
 	private void resetState() {
 		task = null;
-		liveDataResult.clear();
-		liveDataFinished = null;
-		unsentItems.clear();
+		uris.clear();
+		itemResults.clear();
+		result = null;
 	}
 
 	@UiThread
 	public void deleteUnsentAttachments() {
-		// Make a copy for the IoExecutor as we clear the unsentItems soon
-		List<AttachmentItem> itemsToDelete =
-				new ArrayList<>(unsentItems.values());
+		// Make a copy for the IoExecutor as we clear the itemResults soon
+		List<AttachmentHeader> headers = new ArrayList<>(itemResults.size());
+		for (AttachmentItemResult itemResult : itemResults) {
+			// check if we are trying to send attachment items with errors
+			if (itemResult.getItem() != null)
+				headers.add(itemResult.getItem().getHeader());
+		}
 		ioExecutor.execute(() -> {
-			for (AttachmentItem item : itemsToDelete) {
+			for (AttachmentHeader header : headers) {
 				try {
-					messagingManager.removeAttachment(item.getHeader());
+					messagingManager.removeAttachment(header);
 				} catch (DbException e) {
 					logException(LOG, WARNING, e);
 				}
@@ -205,8 +215,4 @@ public class AttachmentCreator {
 		});
 	}
 
-	private boolean isNotStoring() {
-		return liveDataFinished == null;
-	}
-
 }

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

@@ -11,26 +11,24 @@ import javax.annotation.concurrent.Immutable;
 @NotNullByDefault
 public class AttachmentItemResult {
 
-	@Nullable
 	private final Uri uri;
 	@Nullable
 	private final AttachmentItem item;
 	@Nullable
 	private final String errorMsg;
 
-	public AttachmentItemResult(Uri uri, AttachmentItem item) {
+	AttachmentItemResult(Uri uri, AttachmentItem item) {
 		this.uri = uri;
 		this.item = item;
 		this.errorMsg = null;
 	}
 
-	public AttachmentItemResult(@Nullable String errorMsg) {
-		this.uri = null;
+	AttachmentItemResult(Uri uri, @Nullable String errorMsg) {
+		this.uri = uri;
 		this.item = null;
 		this.errorMsg = errorMsg;
 	}
 
-	@Nullable
 	public Uri getUri() {
 		return uri;
 	}
@@ -40,8 +38,8 @@ public class AttachmentItemResult {
 		return item;
 	}
 
-	public boolean isError() {
-		return errorMsg != null;
+	public boolean hasError() {
+		return item == null;
 	}
 
 	@Nullable

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

@@ -1,5 +1,6 @@
 package org.briarproject.briar.android.attachment;
 
+import android.arch.lifecycle.LiveData;
 import android.net.Uri;
 import android.support.annotation.UiThread;
 
@@ -11,7 +12,8 @@ import java.util.List;
 @UiThread
 public interface AttachmentManager {
 
-	AttachmentResult storeAttachments(Collection<Uri> uri, boolean restart);
+	LiveData<AttachmentResult> storeAttachments(Collection<Uri> uri,
+			boolean restart);
 
 	List<AttachmentHeader> getAttachmentHeadersForSending();
 

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

@@ -1,7 +1,5 @@
 package org.briarproject.briar.android.attachment;
 
-import android.arch.lifecycle.LiveData;
-
 import org.briarproject.bramble.api.nullsafety.NotNullByDefault;
 
 import java.util.Collection;
@@ -12,21 +10,20 @@ import javax.annotation.concurrent.Immutable;
 @NotNullByDefault
 public class AttachmentResult {
 
-	private final Collection<LiveData<AttachmentItemResult>> itemResults;
-	private final LiveData<Boolean> finished;
+	private final Collection<AttachmentItemResult> itemResults;
+	private final boolean finished;
 
-	public AttachmentResult(
-			Collection<LiveData<AttachmentItemResult>> itemResults,
-			LiveData<Boolean> finished) {
+	public AttachmentResult(Collection<AttachmentItemResult> itemResults,
+			boolean finished) {
 		this.itemResults = itemResults;
 		this.finished = finished;
 	}
 
-	public Collection<LiveData<AttachmentItemResult>> getItemResults() {
+	public Collection<AttachmentItemResult> getItemResults() {
 		return itemResults;
 	}
 
-	public LiveData<Boolean> getFinished() {
+	public boolean isFinished() {
 		return finished;
 	}
 

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

@@ -54,6 +54,7 @@ import static org.briarproject.bramble.util.LogUtils.logException;
 import static org.briarproject.bramble.util.LogUtils.now;
 import static org.briarproject.briar.android.attachment.AttachmentDimensions.getAttachmentDimensions;
 import static org.briarproject.briar.android.settings.SettingsFragment.SETTINGS_NAMESPACE;
+import static org.briarproject.briar.android.util.UiUtils.observeForeverOnce;
 
 @NotNullByDefault
 public class ConversationViewModel extends AndroidViewModel
@@ -80,13 +81,12 @@ public class ConversationViewModel extends AndroidViewModel
 
 	@Nullable
 	private ContactId contactId = null;
-	@Nullable
-	private volatile GroupId messagingGroupId = null;
 	private final MutableLiveData<Contact> contact = new MutableLiveData<>();
 	private final LiveData<AuthorId> contactAuthorId =
 			Transformations.map(contact, c -> c.getAuthor().getId());
 	private final LiveData<String> contactName =
 			Transformations.map(contact, UiUtils::getContactDisplayName);
+	private final LiveData<GroupId> messagingGroupId;
 	private final MutableLiveData<Boolean> imageSupport =
 			new MutableLiveData<>();
 	private final MutableLiveEvent<Boolean> showImageOnboarding =
@@ -120,6 +120,8 @@ public class ConversationViewModel extends AndroidViewModel
 				getAttachmentDimensions(application.getResources()));
 		this.attachmentCreator = new AttachmentCreator(getApplication(),
 				ioExecutor, messagingManager, attachmentRetriever);
+		messagingGroupId = Transformations
+				.map(contact, c -> messagingManager.getContactGroup(c).getId());
 		contactDeleted.setValue(false);
 	}
 
@@ -150,9 +152,6 @@ public class ConversationViewModel extends AndroidViewModel
 				contact.postValue(c);
 				logDuration(LOG, "Loading contact", start);
 				start = now();
-				messagingGroupId = messagingManager.getContactGroup(c).getId();
-				logDuration(LOG, "Load conversation GroupId", start);
-				start = now();
 				checkFeaturesAndOnboarding(contactId);
 				logDuration(LOG, "Checking for image support", start);
 			} catch (NoSuchContactException e) {
@@ -189,18 +188,20 @@ public class ConversationViewModel extends AndroidViewModel
 
 	void sendMessage(@Nullable String text,
 			List<AttachmentHeader> attachmentHeaders, long timestamp) {
-		GroupId groupId = messagingGroupId;
-		if (groupId == null) throw new IllegalStateException();
-		createMessage(groupId, text, attachmentHeaders, timestamp);
+		// messagingGroupId is loaded with the contact
+		observeForeverOnce(messagingGroupId, groupId -> {
+			if (groupId == null) throw new IllegalStateException();
+			createMessage(groupId, text, attachmentHeaders, timestamp);
+		});
 	}
 
 	@Override
 	@UiThread
-	public AttachmentResult storeAttachments(Collection<Uri> uris,
+	public LiveData<AttachmentResult> storeAttachments(Collection<Uri> uris,
 			boolean restart) {
-		GroupId groupId = messagingGroupId;
-		if (groupId == null) throw new IllegalStateException();
-		return attachmentCreator.storeAttachments(groupId, uris, restart);
+		// messagingGroupId is loaded with the contact
+		return attachmentCreator
+				.storeAttachments(messagingGroupId, uris, restart);
 	}
 
 	@Override

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

@@ -15,6 +15,7 @@ import java.util.Collection;
 
 import static android.content.Context.LAYOUT_INFLATER_SERVICE;
 import static android.support.v4.content.ContextCompat.getColor;
+import static android.support.v7.widget.RecyclerView.NO_POSITION;
 import static android.view.ViewGroup.LayoutParams.MATCH_PARENT;
 import static java.util.Objects.requireNonNull;
 
@@ -75,7 +76,10 @@ public class ImagePreview extends ConstraintLayout {
 	void loadPreviewImage(AttachmentItemResult result) {
 		ImagePreviewAdapter adapter =
 				((ImagePreviewAdapter) imageList.getAdapter());
-		requireNonNull(adapter).loadItemPreview(result);
+		int pos = requireNonNull(adapter).loadItemPreview(result);
+		if (pos != NO_POSITION) {
+			imageList.smoothScrollToPosition(pos);
+		}
 	}
 
 	interface ImagePreviewListener {

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

@@ -50,14 +50,17 @@ class ImagePreviewAdapter extends Adapter<ImagePreviewViewHolder> {
 		return items.size();
 	}
 
-	void loadItemPreview(AttachmentItemResult result) {
-		ImagePreviewItem newItem =
-				new ImagePreviewItem(requireNonNull(result.getUri()));
+	int loadItemPreview(AttachmentItemResult result) {
+		ImagePreviewItem newItem = new ImagePreviewItem(result.getUri());
 		int pos = items.indexOf(newItem);
 		if (pos == NO_POSITION) throw new AssertionError();
 		ImagePreviewItem item = items.get(pos);
-		item.setItem(requireNonNull(result.getItem()));
-		notifyItemChanged(pos, item);
+		if (item.getItem() == null) {
+			item.setItem(requireNonNull(result.getItem()));
+			notifyItemChanged(pos, item);
+			return pos;
+		}
+		return NO_POSITION;
 	}
 
 }

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

@@ -24,6 +24,7 @@ import org.briarproject.briar.android.attachment.AttachmentResult;
 import org.briarproject.briar.android.view.ImagePreview.ImagePreviewListener;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 
 import uk.co.samuelwall.materialtaptargetprompt.MaterialTapTargetPrompt;
@@ -172,40 +173,39 @@ public class TextAttachmentController extends TextSendController
 		List<ImagePreviewItem> items = ImagePreviewItem.fromUris(imageUris);
 		imagePreview.showPreview(items);
 		// store attachments and show preview when successful
-		AttachmentResult result =
+		LiveData<AttachmentResult> result =
 				attachmentManager.storeAttachments(imageUris, restart);
-		for (LiveData<AttachmentItemResult> liveData : result
-				.getItemResults()) {
-			onLiveDataReturned(liveData);
-		}
-		result.getFinished().observe(imageListener, new Observer<Boolean>() {
+		result.observe(imageListener, new Observer<AttachmentResult>() {
 			@Override
-			public void onChanged(@Nullable Boolean finished) {
-				if (finished != null && finished) onAllAttachmentsCreated();
-				result.getFinished().removeObserver(this);
-			}
-		});
-	}
-
-	private void onLiveDataReturned(LiveData<AttachmentItemResult> liveData) {
-		liveData.observe(imageListener, new Observer<AttachmentItemResult>() {
-			@Override
-			public void onChanged(@Nullable AttachmentItemResult result) {
-				if (result != null) {
-					onAttachmentResultReceived(result);
+			public void onChanged(@Nullable AttachmentResult attachmentResult) {
+				if (attachmentResult == null) {
+					// The fresh LiveData was deliberately set to null.
+					// This means that we can stop observing it.
+					result.removeObserver(this);
+				} else {
+					boolean noError = onNewAttachmentItemResults(
+							attachmentResult.getItemResults());
+					if (noError && attachmentResult.isFinished()) {
+						onAllAttachmentsCreated();
+						result.removeObserver(this);
+					}
 				}
-				liveData.removeObserver(this);
 			}
 		});
 	}
 
-	private void onAttachmentResultReceived(AttachmentItemResult result) {
+	private boolean onNewAttachmentItemResults(
+			Collection<AttachmentItemResult> itemResults) {
 		if (!loadingUris) throw new AssertionError();
-		if (result.isError() || result.getUri() == null) {
-			onError(result.getErrorMsg());
-		} else {
-			imagePreview.loadPreviewImage(result);
+		for (AttachmentItemResult result : itemResults) {
+			if (result.hasError()) {
+				onError(result.getErrorMsg());
+				return false;
+			} else {
+				imagePreview.loadPreviewImage(result);
+			}
 		}
+		return true;
 	}
 
 	private void onAllAttachmentsCreated() {