Send attachments in order

Stephen Paul Weber created 1 year ago

Not perfect. It means we lose paralellism of uploads which is maybe bad?
If any one errors the subsequent ones stay queued I think. If you're
offline or similar then it all gets written to the db and we stil lose
ordering.

May still be worth it, worth testing.

Change summary

src/main/java/eu/siacs/conversations/http/HttpConnectionManager.java                |  6 
src/main/java/eu/siacs/conversations/http/HttpUploadConnection.java                 |  7 
src/main/java/eu/siacs/conversations/services/AttachFileToConversationRunnable.java | 12 
src/main/java/eu/siacs/conversations/services/XmppConnectionService.java            | 41 
src/main/java/eu/siacs/conversations/ui/ConversationFragment.java                   | 67 
5 files changed, 81 insertions(+), 52 deletions(-)

Detailed changes

src/main/java/eu/siacs/conversations/http/HttpConnectionManager.java

@@ -111,6 +111,10 @@ public class HttpConnectionManager extends AbstractConnectionManager {
     }
 
     public void createNewUploadConnection(final Message message, boolean delay) {
+        createNewUploadConnection(message, delay, null);
+    }
+
+    public void createNewUploadConnection(final Message message, boolean delay, final Runnable cb) {
         synchronized (this.uploadConnections) {
             for (HttpUploadConnection connection : this.uploadConnections) {
                 if (connection.getMessage() == message) {
@@ -118,7 +122,7 @@ public class HttpConnectionManager extends AbstractConnectionManager {
                     return;
                 }
             }
-            HttpUploadConnection connection = new HttpUploadConnection(message, Method.determine(message.getConversation().getAccount()), this);
+            HttpUploadConnection connection = new HttpUploadConnection(message, Method.determine(message.getConversation().getAccount()), this, cb);
             connection.init(delay);
             this.uploadConnections.add(connection);
         }

src/main/java/eu/siacs/conversations/http/HttpUploadConnection.java

@@ -52,12 +52,14 @@ public class HttpUploadConnection implements Transferable, AbstractConnectionMan
     private long transmitted = 0;
     private Call mostRecentCall;
     private ListenableFuture<SlotRequester.Slot> slotFuture;
+    private Runnable cb;
 
-    public HttpUploadConnection(Message message, Method method, HttpConnectionManager httpConnectionManager) {
+    public HttpUploadConnection(Message message, Method method, HttpConnectionManager httpConnectionManager, Runnable cb) {
         this.message = message;
         this.method = method;
         this.mHttpConnectionManager = httpConnectionManager;
         this.mXmppConnectionService = httpConnectionManager.getXmppConnectionService();
+        this.cb = cb;
     }
 
     @Override
@@ -104,6 +106,7 @@ public class HttpUploadConnection implements Transferable, AbstractConnectionMan
         final Future<SlotRequester.Slot> slotFuture = this.slotFuture;
         final boolean cancelled = (call != null && call.isCanceled()) || (slotFuture != null && slotFuture.isCancelled());
         mXmppConnectionService.markMessage(message, Message.STATUS_SEND_FAILED, cancelled ? Message.ERROR_MESSAGE_CANCELLED : errorMessage);
+        if (cb != null) cb.run();
     }
 
     private void finish() {
@@ -191,7 +194,7 @@ public class HttpUploadConnection implements Transferable, AbstractConnectionMan
                     if (!message.isPrivateMessage()) {
                         message.setCounterpart(message.getConversation().getJid().asBareJid());
                     }
-                    mXmppConnectionService.resendMessage(message, delayed);
+                    mXmppConnectionService.resendMessage(message, delayed, cb);
                 } else {
                     Log.d(Config.LOGTAG, "http upload failed because response code was " + code);
                     fail("http upload failed because response code was " + code);

src/main/java/eu/siacs/conversations/services/AttachFileToConversationRunnable.java

@@ -63,8 +63,7 @@ public class AttachFileToConversationRunnable implements Runnable, TranscoderLis
             final int encryption = message.getEncryption();
             mXmppConnectionService.getHttpConnectionManager().createNewDownloadConnection(message, false, (file) -> {
                 message.setEncryption(encryption);
-                mXmppConnectionService.sendMessage(message);
-                callback.success(message);
+                mXmppConnectionService.sendMessage(message, () -> callback.success(message));
             });
         } else if (path != null && !FileBackend.isPathBlacklisted(path)) {
             message.setRelativeFilePath(path);
@@ -72,8 +71,7 @@ public class AttachFileToConversationRunnable implements Runnable, TranscoderLis
             if (message.getEncryption() == Message.ENCRYPTION_DECRYPTED) {
                 mXmppConnectionService.getPgpEngine().encrypt(message, callback);
             } else {
-                mXmppConnectionService.sendMessage(message);
-                callback.success(message);
+                mXmppConnectionService.sendMessage(message, () -> callback.success(message));
             }
         } else {
             try {
@@ -87,8 +85,7 @@ public class AttachFileToConversationRunnable implements Runnable, TranscoderLis
                         callback.error(R.string.unable_to_connect_to_keychain, null);
                     }
                 } else {
-                    mXmppConnectionService.sendMessage(message);
-                    callback.success(message);
+                    mXmppConnectionService.sendMessage(message, () -> callback.success(message));
                 }
             } catch (FileBackend.FileCopyException e) {
                 callback.error(e.getResId(), message);
@@ -163,8 +160,7 @@ public class AttachFileToConversationRunnable implements Runnable, TranscoderLis
         if (message.getEncryption() == Message.ENCRYPTION_DECRYPTED) {
             mXmppConnectionService.getPgpEngine().encrypt(message, callback);
         } else {
-            mXmppConnectionService.sendMessage(message);
-            callback.success(message);
+            mXmppConnectionService.sendMessage(message, () -> callback.success(message));
         }
     }
 

src/main/java/eu/siacs/conversations/services/XmppConnectionService.java

@@ -709,8 +709,7 @@ public class XmppConnectionService extends Service {
                     callback.error(R.string.unable_to_connect_to_keychain, null);
                 }
             } else {
-                sendMessage(message);
-                callback.success(message);
+                sendMessage(message, false, false, false, () -> callback.success(message));
             }
         });
     }
