refactor correction (message edit) code

Daniel Gultsch created

Change summary

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

Detailed changes

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;
 

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;

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) {

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;
     }

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

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()) {