From 41411b0e2ee55ac68738e817a22a2a03e591479c Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 20 Jun 2019 18:24:23 -0300 Subject: [PATCH 01/10] Refactor attachment loading to support incremental display once loaded --- .../AttachmentRetrieverIntegrationTest.java | 59 ++++----- .../attachment/AttachmentCreatorImpl.java | 12 +- .../android/attachment/AttachmentItem.java | 61 ++++++--- .../attachment/AttachmentRetriever.java | 27 ++-- .../attachment/AttachmentRetrieverImpl.java | 118 ++++++++++++++---- .../android/attachment/UnavailableItem.java | 34 +++++ .../conversation/ConversationActivity.java | 102 ++++++--------- .../conversation/ConversationMessageItem.java | 13 +- .../android/conversation/ImageActivity.java | 8 +- .../android/conversation/ImageAdapter.java | 5 +- .../android/conversation/ImageFragment.java | 11 +- .../android/conversation/ImageViewHolder.java | 27 ++-- .../src/main/res/drawable/ic_image_broken.xml | 4 +- .../main/res/drawable/ic_image_missing.xml | 10 ++ .../attachment/AttachmentRetrieverTest.java | 24 ++-- .../briar/api/messaging/MessagingManager.java | 2 +- 16 files changed, 333 insertions(+), 184 deletions(-) create mode 100644 briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java create mode 100644 briar-android/src/main/res/drawable/ic_image_missing.xml diff --git a/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java b/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java index 55e847ce0..9c4ffc8d5 100644 --- a/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java +++ b/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java @@ -13,8 +13,9 @@ import java.util.Random; import androidx.test.ext.junit.runners.AndroidJUnit4; import static androidx.test.InstrumentationRegistry.getContext; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.AVAILABLE; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.ERROR; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @RunWith(AndroidJUnit4.class) @@ -35,7 +36,7 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/jpeg"); InputStream is = getAssetInputStream("kitten_small.jpg"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(msgId, item.getMessageId()); assertEquals(160, item.getWidth()); assertEquals(240, item.getHeight()); @@ -43,7 +44,7 @@ public class AttachmentRetrieverIntegrationTest { assertEquals(240, item.getThumbnailHeight()); assertEquals("image/jpeg", item.getMimeType()); assertJpgOrJpeg(item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -51,7 +52,7 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/jpeg"); InputStream is = getAssetInputStream("kitten_original.jpg"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(msgId, item.getMessageId()); assertEquals(1728, item.getWidth()); assertEquals(2592, item.getHeight()); @@ -59,7 +60,7 @@ public class AttachmentRetrieverIntegrationTest { assertEquals(dimensions.maxHeight, item.getThumbnailHeight()); assertEquals("image/jpeg", item.getMimeType()); assertJpgOrJpeg(item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -67,7 +68,7 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/png"); InputStream is = getAssetInputStream("kitten.png"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(msgId, item.getMessageId()); assertEquals(737, item.getWidth()); assertEquals(510, item.getHeight()); @@ -75,7 +76,7 @@ public class AttachmentRetrieverIntegrationTest { assertEquals(138, item.getThumbnailHeight()); assertEquals("image/png", item.getMimeType()); assertEquals("png", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -83,14 +84,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/gif"); InputStream is = getAssetInputStream("uber.gif"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(1, item.getWidth()); assertEquals(1, item.getHeight()); assertEquals(dimensions.minHeight, item.getThumbnailWidth()); assertEquals(dimensions.minHeight, item.getThumbnailHeight()); assertEquals("image/gif", item.getMimeType()); assertEquals("gif", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -98,14 +99,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/jpeg"); InputStream is = getAssetInputStream("lottapixel.jpg"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(64250, item.getWidth()); assertEquals(64250, item.getHeight()); assertEquals(dimensions.maxWidth, item.getThumbnailWidth()); assertEquals(dimensions.maxWidth, item.getThumbnailHeight()); assertEquals("image/jpeg", item.getMimeType()); assertJpgOrJpeg(item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -113,14 +114,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/png"); InputStream is = getAssetInputStream("image_io_crash.png"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(1184, item.getWidth()); assertEquals(448, item.getHeight()); assertEquals(dimensions.maxWidth, item.getThumbnailWidth()); assertEquals(dimensions.minHeight, item.getThumbnailHeight()); assertEquals("image/png", item.getMimeType()); assertEquals("png", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -128,14 +129,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/gif"); InputStream is = getAssetInputStream("gimp_crash.gif"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(1, item.getWidth()); assertEquals(1, item.getHeight()); assertEquals(dimensions.minHeight, item.getThumbnailWidth()); assertEquals(dimensions.minHeight, item.getThumbnailHeight()); assertEquals("image/gif", item.getMimeType()); assertEquals("gif", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -143,14 +144,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/gif"); InputStream is = getAssetInputStream("opti_png_afl.gif"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(32, item.getWidth()); assertEquals(32, item.getHeight()); assertEquals(dimensions.minHeight, item.getThumbnailWidth()); assertEquals(dimensions.minHeight, item.getThumbnailHeight()); assertEquals("image/gif", item.getMimeType()); assertEquals("gif", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -158,8 +159,8 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/jpeg"); InputStream is = getAssetInputStream("libraw_error.jpg"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); - assertTrue(item.hasError()); + AttachmentItem item = retriever.createAttachmentItem(a, true); + assertEquals(ERROR, item.getState()); } @Test @@ -167,14 +168,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/gif"); InputStream is = getAssetInputStream("animated.gif"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(65535, item.getWidth()); assertEquals(65535, item.getHeight()); assertEquals(dimensions.maxWidth, item.getThumbnailWidth()); assertEquals(dimensions.maxWidth, item.getThumbnailHeight()); assertEquals("image/gif", item.getMimeType()); assertEquals("gif", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -182,14 +183,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/gif"); InputStream is = getAssetInputStream("animated2.gif"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(10000, item.getWidth()); assertEquals(10000, item.getHeight()); assertEquals(dimensions.maxWidth, item.getThumbnailWidth()); assertEquals(dimensions.maxWidth, item.getThumbnailHeight()); assertEquals("image/gif", item.getMimeType()); assertEquals("gif", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -197,14 +198,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/gif"); InputStream is = getAssetInputStream("error_large.gif"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(16384, item.getWidth()); assertEquals(16384, item.getHeight()); assertEquals(dimensions.maxWidth, item.getThumbnailWidth()); assertEquals(dimensions.maxWidth, item.getThumbnailHeight()); assertEquals("image/gif", item.getMimeType()); assertEquals("gif", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -212,14 +213,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/jpeg"); InputStream is = getAssetInputStream("error_high.jpg"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(1, item.getWidth()); assertEquals(10000, item.getHeight()); assertEquals(dimensions.minWidth, item.getThumbnailWidth()); assertEquals(dimensions.maxHeight, item.getThumbnailHeight()); assertEquals("image/jpeg", item.getMimeType()); assertJpgOrJpeg(item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -227,14 +228,14 @@ public class AttachmentRetrieverIntegrationTest { AttachmentHeader h = new AttachmentHeader(msgId, "image/jpeg"); InputStream is = getAssetInputStream("error_wide.jpg"); Attachment a = new Attachment(h, is); - AttachmentItem item = retriever.getAttachmentItem(a, true); + AttachmentItem item = retriever.createAttachmentItem(a, true); assertEquals(1920, item.getWidth()); assertEquals(1, item.getHeight()); assertEquals(dimensions.maxWidth, item.getThumbnailWidth()); assertEquals(dimensions.minHeight, item.getThumbnailHeight()); assertEquals("image/jpeg", item.getMimeType()); assertJpgOrJpeg(item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } private InputStream getAssetInputStream(String name) throws Exception { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentCreatorImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentCreatorImpl.java index 882ef569f..60f498fdd 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentCreatorImpl.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentCreatorImpl.java @@ -34,6 +34,7 @@ import androidx.lifecycle.MutableLiveData; 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.attachment.AttachmentItem.State.ERROR; import static org.briarproject.briar.android.util.UiUtils.observeForeverOnce; import static org.briarproject.briar.api.messaging.MessagingConstants.MAX_IMAGE_SIZE; @@ -109,8 +110,8 @@ class AttachmentCreatorImpl implements AttachmentCreator { // get and cache AttachmentItem for ImagePreview try { Attachment a = retriever.getMessageAttachment(h); - AttachmentItem item = retriever.getAttachmentItem(a, needsSize); - if (item.hasError()) throw new IOException(); + AttachmentItem item = retriever.createAttachmentItem(a, needsSize); + if (item.getState() == ERROR) throw new IOException(); AttachmentItemResult itemResult = new AttachmentItemResult(uri, item); itemResults.add(itemResult); @@ -167,13 +168,6 @@ class AttachmentCreatorImpl implements AttachmentCreator { @Override @UiThread public void onAttachmentsSent(MessageId id) { - List 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(); } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java index a72c31948..a19827eab 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java @@ -7,24 +7,27 @@ 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; - import javax.annotation.concurrent.Immutable; import androidx.annotation.Nullable; +import static java.lang.System.arraycopy; import static java.util.Objects.requireNonNull; +import static org.briarproject.bramble.util.StringUtils.toHexString; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.LOADING; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.MISSING; @Immutable @NotNullByDefault public class AttachmentItem implements Parcelable { + public enum State {LOADING, MISSING, AVAILABLE, ERROR} + private final AttachmentHeader header; private final int width, height; private final String extension; private final int thumbnailWidth, thumbnailHeight; - private final boolean hasError; - private final long instanceId; + private final State state; public static final Creator CREATOR = new Creator() { @@ -39,19 +42,33 @@ public class AttachmentItem implements Parcelable { } }; - private static final AtomicLong NEXT_INSTANCE_ID = new AtomicLong(0); - AttachmentItem(AttachmentHeader header, int width, int height, String extension, int thumbnailWidth, int thumbnailHeight, - boolean hasError) { + State state) { this.header = header; this.width = width; this.height = height; this.extension = extension; this.thumbnailWidth = thumbnailWidth; this.thumbnailHeight = thumbnailHeight; - this.hasError = hasError; - instanceId = NEXT_INSTANCE_ID.getAndIncrement(); + this.state = state; + } + + /** + * Use only for {@link MISSING} or {@link LOADING} items. + */ + AttachmentItem(AttachmentHeader header, int width, int height, + State state) { + this(header, width, height, "", width, height, state); + if (state != MISSING && state != LOADING) + throw new IllegalArgumentException(); + } + + /** + * Use when the item does not need a size. + */ + AttachmentItem(AttachmentHeader header, String extension, State state) { + this(header, 0, 0, extension, 0, 0, state); } protected AttachmentItem(Parcel in) { @@ -64,8 +81,7 @@ public class AttachmentItem implements Parcelable { extension = requireNonNull(in.readString()); thumbnailWidth = in.readInt(); thumbnailHeight = in.readInt(); - hasError = in.readByte() != 0; - instanceId = in.readLong(); + state = State.valueOf(requireNonNull(in.readString())); header = new AttachmentHeader(messageId, mimeType); } @@ -101,12 +117,20 @@ public class AttachmentItem implements Parcelable { return thumbnailHeight; } - public boolean hasError() { - return hasError; + public State getState() { + return state; } - public String getTransitionName() { - return String.valueOf(instanceId); + public String getTransitionName(MessageId conversationItemId) { + int len = MessageId.LENGTH; + byte[] instanceId = new byte[len * 2]; + arraycopy(header.getMessageId().getBytes(), 0, instanceId, 0, len); + arraycopy(conversationItemId.getBytes(), 0, instanceId, len, len); + return toHexString(instanceId); + } + + boolean hasSize() { + return width != 0 && height != 0; } @Override @@ -123,14 +147,15 @@ public class AttachmentItem implements Parcelable { dest.writeString(extension); dest.writeInt(thumbnailWidth); dest.writeInt(thumbnailHeight); - dest.writeByte((byte) (hasError ? 1 : 0)); - dest.writeLong(instanceId); + dest.writeString(state.name()); } @Override public boolean equals(@Nullable Object o) { return o instanceof AttachmentItem && - instanceId == ((AttachmentItem) o).instanceId; + header.getMessageId().equals( + ((AttachmentItem) o).header.getMessageId() + ); } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java index 33d709ab7..f12b01ecb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java @@ -1,29 +1,40 @@ package org.briarproject.briar.android.attachment; +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; import org.briarproject.briar.api.messaging.Attachment; import org.briarproject.briar.api.messaging.AttachmentHeader; +import org.briarproject.briar.api.messaging.PrivateMessageHeader; import java.io.InputStream; import java.util.List; -import androidx.annotation.Nullable; - @NotNullByDefault public interface AttachmentRetriever { - void cachePut(MessageId messageId, List attachments); - - @Nullable - List cacheGet(MessageId messageId); - + @DatabaseExecutor Attachment getMessageAttachment(AttachmentHeader h) throws DbException; + List getAttachmentItems(PrivateMessageHeader messageHeader); + + /** + * Retrieves item size and adds the item to the cache, if available. + */ + @DatabaseExecutor + void cacheAttachmentItem(MessageId conversationMessageId, + AttachmentHeader h) throws DbException; + /** * Creates an {@link AttachmentItem} from the {@link Attachment}'s * {@link InputStream} which will be closed when this method returns. */ - AttachmentItem getAttachmentItem(Attachment a, boolean needsSize); + AttachmentItem createAttachmentItem(Attachment a, boolean needsSize); + + @DatabaseExecutor + Pair loadAttachmentItem(MessageId attachmentId) + throws DbException; + } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java index 53a376aea..30ee170bb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java @@ -1,14 +1,20 @@ package org.briarproject.briar.android.attachment; +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.db.NoSuchMessageException; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.sync.MessageId; +import org.briarproject.briar.android.attachment.AttachmentItem.State; 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 java.io.BufferedInputStream; import java.io.InputStream; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -16,10 +22,12 @@ import java.util.logging.Logger; import javax.inject.Inject; -import androidx.annotation.Nullable; - import static java.util.logging.Level.WARNING; import static java.util.logging.Logger.getLogger; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.AVAILABLE; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.ERROR; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.LOADING; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.MISSING; @NotNullByDefault class AttachmentRetrieverImpl implements AttachmentRetriever { @@ -34,7 +42,11 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { private final int minWidth, maxWidth; private final int minHeight, maxHeight; - private final Map> attachmentCache = + // Info for AttachmentItems that are either still LOADING or MISSING + private final Map unavailableItems = + new ConcurrentHashMap<>(); + // We cache only items in their final state: AVAILABLE or ERROR + private final Map itemCache = new ConcurrentHashMap<>(); @Inject @@ -52,40 +64,95 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { } @Override - public void cachePut(MessageId messageId, - List attachments) { - attachmentCache.put(messageId, attachments); - } - - @Override - @Nullable - public List cacheGet(MessageId messageId) { - return attachmentCache.get(messageId); - } - - @Override + @DatabaseExecutor public Attachment getMessageAttachment(AttachmentHeader h) throws DbException { return messagingManager.getAttachment(h); } @Override - public AttachmentItem getAttachmentItem(Attachment a, boolean needsSize) { + public List getAttachmentItems( + PrivateMessageHeader messageHeader) { + List headers = messageHeader.getAttachmentHeaders(); + List items = new ArrayList<>(headers.size()); + boolean needsSize = headers.size() == 1; + for (AttachmentHeader h : headers) { + AttachmentItem item = itemCache.get(h.getMessageId()); + if (item == null || (needsSize && !item.hasSize())) { + item = new AttachmentItem(h, defaultSize, defaultSize, LOADING); + UnavailableItem unavailableItem = new UnavailableItem( + messageHeader.getId(), h, needsSize); + unavailableItems.put(h.getMessageId(), unavailableItem); + } + items.add(item); + } + return items; + } + + @Override + @DatabaseExecutor + public void cacheAttachmentItem(MessageId conversationMessageId, + AttachmentHeader h) throws DbException { + try { + Attachment a = messagingManager.getAttachment(h); + // this adds it to the cache automatically + createAttachmentItem(a, true); + } catch (NoSuchMessageException e) { + LOG.info("Attachment not received yet"); + } + } + + @Override + @DatabaseExecutor + public Pair loadAttachmentItem( + MessageId attachmentId) throws DbException { + UnavailableItem unavailableItem = unavailableItems.get(attachmentId); + if (unavailableItem == null) throw new AssertionError(); + + MessageId conversationMessageId = + unavailableItem.getConversationMessageId(); + AttachmentHeader h = unavailableItem.getHeader(); + boolean needsSize = unavailableItem.needsSize(); + + AttachmentItem item; + try { + Attachment a = messagingManager.getAttachment(h); + item = createAttachmentItem(a, needsSize); + unavailableItems.remove(attachmentId); + } catch (NoSuchMessageException e) { + LOG.info("Attachment not received yet"); + // unavailable item is still tracked, no need to add it again + item = new AttachmentItem(h, defaultSize, defaultSize, MISSING); + } + return new Pair<>(conversationMessageId, item); + } + + @Override + public AttachmentItem createAttachmentItem(Attachment a, + boolean needsSize) { AttachmentHeader h = a.getHeader(); - if (!needsSize) { + AttachmentItem item = itemCache.get(h.getMessageId()); + if (item != null && (needsSize && item.hasSize())) return item; + + if (needsSize) { + InputStream is = new BufferedInputStream(a.getStream()); + Size size = imageSizeCalculator.getSize(is, h.getContentType()); + item = createAttachmentItem(h, size); + } else { String extension = imageHelper.getExtensionFromMimeType(h.getContentType()); - boolean hasError = false; + State state = AVAILABLE; if (extension == null) { extension = ""; - hasError = true; + state = ERROR; } - return new AttachmentItem(h, 0, 0, extension, 0, 0, hasError); + item = new AttachmentItem(h, extension, state); } + itemCache.put(h.getMessageId(), item); + return item; + } - InputStream is = new BufferedInputStream(a.getStream()); - Size size = imageSizeCalculator.getSize(is, h.getContentType()); - + private AttachmentItem createAttachmentItem(AttachmentHeader h, Size size) { // calculate thumbnail size Size thumbnailSize = new Size(defaultSize, defaultSize, size.mimeType); if (!size.error) { @@ -104,8 +171,9 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { hasError = true; } if (extension == null) extension = ""; - return new AttachmentItem(h, size.width, size.height, extension, - thumbnailSize.width, thumbnailSize.height, hasError); + State state = hasError ? ERROR : AVAILABLE; + return new AttachmentItem(h, size.width, size.height, + extension, thumbnailSize.width, thumbnailSize.height, state); } private Size getThumbnailSize(int width, int height, String mimeType) { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java new file mode 100644 index 000000000..9aa648020 --- /dev/null +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java @@ -0,0 +1,34 @@ +package org.briarproject.briar.android.attachment; + +import org.briarproject.bramble.api.sync.MessageId; +import org.briarproject.briar.api.messaging.AttachmentHeader; + +import javax.annotation.concurrent.Immutable; + +@Immutable +class UnavailableItem { + + private final MessageId conversationMessageId; + private final AttachmentHeader header; + private final boolean needsSize; + + UnavailableItem(MessageId conversationMessageId, + AttachmentHeader header, boolean needsSize) { + this.conversationMessageId = conversationMessageId; + this.header = header; + this.needsSize = needsSize; + } + + MessageId getConversationMessageId() { + return conversationMessageId; + } + + AttachmentHeader getHeader() { + return header; + } + + boolean needsSize() { + return needsSize; + } + +} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index 6aa6986c7..1aba980d7 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -27,7 +27,6 @@ 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; @@ -65,13 +64,13 @@ import org.briarproject.briar.api.client.ProtocolStateException; import org.briarproject.briar.api.client.SessionId; import org.briarproject.briar.api.conversation.ConversationManager; import org.briarproject.briar.api.conversation.ConversationMessageHeader; +import org.briarproject.briar.api.conversation.ConversationMessageVisitor; import org.briarproject.briar.api.conversation.ConversationRequest; import org.briarproject.briar.api.conversation.ConversationResponse; import org.briarproject.briar.api.conversation.DeletionResult; import org.briarproject.briar.api.conversation.event.ConversationMessageReceivedEvent; import org.briarproject.briar.api.forum.ForumSharingManager; import org.briarproject.briar.api.introduction.IntroductionManager; -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; @@ -118,8 +117,6 @@ import static androidx.core.app.ActivityOptionsCompat.makeSceneTransitionAnimati import static androidx.core.view.ViewCompat.setTransitionName; import static androidx.lifecycle.Lifecycle.State.STARTED; import static androidx.recyclerview.widget.SortedList.INVALID_POSITION; -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; @@ -133,9 +130,11 @@ import static org.briarproject.bramble.util.StringUtils.isNullOrEmpty; import static org.briarproject.bramble.util.StringUtils.join; import static org.briarproject.briar.android.activity.RequestCodes.REQUEST_ATTACH_IMAGE; import static org.briarproject.briar.android.activity.RequestCodes.REQUEST_INTRODUCTION; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.LOADING; import static org.briarproject.briar.android.conversation.ImageActivity.ATTACHMENTS; import static org.briarproject.briar.android.conversation.ImageActivity.ATTACHMENT_POSITION; import static org.briarproject.briar.android.conversation.ImageActivity.DATE; +import static org.briarproject.briar.android.conversation.ImageActivity.ITEM_ID; import static org.briarproject.briar.android.conversation.ImageActivity.NAME; import static org.briarproject.briar.android.util.UiUtils.getAvatarTransitionName; import static org.briarproject.briar.android.util.UiUtils.getBulbTransitionName; @@ -185,8 +184,6 @@ public class ConversationActivity extends BriarActivity volatile GroupInvitationManager groupInvitationManager; private final Map textCache = new ConcurrentHashMap<>(); - private final Map missingAttachments = - new ConcurrentHashMap<>(); private final Observer contactNameObserver = name -> { requireNonNull(name); loadMessages(); @@ -264,6 +261,7 @@ public class ConversationActivity extends BriarActivity adapter = new ConversationAdapter(this, this); list = findViewById(R.id.conversationView); layoutManager = new LinearLayoutManager(this); + layoutManager.setStackFromEnd(true); list.setLayoutManager(layoutManager); list.setAdapter(adapter); list.setEmptyText(getString(R.string.no_private_messages)); @@ -540,6 +538,7 @@ public class ConversationActivity extends BriarActivity }); } + @DatabaseExecutor private void eagerlyLoadMessageSize(PrivateMessageHeader h) { try { MessageId id = h.getId(); @@ -556,21 +555,10 @@ public class ConversationActivity extends BriarActivity // images we use a grid so the size is fixed List headers = h.getAttachmentHeaders(); if (headers.size() == 1) { - List 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); - } - } + LOG.info("Eagerly loading image size for latest message"); + AttachmentHeader header = headers.get(0); + // get the item to retrieve its size + attachmentRetriever.cacheAttachmentItem(h.getId(), header); } } catch (DbException e) { logException(LOG, WARNING, e); @@ -651,44 +639,34 @@ public class ConversationActivity extends BriarActivity && adapter.isScrolledToBottom(layoutManager); } - private void loadMessageAttachments(PrivateMessageHeader h) { - // TODO: Use placeholders for missing/invalid attachments + private void loadMessageAttachments(List items) { runOnDbThread(() -> { - try { - // TODO move getting the items off to IoExecutor, if size == 1 - List headers = h.getAttachmentHeaders(); - boolean needsSize = headers.size() == 1; - List 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); + for (AttachmentItem item : items) { + if (item.getState() == LOADING) + loadMessageAttachment(item.getMessageId()); } }); } - private void displayMessageAttachments(MessageId m, - List items) { + @DatabaseExecutor + private void loadMessageAttachment(MessageId attachmentId) { + try { + Pair pair = + attachmentRetriever.loadAttachmentItem(attachmentId); + MessageId conversationMessageId = pair.getFirst(); + AttachmentItem item = pair.getSecond(); + updateMessageAttachment(conversationMessageId, item); + } catch (DbException e) { + logException(LOG, WARNING, e); + } + } + + private void updateMessageAttachment(MessageId m, AttachmentItem item) { runOnUiThreadUnlessDestroyed(() -> { Pair pair = adapter.getMessageItem(m); - if (pair != null) { + if (pair != null && pair.getSecond().updateAttachments(item)) { boolean scroll = shouldScrollWhenUpdatingMessage(); - pair.getSecond().setAttachments(items); adapter.notifyItemChanged(pair.getFirst()); if (scroll) scrollToBottom(); } @@ -765,11 +743,7 @@ public class ConversationActivity extends BriarActivity @UiThread private void onAttachmentReceived(MessageId attachmentId) { - PrivateMessageHeader h = missingAttachments.remove(attachmentId); - if (h != null) { - LOG.info("Missing attachment received"); - loadMessageAttachments(h); - } + runOnDbThread(() -> loadMessageAttachment(attachmentId)); } @UiThread @@ -780,7 +754,7 @@ public class ConversationActivity extends BriarActivity observeOnce(viewModel.getContactDisplayName(), this, name -> addConversationItem(h.accept(visitor))); } else { - // visitor also loads message text (if existing) + // visitor also loads message text and attachments (if existing) addConversationItem(h.accept(visitor)); } } @@ -1107,8 +1081,9 @@ public class ConversationActivity extends BriarActivity i.putExtra(ATTACHMENT_POSITION, attachments.indexOf(item)); i.putExtra(NAME, name); i.putExtra(DATE, messageItem.getTime()); + i.putExtra(ITEM_ID, messageItem.getId().getBytes()); // restoring list position should not trigger android bug #224270 - String transitionName = item.getTransitionName(); + String transitionName = item.getTransitionName(messageItem.getId()); ActivityOptionsCompat options = makeSceneTransitionAnimation(this, view, transitionName); ActivityCompat.startActivity(this, i, options.toBundle()); @@ -1147,15 +1122,14 @@ public class ConversationActivity extends BriarActivity return text; } + /** + * Called by {@link PrivateMessageHeader#accept(ConversationMessageVisitor)} + */ @Override public List getAttachmentItems(PrivateMessageHeader h) { - List attachments = - attachmentRetriever.cacheGet(h.getId()); - if (attachments == null) { - loadMessageAttachments(h); - return emptyList(); - } - return attachments; + List items = attachmentRetriever.getAttachmentItems(h); + loadMessageAttachments(items); + return items; } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageItem.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageItem.java index b3e20b3f7..8fc50d299 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageItem.java @@ -9,12 +9,13 @@ import java.util.List; import javax.annotation.concurrent.NotThreadSafe; import androidx.annotation.LayoutRes; +import androidx.annotation.UiThread; @NotThreadSafe @NotNullByDefault class ConversationMessageItem extends ConversationItem { - private List attachments; + private final List attachments; ConversationMessageItem(@LayoutRes int layoutRes, PrivateMessageHeader h, List attachments) { @@ -26,8 +27,14 @@ class ConversationMessageItem extends ConversationItem { return attachments; } - void setAttachments(List attachments) { - this.attachments = attachments; + @UiThread + boolean updateAttachments(AttachmentItem item) { + int pos = attachments.indexOf(item); + if (pos != -1 && attachments.get(pos).getState() != item.getState()) { + attachments.set(pos, item); + return true; + } + return false; } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java index 749ef495c..874e4b95f 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java @@ -17,6 +17,7 @@ import com.google.android.material.appbar.AppBarLayout; import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault; import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault; +import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.briar.R; import org.briarproject.briar.android.activity.ActivityComponent; import org.briarproject.briar.android.activity.BriarActivity; @@ -69,6 +70,7 @@ public class ImageActivity extends BriarActivity final static String ATTACHMENT_POSITION = "position"; final static String NAME = "name"; final static String DATE = "date"; + final static String ITEM_ID = "itemId"; @RequiresApi(api = 16) private final static int UI_FLAGS_DEFAULT = @@ -82,6 +84,7 @@ public class ImageActivity extends BriarActivity private AppBarLayout appBarLayout; private ViewPager viewPager; private List attachments; + private MessageId conversationMessageId; @Override public void injectActivity(ActivityComponent component) { @@ -136,6 +139,7 @@ public class ImageActivity extends BriarActivity String date = formatDateAbsolute(this, time); contactName.setText(name); dateView.setText(date); + conversationMessageId = new MessageId(i.getByteArrayExtra(ITEM_ID)); // Set up image ViewPager viewPager = findViewById(R.id.viewPager); @@ -325,8 +329,8 @@ public class ImageActivity extends BriarActivity @Override public Fragment getItem(int position) { - Fragment f = ImageFragment - .newInstance(attachments.get(position), isFirst); + Fragment f = ImageFragment.newInstance( + attachments.get(position), conversationMessageId, isFirst); isFirst = false; return f; } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java index 9f456f71b..f0401b4cc 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java @@ -49,7 +49,8 @@ class ImageAdapter extends Adapter { public ImageViewHolder onCreateViewHolder(ViewGroup viewGroup, int type) { View v = LayoutInflater.from(viewGroup.getContext()).inflate( R.layout.list_item_image, viewGroup, false); - return new ImageViewHolder(v, imageSize); + requireNonNull(conversationItem); + return new ImageViewHolder(v, imageSize, conversationItem.getId()); } @Override @@ -58,7 +59,7 @@ class ImageAdapter extends Adapter { // get item requireNonNull(conversationItem); AttachmentItem item = items.get(position); - // set onClick listener + // set onClick listener, if not missing or error imageViewHolder.itemView.setOnClickListener(v -> listener.onAttachmentClicked(v, conversationItem, item) ); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java index 9f8ee39d4..c2f368df6 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java @@ -15,6 +15,7 @@ import com.bumptech.glide.request.target.Target; import com.github.chrisbanes.photoview.PhotoView; import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault; +import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.briar.R; import org.briarproject.briar.android.activity.BaseActivity; import org.briarproject.briar.android.attachment.AttachmentItem; @@ -33,6 +34,7 @@ import static android.widget.ImageView.ScaleType.FIT_START; import static com.bumptech.glide.load.engine.DiskCacheStrategy.NONE; import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; import static org.briarproject.briar.android.conversation.ImageActivity.ATTACHMENT_POSITION; +import static org.briarproject.briar.android.conversation.ImageActivity.ITEM_ID; @MethodsNotNullByDefault @ParametersAreNonnullByDefault @@ -45,14 +47,17 @@ public class ImageFragment extends Fragment { private AttachmentItem attachment; private boolean isFirst; + private MessageId conversationItemId; private ImageViewModel viewModel; private PhotoView photoView; - static ImageFragment newInstance(AttachmentItem a, boolean isFirst) { + static ImageFragment newInstance(AttachmentItem a, + MessageId conversationMessageId, boolean isFirst) { ImageFragment f = new ImageFragment(); Bundle args = new Bundle(); args.putParcelable(ATTACHMENT_POSITION, a); args.putBoolean(IS_FIRST, isFirst); + args.putByteArray(ITEM_ID, conversationMessageId.getBytes()); f.setArguments(args); return f; } @@ -70,6 +75,8 @@ public class ImageFragment extends Fragment { Bundle args = requireNonNull(getArguments()); attachment = requireNonNull(args.getParcelable(ATTACHMENT_POSITION)); isFirst = args.getBoolean(IS_FIRST); + conversationItemId = + new MessageId(requireNonNull(args.getByteArray(ITEM_ID))); } @Nullable @@ -107,7 +114,7 @@ public class ImageFragment extends Fragment { // set transition name only when not animatable, // because the animation won't start otherwise photoView.setTransitionName( - attachment.getTransitionName()); + attachment.getTransitionName(conversationItemId)); } // Move image to the top if overlapping toolbar if (viewModel.isOverlappingToolbar(photoView, resource)) { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java index 76eaab5e3..344d7dde0 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java @@ -7,6 +7,7 @@ import android.widget.ImageView; import com.bumptech.glide.load.Transformation; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.briar.R; import org.briarproject.briar.android.attachment.AttachmentItem; import org.briarproject.briar.android.conversation.glide.BriarImageTransformation; @@ -18,8 +19,12 @@ import androidx.recyclerview.widget.RecyclerView.ViewHolder; import androidx.recyclerview.widget.StaggeredGridLayoutManager.LayoutParams; import static android.os.Build.VERSION.SDK_INT; +import static android.widget.ImageView.ScaleType.CENTER_CROP; +import static android.widget.ImageView.ScaleType.FIT_CENTER; import static com.bumptech.glide.load.engine.DiskCacheStrategy.NONE; import static com.bumptech.glide.load.resource.drawable.DrawableTransitionOptions.withCrossFade; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.AVAILABLE; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.ERROR; @NotNullByDefault class ImageViewHolder extends ViewHolder { @@ -29,25 +34,33 @@ class ImageViewHolder extends ViewHolder { protected final ImageView imageView; private final int imageSize; + private final MessageId conversationItemId; - ImageViewHolder(View v, int imageSize) { + ImageViewHolder(View v, int imageSize, MessageId conversationItemId) { super(v); imageView = v.findViewById(R.id.imageView); this.imageSize = imageSize; + this.conversationItemId = conversationItemId; } void bind(AttachmentItem attachment, Radii r, boolean single, boolean needsStretch) { - if (attachment.hasError()) { - GlideApp.with(imageView) - .clear(imageView); - imageView.setImageResource(ERROR_RES); + setImageViewDimensions(attachment, single, needsStretch); + if (attachment.getState() != AVAILABLE) { + GlideApp.with(imageView).clear(imageView); + if (attachment.getState() == ERROR) { + imageView.setImageResource(ERROR_RES); + } else { + imageView.setImageResource(R.drawable.ic_image_missing); + } + imageView.setScaleType(FIT_CENTER); } else { - setImageViewDimensions(attachment, single, needsStretch); loadImage(attachment, r); if (SDK_INT >= 21) { - imageView.setTransitionName(attachment.getTransitionName()); + imageView.setTransitionName( + attachment.getTransitionName(conversationItemId)); } + imageView.setScaleType(CENTER_CROP); } } diff --git a/briar-android/src/main/res/drawable/ic_image_broken.xml b/briar-android/src/main/res/drawable/ic_image_broken.xml index 318d05b36..096a4b505 100644 --- a/briar-android/src/main/res/drawable/ic_image_broken.xml +++ b/briar-android/src/main/res/drawable/ic_image_broken.xml @@ -1,6 +1,6 @@ + + diff --git a/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java b/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java index 4558c2ff8..8fae2c224 100644 --- a/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java +++ b/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java @@ -14,9 +14,9 @@ import java.io.InputStream; import static org.briarproject.bramble.test.TestUtils.getRandomBytes; import static org.briarproject.bramble.test.TestUtils.getRandomId; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.AVAILABLE; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.ERROR; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; public class AttachmentRetrieverTest extends BrambleMockTestCase { @@ -47,10 +47,10 @@ public class AttachmentRetrieverTest extends BrambleMockTestCase { will(returnValue("jpg")); }}); - AttachmentItem item = retriever.getAttachmentItem(attachment, false); + AttachmentItem item = retriever.createAttachmentItem(attachment, false); assertEquals(mimeType, item.getMimeType()); assertEquals("jpg", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -63,8 +63,8 @@ public class AttachmentRetrieverTest extends BrambleMockTestCase { will(returnValue(null)); }}); - AttachmentItem item = retriever.getAttachmentItem(attachment, false); - assertTrue(item.hasError()); + AttachmentItem item = retriever.createAttachmentItem(attachment, false); + assertEquals(ERROR, item.getState()); } @Test @@ -80,7 +80,7 @@ public class AttachmentRetrieverTest extends BrambleMockTestCase { will(returnValue("jpg")); }}); - AttachmentItem item = retriever.getAttachmentItem(attachment, true); + AttachmentItem item = retriever.createAttachmentItem(attachment, true); assertEquals(msgId, item.getMessageId()); assertEquals(160, item.getWidth()); assertEquals(240, item.getHeight()); @@ -88,7 +88,7 @@ public class AttachmentRetrieverTest extends BrambleMockTestCase { assertEquals(240, item.getThumbnailHeight()); assertEquals(mimeType, item.getMimeType()); assertEquals("jpg", item.getExtension()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -104,12 +104,12 @@ public class AttachmentRetrieverTest extends BrambleMockTestCase { will(returnValue("jpg")); }}); - AttachmentItem item = retriever.getAttachmentItem(attachment, true); + AttachmentItem item = retriever.createAttachmentItem(attachment, true); assertEquals(1728, item.getWidth()); assertEquals(2592, item.getHeight()); assertEquals(dimensions.maxWidth, item.getThumbnailWidth()); assertEquals(dimensions.maxHeight, item.getThumbnailHeight()); - assertFalse(item.hasError()); + assertEquals(AVAILABLE, item.getState()); } @Test @@ -125,8 +125,8 @@ public class AttachmentRetrieverTest extends BrambleMockTestCase { will(returnValue(null)); }}); - AttachmentItem item = retriever.getAttachmentItem(attachment, true); - assertTrue(item.hasError()); + AttachmentItem item = retriever.createAttachmentItem(attachment, true); + assertEquals(ERROR, item.getState()); } private Attachment getAttachment(String contentType) { diff --git a/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java b/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java index 0dfecb8ab..ccb8f776f 100644 --- a/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java +++ b/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java @@ -68,7 +68,7 @@ public interface MessagingManager extends ConversationClient { String getMessageText(MessageId m) throws DbException; /** - * Returns the attachment with the given message ID and content type. + * Returns the attachment with the given attachment header. * * @throws InvalidAttachmentException If the header refers to a message * that is not an attachment, or to an attachment that does not have the From 4122e0852ad15ed3329e8ac776339375b80dc0a2 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 24 Sep 2019 13:45:28 -0300 Subject: [PATCH 02/10] Show placeholders for missing attachments in ImageActivity and display attachments as they arrive while ImageActivity is open. --- .../android/conversation/ImageFragment.java | 109 +++++++++++------- .../android/conversation/ImageViewHolder.java | 8 +- .../android/conversation/ImageViewModel.java | 33 +++++- 3 files changed, 101 insertions(+), 49 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java index c2f368df6..4a61b0dfb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java @@ -24,6 +24,7 @@ import org.briarproject.briar.android.conversation.glide.GlideApp; import javax.annotation.ParametersAreNonnullByDefault; import javax.inject.Inject; +import androidx.annotation.DrawableRes; import androidx.annotation.Nullable; import androidx.fragment.app.Fragment; import androidx.lifecycle.ViewModelProvider; @@ -33,14 +34,19 @@ import static android.os.Build.VERSION.SDK_INT; import static android.widget.ImageView.ScaleType.FIT_START; import static com.bumptech.glide.load.engine.DiskCacheStrategy.NONE; import static org.briarproject.bramble.api.nullsafety.NullSafety.requireNonNull; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.AVAILABLE; +import static org.briarproject.briar.android.attachment.AttachmentItem.State.ERROR; import static org.briarproject.briar.android.conversation.ImageActivity.ATTACHMENT_POSITION; import static org.briarproject.briar.android.conversation.ImageActivity.ITEM_ID; @MethodsNotNullByDefault @ParametersAreNonnullByDefault -public class ImageFragment extends Fragment { +public class ImageFragment extends Fragment + implements RequestListener { private final static String IS_FIRST = "isFirst"; + @DrawableRes + private static final int ERROR_RES = R.drawable.ic_image_broken; @Inject ViewModelProvider.Factory viewModelFactory; @@ -89,55 +95,72 @@ public class ImageFragment extends Fragment { viewModel = ViewModelProviders.of(requireNonNull(getActivity()), viewModelFactory).get(ImageViewModel.class); + viewModel.getOnAttachmentLoaded() + .observeEvent(this, this::onAttachmentLoaded); photoView = v.findViewById(R.id.photoView); photoView.setScaleLevels(1, 2, 4); photoView.setOnClickListener(view -> viewModel.clickImage()); - // Request Listener - RequestListener listener = new RequestListener() { - - @Override - public boolean onLoadFailed(@Nullable GlideException e, - Object model, Target target, - boolean isFirstResource) { - if (getActivity() != null && isFirst) - getActivity().supportStartPostponedEnterTransition(); - return false; - } - - @Override - public boolean onResourceReady(Drawable resource, Object model, - Target target, DataSource dataSource, - boolean isFirstResource) { - if (SDK_INT >= 21 && !(resource instanceof Animatable)) { - // set transition name only when not animatable, - // because the animation won't start otherwise - photoView.setTransitionName( - attachment.getTransitionName(conversationItemId)); - } - // Move image to the top if overlapping toolbar - if (viewModel.isOverlappingToolbar(photoView, resource)) { - photoView.setScaleType(FIT_START); - } - if (getActivity() != null && isFirst) { - getActivity().supportStartPostponedEnterTransition(); - } - return false; - } - }; - - // Load Image - GlideApp.with(this) - .load(attachment) - // TODO allow if size < maxTextureSize ? -// .override(SIZE_ORIGINAL) - .diskCacheStrategy(NONE) - .error(R.drawable.ic_image_broken) - .addListener(listener) - .into(photoView); + if (attachment.getState() == AVAILABLE) { + loadImage(); + // postponed transition will be started when Image was loaded + } else if (attachment.getState() == ERROR) { + photoView.setImageResource(ERROR_RES); + startPostponedTransition(); + } else { + photoView.setImageResource(R.drawable.ic_image_missing); + startPostponedTransition(); + } return v; } + private void loadImage() { + GlideApp.with(this) + .load(attachment) + // TODO allow if size < maxTextureSize ? +// .override(SIZE_ORIGINAL) + .diskCacheStrategy(NONE) + .error(ERROR_RES) + .addListener(this) + .into(photoView); + } + + private void onAttachmentLoaded(MessageId messageId) { + if (attachment.getMessageId().equals(messageId)) loadImage(); + } + + @Override + public boolean onLoadFailed(@Nullable GlideException e, + Object model, Target target, + boolean isFirstResource) { + startPostponedTransition(); + return false; + } + + @Override + public boolean onResourceReady(Drawable resource, Object model, + Target target, DataSource dataSource, + boolean isFirstResource) { + if (SDK_INT >= 21 && !(resource instanceof Animatable)) { + // set transition name only when not animatable, + // because the animation won't start otherwise + photoView.setTransitionName( + attachment.getTransitionName(conversationItemId)); + } + // Move image to the top if overlapping toolbar + if (viewModel.isOverlappingToolbar(photoView, resource)) { + photoView.setScaleType(FIT_START); + } + startPostponedTransition(); + return false; + } + + private void startPostponedTransition() { + if (getActivity() != null && isFirst) { + getActivity().supportStartPostponedEnterTransition(); + } + } + } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java index 344d7dde0..659bcf29e 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewHolder.java @@ -56,12 +56,12 @@ class ImageViewHolder extends ViewHolder { imageView.setScaleType(FIT_CENTER); } else { loadImage(attachment, r); - if (SDK_INT >= 21) { - imageView.setTransitionName( - attachment.getTransitionName(conversationItemId)); - } imageView.setScaleType(CENTER_CROP); } + if (SDK_INT >= 21) { + imageView.setTransitionName( + attachment.getTransitionName(conversationItemId)); + } } private void setImageViewDimensions(AttachmentItem a, boolean single, diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java index 0c251cf60..58ef47dba 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java @@ -7,13 +7,18 @@ import android.view.View; import org.briarproject.bramble.api.db.DatabaseExecutor; import org.briarproject.bramble.api.db.DbException; +import org.briarproject.bramble.api.event.Event; +import org.briarproject.bramble.api.event.EventBus; +import org.briarproject.bramble.api.event.EventListener; 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; import org.briarproject.briar.api.messaging.Attachment; import org.briarproject.briar.api.messaging.MessagingManager; +import org.briarproject.briar.api.messaging.event.AttachmentReceivedEvent; import java.io.File; import java.io.FileOutputStream; @@ -41,16 +46,19 @@ import static org.briarproject.bramble.util.IoUtils.copyAndClose; import static org.briarproject.bramble.util.LogUtils.logException; @NotNullByDefault -public class ImageViewModel extends AndroidViewModel { +public class ImageViewModel extends AndroidViewModel implements EventListener { private static Logger LOG = getLogger(ImageViewModel.class.getName()); private final MessagingManager messagingManager; + private final EventBus eventBus; @DatabaseExecutor private final Executor dbExecutor; @IoExecutor private final Executor ioExecutor; + private final MutableLiveEvent attachmentLoaded = + new MutableLiveEvent<>(); /** * true means there was an error saving the image, false if image was saved. */ @@ -62,13 +70,34 @@ public class ImageViewModel extends AndroidViewModel { @Inject ImageViewModel(Application application, - MessagingManager messagingManager, + MessagingManager messagingManager, EventBus eventBus, @DatabaseExecutor Executor dbExecutor, @IoExecutor Executor ioExecutor) { super(application); this.messagingManager = messagingManager; + this.eventBus = eventBus; this.dbExecutor = dbExecutor; this.ioExecutor = ioExecutor; + + eventBus.addListener(this); + } + + @Override + protected void onCleared() { + super.onCleared(); + eventBus.removeListener(this); + } + + @Override + public void eventOccurred(Event e) { + if (e instanceof AttachmentReceivedEvent) { + attachmentLoaded + .postEvent(((AttachmentReceivedEvent) e).getMessageId()); + } + } + + LiveEvent getOnAttachmentLoaded() { + return attachmentLoaded; } void clickImage() { From b7d3cd7990400a1c710bf53c318843dfc7f9d2ae Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 17 Oct 2019 13:25:04 -0300 Subject: [PATCH 03/10] [android] support attachments arriving *before* the message containing them --- .../android/attachment/AttachmentRetriever.java | 11 +++++++++++ .../attachment/AttachmentRetrieverImpl.java | 5 ++++- .../conversation/ConversationActivity.java | 17 ++++++++++++----- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java index f12b01ecb..9b5d14812 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java @@ -12,6 +12,9 @@ import org.briarproject.briar.api.messaging.PrivateMessageHeader; import java.io.InputStream; import java.util.List; +import androidx.annotation.Nullable; + + @NotNullByDefault public interface AttachmentRetriever { @@ -33,6 +36,14 @@ public interface AttachmentRetriever { */ AttachmentItem createAttachmentItem(Attachment a, boolean needsSize); + /** + * Load an {@link AttachmentItem} from the database. + * + * @return a pair of the {@link MessageId} of the conversation message + * and the {@link AttachmentItem} + * or {@code null} when the conversation message did not yet arrive. + */ + @Nullable @DatabaseExecutor Pair loadAttachmentItem(MessageId attachmentId) throws DbException; diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java index 30ee170bb..f13556b12 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java @@ -22,6 +22,8 @@ import java.util.logging.Logger; import javax.inject.Inject; +import androidx.annotation.Nullable; + import static java.util.logging.Level.WARNING; import static java.util.logging.Logger.getLogger; import static org.briarproject.briar.android.attachment.AttachmentItem.State.AVAILABLE; @@ -103,11 +105,12 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { } @Override + @Nullable @DatabaseExecutor public Pair loadAttachmentItem( MessageId attachmentId) throws DbException { UnavailableItem unavailableItem = unavailableItems.get(attachmentId); - if (unavailableItem == null) throw new AssertionError(); + if (unavailableItem == null) return null; MessageId conversationMessageId = unavailableItem.getConversationMessageId(); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index 1aba980d7..fccf6ae59 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -643,16 +643,21 @@ public class ConversationActivity extends BriarActivity runOnDbThread(() -> { for (AttachmentItem item : items) { if (item.getState() == LOADING) - loadMessageAttachment(item.getMessageId()); + loadMessageAttachment(item.getMessageId(), false); } }); } @DatabaseExecutor - private void loadMessageAttachment(MessageId attachmentId) { + private void loadMessageAttachment(MessageId attachmentId, + boolean allowedToFail) { try { - Pair pair = - attachmentRetriever.loadAttachmentItem(attachmentId); + Pair pair = attachmentRetriever + .loadAttachmentItem(attachmentId); + if (pair == null && allowedToFail) { + LOG.warning("Attachment arrived before message"); + return; + } else if (pair == null) throw new AssertionError(); MessageId conversationMessageId = pair.getFirst(); AttachmentItem item = pair.getSecond(); updateMessageAttachment(conversationMessageId, item); @@ -743,7 +748,9 @@ public class ConversationActivity extends BriarActivity @UiThread private void onAttachmentReceived(MessageId attachmentId) { - runOnDbThread(() -> loadMessageAttachment(attachmentId)); + // This is allowed to fail, because the conversation message + // might arrive *after* the attachment. + runOnDbThread(() -> loadMessageAttachment(attachmentId, true)); } @UiThread From a1cf485ecc71d41e507a6b61d201bbd0feb532bb Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 7 Nov 2019 16:53:42 -0300 Subject: [PATCH 04/10] [android] address first round of code review for attachment placeholders --- .../briar/android/attachment/AttachmentItem.java | 2 +- .../briar/android/attachment/AttachmentRetriever.java | 4 ++-- .../briar/android/attachment/AttachmentRetrieverImpl.java | 6 ++++-- .../briar/android/attachment/UnavailableItem.java | 2 ++ .../briar/android/conversation/ConversationActivity.java | 2 +- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java index a19827eab..e0c4b8694 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java @@ -55,7 +55,7 @@ public class AttachmentItem implements Parcelable { } /** - * Use only for {@link MISSING} or {@link LOADING} items. + * Use only for {@link State MISSING} or {@link State LOADING} items. */ AttachmentItem(AttachmentHeader header, int width, int height, State state) { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java index 9b5d14812..298c8cbbb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java @@ -27,7 +27,7 @@ public interface AttachmentRetriever { * Retrieves item size and adds the item to the cache, if available. */ @DatabaseExecutor - void cacheAttachmentItem(MessageId conversationMessageId, + void cacheAttachmentItemWithSize(MessageId conversationMessageId, AttachmentHeader h) throws DbException; /** @@ -41,7 +41,7 @@ public interface AttachmentRetriever { * * @return a pair of the {@link MessageId} of the conversation message * and the {@link AttachmentItem} - * or {@code null} when the conversation message did not yet arrive. + * or {@code null} when the private message did not yet arrive. */ @Nullable @DatabaseExecutor diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java index f13556b12..8724ff8ee 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java @@ -26,6 +26,7 @@ import androidx.annotation.Nullable; 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.briar.android.attachment.AttachmentItem.State.AVAILABLE; import static org.briarproject.briar.android.attachment.AttachmentItem.State.ERROR; import static org.briarproject.briar.android.attachment.AttachmentItem.State.LOADING; @@ -93,7 +94,7 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { @Override @DatabaseExecutor - public void cacheAttachmentItem(MessageId conversationMessageId, + public void cacheAttachmentItemWithSize(MessageId conversationMessageId, AttachmentHeader h) throws DbException { try { Attachment a = messagingManager.getAttachment(h); @@ -135,11 +136,12 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { boolean needsSize) { AttachmentHeader h = a.getHeader(); AttachmentItem item = itemCache.get(h.getMessageId()); - if (item != null && (needsSize && item.hasSize())) return item; + if (item != null && (needsSize == item.hasSize())) return item; if (needsSize) { InputStream is = new BufferedInputStream(a.getStream()); Size size = imageSizeCalculator.getSize(is, h.getContentType()); + tryToClose(is, LOG, WARNING); item = createAttachmentItem(h, size); } else { String extension = diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java index 9aa648020..72951da3b 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java @@ -1,11 +1,13 @@ package org.briarproject.briar.android.attachment; +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.briar.api.messaging.AttachmentHeader; import javax.annotation.concurrent.Immutable; @Immutable +@NotNullByDefault class UnavailableItem { private final MessageId conversationMessageId; diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index fccf6ae59..ac03699a6 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -558,7 +558,7 @@ public class ConversationActivity extends BriarActivity LOG.info("Eagerly loading image size for latest message"); AttachmentHeader header = headers.get(0); // get the item to retrieve its size - attachmentRetriever.cacheAttachmentItem(h.getId(), header); + attachmentRetriever.cacheAttachmentItemWithSize(h.getId(), header); } } catch (DbException e) { logException(LOG, WARNING, e); From 31f42d44af47f383d8e6a293caee55e93276ba08 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 11 Nov 2019 16:39:35 -0300 Subject: [PATCH 05/10] [android] Refactor attachment loading to use LiveData --- .../AttachmentRetrieverIntegrationTest.java | 2 +- .../android/attachment/AttachmentItem.java | 8 +- .../attachment/AttachmentRetriever.java | 27 +++-- .../attachment/AttachmentRetrieverImpl.java | 112 ++++++++++++------ .../conversation/ConversationActivity.java | 64 +++++----- .../attachment/AttachmentRetrieverTest.java | 7 +- 6 files changed, 134 insertions(+), 86 deletions(-) diff --git a/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java b/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java index 9c4ffc8d5..866aecec4 100644 --- a/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java +++ b/briar-android/src/androidTestOfficial/java/org/briarproject/briar/android/attachment/AttachmentRetrieverIntegrationTest.java @@ -28,7 +28,7 @@ public class AttachmentRetrieverIntegrationTest { private final ImageHelper imageHelper = new ImageHelperImpl(); private final AttachmentRetriever retriever = - new AttachmentRetrieverImpl(null, dimensions, imageHelper, + new AttachmentRetrieverImpl(null, null, dimensions, imageHelper, new ImageSizeCalculator(imageHelper)); @Test diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java index e0c4b8694..58c5d9160 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java @@ -21,7 +21,13 @@ import static org.briarproject.briar.android.attachment.AttachmentItem.State.MIS @NotNullByDefault public class AttachmentItem implements Parcelable { - public enum State {LOADING, MISSING, AVAILABLE, ERROR} + public enum State { + LOADING, MISSING, AVAILABLE, ERROR; + + public boolean isFinal() { + return this == AVAILABLE || this == ERROR; + } + } private final AttachmentHeader header; private final int width, height; diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java index 298c8cbbb..c45c36ab7 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java @@ -1,6 +1,5 @@ package org.briarproject.briar.android.attachment; -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; @@ -8,11 +7,12 @@ import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.briar.api.messaging.Attachment; import org.briarproject.briar.api.messaging.AttachmentHeader; import org.briarproject.briar.api.messaging.PrivateMessageHeader; +import org.briarproject.briar.api.messaging.event.AttachmentReceivedEvent; import java.io.InputStream; import java.util.List; -import androidx.annotation.Nullable; +import androidx.lifecycle.LiveData; @NotNullByDefault @@ -21,10 +21,19 @@ public interface AttachmentRetriever { @DatabaseExecutor Attachment getMessageAttachment(AttachmentHeader h) throws DbException; - List getAttachmentItems(PrivateMessageHeader messageHeader); + /** + * Returns a list of observable {@link LiveData} + * that get updated as the state of their {@link AttachmentItem}s changes. + */ + List> getAttachmentItems( + PrivateMessageHeader messageHeader); /** * Retrieves item size and adds the item to the cache, if available. + *

+ * Use this to eagerly load the attachment size before it gets displayed. + * This is needed for messages containing a single attachment. + * Messages with more than one attachment use a standard size. */ @DatabaseExecutor void cacheAttachmentItemWithSize(MessageId conversationMessageId, @@ -37,15 +46,11 @@ public interface AttachmentRetriever { AttachmentItem createAttachmentItem(Attachment a, boolean needsSize); /** - * Load an {@link AttachmentItem} from the database. - * - * @return a pair of the {@link MessageId} of the conversation message - * and the {@link AttachmentItem} - * or {@code null} when the private message did not yet arrive. + * Loads an {@link AttachmentItem} + * that arrived via an {@link AttachmentReceivedEvent} + * and notifies the associated {@link LiveData}. */ - @Nullable @DatabaseExecutor - Pair loadAttachmentItem(MessageId attachmentId) - throws DbException; + void loadAttachmentItem(MessageId attachmentId); } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java index 8724ff8ee..2fa8c5849 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java @@ -1,6 +1,5 @@ package org.briarproject.briar.android.attachment; -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.db.NoSuchMessageException; @@ -18,15 +17,19 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executor; import java.util.logging.Logger; import javax.inject.Inject; -import androidx.annotation.Nullable; +import androidx.lifecycle.LiveData; +import androidx.lifecycle.MutableLiveData; +import static java.util.Objects.requireNonNull; 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.logException; import static org.briarproject.briar.android.attachment.AttachmentItem.State.AVAILABLE; import static org.briarproject.briar.android.attachment.AttachmentItem.State.ERROR; import static org.briarproject.briar.android.attachment.AttachmentItem.State.LOADING; @@ -38,6 +41,8 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { private static final Logger LOG = getLogger(AttachmentRetrieverImpl.class.getName()); + @DatabaseExecutor + private final Executor dbExecutor; private final MessagingManager messagingManager; private final ImageHelper imageHelper; private final ImageSizeCalculator imageSizeCalculator; @@ -45,17 +50,17 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { private final int minWidth, maxWidth; private final int minHeight, maxHeight; - // Info for AttachmentItems that are either still LOADING or MISSING - private final Map unavailableItems = - new ConcurrentHashMap<>(); - // We cache only items in their final state: AVAILABLE or ERROR - private final Map itemCache = - new ConcurrentHashMap<>(); + private final Map> + itemsWithSize = new ConcurrentHashMap<>(); + private final Map> + itemsWithoutSize = new ConcurrentHashMap<>(); @Inject - AttachmentRetrieverImpl(MessagingManager messagingManager, + AttachmentRetrieverImpl(@DatabaseExecutor Executor dbExecutor, + MessagingManager messagingManager, AttachmentDimensions dimensions, ImageHelper imageHelper, ImageSizeCalculator imageSizeCalculator) { + this.dbExecutor = dbExecutor; this.messagingManager = messagingManager; this.imageHelper = imageHelper; this.imageSizeCalculator = imageSizeCalculator; @@ -74,20 +79,37 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { } @Override - public List getAttachmentItems( + public List> getAttachmentItems( PrivateMessageHeader messageHeader) { List headers = messageHeader.getAttachmentHeaders(); - List items = new ArrayList<>(headers.size()); + List> items = new ArrayList<>(headers.size()); boolean needsSize = headers.size() == 1; for (AttachmentHeader h : headers) { - AttachmentItem item = itemCache.get(h.getMessageId()); - if (item == null || (needsSize && !item.hasSize())) { - item = new AttachmentItem(h, defaultSize, defaultSize, LOADING); - UnavailableItem unavailableItem = new UnavailableItem( - messageHeader.getId(), h, needsSize); - unavailableItems.put(h.getMessageId(), unavailableItem); + // try cache for existing item live data + MutableLiveData liveData; + if (needsSize) liveData = itemsWithSize.get(h.getMessageId()); + else { + // try items with size first, as they work as well + liveData = itemsWithSize.get(h.getMessageId()); + if (liveData == null) + liveData = itemsWithoutSize.get(h.getMessageId()); } - items.add(item); + + // create new live data with LOADING item if cache miss + if (liveData == null) { + AttachmentItem item = new AttachmentItem(h, + defaultSize, defaultSize, LOADING); + final MutableLiveData finalLiveData = + new MutableLiveData<>(item); + // kick-off loading of attachment, will post to live data + dbExecutor.execute( + () -> loadAttachmentItem(h, needsSize, finalLiveData)); + // add new LiveData to cache + liveData = finalLiveData; + if (needsSize) itemsWithSize.put(h.getMessageId(), liveData); + else itemsWithoutSize.put(h.getMessageId(), liveData); + } + items.add(liveData); } return items; } @@ -98,46 +120,63 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { AttachmentHeader h) throws DbException { try { Attachment a = messagingManager.getAttachment(h); - // this adds it to the cache automatically - createAttachmentItem(a, true); + AttachmentItem item = createAttachmentItem(a, true); + MutableLiveData liveData = + new MutableLiveData<>(item); + itemsWithSize.put(h.getMessageId(), liveData); } catch (NoSuchMessageException e) { LOG.info("Attachment not received yet"); } } @Override - @Nullable @DatabaseExecutor - public Pair loadAttachmentItem( - MessageId attachmentId) throws DbException { - UnavailableItem unavailableItem = unavailableItems.get(attachmentId); - if (unavailableItem == null) return null; + public void loadAttachmentItem(MessageId attachmentId) { + // try to find LiveData for attachment in both caches + MutableLiveData liveData; + boolean needsSize = true; + liveData = itemsWithSize.get(attachmentId); + if (liveData == null) { + needsSize = false; + liveData = itemsWithoutSize.get(attachmentId); + } - MessageId conversationMessageId = - unavailableItem.getConversationMessageId(); - AttachmentHeader h = unavailableItem.getHeader(); - boolean needsSize = unavailableItem.needsSize(); + // If no LiveData for the attachment exists, + // its message did not yet arrive and we can ignore it for now. + if (liveData == null) return; + // actually load the attachment item + AttachmentHeader h = requireNonNull(liveData.getValue()).getHeader(); + loadAttachmentItem(h, needsSize, liveData); + } + + /** + * Loads an {@link AttachmentItem} from the database + * and notifies the given {@link LiveData}. + */ + @DatabaseExecutor + private void loadAttachmentItem(AttachmentHeader h, boolean needsSize, + MutableLiveData liveData) { + Attachment a; AttachmentItem item; try { - Attachment a = messagingManager.getAttachment(h); + a = messagingManager.getAttachment(h); item = createAttachmentItem(a, needsSize); - unavailableItems.remove(attachmentId); } catch (NoSuchMessageException e) { LOG.info("Attachment not received yet"); - // unavailable item is still tracked, no need to add it again item = new AttachmentItem(h, defaultSize, defaultSize, MISSING); + } catch (DbException e) { + logException(LOG, WARNING, e); + item = new AttachmentItem(h, "", ERROR); } - return new Pair<>(conversationMessageId, item); + liveData.postValue(item); } @Override public AttachmentItem createAttachmentItem(Attachment a, boolean needsSize) { + AttachmentItem item; AttachmentHeader h = a.getHeader(); - AttachmentItem item = itemCache.get(h.getMessageId()); - if (item != null && (needsSize == item.hasSize())) return item; - if (needsSize) { InputStream is = new BufferedInputStream(a.getStream()); Size size = imageSizeCalculator.getSize(is, h.getContentType()); @@ -153,7 +192,6 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { } item = new AttachmentItem(h, extension, state); } - itemCache.put(h.getMessageId(), item); return item; } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index ac03699a6..539d0e0aa 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -96,6 +96,7 @@ import androidx.appcompat.widget.Toolbar; import androidx.core.app.ActivityCompat; import androidx.core.app.ActivityOptionsCompat; import androidx.core.content.ContextCompat; +import androidx.lifecycle.LiveData; import androidx.lifecycle.Observer; import androidx.lifecycle.ViewModelProvider; import androidx.lifecycle.ViewModelProviders; @@ -130,7 +131,6 @@ import static org.briarproject.bramble.util.StringUtils.isNullOrEmpty; import static org.briarproject.bramble.util.StringUtils.join; import static org.briarproject.briar.android.activity.RequestCodes.REQUEST_ATTACH_IMAGE; import static org.briarproject.briar.android.activity.RequestCodes.REQUEST_INTRODUCTION; -import static org.briarproject.briar.android.attachment.AttachmentItem.State.LOADING; import static org.briarproject.briar.android.conversation.ImageActivity.ATTACHMENTS; import static org.briarproject.briar.android.conversation.ImageActivity.ATTACHMENT_POSITION; import static org.briarproject.briar.android.conversation.ImageActivity.DATE; @@ -558,7 +558,8 @@ public class ConversationActivity extends BriarActivity LOG.info("Eagerly loading image size for latest message"); AttachmentHeader header = headers.get(0); // get the item to retrieve its size - attachmentRetriever.cacheAttachmentItemWithSize(h.getId(), header); + attachmentRetriever + .cacheAttachmentItemWithSize(h.getId(), header); } } catch (DbException e) { logException(LOG, WARNING, e); @@ -639,33 +640,6 @@ public class ConversationActivity extends BriarActivity && adapter.isScrolledToBottom(layoutManager); } - private void loadMessageAttachments(List items) { - runOnDbThread(() -> { - for (AttachmentItem item : items) { - if (item.getState() == LOADING) - loadMessageAttachment(item.getMessageId(), false); - } - }); - } - - @DatabaseExecutor - private void loadMessageAttachment(MessageId attachmentId, - boolean allowedToFail) { - try { - Pair pair = attachmentRetriever - .loadAttachmentItem(attachmentId); - if (pair == null && allowedToFail) { - LOG.warning("Attachment arrived before message"); - return; - } else if (pair == null) throw new AssertionError(); - MessageId conversationMessageId = pair.getFirst(); - AttachmentItem item = pair.getSecond(); - updateMessageAttachment(conversationMessageId, item); - } catch (DbException e) { - logException(LOG, WARNING, e); - } - } - private void updateMessageAttachment(MessageId m, AttachmentItem item) { runOnUiThreadUnlessDestroyed(() -> { Pair pair = @@ -748,9 +722,8 @@ public class ConversationActivity extends BriarActivity @UiThread private void onAttachmentReceived(MessageId attachmentId) { - // This is allowed to fail, because the conversation message - // might arrive *after* the attachment. - runOnDbThread(() -> loadMessageAttachment(attachmentId, true)); + runOnDbThread( + () -> attachmentRetriever.loadAttachmentItem(attachmentId)); } @UiThread @@ -1134,9 +1107,32 @@ public class ConversationActivity extends BriarActivity */ @Override public List getAttachmentItems(PrivateMessageHeader h) { - List items = attachmentRetriever.getAttachmentItems(h); - loadMessageAttachments(items); + List> liveDataList = + attachmentRetriever.getAttachmentItems(h); + List items = new ArrayList<>(liveDataList.size()); + for (LiveData liveData : liveDataList) { + liveData.observe(this, new AttachmentObserver(h.getId(), liveData)); + items.add(requireNonNull(liveData.getValue())); + } return items; } + private class AttachmentObserver implements Observer { + private final MessageId conversationMessageId; + private final LiveData liveData; + + private AttachmentObserver(MessageId conversationMessageId, + LiveData liveData) { + this.conversationMessageId = conversationMessageId; + this.liveData = liveData; + } + + @Override + public void onChanged(AttachmentItem attachmentItem) { + updateMessageAttachment(conversationMessageId, attachmentItem); + if (attachmentItem.getState().isFinal()) + liveData.removeObserver(this); + } + } + } diff --git a/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java b/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java index 8fae2c224..5504da00b 100644 --- a/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java +++ b/briar-android/src/test/java/org/briarproject/briar/android/attachment/AttachmentRetrieverTest.java @@ -2,6 +2,7 @@ package org.briarproject.briar.android.attachment; import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.bramble.test.BrambleMockTestCase; +import org.briarproject.bramble.test.ImmediateExecutor; import org.briarproject.briar.api.messaging.Attachment; import org.briarproject.briar.api.messaging.AttachmentHeader; import org.briarproject.briar.api.messaging.MessagingManager; @@ -11,6 +12,7 @@ import org.junit.Test; import java.io.ByteArrayInputStream; import java.io.InputStream; +import java.util.concurrent.Executor; import static org.briarproject.bramble.test.TestUtils.getRandomBytes; import static org.briarproject.bramble.test.TestUtils.getRandomId; @@ -33,8 +35,9 @@ public class AttachmentRetrieverTest extends BrambleMockTestCase { MessagingManager messagingManager = context.mock(MessagingManager.class); imageSizeCalculator = context.mock(ImageSizeCalculator.class); - retriever = new AttachmentRetrieverImpl(messagingManager, dimensions, - imageHelper, imageSizeCalculator); + Executor dbExecutor = new ImmediateExecutor(); + retriever = new AttachmentRetrieverImpl(dbExecutor, messagingManager, + dimensions, imageHelper, imageSizeCalculator); } @Test From 7c22016b811efd1ff0ade9ce8ae7200767956627 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 23 Jan 2020 10:22:02 -0300 Subject: [PATCH 06/10] [android] attach some smaller image attachment issues --- .../android/attachment/AttachmentItem.java | 12 ++++--- .../attachment/AttachmentRetriever.java | 7 ++++ .../android/attachment/UnavailableItem.java | 36 ------------------- .../conversation/ConversationActivity.java | 16 ++++----- .../android/conversation/ImageAdapter.java | 2 +- 5 files changed, 23 insertions(+), 50 deletions(-) delete mode 100644 briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java index 58c5d9160..9ce231c7d 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentItem.java @@ -135,10 +135,6 @@ public class AttachmentItem implements Parcelable { return toHexString(instanceId); } - boolean hasSize() { - return width != 0 && height != 0; - } - @Override public int describeContents() { return 0; @@ -156,6 +152,10 @@ public class AttachmentItem implements Parcelable { dest.writeString(state.name()); } + /** + * This is used to identity if two items are the same, + * irrespective of their state or size. + */ @Override public boolean equals(@Nullable Object o) { return o instanceof AttachmentItem && @@ -164,4 +164,8 @@ public class AttachmentItem implements Parcelable { ); } + @Override + public int hashCode() { + return header.getMessageId().hashCode(); + } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java index c45c36ab7..2bf6b69ea 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetriever.java @@ -49,6 +49,13 @@ public interface AttachmentRetriever { * Loads an {@link AttachmentItem} * that arrived via an {@link AttachmentReceivedEvent} * and notifies the associated {@link LiveData}. + * + * Note that you need to call {@link #getAttachmentItems(PrivateMessageHeader)} + * first to get the LiveData. + * + * It is possible that no LiveData is available, + * because the message of the AttachmentItem did not arrive, yet. + * In this case, the load wil be deferred until the message arrives. */ @DatabaseExecutor void loadAttachmentItem(MessageId attachmentId); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java deleted file mode 100644 index 72951da3b..000000000 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/UnavailableItem.java +++ /dev/null @@ -1,36 +0,0 @@ -package org.briarproject.briar.android.attachment; - -import org.briarproject.bramble.api.nullsafety.NotNullByDefault; -import org.briarproject.bramble.api.sync.MessageId; -import org.briarproject.briar.api.messaging.AttachmentHeader; - -import javax.annotation.concurrent.Immutable; - -@Immutable -@NotNullByDefault -class UnavailableItem { - - private final MessageId conversationMessageId; - private final AttachmentHeader header; - private final boolean needsSize; - - UnavailableItem(MessageId conversationMessageId, - AttachmentHeader header, boolean needsSize) { - this.conversationMessageId = conversationMessageId; - this.header = header; - this.needsSize = needsSize; - } - - MessageId getConversationMessageId() { - return conversationMessageId; - } - - AttachmentHeader getHeader() { - return header; - } - - boolean needsSize() { - return needsSize; - } - -} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index 539d0e0aa..bf39383ad 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -640,16 +640,14 @@ public class ConversationActivity extends BriarActivity && adapter.isScrolledToBottom(layoutManager); } + @UiThread private void updateMessageAttachment(MessageId m, AttachmentItem item) { - runOnUiThreadUnlessDestroyed(() -> { - Pair pair = - adapter.getMessageItem(m); - if (pair != null && pair.getSecond().updateAttachments(item)) { - boolean scroll = shouldScrollWhenUpdatingMessage(); - adapter.notifyItemChanged(pair.getFirst()); - if (scroll) scrollToBottom(); - } - }); + Pair pair = adapter.getMessageItem(m); + if (pair != null && pair.getSecond().updateAttachments(item)) { + boolean scroll = shouldScrollWhenUpdatingMessage(); + adapter.notifyItemChanged(pair.getFirst()); + if (scroll) scrollToBottom(); + } } @Override diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java index f0401b4cc..02f97730c 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageAdapter.java @@ -59,7 +59,7 @@ class ImageAdapter extends Adapter { // get item requireNonNull(conversationItem); AttachmentItem item = items.get(position); - // set onClick listener, if not missing or error + // set onClick listener imageViewHolder.itemView.setOnClickListener(v -> listener.onAttachmentClicked(v, conversationItem, item) ); From c1cf6f61b90e231bfd4b6b812ba5c160fd9572e9 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 11 Feb 2020 10:11:09 -0300 Subject: [PATCH 07/10] [android] fix concurrency issues when attachments are received delayed Do not observe attachment live data multiple times and don't miss received attachments in ImageActivity resp. ImageViewModel. --- .../conversation/ConversationActivity.java | 4 ++ .../android/conversation/ImageActivity.java | 21 ++++++---- .../android/conversation/ImageFragment.java | 9 +++-- .../android/conversation/ImageViewModel.java | 40 ++++++++++++++++--- .../briar/android/viewmodel/LiveEvent.java | 16 ++++++++ .../android/viewmodel/MutableLiveEvent.java | 16 ++++++++ 6 files changed, 88 insertions(+), 18 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index bf39383ad..3bbf22011 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -1109,6 +1109,10 @@ public class ConversationActivity extends BriarActivity attachmentRetriever.getAttachmentItems(h); List items = new ArrayList<>(liveDataList.size()); for (LiveData liveData : liveDataList) { + // first remove all our observers to avoid having more than one + // in case we reload the conversation, e.g. after deleting messages + liveData.removeObservers(this); + // add a new observer liveData.observe(this, new AttachmentObserver(h.getId(), liveData)); items.add(requireNonNull(liveData.getValue())); } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java index 874e4b95f..7f8e3bedb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageActivity.java @@ -103,9 +103,20 @@ public class ImageActivity extends BriarActivity setSceneTransitionAnimation(transition, null, transition); } + // Intent Extras + Intent i = getIntent(); + attachments = + requireNonNull(i.getParcelableArrayListExtra(ATTACHMENTS)); + int position = i.getIntExtra(ATTACHMENT_POSITION, -1); + if (position == -1) throw new IllegalStateException(); + String name = i.getStringExtra(NAME); + long time = i.getLongExtra(DATE, 0); + byte[] messageIdBytes = requireNonNull(i.getByteArrayExtra(ITEM_ID)); + // get View Model viewModel = ViewModelProviders.of(this, viewModelFactory) .get(ImageViewModel.class); + viewModel.expectAttachments(attachments); viewModel.getSaveState().observeEvent(this, this::onImageSaveStateChanged); @@ -129,17 +140,11 @@ public class ImageActivity extends BriarActivity TextView contactName = toolbar.findViewById(R.id.contactName); TextView dateView = toolbar.findViewById(R.id.dateView); - // Intent Extras - Intent i = getIntent(); - attachments = i.getParcelableArrayListExtra(ATTACHMENTS); - int position = i.getIntExtra(ATTACHMENT_POSITION, -1); - if (position == -1) throw new IllegalStateException(); - String name = i.getStringExtra(NAME); - long time = i.getLongExtra(DATE, 0); + // Set contact name and message time String date = formatDateAbsolute(this, time); contactName.setText(name); dateView.setText(date); - conversationMessageId = new MessageId(i.getByteArrayExtra(ITEM_ID)); + conversationMessageId = new MessageId(messageIdBytes); // Set up image ViewPager viewPager = findViewById(R.id.viewPager); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java index 4a61b0dfb..09ec190eb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageFragment.java @@ -95,8 +95,6 @@ public class ImageFragment extends Fragment viewModel = ViewModelProviders.of(requireNonNull(getActivity()), viewModelFactory).get(ImageViewModel.class); - viewModel.getOnAttachmentLoaded() - .observeEvent(this, this::onAttachmentLoaded); photoView = v.findViewById(R.id.photoView); photoView.setScaleLevels(1, 2, 4); @@ -111,6 +109,9 @@ public class ImageFragment extends Fragment } else { photoView.setImageResource(R.drawable.ic_image_missing); startPostponedTransition(); + // state is not final, so observe state changes + viewModel.getOnAttachmentReceived(attachment.getMessageId()) + .observeEvent(this, this::onAttachmentReceived); } return v; @@ -127,8 +128,8 @@ public class ImageFragment extends Fragment .into(photoView); } - private void onAttachmentLoaded(MessageId messageId) { - if (attachment.getMessageId().equals(messageId)) loadImage(); + private void onAttachmentReceived(Boolean received) { + if (received) loadImage(); } @Override diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java index 58ef47dba..d53f26e7b 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java @@ -27,7 +27,9 @@ import java.io.InputStream; import java.io.OutputStream; import java.text.SimpleDateFormat; import java.util.Date; +import java.util.List; import java.util.Locale; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import java.util.logging.Logger; @@ -40,6 +42,7 @@ import androidx.lifecycle.AndroidViewModel; import static android.media.MediaScannerConnection.scanFile; import static android.os.Environment.DIRECTORY_PICTURES; import static android.os.Environment.getExternalStoragePublicDirectory; +import static java.util.Objects.requireNonNull; import static java.util.logging.Level.WARNING; import static java.util.logging.Logger.getLogger; import static org.briarproject.bramble.util.IoUtils.copyAndClose; @@ -57,8 +60,10 @@ public class ImageViewModel extends AndroidViewModel implements EventListener { @IoExecutor private final Executor ioExecutor; - private final MutableLiveEvent attachmentLoaded = - new MutableLiveEvent<>(); + private volatile boolean receivedAttachmentsInitialized = false; + private ConcurrentHashMap> + receivedAttachments = new ConcurrentHashMap<>(); + /** * true means there was an error saving the image, false if image was saved. */ @@ -91,13 +96,36 @@ public class ImageViewModel extends AndroidViewModel implements EventListener { @Override public void eventOccurred(Event e) { if (e instanceof AttachmentReceivedEvent) { - attachmentLoaded - .postEvent(((AttachmentReceivedEvent) e).getMessageId()); + MessageId id = ((AttachmentReceivedEvent) e).getMessageId(); + MutableLiveEvent oldEvent; + if (receivedAttachmentsInitialized) { + oldEvent = receivedAttachments.get(id); + } else { + oldEvent = receivedAttachments + .putIfAbsent(id, new MutableLiveEvent<>(true)); + } + if (oldEvent != null) oldEvent.postEvent(true); } } - LiveEvent getOnAttachmentLoaded() { - return attachmentLoaded; + @UiThread + public void expectAttachments(List attachments) { + for (AttachmentItem item : attachments) { + // no need to track items that are in a final state already + if (item.getState().isFinal()) continue; + // add new live events, if not added concurrently by Event + MessageId id = item.getMessageId(); + receivedAttachments.putIfAbsent(id, new MutableLiveEvent<>()); + } + receivedAttachmentsInitialized = true; + } + + @UiThread + LiveEvent getOnAttachmentReceived(MessageId messageId) { + if (receivedAttachments.size() == 0) { + throw new IllegalStateException("expectAttachments() not called"); + } + return requireNonNull(receivedAttachments.get(messageId)); } void clickImage() { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/LiveEvent.java b/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/LiveEvent.java index d7ff0cef5..d4e1b9cff 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/LiveEvent.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/LiveEvent.java @@ -12,6 +12,22 @@ import androidx.lifecycle.Observer; @NotNullByDefault public class LiveEvent extends LiveData> { + /** + * Creates a LiveEvent initialized with the given {@code value}. + * + * @param value initial value + */ + public LiveEvent(T value) { + super(new ConsumableEvent<>(value)); + } + + /** + * Creates a LiveEvent with no value assigned to it. + */ + public LiveEvent() { + super(); + } + public void observeEvent(LifecycleOwner owner, LiveEventHandler handler) { LiveEventObserver observer = new LiveEventObserver<>(handler); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/MutableLiveEvent.java b/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/MutableLiveEvent.java index dac33ecfc..fda307d5d 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/MutableLiveEvent.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/viewmodel/MutableLiveEvent.java @@ -5,6 +5,22 @@ import org.briarproject.bramble.api.nullsafety.NotNullByDefault; @NotNullByDefault public class MutableLiveEvent extends LiveEvent { + /** + * Creates a MutableLiveEvent initialized with the given {@code value}. + * + * @param value initial value + */ + public MutableLiveEvent(T value) { + super(value); + } + + /** + * Creates a MutableLiveEvent with no value assigned to it. + */ + public MutableLiveEvent() { + super(); + } + public void postEvent(T value) { super.postValue(new ConsumableEvent<>(value)); } From 0f6f52c37a66270b2ee59c5e1d50b0e51cb59c22 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 11 Feb 2020 12:41:41 -0300 Subject: [PATCH 08/10] [android] Listen to AttachmentReceivedEvents when ConversationActivity is stopped This way Attachments get shown when the activity resumes. --- .../conversation/ConversationActivity.java | 14 ----------- .../conversation/ConversationViewModel.java | 24 ++++++++++++++++++- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index 3bbf22011..4a73c33e8 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -74,7 +74,6 @@ import org.briarproject.briar.api.introduction.IntroductionManager; 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; @@ -652,13 +651,6 @@ 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)) { @@ -718,12 +710,6 @@ public class ConversationActivity extends BriarActivity scrollToBottom(); } - @UiThread - private void onAttachmentReceived(MessageId attachmentId) { - runOnDbThread( - () -> attachmentRetriever.loadAttachmentItem(attachmentId)); - } - @UiThread private void onNewConversationMessage(ConversationMessageHeader h) { if (h instanceof ConversationRequest || diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java index 6670bf5ea..5becffce2 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java @@ -11,6 +11,9 @@ 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.TransactionManager; +import org.briarproject.bramble.api.event.Event; +import org.briarproject.bramble.api.event.EventBus; +import org.briarproject.bramble.api.event.EventListener; import org.briarproject.bramble.api.identity.AuthorId; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.bramble.api.settings.Settings; @@ -30,6 +33,7 @@ import org.briarproject.briar.api.messaging.MessagingManager; import org.briarproject.briar.api.messaging.PrivateMessage; import org.briarproject.briar.api.messaging.PrivateMessageFactory; import org.briarproject.briar.api.messaging.PrivateMessageHeader; +import org.briarproject.briar.api.messaging.event.AttachmentReceivedEvent; import java.util.Collection; import java.util.List; @@ -56,7 +60,7 @@ import static org.briarproject.briar.android.util.UiUtils.observeForeverOnce; @NotNullByDefault public class ConversationViewModel extends AndroidViewModel - implements AttachmentManager { + implements EventListener, AttachmentManager { private static Logger LOG = getLogger(ConversationViewModel.class.getName()); @@ -69,6 +73,7 @@ public class ConversationViewModel extends AndroidViewModel @DatabaseExecutor private final Executor dbExecutor; private final TransactionManager db; + private final EventBus eventBus; private final MessagingManager messagingManager; private final ContactManager contactManager; private final SettingsManager settingsManager; @@ -101,6 +106,7 @@ public class ConversationViewModel extends AndroidViewModel ConversationViewModel(Application application, @DatabaseExecutor Executor dbExecutor, TransactionManager db, + EventBus eventBus, MessagingManager messagingManager, ContactManager contactManager, SettingsManager settingsManager, @@ -110,6 +116,7 @@ public class ConversationViewModel extends AndroidViewModel super(application); this.dbExecutor = dbExecutor; this.db = db; + this.eventBus = eventBus; this.messagingManager = messagingManager; this.contactManager = contactManager; this.settingsManager = settingsManager; @@ -119,12 +126,27 @@ public class ConversationViewModel extends AndroidViewModel messagingGroupId = Transformations .map(contact, c -> messagingManager.getContactGroup(c).getId()); contactDeleted.setValue(false); + + eventBus.addListener(this); } @Override protected void onCleared() { super.onCleared(); attachmentCreator.deleteUnsentAttachments(); + eventBus.removeListener(this); + } + + @Override + public void eventOccurred(Event e) { + if (e instanceof AttachmentReceivedEvent) { + AttachmentReceivedEvent a = (AttachmentReceivedEvent) e; + if (a.getContactId().equals(contactId)) { + LOG.info("Attachment received"); + dbExecutor.execute(() -> attachmentRetriever + .loadAttachmentItem(a.getMessageId())); + } + } } /** From 4d27828712db7f4a6dd492def966dc09716fecd8 Mon Sep 17 00:00:00 2001 From: akwizgran Date: Fri, 24 Jan 2020 14:48:47 +0000 Subject: [PATCH 09/10] Check for concurrent cache updates. --- .../attachment/AttachmentRetrieverImpl.java | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java index 2fa8c5849..1faa7dd1e 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/attachment/AttachmentRetrieverImpl.java @@ -15,8 +15,8 @@ import java.io.BufferedInputStream; import java.io.InputStream; import java.util.ArrayList; import java.util.List; -import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.concurrent.Executor; import java.util.logging.Logger; @@ -50,9 +50,9 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { private final int minWidth, maxWidth; private final int minHeight, maxHeight; - private final Map> + private final ConcurrentMap> itemsWithSize = new ConcurrentHashMap<>(); - private final Map> + private final ConcurrentMap> itemsWithoutSize = new ConcurrentHashMap<>(); @Inject @@ -99,15 +99,25 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { if (liveData == null) { AttachmentItem item = new AttachmentItem(h, defaultSize, defaultSize, LOADING); - final MutableLiveData finalLiveData = - new MutableLiveData<>(item); - // kick-off loading of attachment, will post to live data - dbExecutor.execute( - () -> loadAttachmentItem(h, needsSize, finalLiveData)); - // add new LiveData to cache - liveData = finalLiveData; - if (needsSize) itemsWithSize.put(h.getMessageId(), liveData); - else itemsWithoutSize.put(h.getMessageId(), liveData); + liveData = new MutableLiveData<>(item); + // add new LiveData to cache, checking for concurrent updates + MutableLiveData oldLiveData; + if (needsSize) { + oldLiveData = itemsWithSize.putIfAbsent(h.getMessageId(), + liveData); + } else { + oldLiveData = itemsWithoutSize.putIfAbsent(h.getMessageId(), + liveData); + } + if (oldLiveData == null) { + // kick-off loading of attachment, will post to live data + MutableLiveData finalLiveData = liveData; + dbExecutor.execute(() -> + loadAttachmentItem(h, needsSize, finalLiveData)); + } else { + // Concurrent cache update - use the existing live data + liveData = oldLiveData; + } } items.add(liveData); } @@ -118,12 +128,15 @@ class AttachmentRetrieverImpl implements AttachmentRetriever { @DatabaseExecutor public void cacheAttachmentItemWithSize(MessageId conversationMessageId, AttachmentHeader h) throws DbException { + // If a live data is already cached we don't need to do anything + if (itemsWithSize.containsKey(h.getMessageId())) return; try { Attachment a = messagingManager.getAttachment(h); AttachmentItem item = createAttachmentItem(a, true); MutableLiveData liveData = new MutableLiveData<>(item); - itemsWithSize.put(h.getMessageId(), liveData); + // If a live data was concurrently cached, don't replace it + itemsWithSize.putIfAbsent(h.getMessageId(), liveData); } catch (NoSuchMessageException e) { LOG.info("Attachment not received yet"); } From bded1edb2bf043431a2419c2d72be44092188430 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Thu, 13 Feb 2020 10:24:36 -0300 Subject: [PATCH 10/10] [android] Use ordinary HashMap for to be received attachments Also don't do list stacking from end for now. --- .../conversation/ConversationActivity.java | 1 - .../android/conversation/ImageViewModel.java | 27 ++++++++++--------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java index 4a73c33e8..7a164c388 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationActivity.java @@ -260,7 +260,6 @@ public class ConversationActivity extends BriarActivity adapter = new ConversationAdapter(this, this); list = findViewById(R.id.conversationView); layoutManager = new LinearLayoutManager(this); - layoutManager.setStackFromEnd(true); list.setLayoutManager(layoutManager); list.setAdapter(adapter); list.setEmptyText(getString(R.string.no_private_messages)); diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java index d53f26e7b..1a59b02bf 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ImageViewModel.java @@ -27,9 +27,9 @@ import java.io.InputStream; import java.io.OutputStream; import java.text.SimpleDateFormat; import java.util.Date; +import java.util.HashMap; import java.util.List; import java.util.Locale; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import java.util.logging.Logger; @@ -60,9 +60,9 @@ public class ImageViewModel extends AndroidViewModel implements EventListener { @IoExecutor private final Executor ioExecutor; - private volatile boolean receivedAttachmentsInitialized = false; - private ConcurrentHashMap> - receivedAttachments = new ConcurrentHashMap<>(); + private boolean receivedAttachmentsInitialized = false; + private HashMap> receivedAttachments = + new HashMap<>(); /** * true means there was an error saving the image, false if image was saved. @@ -93,6 +93,7 @@ public class ImageViewModel extends AndroidViewModel implements EventListener { eventBus.removeListener(this); } + @UiThread @Override public void eventOccurred(Event e) { if (e instanceof AttachmentReceivedEvent) { @@ -100,11 +101,10 @@ public class ImageViewModel extends AndroidViewModel implements EventListener { MutableLiveEvent oldEvent; if (receivedAttachmentsInitialized) { oldEvent = receivedAttachments.get(id); + if (oldEvent != null) oldEvent.postEvent(true); } else { - oldEvent = receivedAttachments - .putIfAbsent(id, new MutableLiveEvent<>(true)); + receivedAttachments.put(id, new MutableLiveEvent<>(true)); } - if (oldEvent != null) oldEvent.postEvent(true); } } @@ -113,18 +113,21 @@ public class ImageViewModel extends AndroidViewModel implements EventListener { for (AttachmentItem item : attachments) { // no need to track items that are in a final state already if (item.getState().isFinal()) continue; - // add new live events, if not added concurrently by Event + // add new live events, if not already added by eventOccurred() MessageId id = item.getMessageId(); - receivedAttachments.putIfAbsent(id, new MutableLiveEvent<>()); + if (!receivedAttachments.containsKey(id)) { + receivedAttachments.put(id, new MutableLiveEvent<>()); + } } receivedAttachmentsInitialized = true; } + /** + * Returns a LiveData for attachments in a non-final state. + * Note that you need to call {@link #expectAttachments(List)} first. + */ @UiThread LiveEvent getOnAttachmentReceived(MessageId messageId) { - if (receivedAttachments.size() == 0) { - throw new IllegalStateException("expectAttachments() not called"); - } return requireNonNull(receivedAttachments.get(messageId)); }