Consider enforcing 'ement--original-event-for' usage at lower levels for editing and replying

[phil-s]

The recent issues #226, #227, #228 addressed some bugs and incompatibilities with Element wrt edited messages, and introduced ement--original-event-for for obtaining the ID of the 'original event' when it was necessary to provide that instead of the ID of an edited event.

At present ement--original-event-for is being used in relatively high-level code/commands (currently ement-room-edit-message, ement-room-write-reply, and ement-room-delete-message), and I'm inclined to suggest that it should be moved to lower-level functions in order to enforce its usage for the editing and replying cases.

This is partly because I'm currently working on some changes for #140 which introduce additional commands that also need to explicitly obtain the original event, and I feel that it would be good if these changes and any future code -- whether in ement itself or custom code written by users -- were protected from needing to worry (or even know) about these nuances. My motivation for suggesting the changes is to prevent potential future bugs. If the conclusion is that these events should always use the 'original event', then we eliminate this class of bug by making it impossible for Ement.el to send an event with 'invalid' m.relates_to data.

n.b. For the "deletion" case I believe the current code is good as-is. https://spec.matrix.org/v1.8/client-server-api/#redactions-of-edited-events makes it clear that both original and edit events may be targeted for redaction (and describes the expected effect in each case), so the low-level ement-redact shouldn't care either way. The command ement-room-delete-message was changed to target the original event in #228 in order to align with Element's behaviour, and I don't think we should do anything beyond that.

For "editing", I feel it's pretty clear-cut that we should enforce the use of the original event ID at a lower level. https://spec.matrix.org/v1.8/client-server-api/#validity-of-replacement-events states "you cannot edit an edit — though you can send multiple edits for a single original event" and so I think that ement-room-edit-message should move the use of ement--original-event-for into its function body, rather than having it in its interactive form, as the current code still enables ement-room-edit-message to act upon edited events (i.e. if it is called in lisp code with an event argument which is not an 'original event'). Leaving it possible to edit other events seems like leaving a door open for future bugs.

For "replying" it's a bit ambiguous, but I'm proposing that we also enforce the use of the original event ID in this case as well, to avoid conflicts with Element. In #227 we observed that Element doesn't display replies to edited messages correctly unless the m.relates_to data of the reply specifies the ID of the original event.

I can't find anything in the spec mandating that this should be the case. https://spec.matrix.org/v1.8/client-server-api/#forming-relationships-between-events says "A child event can point at any other event, including another child event, to build the relationship so long as both events are in the same room, however additional restrictions might be imposed by the type of the relationship (the rel_type)", but I didn't find anything besides the (rel_type . "m.replace") case (i.e. editing) where such a restriction was stated; so in theory the spec does seem to allow for replies to edits.

On the other hand, it certainly doesn't say that we shouldn't reply to the original event, and moreover Element apparently requires us to do this, so in practice it seems to me like a safe bet to follow that behaviour in the absence of any clarification, and that it should be consistent (i.e. enforced).

At present we pass around :replying-to-event keyword arguments, but these arguments ultimately wind up in ement-send-message and finally in ement--add-reply, so I think that ement--original-event-for could be applied in one of those two places in order to enforce this behaviour for all reply events that Ement.el sends, and then no higher-level code needs to worry about what it's passing as :replying-to-event.

[phil-s]

Suggested diff for editing:

