-
Notifications
You must be signed in to change notification settings - Fork 383
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
MSC2881: Message Attachments #2881
base: main
Are you sure you want to change the base?
Conversation
The Hummingbard project seems use "Option two" implementation from this MSC for attaching multiple files/images/links to event! 🎉 |
|
||
When user press "Send" button, Matrix client do the upload of all media, that user attached to message, as separate events to room (how it is done now), before sending message with typed text. And after sending of all attachments is finished, client send message with aggregating event, using `m.relates_to` field (from the [MSC2674: Event relationships](https://github.com/matrix-org/matrix-doc/pull/2674)), that points to all previously sent events with media, to group them into one gallery. | ||
|
||
For exclude showing those events in modern clients before grouping event added, I propose extend separate media events via adding "marker" field `is_attachment: true`, if clients got this value - they must exclude showing this media in timeline, and shows them only in gallery with grouping event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if such an event is never referenced? Does it just stay hidden forever? This is probably the best option but it is worth discussing.
Pros:
- Allows uploading the media as the user composes so that sending is quicker.
- Messages don't suddenly appear (for example) a day later higher up in the timeline.
- If the sender crashes or fails it can just restart. It isn't required to recollect the event IDs and reuse them. (Although reuse would be more efficient)
Cons:
- Invisible events may be an abuse vector. (DoS or covert channel)
- Upon a failed send or deleted message (if the client eagerly uploads media) the user likely doesn't realize that the media was uploaded and available in the room history. This may be a concern if the user then realized that they didn't want to send that info. (Maybe they tried to cancel the send because it was in the wrong room and would have redacted the media if they realized it was actually sent).
So basically it is nice because it emulates an atomic commit. But it is not nice because it does expose some side effects even if not committed.
|
||
### Display recommendations: | ||
|
||
On the client site, attachments can be displayed as grid of clickable thumbnails, like the current `m.image` events, but with a smaller size, having fixed height, like a regular image gallery. On click, Matrix client must display media in full size, and, if possible, as a gallery with "next-previous" buttons. Also clients can implement collapse/expand action on gallery grid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a large number of attachments (for example "look at thses photos from my two week vacation!") many clients won't want to show all of the images upfront. Most would probably render some sort of grid with the first couple then a "37 more" link.
In order to help with cross-client consistency should we specify something about the order and how that should be handled? You do mention "rearranging" attachments earlier.
For example:
- Clients SHOULD show the attachments in the order the appear in the
m.relates_to
array. - If the client decides to show only a subset of photos upfront it SHOULD prefer to show the ones earliest in the
m.relates_to
array.
I think it should be non-normative, but basically if you are going to only show some show the first ones rather than the last ones.
|
||
### Display recommendations: | ||
|
||
On the client site, attachments can be displayed as grid of clickable thumbnails, like the current `m.image` events, but with a smaller size, having fixed height, like a regular image gallery. On click, Matrix client must display media in full size, and, if possible, as a gallery with "next-previous" buttons. Also clients can implement collapse/expand action on gallery grid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to specify "must" here. For example if the image is bigger than the users display I think most users would rather have it scaled to fit rather than needing to scroll around. I would leave this section as an example of a possible implementation.
|
||
## Potential issues | ||
|
||
1. On bad connection to server Matrix client can send attachments as events with `"is_attachment": true` but not send final `m.message` event, this will lead to posting invisible media to room. This can be solved on client side via caching unsent group of events, and repeat sending when connection will be recovered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if such an event is never referenced? Does it just stay hidden forever? This is probably the best option but it is worth discussing.
Pros:
- Allows uploading the media as the user composes so that sending is quicker.
- Messages don't suddenly appear (for example) a day later higher up in the timeline.
- If the sender crashes or fails it can just restart. It isn't required to recollect the event IDs and reuse them. (Although reuse would be more efficient)
Cons:
- Invisible events may be an abuse vector. (DoS or covert channel)
- Upon a failed send or deleted message (if the client eagerly uploads media) the user likely doesn't realize that the media was uploaded and available in the room history. This may be a concern if the user then realizes that they didn't want to send that info. (Maybe they tried to cancel the send because it was in the wrong room and would have redacted the media if they realized it was actually sent).
So basically it is nice because it emulates an atomic commit. But it is not nice because it does expose some side effects even if not committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is already many ways to send an invisible event, that by itself shouldn't be considered an issue. The other points are still valid though
|
||
1. On bad connection to server Matrix client can send attachments as events with `"is_attachment": true` but not send final `m.message` event, this will lead to posting invisible media to room. This can be solved on client side via caching unsent group of events, and repeat sending when connection will be recovered. | ||
|
||
2. In option one - individual media event, to which `m.message` refers, can be deleted (redacted) after. As result, `m.message` will contain relation to redacted event. In this situation Matrix clients can exclude this item from display. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be put into the MSC proper. I think it makes sense that redacting an attachment implies that the attachment is removed from the messages that include it. However I would leave it open to clients showing a "redacted" placeholder or just hiding it outright. (For example if an attachment was referenced from multiple events it may be helpful to see which events now have missing attachments).
Co-authored-by: Kevin Cox <[email protected]>
Co-authored-by: Kevin Cox <[email protected]>
Co-authored-by: Kevin Cox <[email protected]>
Currently Synapse doesn't allow |
@pavlukivan Thanks for identifying the problem, {
"type": "m.room.message",
"content": {
"msgtype": "m.text",
"body": "Here is my photos and videos from yesterday event",
"m.relates_to": [
{
"m.in_reply_to": {
"event_id": "$id_of_replied_message"
},
"attachments": {
"event_ids": [
"$id_of_previosly_send_media_event_1",
"$id_of_previosly_send_media_event_2",
"$id_of_previosly_send_media_event_3",
]
}
},
{
"rel_type": "m.attachment",
"event_id": "$id_of_previosly_send_media_event_2"
}
]
}
} Or even we can move |
While it seems like a hack, I guess it will offer the best compatibility (old clients won't be required to handle array relations). Of course I'd prefer the option where relations become an array (or similar), but it would require client support to understand the new format at all. |
I suppose that implementing multiple attachments without array as storage is impossible, and implementing with limitation of only one attachment is useless! Thus we still need to insert the array somewhere :) And the best way, I think, is to implement some type of arrays in MSC1767: Extensible events in Matrix (and follow it in current MSC) - here is my comment about this #1767 (comment) |
I think using an array still makes sense, considering it adds very little overhead but adds use cases like this. The only downside I see is it being harder to store relations in the database, as one-to-many is considerably easier to store. |
This seems like a clear answer of "we will need it". It is easy to imagine and we have a real-world use case here. |
Off the top of my head: replying to multiple message at the same time, or starting a thread from multiple messages could be done with that |
Option two is moved to matrix-org#3382 Also added link to matrix-org#3051 with array implementation of relations.
@MurzNN I'm not sure what happened here, but the diff is showing quite a few changes. I would recommend either fixing the branch, or, if preferred, closing this PR and opening a new MSC off a dedicated branch. |
suspect this is the problem: see #3367 |
@richvdh Thanks for the tip, I've switched to |
Rendered
Depends on MSC3051: A scalable relation format
Related issues:
Also lookup alternative and more optimal implementation: #3382
Signed-off-by: Alexey Korepov [email protected]