Matthew/msc1849 rewrite #2154
Matthew/msc1849 rewrite #2154
Conversation
| "msgtype": "m.text", | ||
| }, | ||
| "m.relates_to": { | ||
| "rel_type": "m.replace", |
There was a problem hiding this comment.
I think we might need a way to preserve the rel_type upon redacting an edit. As it stands we can't tell a redacted edit from a redacted message, causing riot to render redacted edits in the timeline. We could keep m.relates_to/rel_type in the content, like we do for other fields.
proposals/1849-aggregations.md
Outdated
|
|
||
| Bundles of relations for a given event are | ||
| paginated to prevent overloading the client with relations, and may be traversed by | ||
| via the new `/relations` API (which iterates over all relations for an event) or the |
There was a problem hiding this comment.
Heads up that we'd like the original event (not just the id) to be returned in calls to /relations to make showing message edit history simpler.
Context: matrix-org/synapse#5595
There was a problem hiding this comment.
@anoadragon453 i'm a bit dubious about the implementation here. the requirement to show the original message is specific to edits - not relations in general. for instance, if i iterate over the reactions for a message, i don't want to waste bandwidth getting sent the original message again.
Was there a reason we don't just call /event to grab the original event as a separate API call to iterating over the actual relations via /relations?
Or alternatively, make the field only show up if you're explicitly paginating m.replace relations?
| @@ -1,241 +1,245 @@ | |||
| # Proposal for aggregations via m.relates_to | |||
There was a problem hiding this comment.
Should mention that redacting an original event will redact all relations.
Should this be done by sending hundreds of reactions event into a room (one for each reaction/edit)? Or would it be better to be able to specify something that redacts multiple events.
There was a problem hiding this comment.
have tried to clear this up by explaining you don't have to redact the relations.
proposals/1849-aggregations.md
Outdated
| * `m.annotation` aggregations provide the `type` of the relation event, the | ||
| aggregation `key`, and the `count` of the number of annotations of that | ||
| `type` and `key` which reference that event. | ||
| * `m.replace` relations do not appear in bundled aggregations at all, as they |
There was a problem hiding this comment.
They do appear in the bundle as it's implemented currently in synapse, and that is needed for clients to know that the event is edited, at what time, and by who, and to continue aggregating m.replace events locally on top of it.
Ftr, the aggregated data looks like this:
"m.replace": {
"event_id": "$PHdn4WW8aOhdEPfAfZWmwYJSodFSJrDzojabJ0u1iao",
"origin_server_ts": 1562763768320,
"sender": "@bruno1:localhost"
}
There was a problem hiding this comment.
or not... i've added XXXs to try to explain my confusion.
proposals/1849-aggregations.md
Outdated
|
|
||
| ### Relation types | ||
|
|
||
| This proposal defines three `rel_type`s, each which provide different behaviour |
There was a problem hiding this comment.
| This proposal defines three `rel_type`s, each which provide different behaviour | |
| This proposal defines three `rel_type`s, each of which provide different behaviour |
proposals/1849-aggregations.md
Outdated
| original event to clients. Another usage of an annotation is e.g. for bots, who | ||
| could use annotations to report the success/failure or progress of a command. | ||
| ```json | ||
| "type": "m.room.message", |
There was a problem hiding this comment.
Add an outer level of braces to match the other examples here.
|
element-hq/element-web#10136 has some discussion on how reactions should interact with edited messages. The consensus there seems that reactions should be sent to the last edit (if any). Right now clients always send a reaction to the original event. We should probably spec this behaviour, as diverging clients behaviour will end up not showing each others reactions. We should potentially also enforce this server-side (reject reactions for edited messages on anything but the last edit). |
| "msgtype": "m.text", | ||
| "m.new_content": { | ||
| "body": "Hello! My name is bar", | ||
| "msgtype": "m.text", |
There was a problem hiding this comment.
The proposal doesn't seem to mention reply fallbacks at all, even though they're a problem. Riot was changed to add the fallbacks to m.new_content in matrix-org/matrix-react-sdk#3192, but edit events don't contain any metadata indicating that the reply fallback exists.
While the HTML <mx-reply> tag can technically be parsed away safely even without knowing the event has a reply fallback, the same does not apply for the plaintext body. Therefore, it's impossible to use the actual plaintext of an edit without fetching the original event and checking if it was a reply, which is a huge waste of time.
It might not be a problem in clients who probably have the original event stored somewhere, but it is in bots and bridges where the original event is really not needed for anything. It's especially a problem in bots since they don't deal with formatted_body at all in most cases.
IMO we should be phasing out reply fallbacks by not including them in m.new_content, because clients that understand edits are new enough to understand replies (and if they don't understand replies, that's completely the client dev's fault).
|
i'm going to merge this into the 1849 branch to avoid further comment fragmentation and confusion. i think the only orphaned comment this leaves here is #2154 (comment)... which has made its way into the XXXs on the MSC anyway. |
|
superceded by #1849 |
WIP of rewriting MSC1849