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

MSC2881: Message Attachments #2881

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

MSC2881: Message Attachments #2881

wants to merge 40 commits into from

Conversation

MurzNN
Copy link
Contributor

@MurzNN MurzNN commented Nov 27, 2020

@MurzNN MurzNN changed the title Added proposial "Message Attachments" Added proposal "Message Attachments" Nov 27, 2020
@MurzNN MurzNN changed the title Added proposal "Message Attachments" MSC2881: Message Attachments Nov 27, 2020
@turt2live turt2live added proposal-in-review proposal A matrix spec change proposal kind:feature MSC for not-core and not-maintenance stuff labels Nov 27, 2020
proposals/2881-message-attachments.md Outdated Show resolved Hide resolved
proposals/2881-message-attachments.md Outdated Show resolved Hide resolved
proposals/2881-message-attachments.md Show resolved Hide resolved
@MurzNN
Copy link
Contributor Author

MurzNN commented Feb 26, 2021

The Hummingbard project seems use "Option two" implementation from this MSC for attaching multiple files/images/links to event! 🎉

proposals/2881-message-attachments.md Outdated Show resolved Hide resolved
proposals/2881-message-attachments.md Outdated Show resolved Hide resolved

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.

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.

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.

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.

proposals/2881-message-attachments.md Outdated Show resolved Hide resolved

## 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.

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.

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.

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

MurzNN and others added 3 commits April 12, 2021 18:08
Co-authored-by: Kevin Cox <kevincox@kevincox.ca>
Co-authored-by: Kevin Cox <kevincox@kevincox.ca>
Co-authored-by: Kevin Cox <kevincox@kevincox.ca>
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@chayleaf
Copy link

chayleaf commented Aug 25, 2021

Currently Synapse doesn't allow m.relates_to to be an array, so the first option isn't doable as-is, but it's possible to implement with a limited attachment count (1). That also won't allow to reply with images.

@MurzNN
Copy link
Contributor Author

MurzNN commented Aug 26, 2021

Currently Synapse doesn't allow m.relates_to to be an array, so the first option isn't doable as-is, but it's possible to implement with a limited attachment count (1). That also won't allow to reply with images.

@pavlukivan Thanks for identifying the problem, m.relates_to is object, not array, and now already late to change it. What do you think about using the separate key in object for attachments? Something like this:

{
  "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 attachments to upper level, but, as I guess, Synapse must implement some optimizations to deliver events, mentioned in m.relates_to together with main event (or at least preload them for quicker access).

@chayleaf
Copy link

chayleaf commented Aug 26, 2021

What do you think about using the separate key in object for attachments?

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.

@MurzNN
Copy link
Contributor Author

MurzNN commented Aug 26, 2021

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)

@deepbluev7
Copy link
Contributor

Thanks for identifying the problem, m.relates_to is object, not array, and now already late to change it.

Not really. #2674 hasn't landed yet and one could use #3051 instead. But so far there seem to be not enough use cases to allow multiple relations.

@chayleaf
Copy link

chayleaf commented Aug 26, 2021

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.

@kevincox
Copy link

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.

@chayleaf
Copy link

chayleaf commented Aug 28, 2021

But so far there seem to be not enough use cases to allow multiple relations.

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

@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.

@richvdh
Copy link
Member

richvdh commented Nov 23, 2021

suspect this is the problem:

image

see #3367

@MurzNN MurzNN changed the base branch from old_master to main November 23, 2021 11:54
@MurzNN
Copy link
Contributor Author

MurzNN commented Nov 23, 2021

@richvdh Thanks for the tip, I've switched to main branch, seems now all is well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet