From 7cc67daf80603e204fb0c669607aab0623e6bf18 Mon Sep 17 00:00:00 2001
From: Anton Chekulaev <metaphore@bk.ru>
Date: Mon, 31 Aug 2020 19:46:05 +1000
Subject: [PATCH] DB attachment record gets properly updated on upload error.
 Individual loading indicators per conversation item thumbnail.

---
 res/layout/conversation_item_thumbnail.xml    | 11 ----------
 res/layout/thumbnail_view.xml                 |  9 ++++++++
 .../components/AlbumThumbnailView.java        |  7 ++-----
 .../components/ConversationItemThumbnail.java | 21 ++++---------------
 .../securesms/components/ThumbnailView.java   | 12 ++++++++---
 .../database/AttachmentDatabase.java          | 11 +++++++++-
 .../securesms/jobs/AttachmentUploadJob.java   | 19 +++++++++++------
 7 files changed, 47 insertions(+), 43 deletions(-)

diff --git a/res/layout/conversation_item_thumbnail.xml b/res/layout/conversation_item_thumbnail.xml
index fe438060da..bff1a90fa8 100644
--- a/res/layout/conversation_item_thumbnail.xml
+++ b/res/layout/conversation_item_thumbnail.xml
@@ -45,15 +45,4 @@
         app:footer_text_color="@android:color/white"
         app:footer_icon_color="@android:color/white"/>
 
-    <ProgressBar
-        android:id="@+id/conversation_thumbnail_load_indicator"
-        android:layout_width="wrap_content"
-        android:layout_height="wrap_content"
-        android:layout_gravity="center"
-        android:maxWidth="48dp"
-        android:maxHeight="48dp"
-        android:indeterminate="true"
-        android:visibility="gone"
-        tools:visibility="visible"/>
-
 </merge>
diff --git a/res/layout/thumbnail_view.xml b/res/layout/thumbnail_view.xml
index a9f979014c..96ec33477e 100644
--- a/res/layout/thumbnail_view.xml
+++ b/res/layout/thumbnail_view.xml
@@ -22,6 +22,15 @@
         android:src="@drawable/ic_caption_28"
         android:visibility="gone" />
 
+    <ProgressBar
+        android:id="@+id/thumbnail_load_indicator"
+        android:layout_width="48dp"
+        android:layout_height="48dp"
+        android:layout_gravity="center"
+        android:indeterminate="true"
+        android:visibility="gone"
+        tools:visibility="visible"/>
+
     <FrameLayout
         android:id="@+id/play_overlay"
         android:layout_width="48dp"
diff --git a/src/org/thoughtcrime/securesms/components/AlbumThumbnailView.java b/src/org/thoughtcrime/securesms/components/AlbumThumbnailView.java
index a1b5b71441..6b0b35bc8d 100644
--- a/src/org/thoughtcrime/securesms/components/AlbumThumbnailView.java
+++ b/src/org/thoughtcrime/securesms/components/AlbumThumbnailView.java
@@ -73,7 +73,7 @@ public class AlbumThumbnailView extends FrameLayout {
       }
     }
 
-    int sizeClass = sizeClass(slides.size());
+    int sizeClass = Math.min(slides.size(), 6);
 
     if (sizeClass != currentSizeClass) {
       inflateLayout(sizeClass);
@@ -148,11 +148,8 @@ public class AlbumThumbnailView extends FrameLayout {
   private void setSlide(@NonNull GlideRequests glideRequests, @NonNull Slide slide, @IdRes int id) {
     ThumbnailView cell = findViewById(id);
     cell.setImageResource(glideRequests, slide, false, false);
+    cell.setLoadIndicatorVisibile(slide.isInProgress());
     cell.setThumbnailClickListener(defaultThumbnailClickListener);
     cell.setOnLongClickListener(defaultLongClickListener);
   }
-
-  private int sizeClass(int size) {
-    return Math.min(size, 6);
-  }
 }
diff --git a/src/org/thoughtcrime/securesms/components/ConversationItemThumbnail.java b/src/org/thoughtcrime/securesms/components/ConversationItemThumbnail.java
index ef7db1abb1..ba86eb8993 100644
--- a/src/org/thoughtcrime/securesms/components/ConversationItemThumbnail.java
+++ b/src/org/thoughtcrime/securesms/components/ConversationItemThumbnail.java
@@ -6,7 +6,6 @@ import android.graphics.Canvas;
 import android.util.AttributeSet;
 import android.widget.FrameLayout;
 import android.widget.ImageView;
-import android.widget.ProgressBar;
 
 import androidx.annotation.ColorInt;
 import androidx.annotation.NonNull;
@@ -30,7 +29,6 @@ public class ConversationItemThumbnail extends FrameLayout {
   private AlbumThumbnailView     album;
   private ImageView              shade;
   private ConversationItemFooter footer;
-  private ProgressBar            loadIndicator;
   private CornerMask             cornerMask;
   private Outliner               outliner;
   private boolean                borderless;
@@ -57,7 +55,6 @@ public class ConversationItemThumbnail extends FrameLayout {
     this.album         = findViewById(R.id.conversation_thumbnail_album);
     this.shade         = findViewById(R.id.conversation_thumbnail_shade);
     this.footer        = findViewById(R.id.conversation_thumbnail_footer);
-    this.loadIndicator = findViewById(R.id.conversation_thumbnail_load_indicator);
     this.cornerMask    = new CornerMask(this);
     this.outliner      = new Outliner();
 
@@ -131,8 +128,10 @@ public class ConversationItemThumbnail extends FrameLayout {
       thumbnail.setVisibility(VISIBLE);
       album.setVisibility(GONE);
 
-      Attachment attachment = slides.get(0).asAttachment();
-      thumbnail.setImageResource(glideRequests, slides.get(0), showControls, isPreview, attachment.getWidth(), attachment.getHeight());
+      Slide slide = slides.get(0);
+      Attachment attachment = slide.asAttachment();
+      thumbnail.setImageResource(glideRequests, slide, showControls, isPreview, attachment.getWidth(), attachment.getHeight());
+      thumbnail.setLoadIndicatorVisibile(slide.isInProgress());
       setTouchDelegate(thumbnail.getTouchDelegate());
     } else {
       thumbnail.setVisibility(GONE);
@@ -141,18 +140,6 @@ public class ConversationItemThumbnail extends FrameLayout {
       album.setSlides(glideRequests, slides, showControls);
       setTouchDelegate(album.getTouchDelegate());
     }
-
-    // Display loading indicator if any attachment is in loading state.
-    {
-      boolean anyLoading = false;
-      for (int i = 0; i < slides.size(); i++) {
-        if (slides.get(i).asAttachment().isInProgress()) {
-          anyLoading = true;
-          break;
-        }
-      }
-      loadIndicator.setVisibility(anyLoading ? VISIBLE : GONE);
-    }
   }
 
   public void setConversationColor(@ColorInt int color) {
diff --git a/src/org/thoughtcrime/securesms/components/ThumbnailView.java b/src/org/thoughtcrime/securesms/components/ThumbnailView.java
index e648fcb0c8..1cdf1949bc 100644
--- a/src/org/thoughtcrime/securesms/components/ThumbnailView.java
+++ b/src/org/thoughtcrime/securesms/components/ThumbnailView.java
@@ -52,6 +52,7 @@ public class ThumbnailView extends FrameLayout {
   private ImageView       image;
   private View            playOverlay;
   private View            captionIcon;
+  private View            loadIndicator;
   private OnClickListener parentClickListener;
 
   private final int[] dimens        = new int[2];
@@ -78,9 +79,10 @@ public class ThumbnailView extends FrameLayout {
 
     inflate(context, R.layout.thumbnail_view, this);
 
-    this.image       = findViewById(R.id.thumbnail_image);
-    this.playOverlay = findViewById(R.id.play_overlay);
-    this.captionIcon = findViewById(R.id.thumbnail_caption_icon);
+    this.image          = findViewById(R.id.thumbnail_image);
+    this.playOverlay    = findViewById(R.id.play_overlay);
+    this.captionIcon    = findViewById(R.id.thumbnail_caption_icon);
+    this.loadIndicator  = findViewById(R.id.thumbnail_load_indicator);
     super.setOnClickListener(new ThumbnailClickDispatcher());
 
     if (attrs != null) {
@@ -336,6 +338,10 @@ public class ThumbnailView extends FrameLayout {
     getTransferControls().showProgressSpinner();
   }
 
+  public void setLoadIndicatorVisibile(boolean visible) {
+    this.loadIndicator.setVisibility(visible ? VISIBLE : GONE);
+  }
+
   protected void setRadius(int radius) {
     this.radius = radius;
   }
diff --git a/src/org/thoughtcrime/securesms/database/AttachmentDatabase.java b/src/org/thoughtcrime/securesms/database/AttachmentDatabase.java
index 9342c63c71..8f6f663fe6 100644
--- a/src/org/thoughtcrime/securesms/database/AttachmentDatabase.java
+++ b/src/org/thoughtcrime/securesms/database/AttachmentDatabase.java
@@ -374,7 +374,7 @@ public class AttachmentDatabase extends Database {
     thumbnailExecutor.submit(new ThumbnailFetchCallable(attachmentId));
   }
 
-  public void updateAttachmentAfterUpload(@NonNull AttachmentId id, @NonNull Attachment attachment) {
+  public void updateAttachmentAfterUploadSucceeded(@NonNull AttachmentId id, @NonNull Attachment attachment) {
     SQLiteDatabase database = databaseHelper.getWritableDatabase();
     ContentValues  values   = new ContentValues();
 
@@ -390,6 +390,15 @@ public class AttachmentDatabase extends Database {
     database.update(TABLE_NAME, values, PART_ID_WHERE, id.toStrings());
   }
 
+  public void updateAttachmentAfterUploadFailed(@NonNull AttachmentId id) {
+    SQLiteDatabase database = databaseHelper.getWritableDatabase();
+    ContentValues  values   = new ContentValues();
+
+    values.put(TRANSFER_STATE, TRANSFER_PROGRESS_FAILED);
+
+    database.update(TABLE_NAME, values, PART_ID_WHERE, id.toStrings());
+  }
+
   @NonNull Map<Attachment, AttachmentId> insertAttachmentsForMessage(long mmsId, @NonNull List<Attachment> attachments, @NonNull List<Attachment> quoteAttachment)
       throws MmsException
   {
diff --git a/src/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.java b/src/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.java
index 766e8f20f6..a356deabec 100644
--- a/src/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.java
+++ b/src/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.java
@@ -87,13 +87,20 @@ public class AttachmentUploadJob extends BaseJob implements InjectableType {
     
     // Only upload attachment if necessary
     if (databaseAttachment.getUrl().isEmpty()) {
-      MediaConstraints mediaConstraints = MediaConstraints.getPushMediaConstraints();
-      Attachment scaledAttachment = scaleAndStripExif(database, mediaConstraints, databaseAttachment);
-      SignalServiceAttachment localAttachment = getAttachmentFor(scaledAttachment);
-      SignalServiceAttachmentPointer remoteAttachment = messageSender.uploadAttachment(localAttachment.asStream(), databaseAttachment.isSticker(), new SignalServiceAddress(destination.serialize()));
-      Attachment attachment = PointerAttachment.forPointer(Optional.of(remoteAttachment), null, databaseAttachment.getFastPreflightId()).get();
+      final Attachment attachment;
+      try {
+        MediaConstraints mediaConstraints = MediaConstraints.getPushMediaConstraints();
+        Attachment scaledAttachment = scaleAndStripExif(database, mediaConstraints, databaseAttachment);
+        SignalServiceAttachment localAttachment = getAttachmentFor(scaledAttachment);
+        SignalServiceAttachmentPointer remoteAttachment = messageSender.uploadAttachment(localAttachment.asStream(), databaseAttachment.isSticker(), new SignalServiceAddress(destination.serialize()));
+        attachment = PointerAttachment.forPointer(Optional.of(remoteAttachment), null, databaseAttachment.getFastPreflightId()).get();
+      } catch (Exception e) {
+        // On any error make sure we mark the related DB record's transfer state as failed.
+        database.updateAttachmentAfterUploadFailed(databaseAttachment.getAttachmentId());
+        throw e;
+      }
 
-      database.updateAttachmentAfterUpload(databaseAttachment.getAttachmentId(), attachment);
+      database.updateAttachmentAfterUploadSucceeded(databaseAttachment.getAttachmentId(), attachment);
     }
   }