@@ -1868,22 +1867,27 @@ public class XmppConnectionService extends Service {
         }
     }
 
-    private void sendFileMessage(final Message message, final boolean delay) {
+    private void sendFileMessage(final Message message, final boolean delay, final Runnable cb) {
         Log.d(Config.LOGTAG, "send file message");
         final Account account = message.getConversation().getAccount();
         if (account.httpUploadAvailable(fileBackend.getFile(message, false).getSize())
                 || message.getConversation().getMode() == Conversation.MODE_MULTI) {
-            mHttpConnectionManager.createNewUploadConnection(message, delay);
+            mHttpConnectionManager.createNewUploadConnection(message, delay, cb);
         } else {
             mJingleConnectionManager.startJingleFileTransfer(message);
+            if (cb != null) cb.run();
         }
     }
 
     public void sendMessage(final Message message) {
-        sendMessage(message, false, false, false);
+        sendMessage(message, false, false, false, null);
     }
 
-    private void sendMessage(final Message message, final boolean resend, final boolean previewedLinks, final boolean delay) {
+    public void sendMessage(final Message message, final Runnable cb) {
+        sendMessage(message, false, false, false, cb);
+    }
+
+    private void sendMessage(final Message message, final boolean resend, final boolean previewedLinks, final boolean delay, final Runnable cb) {
         final Account account = message.getConversation().getAccount();
         if (account.setShowErrorNotification(true)) {
             databaseBackend.updateAccount(account);
@@ -1892,7 +1896,6 @@ public class XmppConnectionService extends Service {
         final Conversation conversation = (Conversation) message.getConversation();
         account.deactivateGracePeriod();
 
-
         if (QuickConversationsService.isQuicksy() && conversation.getMode() == Conversation.MODE_SINGLE) {
             final Contact contact = conversation.getContact();
             if (!contact.showInRoster() && contact.getOption(Contact.Options.SYNCED_VIA_OTHER)) {
@@ -1964,7 +1967,7 @@ public class XmppConnectionService extends Service {
                                         getHttpConnectionManager().createNewDownloadConnection(message, false, (file) -> {
                                             message.setEncryption(encryption);
                                             synchronized (message.getConversation()) {
-                                                if (message.getStatus() == Message.STATUS_WAITING) sendMessage(message, true, true, false);
+                                                if (message.getStatus() == Message.STATUS_WAITING) sendMessage(message, true, true, false, cb);
                                             }
                                         });
                                         return;
@@ -2018,13 +2021,14 @@ public class XmppConnectionService extends Service {
                             }
                         }
                         synchronized (message.getConversation()) {
-                            if (message.getStatus() == Message.STATUS_WAITING) sendMessage(message, true, true, false);
+                            if (message.getStatus() == Message.STATUS_WAITING) sendMessage(message, true, true, false, cb);
                         }
                     });
                 }
             }
         }
 
+        boolean passedCbOn = false;
         if (account.isOnlineAndConnected() && !inProgressJoin && !waitForPreview && message.getTimeSent() <= System.currentTimeMillis()) {
             switch (message.getEncryption()) {
                 case Message.ENCRYPTION_NONE:
@@ -2032,7 +2036,8 @@ public class XmppConnectionService extends Service {
                         if (account.httpUploadAvailable(fileBackend.getFile(message, false).getSize())
                                 || conversation.getMode() == Conversation.MODE_MULTI
                                 || message.fixCounterpart()) {
-                            this.sendFileMessage(message, delay);
+                            this.sendFileMessage(message, delay, cb);
+                            passedCbOn = true;
                         } else {
                             break;
                         }
@@ -2046,7 +2051,8 @@ public class XmppConnectionService extends Service {
                         if (account.httpUploadAvailable(fileBackend.getFile(message, false).getSize())
                                 || conversation.getMode() == Conversation.MODE_MULTI
                                 || message.fixCounterpart()) {
-                            this.sendFileMessage(message, delay);
+                            this.sendFileMessage(message, delay, cb);
+                            passedCbOn = true;
                         } else {
                             break;
                         }
@@ -2060,7 +2066,8 @@ public class XmppConnectionService extends Service {
                         if (account.httpUploadAvailable(fileBackend.getFile(message, false).getSize())
                                 || conversation.getMode() == Conversation.MODE_MULTI
                                 || message.fixCounterpart()) {
-                            this.sendFileMessage(message, delay);
+                            this.sendFileMessage(message, delay, cb);
+                            passedCbOn = true;
                         } else {
                             break;
                         }
@@ -2098,6 +2105,7 @@ public class XmppConnectionService extends Service {
                                 Log.e(Config.LOGTAG, "error updated message in DB after edit");
                             }
                             updateConversationUi();
+                            if (!waitForPreview && cb != null) cb.run();
                             return;
                         } else {
                             databaseBackend.createMessage(message);
@@ -2162,6 +2170,7 @@ public class XmppConnectionService extends Service {
                 if (message.getConversation() instanceof Conversation) presenceToMuc((Conversation) message.getConversation());
             }
         }
+        if (!waitForPreview && !passedCbOn && cb != null) { Log.d("WUT cb", message.getRawBody()); cb.run(); }
     }
 
     private boolean isJoinInProgress(final Conversation conversation) {
@@ -2188,11 +2197,15 @@ public class XmppConnectionService extends Service {
     }
 
     public void resendMessage(final Message message, final boolean delay) {
-        sendMessage(message, true, false, delay);
+        sendMessage(message, true, false, delay, null);
+    }
+
+    public void resendMessage(final Message message, final boolean delay, final Runnable cb) {
+        sendMessage(message, true, false, delay, cb);
     }
 
     public void resendMessage(final Message message, final boolean delay, final boolean previewedLinks) {
-        sendMessage(message, true, previewedLinks, delay);
+        sendMessage(message, true, previewedLinks, delay, null);
     }
 
     public Pair<Account,Account> onboardingIncomplete() {

src/main/java/eu/siacs/conversations/ui/ConversationFragment.java

@@ -853,7 +853,7 @@ public class ConversationFragment extends XmppFragment
                 });
     }
 
-    private void attachFileToConversation(Conversation conversation, Uri uri, String type) {
+    private void attachFileToConversation(Conversation conversation, Uri uri, String type, Runnable next) {
         if (conversation == null) {
             return;
         }
@@ -877,10 +877,14 @@ public class ConversationFragment extends XmppFragment
 
                     @Override
                     public void success(Message message) {
-                        runOnUiThread(() -> {
-                            activity.hideToast();
-                            messageSent();
-                        });
+                        if (next == null) {
+                            runOnUiThread(() -> {
+                                activity.hideToast();
+                                messageSent();
+                            });
+                        } else {
+                            runOnUiThread(next);
+                        }
                         hidePrepareFileToast(prepareFileToast);
                     }
 
@@ -903,7 +907,7 @@ public class ConversationFragment extends XmppFragment
         toggleInputMethod();
     }
 
-    private void attachImageToConversation(Conversation conversation, Uri uri, String type) {
+    private void attachImageToConversation(Conversation conversation, Uri uri, String type, Runnable next) {
         if (conversation == null) {
             return;
         }
@@ -927,7 +931,11 @@ public class ConversationFragment extends XmppFragment
                     @Override
                     public void success(Message message) {
                         hidePrepareFileToast(prepareFileToast);
-                        runOnUiThread(() -> messageSent());
+                        if (next == null) {
+                            runOnUiThread(() -> messageSent());
+                        } else {
+                            runOnUiThread(next);
+                        }
                     }
 
                     @Override
@@ -1258,26 +1266,31 @@ public class ConversationFragment extends XmppFragment
         }
         final PresenceSelector.OnPresenceSelected callback =
                 () -> {
-                    for (Iterator<Attachment> i = attachments.iterator(); i.hasNext(); i.remove()) {
-                        final Attachment attachment = i.next();
-                        if (attachment.getType() == Attachment.Type.LOCATION) {
-                            attachLocationToConversation(conversation, attachment.getUri());
-                        } else if (attachment.getType() == Attachment.Type.IMAGE) {
-                            Log.d(
-                                    Config.LOGTAG,
-                                    "ConversationsActivity.commitAttachments() - attaching image to conversations. CHOOSE_IMAGE");
-                            attachImageToConversation(
-                                    conversation, attachment.getUri(), attachment.getMime());
-                        } else {
-                            Log.d(
-                                    Config.LOGTAG,
-                                    "ConversationsActivity.commitAttachments() - attaching file to conversations. CHOOSE_FILE/RECORD_VOICE/RECORD_VIDEO");
-                            attachFileToConversation(
-                                    conversation, attachment.getUri(), attachment.getMime());
+                    final Iterator<Attachment> i = attachments.iterator();
+                    final Runnable next = new Runnable() {
+                        @Override
+                        public void run() {
+                            final Attachment attachment = i.next();
+                            if (attachment.getType() == Attachment.Type.LOCATION) {
+                                attachLocationToConversation(conversation, attachment.getUri());
+                                if (i.hasNext()) runOnUiThread(this);
+                            } else if (attachment.getType() == Attachment.Type.IMAGE) {
+                                Log.d(
+                                      Config.LOGTAG,
+                                      "ConversationsActivity.commitAttachments() - attaching image to conversations. CHOOSE_IMAGE");
+                                attachImageToConversation(conversation, attachment.getUri(), attachment.getMime(), i.hasNext() ? this : null);
+                            } else {
+                                Log.d(
+                                      Config.LOGTAG,
+                                      "ConversationsActivity.commitAttachments() - attaching file to conversations. CHOOSE_FILE/RECORD_VOICE/RECORD_VIDEO");
+                                attachFileToConversation(conversation, attachment.getUri(), attachment.getMime(), i.hasNext() ? this : null);
+                            }
+                            i.remove();
+                            mediaPreviewAdapter.notifyDataSetChanged();
+                            toggleInputMethod();
                         }
-                    }
-                    mediaPreviewAdapter.notifyDataSetChanged();
-                    toggleInputMethod();
+                    };
+                    next.run();
                 };
         if (conversation == null
                 || conversation.getMode() == Conversation.MODE_MULTI
@@ -1590,7 +1603,7 @@ public class ConversationFragment extends XmppFragment
                     if (range == null) return false;
                     range[0] -= 1;
                     if ("\0attention".equals(user.getOccupantId())) {
-	                    editable.delete(Math.max(0, range[0]), Math.min(editable.length(), range[1]));
+                        editable.delete(Math.max(0, range[0]), Math.min(editable.length(), range[1]));
                         editable.insert(0, "@here ");
                         return true;
                     }