From 10e9fb308d971c95ddd47508c47992f392d8280b Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 19 Nov 2018 20:32:43 -0200 Subject: [PATCH] [android] Display Image Attachements: Address first round of review comments --- .../conversation/AttachmentController.java | 16 +++++++++++----- .../conversation/ConversationActivity.java | 18 +++++++++++------- .../conversation/ConversationMessageItem.java | 5 +---- .../ConversationMessageViewHolder.java | 6 ++---- .../conversation/ConversationVisitor.java | 1 - .../conversation/glide/BriarGlideModule.java | 5 +++-- 6 files changed, 28 insertions(+), 23 deletions(-) diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentController.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentController.java index 7fa5f3aba..7c4339c3e 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentController.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentController.java @@ -119,9 +119,14 @@ class AttachmentController { is.reset(); size = getSizeFromBitmap(is); } - is.close(); } catch (IOException e) { logException(LOG, WARNING, e); + } finally { + try { + is.close(); + } catch (IOException e) { + logException(LOG, WARNING, e); + } } // calculate thumbnail size @@ -161,7 +166,7 @@ class AttachmentController { BitmapFactory.Options options = new BitmapFactory.Options(); options.inJustDecodeBounds = true; BitmapFactory.decodeStream(is, null, options); - if (options.outWidth == -1 || options.outHeight == -1) + if (options.outWidth < 1 || options.outHeight < 1) return new Size(); return new Size(options.outWidth, options.outHeight); } @@ -178,9 +183,10 @@ class AttachmentController { } private static class Size { - private int width; - private int height; - private boolean error; + + private final int width; + private final int height; + private final boolean error; private Size(int width, int height) { this.width = width; 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 5050bc3ec..80d21d65b 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 @@ -444,8 +444,12 @@ public class ConversationActivity extends BriarActivity List headers) { runOnDbThread(() -> { try { - displayMessageAttachments(messageId, - attachmentController.getMessageAttachments(headers)); + List> attachments = + attachmentController.getMessageAttachments(headers); + // TODO move getting the items off to the IoExecutor + List items = + attachmentController.getAttachmentItems(attachments); + displayMessageAttachments(messageId, items); } catch (DbException e) { logException(LOG, WARNING, e); } @@ -453,10 +457,8 @@ public class ConversationActivity extends BriarActivity } private void displayMessageAttachments(MessageId m, - List> attachments) { + List items) { runOnUiThreadUnlessDestroyed(() -> { - List items = - attachmentController.getAttachmentItems(attachments); attachmentController.put(m, items); Pair pair = adapter.getMessageItem(m); @@ -829,12 +831,14 @@ public class ConversationActivity extends BriarActivity return text; } - @Nullable @Override public List getAttachmentItems(MessageId m, List headers) { List attachments = attachmentController.get(m); - if (attachments == null) loadMessageAttachments(m, headers); + if (attachments == null) { + loadMessageAttachments(m, headers); + return emptyList(); + } return attachments; } } 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 106b0aed2..1732e3ab8 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 @@ -1,7 +1,6 @@ package org.briarproject.briar.android.conversation; import android.support.annotation.LayoutRes; -import android.support.annotation.Nullable; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.briar.api.messaging.PrivateMessageHeader; @@ -14,16 +13,14 @@ import javax.annotation.concurrent.NotThreadSafe; @NotNullByDefault class ConversationMessageItem extends ConversationItem { - @Nullable private List attachments; ConversationMessageItem(@LayoutRes int layoutRes, PrivateMessageHeader h, - @Nullable List attachments) { + List attachments) { super(layoutRes, h); this.attachments = attachments; } - @Nullable List getAttachments() { return attachments; } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageViewHolder.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageViewHolder.java index aabab258e..74f269b19 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageViewHolder.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationMessageViewHolder.java @@ -21,7 +21,6 @@ import static android.os.Build.VERSION.SDK_INT; import static android.support.v4.view.ViewCompat.LAYOUT_DIRECTION_RTL; import static com.bumptech.glide.load.engine.DiskCacheStrategy.NONE; import static com.bumptech.glide.load.resource.drawable.DrawableTransitionOptions.withCrossFade; -import static java.util.Objects.requireNonNull; @UiThread @NotNullByDefault @@ -83,7 +82,7 @@ class ConversationMessageViewHolder extends ConversationItemViewHolder { super.bind(conversationItem, listener); ConversationMessageItem item = (ConversationMessageItem) conversationItem; - if (item.getAttachments() == null || item.getAttachments().isEmpty()) { + if (item.getAttachments().isEmpty()) { bindTextItem(); } else { bindImageItem(item); @@ -101,8 +100,7 @@ class ConversationMessageViewHolder extends ConversationItemViewHolder { private void bindImageItem(ConversationMessageItem item) { // TODO show more than just the first image - AttachmentItem attachment = - requireNonNull(item.getAttachments()).get(0); + AttachmentItem attachment = item.getAttachments().get(0); ConstraintSet constraintSet; if (item.getText() == null) { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationVisitor.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationVisitor.java index b8b57488e..3d6a85db9 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationVisitor.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationVisitor.java @@ -294,7 +294,6 @@ class ConversationVisitor implements } interface AttachmentCache { - @Nullable List getAttachmentItems(MessageId m, List headers); } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/glide/BriarGlideModule.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/glide/BriarGlideModule.java index f0776bae2..a1a66ab33 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/glide/BriarGlideModule.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/glide/BriarGlideModule.java @@ -25,8 +25,9 @@ public final class BriarGlideModule extends AppGlideModule { @Override public void registerComponents(Context context, Glide glide, Registry registry) { - BriarModelLoaderFactory factory = - new BriarModelLoaderFactory((BriarApplication) context); + BriarApplication app = + (BriarApplication) context.getApplicationContext(); + BriarModelLoaderFactory factory = new BriarModelLoaderFactory(app); registry.prepend(AttachmentItem.class, InputStream.class, factory); }