diff --git a/ement-room.el b/ement-room.el
index 39876fc..d046a61 100644
--- a/ement-room.el
+++ b/ement-room.el
@@ -1732,11 +1732,9 @@ ement-room-edit-message
 itself an edit of another event, the original event is edited."
   (interactive (ement-room-with-highlighted-event-at (point)
                  (cl-assert ement-session) (cl-assert ement-room)
-                 (pcase-let* ((event (ewoc-data (ewoc-locate ement-ewoc)))
-                              ((cl-struct ement-session user) ement-session)
-                              ((cl-struct ement-event sender (content (map body))) event)
-                              (ement-room-editing-event event)
-                              (edited-event (ement--original-event-for event ement-session)))
+                 (pcase-let* ((ement-room-editing-event (ewoc-data (ewoc-locate ement-ewoc)))
+                              ((cl-struct ement-event sender (content (map body))) ement-room-editing-event)
+                              ((cl-struct ement-session user) ement-session))
                    (unless (equal (ement-user-id sender) (ement-user-id user))
                      (user-error "You may only edit your own messages"))
                    ;; Remove any leading asterisk from the plain-text body.
@@ -1749,19 +1747,20 @@ ement-room-edit-message
                        (when (string-empty-p body)
                          (user-error "To delete a message, use command `ement-room-delete-message'"))
                        (when (yes-or-no-p (format "Edit message to: %S? " body))
-                         (list edited-event ement-room ement-session body)))))))
+                         (list ement-room-editing-event ement-room ement-session body)))))))
   (let* ((endpoint (format "rooms/%s/send/%s/%s" (url-hexify-string (ement-room-id room))
                            "m.room.message" (ement--update-transaction-id session)))
          (new-content (ement-alist "body" body
                                    "msgtype" "m.text"))
          (_ (when ement-room-send-message-filter
               (setf new-content (funcall ement-room-send-message-filter new-content room))))
+         (original-event (ement--original-event-for event session))
          (content (ement-alist "msgtype" "m.text"
                                "body" body
                                "m.new_content" new-content
                                "m.relates_to" (ement-alist
                                                "rel_type" "m.replace"
-                                               "event_id" (ement-event-id event)))))
+                                               "event_id" (ement-event-id original-event)))))
     ;; Prepend the asterisk after the filter may have modified the content.  Note that the
     ;; "m.new_content" body does not get the leading asterisk, only the "content" body,
     ;; which is intended as a fallback.

[phil-s]

Suggested diff for replying:

diff --git a/ement-lib.el b/ement-lib.el
index 9682aa8..95da5bb 100644
--- a/ement-lib.el
+++ b/ement-lib.el
@@ -1142,7 +1142,8 @@ ement-send-message
     (when filter
       (setf content (funcall filter content room)))
     (when replying-to-event
-      (setf content (ement--add-reply content replying-to-event room)))
+      (setf replying-to-event (ement--original-event-for replying-to-event session)
+            content (ement--add-reply content replying-to-event room)))
     (ement-api session endpoint :method 'put :data (json-encode content)
       :then (apply-partially then :room room :session session
                              ;; Data is added when calling back.
diff --git a/ement-room.el b/ement-room.el
index 143c0dc..fb8dcc0 100644
--- a/ement-room.el
+++ b/ement-room.el
@@ -1770,9 +1770,8 @@ ement-room-write-reply
                       (setq-local ement-room-replying-to-event event)))
                    (body (ement-room-with-typing
                            (ement-room-read-string prompt nil 'ement-room-message-history
-                                                   nil 'inherit-input-method)))
-                   (replying-to-event (ement--original-event-for event ement-session)))
-        (ement-room-send-message room session :body body :replying-to-event replying-to-event)))))
+                                                   nil 'inherit-input-method))))
+        (ement-room-send-message room session :body body :replying-to-event event)))))
 
 (defun ement-room-send-reaction (key position)
   "Send reaction of KEY to event at POSITION.

[alphapapa]

Well, you argued your case very well and I think you've convinced me. I'd like to study the diffs a bit more carefully before applying them, but it's hard to disagree with you. :)

[phil-s]

The following from ement-room-compose-send seems to represent potentially still using an edited event ID in the current v0.13:

ement.el/ement-room.el

Lines 3786 to 3788 in 26be559

	(if editing-event
	(ement-room-edit-message editing-event ement-room ement-session body)
	(ement-room-send-message ement-room ement-session :body body :replying-to-event replying-to-event)))))

The code flow from minibuffer to compose buffer and back to minibuffer was tricky to follow, so I just tested instead, grabbing the event data at each step:

1. write test message
2. edit message in minibuffer -- m.relates_to (1)
3. reply to edited message in minibuffer -- m.relates_to (1)
4. reply to edited message via compose buffer -- m.relates_to (2)
5. re-edit message via compose buffer -- m.relates_to (2)
6. reply to composed edit in minibuffer -- m.relates_to (2)
7. reply to composed edit via compose buffer -- m.relates_to (5)

(I believe these cases would be resolved with the suggested changes.)