Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC1695 Message Edits #1695

Open
wants to merge 13 commits into
base: master
from

Conversation

@Half-Shot
Copy link
Contributor

commented Oct 12, 2018

Rendered

Addresses #682

Half-Shot added some commits Oct 12, 2018

@Half-Shot Half-Shot changed the title MSC1695 Message Edits [Placeholder] MSC1695 Message Edits Oct 12, 2018

Half-Shot added some commits Oct 12, 2018


If the edit event's content is invalid, it is acceptable to display/keep the old event in place with a warning.

User should be warned that editing an an

This comment has been minimized.

Copy link
@ara4n

ara4n Oct 12, 2018

Member

an an?! :D

Clients will also render the original event without the edit if the client isn't aware of
the edited event since event aggregations aren't a thing yet. This is considered an
acceptable risk for this proposal and aggregations are considered an extension to
message edits for Matrix.

This comment has been minimized.

Copy link
@ara4n

ara4n Oct 12, 2018

Member

Probably worth spelling out here too that the intention here is more to standardise metadata than provide a good UX in clients pre-aggregations.

@turt2live
Copy link
Member

left a comment

Generally looks good from my point of view. Minor comments included about very minor things.

Show resolved Hide resolved proposals/1695-message-edits.md
Show resolved Hide resolved proposals/1695-message-edits.md
Show resolved Hide resolved proposals/1695-message-edits.md

Half-Shot added some commits Oct 12, 2018

@NotAFile

This comment has been minimized.

Copy link

commented Oct 12, 2018

One option I initially mentioned was to annotate the original message with a link to a new message Otherwise things like editing pinned messages would behave very weird indeed. I'm not sure this can be done without destroying backwards compatibility by invalidating the original event, though.

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2018

One option I initially mentioned was to annotate the original message with a link to a new message

So this is something an implementation could provide in unsigned if they wanted to, since we don't encrypt the m.relates_to metadata. You would need to maintain a simple table with links.

This would let you annotate without destroying the actual event content, and would just be a hint for clients which is probably the best way to go about it.

@Cadair

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

It's worth noting that the gitter bridge doesn't render the whole old / new message. It only shows a small context around the change. I am a little sceptical about being prescriptive about the fallback in the spec.

That being said, I do very much prefer this approach of putting the actual body in m.relates_to.

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Looks overall good to me. I'm assuming clients could take the leniency to replace an old message with the content of the new one if they want to. Definitely want this specced so that all bots standardize around it.

So 👍 from me.

@erikjohnston

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Michael raised an interesting question on how this works with notifications? It'd be annoying if an edited message re-bings people each time its edited.

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Michael raised an interesting question on how this works with notifications? It'd be annoying if an edited message re-bings people each time its edited.

True, but the only resolution I can see is the homeserver keeping track of which event's it's notified on and disqualifies edits on event's it has already sent.

Which is quite nasty to write and involves mucking about with event contents.


On the flipside, people already redact and resend events which would result in the same behaviour so I don't think this makes the situation any worse.

Half-Shot added some commits Oct 17, 2018

Clients must refuse to display edits by other people
Clients must show that an event has been edited.
@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

In the last commit I've added some rules:

  • Clients must show edits with some sort of marker
  • Clients must only show edits from the source sender.
"body": "Edited: ~~This is an example message I want to edit~~ This is the edited message",
"format": "org.matrix.custom.html",
"formatted_body": "Edited: <del>This is an example message I want to edit</del> This is the edited message",
"m.new_content": {

This comment has been minimized.

Copy link
@non-Jedi

non-Jedi Oct 17, 2018

Contributor

Can we propagate this field to replies as a quick fix per the discussion in #matrix-architecture yesterday? The name actually works generically between the two though it could be better. Basically, it would be great if all m.relates_to events could have a field with non-fallback text in the same place.

This comment has been minimized.

Copy link
@turt2live

turt2live Oct 17, 2018

Member

I'm tempted to agree, and if Half-Shot is okay with the tiny bit of scope creep it should be fine to put it in this proposal too.

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Oct 17, 2018

Author Contributor

Fine by me.

This comment has been minimized.

Copy link
@non-Jedi

non-Jedi Oct 17, 2018

Contributor

😄 In that case, proposal should be explicit that m.new_content is a field containing an object that corresponds to the non-fallback representation of content if meaningful for a specific m.relates_to duck-type.

In only slightly related matters, is there a convention about when to prefix a key with m and when not to? From here it appears fairly random (why m.new_content and not new_content, m.relates_to and not relates_to, etc.).

This comment has been minimized.

Copy link
@turt2live

turt2live Oct 17, 2018

Member

Things that could cause a namespace conflict generally get an m. prefix when they are in the spec. Therefore, because new_content is a legitimate key that other events might use it gets an m. prefix.

@non-Jedi

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

One thing I thought of this morning. This whole setup feels very non-orthogonal with m.room.redaction. Especially the stuff with m.replaces and the reason. Might it not be better to generalize redaction events to include edits rather than to generalize m.room.message even further?

In general, the principal we should be moving towards I think is for the type of a message to tell the client as much about how to handle the message as possible. From that perspective, this seems like a step backwards. Maybe the generalized redaction option is, too.

EDIT: Along these lines, something this proposal elides over a bit is that this requires even more client-side validation of events, which is becoming a greater and greater burden. If edits had some distinct event-type, they could plug into all of the current matrix power-level architecture for server-side validation of whether a user has permission to make the given edit.

@dkanada

This comment has been minimized.

Copy link

commented Oct 20, 2018

I am not extremely familiar with the spec but I agree with @non-Jedi, server-side validation sounds better. Lots of forums allow mods to edit comments rather than remove them if they just want to make a slight change rather than erase a message completely, and this behavior seems to be similar to how redactions can be used in Matrix. To be fair, messages on forums are often visible much longer than messages in a chatroom, so mod edits can make more sense in that situation.

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

One thing I thought of this morning. This whole setup feels very non-orthogonal with m.room.redaction. Especially the stuff with m.replaces and the reason. Might it not be better to generalize redaction events to include edits rather than to generalize m.room.message even further?

One of the points I want to make with this proposal is it's definitely not redaction. Redaction (for me) is removing content and making it a priority not to show it to the user. Edits are supplementary to fix or improve a statement but are not intended to delete sensitive content or replace large bodies of text. The hope is eventually homeservers will be able to aggregate this so you can rely on edits always applying but:

I would really rather not risk this proposal ending up in the bikeshed of "we need to propose a whole API around edits that works over federation" because it's going to end up being far more work than it needs to be.

So my stance is redactions are "oh shit that was wrong, tell everyone to remove this message" and edits are "I slipped up there, I'll rephrase/add content". Different purposes, different mechanisms.

In general, the principal we should be moving towards I think is for the type of a message to tell the client as much about how to handle the message as possible

Do you think this proposal isn't telling the client enough about how to handle it as is? I think I covered that area farily well. The spec (and type/keys) is really what should be telling client's how to handle messages rather than a dedicated redaction system.

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

EDIT: Along these lines, something this proposal elides over a bit is that this requires even more client-side validation of events, which is becoming a greater and greater burden. If edits had some distinct event-type, they could plug into all of the current matrix power-level architecture for server-side validation of whether a user has permission to make the given edit.

While I've not stated or recommended it, there isn't anything stopping homeservers from enforcing the rules themselves but it does reduce a lot of complication for them. If it's a popular idea, I'd be happy to do it but right now I don't think the burden is that huge.

I do admit I'm somewhat keen on edit-less rooms which would be more achievable in PLs.

In terms of editing another's messages, I'm quite against it due to misuse. While it can be well-intentioned, I've never found a good reasoning for why the user can't actually be asked to edit their own message. For moderation, I think it's just too open to abuse. Certain individuals in the matrix community already are redact heavy and I don't wish to give those people the power to modify what I am actually saying.

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

Presumably with clients showing edit history we could link the userID of who edited the message to each edit, such that if your message was nefariously edited, people could just click the "edited" button and see that it was in fact done by someone else (perhaps a small warning could be shown when one of the edits was not by the original author).

I only stand for this point because I have made bots in the past that would greatly benefit from the ability to edit other's messages, such as one which translates messages from one language to another, and those where a user only has to write :something: and it would be automatically transformed into Something Else.

I get the concern, but don't like the fact that we're removing global functionality at the cost of a situation that could be fixed in client UI.

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

I thought about this some more and felt like there are good reasons for having it at the protocol level (e.g not all edits are malicious, lots of bridged networks might need it). I think clients should be responsible to state clearly who edited the message.

There should probably be some rules about who's edits should show up.

@dkanada

This comment has been minimized.

Copy link

commented Oct 25, 2018

@Half-Shot can you clarify what you mean by whose edits should show up? Do you mean that certain edits will not show in clients or something else?

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

@Half-Shot can you clarify what you mean by whose edits should show up? Do you mean that certain edits will not show in clients or something else?

Yes I was rubbish at explaining which area I was talking about: Showing other peoples edits on your messages.

@dkanada

This comment has been minimized.

Copy link

commented Oct 25, 2018

The easiest way to highlight edits is an edited keyword when you edit your own messages and edited by user or edited by user, user, etc when someone else edits your message, although an icon would look nicer. As far as rules go, I think the same behavior as redactions is fine, which I believe is that you can only redact messages for users at or below your power level. Power tripping users trying to modify messages would be harder with edits than with redactions in any case since there would be a clear indication that the message was modified.

@MurzNN

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

As far as rules go, I think the same behavior as redactions is fine, which I believe is that you can only redact messages for users at or below your power level. Power tripping users trying to modify messages would be harder with edits than with redactions in any case since there would be a clear indication that the message was modified.

I think that we can merge Redact and Edit events in one rule, because there are very similar actions, and I can't up with situation where they must be splitted. Redaction can be implemented as Editing to empty text :) The difference is only that user will can see history, and that's all.

rather than using the fallback as the fallback can be faked.

Clients MUST refuse to display with an appropriate message when the sender of the source event
and the edit event differ. Edits made to messages can only be performed by the original sender.

This comment has been minimized.

Copy link
@dali99

dali99 Dec 3, 2018

I would love to be able to edit other users messages for bots. Like the github bot could edit all messages containing #XXXX replacing #XXXX with a hyperlink to the issue rather than bogging the room history down with a message link.

Would also be useful for https://github.com/dali99/matrix-wug as a way to use it as more of an input method rather than a dictionary

@dali99

This comment has been minimized.

Copy link

commented Dec 3, 2018

Also since the proposal actually does m.replaces.new_content and m.relates_to and even adding the key "reason": "m.edited"

couldn't we likewise make redactions as "reason": "m.redacted" since redactions already overlap quite a bit with relates to by having the "redacts": "$something:example.com" key

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

@dali99 @MurzNN @dkanada Thanks, I think there does appear to be a huge overlap here with redactions and I think I'll take a look at this again and see if we can try and merge the two somewhat.

@NoraCodes

This comment has been minimized.

Copy link

commented Apr 23, 2019

Any update on this? Having editing is a blocker to using Matrix for people I know

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@NoraCodes Yes indeed! It is currently very high priority on the roadmap and is being worked on currently along with message reactions (which alongside edits is powered by the same underlying technology).

Edit: Well actually what's being worked on now is message editing for everything whereas this proposal was just for bots/bridges. Not sure if this will be obsoleted by aggregations @Half-Shot?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.