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

Matthew/msc1849 rewrite #2154

Draft
wants to merge 4 commits into
base: matthew/msc1849
from

Conversation

Projects
None yet
4 participants
@ara4n
Copy link
Member

commented Jul 5, 2019

WIP of rewriting MSC1849

"msgtype": "m.text",
},
"m.relates_to": {
"rel_type": "m.replace",

This comment has been minimized.

Copy link
@bwindels

bwindels Jul 5, 2019

Contributor

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.


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

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jul 8, 2019

Member

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

@@ -1,241 +1,245 @@
# Proposal for aggregations via m.relates_to

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jul 8, 2019

Member

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.

* `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

This comment has been minimized.

Copy link
@bwindels

bwindels Jul 10, 2019

Contributor

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 date looks like this:

      "m.replace": {
        "event_id": "$PHdn4WW8aOhdEPfAfZWmwYJSodFSJrDzojabJ0u1iao",
        "origin_server_ts": 1562763768320,
        "sender": "@bruno1:localhost"
      }

### Relation types

This proposal defines three `rel_type`s, each which provide different behaviour

This comment has been minimized.

Copy link
@jryans

jryans Jul 10, 2019

Member
Suggested change
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
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",

This comment has been minimized.

Copy link
@jryans

jryans Jul 10, 2019

Member

Add an outer level of braces to match the other examples here.


* `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.

This comment has been minimized.

Copy link
@jryans

jryans Jul 10, 2019

Member

We need some mechanism for getting the annotation senders so they they can be shown on hover. Is the intention that we should make an additional call to paginate all reactions to retrieve this?

See also same comment on the prior version.

@bwindels

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

vector-im/riot-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).

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.