From f50506a265f4580f35c96e513627f25311b0ddf0 Mon Sep 17 00:00:00 2001
From: Daniel Gultsch <daniel@gultsch.de>
Date: Wed, 13 Nov 2024 17:27:16 +0100
Subject: [PATCH] refactor correction (message edit) code

---
 .../java/eu/siacs/conversations/Config.java   |  2 -
 .../conversations/entities/Conversation.java  |  3 +-
 .../siacs/conversations/entities/Message.java | 25 +++--------
 .../generator/MessageGenerator.java           |  3 +-
 .../conversations/parser/MessageParser.java   | 43 ++++++++++++++-----
 .../ui/ConversationFragment.java              |  3 +-
 .../xmpp/model/correction/Replace.java        |  5 +++
 7 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/src/main/java/eu/siacs/conversations/Config.java b/src/main/java/eu/siacs/conversations/Config.java
index 985f03ebb8a280cf45c1a9ce10e17f70eb375496..cfe1cc60fafda27271c073c70fb88ac437bc2a1f 100644
--- a/src/main/java/eu/siacs/conversations/Config.java
+++ b/src/main/java/eu/siacs/conversations/Config.java
@@ -144,8 +144,6 @@ public final class Config {
     public static final boolean IGNORE_ID_REWRITE_IN_MUC = true;
     public static final boolean MUC_LEAVE_BEFORE_JOIN = false;
 
-    public static final boolean USE_LMC_VERSION_1_1 = true;
-
     public static final long MAM_MAX_CATCHUP = MILLISECONDS_IN_DAY * 5;
     public static final int MAM_MAX_MESSAGES = 750;
 
diff --git a/src/main/java/eu/siacs/conversations/entities/Conversation.java b/src/main/java/eu/siacs/conversations/entities/Conversation.java
index f2b17015f85c84e4afa5c2fbec20b1e679b65a85..bf8bdd7fb1d729cb73070b02f85f5a788cebdb58 100644
--- a/src/main/java/eu/siacs/conversations/entities/Conversation.java
+++ b/src/main/java/eu/siacs/conversations/entities/Conversation.java
@@ -415,8 +415,7 @@ public class Conversation extends AbstractEntity implements Blockable, Comparabl
                 }
                 if (mcp.equals(counterpart) && ((message.getStatus() == Message.STATUS_RECEIVED) == received)
                         && (carbon == message.isCarbon() || received)) {
-                    final boolean idMatch = id.equals(message.getRemoteMsgId()) || message.remoteMsgIdMatchInEdit(id);
-                    if (idMatch && !message.isFileOrImage() && !message.treatAsDownloadable()) {
+                    if (id.equals(message.getRemoteMsgId()) && !message.isFileOrImage() && !message.treatAsDownloadable()) {
                         return message;
                     } else {
                         return null;
diff --git a/src/main/java/eu/siacs/conversations/entities/Message.java b/src/main/java/eu/siacs/conversations/entities/Message.java
index 237525a0859ba010d807f8e9d7f2db746451e378..6fb1ec2bd1e5c470635a8e3989d3e91ace474967 100644
--- a/src/main/java/eu/siacs/conversations/entities/Message.java
+++ b/src/main/java/eu/siacs/conversations/entities/Message.java
@@ -489,15 +489,6 @@ public class Message extends AbstractEntity implements AvatarService.Avatarable
         }
     }
 
-    boolean remoteMsgIdMatchInEdit(String id) {
-        for (Edit edit : this.edits) {
-            if (id.equals(edit.getEditedId())) {
-                return true;
-            }
-        }
-        return false;
-    }
-
     public String getBodyLanguage() {
         return this.bodyLanguage;
     }
@@ -507,7 +498,7 @@ public class Message extends AbstractEntity implements AvatarService.Avatarable
     }
 
     public boolean edited() {
-        return this.edits.size() > 0;
+        return !this.edits.isEmpty();
     }
 
     public void setTrueCounterpart(Jid trueCounterpart) {
@@ -828,19 +819,17 @@ public class Message extends AbstractEntity implements AvatarService.Avatarable
     }
 
     public String getEditedId() {
-        if (edits.size() > 0) {
-            return edits.get(edits.size() - 1).getEditedId();
-        } else {
-            throw new IllegalStateException("Attempting to store unedited message");
+        if (this.edits.isEmpty()) {
+            throw new IllegalStateException("Attempting to access unedited message");
         }
+        return edits.get(edits.size() - 1).getEditedId();
     }
 
     public String getEditedIdWireFormat() {
-        if (edits.size() > 0) {
-            return edits.get(Config.USE_LMC_VERSION_1_1 ? 0 : edits.size() - 1).getEditedId();
-        } else {
-            throw new IllegalStateException("Attempting to store unedited message");
+        if (this.edits.isEmpty()) {
+            throw new IllegalStateException("Attempting to access unedited message");
         }
+        return edits.get(0).getEditedId();
     }
 
     public void setOob(boolean isOob) {
diff --git a/src/main/java/eu/siacs/conversations/generator/MessageGenerator.java b/src/main/java/eu/siacs/conversations/generator/MessageGenerator.java
index ff5dddb0f7e4f6fd0f4e31362957a139dd491f38..af4349350a7ca0b437f33c42f1e24feb8effa126 100644
--- a/src/main/java/eu/siacs/conversations/generator/MessageGenerator.java
+++ b/src/main/java/eu/siacs/conversations/generator/MessageGenerator.java
@@ -23,6 +23,7 @@ import eu.siacs.conversations.xmpp.jingle.JingleConnectionManager;
 import eu.siacs.conversations.xmpp.jingle.JingleRtpConnection;
 import eu.siacs.conversations.xmpp.jingle.Media;
 import eu.siacs.conversations.xmpp.jingle.stanzas.Reason;
+import im.conversations.android.xmpp.model.correction.Replace;
 import im.conversations.android.xmpp.model.reactions.Reaction;
 import im.conversations.android.xmpp.model.reactions.Reactions;
 
@@ -63,7 +64,7 @@ public class MessageGenerator extends AbstractGenerator {
             packet.addChild("origin-id", Namespace.STANZA_IDS).setAttribute("id", message.getUuid());
         }
         if (message.edited()) {
-            packet.addChild("replace", "urn:xmpp:message-correct:0").setAttribute("id", message.getEditedIdWireFormat());
+            packet.addExtension(new Replace(message.getEditedIdWireFormat()));
         }
         return packet;
     }
diff --git a/src/main/java/eu/siacs/conversations/parser/MessageParser.java b/src/main/java/eu/siacs/conversations/parser/MessageParser.java
index 681c2b4bd230c11e2a2c0ce9682f6f524db041fe..3ddb981240cf68030eb37bfbd656b71459b1282e 100644
--- a/src/main/java/eu/siacs/conversations/parser/MessageParser.java
+++ b/src/main/java/eu/siacs/conversations/parser/MessageParser.java
@@ -3,6 +3,7 @@ package eu.siacs.conversations.parser;
 import android.util.Log;
 import android.util.Pair;
 
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSet;
 
 import java.text.SimpleDateFormat;
@@ -54,6 +55,7 @@ import eu.siacs.conversations.xmpp.pep.Avatar;
 import im.conversations.android.xmpp.model.Extension;
 import im.conversations.android.xmpp.model.carbons.Received;
 import im.conversations.android.xmpp.model.carbons.Sent;
+import im.conversations.android.xmpp.model.correction.Replace;
 import im.conversations.android.xmpp.model.forward.Forwarded;
 import im.conversations.android.xmpp.model.occupant.OccupantId;
 import im.conversations.android.xmpp.model.reactions.Reactions;
@@ -456,10 +458,11 @@ public class MessageParser extends AbstractParser implements Consumer<im.convers
         final Element mucUserElement = packet.findChild("x", Namespace.MUC_USER);
         final boolean isTypeGroupChat = packet.getType() == im.conversations.android.xmpp.model.stanza.Message.Type.GROUPCHAT;
         final String pgpEncrypted = packet.findChildContent("x", "jabber:x:encrypted");
-        final Element replaceElement = packet.findChild("replace", "urn:xmpp:message-correct:0");
+
         final Element oob = packet.findChild("x", Namespace.OOB);
         final String oobUrl = oob != null ? oob.findChildContent("url") : null;
-        final String replacementId = replaceElement == null ? null : replaceElement.getAttribute("id");
+        final var replace = packet.getExtension(Replace.class);
+        final var replacementId = replace == null ? null : replace.getId();
         final Element axolotlEncrypted = packet.findChildEnsureSingle(XmppAxolotlMessage.CONTAINERTAG, AxolotlService.PEP_PREFIX);
         int status;
         final Jid counterpart;
@@ -549,7 +552,14 @@ public class MessageParser extends AbstractParser implements Consumer<im.convers
 
 
             if (selfAddressed) {
-                if (mXmppConnectionService.markMessage(conversation, remoteMsgId, Message.STATUS_SEND_RECEIVED, serverMsgId)) {
+                // don’t store serverMsgId on reflections for edits
+                final var reflectedServerMsgId =
+                        Strings.isNullOrEmpty(replacementId) ? serverMsgId : null;
+                if (mXmppConnectionService.markMessage(
+                        conversation,
+                        remoteMsgId,
+                        Message.STATUS_SEND_RECEIVED,
+                        reflectedServerMsgId)) {
                     return;
                 }
                 status = Message.STATUS_RECEIVED;
@@ -562,7 +572,10 @@ public class MessageParser extends AbstractParser implements Consumer<im.convers
                 if (conversation.getMucOptions().isSelf(counterpart)) {
                     status = Message.STATUS_SEND_RECEIVED;
                     isCarbon = true; //not really carbon but received from another resource
-                    if (mXmppConnectionService.markMessage(conversation, remoteMsgId, status, serverMsgId, body)) {
+                    // don’t store serverMsgId on reflections for edits
+                    final var reflectedServerMsgId =
+                            Strings.isNullOrEmpty(replacementId) ? serverMsgId : null;
+                    if (mXmppConnectionService.markMessage(conversation, remoteMsgId, status, reflectedServerMsgId, body)) {
                         return;
                     } else if (remoteMsgId == null || Config.IGNORE_ID_REWRITE_IN_MUC) {
                         if (body != null) {
@@ -716,16 +729,26 @@ public class MessageParser extends AbstractParser implements Consumer<im.convers
                             final String uuid = replacedMessage.getUuid();
                             replacedMessage.setUuid(UUID.randomUUID().toString());
                             replacedMessage.setBody(message.getBody());
-                            replacedMessage.putEdited(replacedMessage.getRemoteMsgId(), replacedMessage.getServerMsgId());
-                            replacedMessage.setRemoteMsgId(remoteMsgId);
-                            if (replacedMessage.getServerMsgId() == null || message.getServerMsgId() != null) {
-                                replacedMessage.setServerMsgId(message.getServerMsgId());
-                            }
+                            // we store the IDs of the replacing message. This is essentially unused
+                            // today (only the fact that there are _some_ edits causes the edit icon
+                            // to appear)
+                            replacedMessage.putEdited(
+                                    message.getRemoteMsgId(), message.getServerMsgId());
+
+                            // we used to call
+                            // `replacedMessage.setServerMsgId(message.getServerMsgId());` so during
+                            // catchup we could start from the edit; not the original message
+                            // however this caused problems for things like reactions that refer to
+                            // the serverMsgId
+
                             replacedMessage.setEncryption(message.getEncryption());
                             if (replacedMessage.getStatus() == Message.STATUS_RECEIVED) {
                                 replacedMessage.markUnread();
                             }
-                            extractChatState(mXmppConnectionService.find(account, counterpart.asBareJid()), isTypeGroupChat, packet);
+                            extractChatState(
+                                    mXmppConnectionService.find(account, counterpart.asBareJid()),
+                                    isTypeGroupChat,
+                                    packet);
                             mXmppConnectionService.updateMessage(replacedMessage, uuid);
                             if (mXmppConnectionService.confirmMessages()
                                     && replacedMessage.getStatus() == Message.STATUS_RECEIVED
diff --git a/src/main/java/eu/siacs/conversations/ui/ConversationFragment.java b/src/main/java/eu/siacs/conversations/ui/ConversationFragment.java
index d8b8b4151cc9f19171c81f155b31f1ec24940ba5..e118c0a4302b007f31d9fca8958abf461b1e99b6 100644
--- a/src/main/java/eu/siacs/conversations/ui/ConversationFragment.java
+++ b/src/main/java/eu/siacs/conversations/ui/ConversationFragment.java
@@ -853,7 +853,7 @@ public class ConversationFragment extends XmppFragment
         final Editable text = this.binding.textinput.getText();
         final String body = text == null ? "" : text.toString();
         final Conversation conversation = this.conversation;
-        if (body.length() == 0 || conversation == null) {
+        if (body.isEmpty() || conversation == null) {
             return;
         }
         if (trustKeysIfNeeded(conversation, REQUEST_TRUST_KEYS_TEXT)) {
@@ -867,7 +867,6 @@ public class ConversationFragment extends XmppFragment
             message = conversation.getCorrectingMessage();
             message.setBody(body);
             message.putEdited(message.getUuid(), message.getServerMsgId());
-            message.setServerMsgId(null);
             message.setUuid(UUID.randomUUID().toString());
         }
         switch (conversation.getNextEncryption()) {
diff --git a/src/main/java/im/conversations/android/xmpp/model/correction/Replace.java b/src/main/java/im/conversations/android/xmpp/model/correction/Replace.java
index dbd7395572c7b614f9c7caf83996fbe94e8aca55..7c96118be656520c1bbb04d167edd366d337916b 100644
--- a/src/main/java/im/conversations/android/xmpp/model/correction/Replace.java
+++ b/src/main/java/im/conversations/android/xmpp/model/correction/Replace.java
@@ -14,6 +14,11 @@ public class Replace extends Extension {
         super(Replace.class);
     }
 
+    public Replace(final String id) {
+        this();
+        this.setId(id);
+    }
+
     public String getId() {
         return Strings.emptyToNull(this.getAttribute("id"));
     }