Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
MSC2881: Message Attachments #2881
Changes from 35 commits
e0e7133
96fef8b
8065221
0de8e47
45c91fd
a226142
616dcba
6b5ccbe
e343893
d448559
c33ef1a
ad7f2fa
dd423aa
76e0522
9a7f694
910be82
abc7edf
108ffa7
2be176c
eea1ba0
87bfff2
ba19693
6147bd6
6da8d3a
fa6f59b
2e8536e
0eb7773
00ade35
5df15b6
a470fba
39c08c4
12c0a3f
21f14c2
cfb8f55
55db2fe
9ebce4a
633160a
d77ba1e
ae52584
7d4542f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
Cons:
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.
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:
m.relates_to
array.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.
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.
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:
Cons:
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
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).