Skip to content

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

@phil-s

Description

@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.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions