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

Closed

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Oct 12, 2018

@Half-Shot Half-Shot changed the title MSC1695 Message Edits [Placeholder] MSC1695 Message Edits 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 turt2live added proposal A matrix spec change proposal T-Core client-server Client-Server API labels Oct 12, 2018
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

proposals/1695-message-edits.md Show resolved Hide resolved
proposals/1695-message-edits.md Show resolved Hide resolved
proposals/1695-message-edits.md Show resolved Hide resolved
@NotAFile
Copy link

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
Copy link
Contributor Author

Half-Shot 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
Copy link
Contributor

Cadair 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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

Half-Shot 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.

"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": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me.

Copy link
Contributor

@non-Jedi non-Jedi Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 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.).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

non-Jedi 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
Copy link

dkanada 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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

dkanada 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
Copy link
Contributor Author

@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
Copy link

dkanada 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
Copy link
Contributor

MurzNN 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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

dali99 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
Copy link
Contributor Author

@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
Copy link

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

@anoadragon453
Copy link
Member

anoadragon453 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?

@Half-Shot Half-Shot closed this Jul 23, 2019
@turt2live turt2live added abandoned A proposal where the author/shepherd is not responsive and removed proposal-in-review labels Jul 23, 2019
@Half-Shot
Copy link
Contributor Author

This is obsoleted by #1849

@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed abandoned A proposal where the author/shepherd is not responsive labels Jul 23, 2019
@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:core MSC which is critical to the protocol's success obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal proposal-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.