From e67e55227bca1b71f2d42a0cbcdbc46fe6bbe97d Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 11 Nov 2019 16:39:35 -0300 Subject: [PATCH] [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