Add basic support for attachments (as per MSC2881) #6683
Conversation
Signed-off-by: Ivan Pavluk <chayleaf@protonmail.com>
github-actions
bot
added
the
T-Enhancement
There was a problem hiding this comment.
Thank you for working on this! It's looking great! Just a couple of readability comments
Few other things:
- Could you explain in the description which version of the MSC are you implementing (though hopefully it gets split up MSC2881: Message Attachments matrix-spec-proposals#2881 (comment))
- AFAIK, this isn't really implementing the MSC since it doesn't use an array I think that should be somehow made more clear
- Could you also please document this in https://github.com/vector-im/element-web/blob/develop/docs/labs.md
- I think it would be better UX wise to add a composer to the upload dialog but that is not a necessity since this is in labs and you may want to do that in a separate PR
| const promAfter = (SettingsStore.getValue("feature_message_attachments") && this.props.composer | ||
| && ev.target.files.length === 1 | ||
| && (!this.props.composer.state.isComposerEmpty || this.props.composer.props.replyToEvent)) ? | ||
| (event: ISendEventResponse) => { | ||
| return this.props.composer.sendMessage(event.event_id); | ||
| } : null; |
There was a problem hiding this comment.
I think this would be much more readable if it were split into variables
| const promAfter = (SettingsStore.getValue("feature_message_attachments") | ||
| && clipboardData.files.length === 1 && (!this.model.isEmpty || this.props.replyToEvent)) ? | ||
| (event: ISendEventResponse) => { | ||
| return this.sendMessage(event.event_id); | ||
| } : null; |
There was a problem hiding this comment.
I think this would be more readable if it were split into variables
| async sendContentListToRoom( | ||
| files: File[], roomId: string, matrixClient: MatrixClient, | ||
| attachToMessage?: (ISendEventResponse) => Promise<any>, | ||
| ) { |
There was a problem hiding this comment.
Would it be possible to avoid the callback here? It would be more readable, imo
| @@ -203,6 +204,7 @@ export default class RoomView extends React.Component<IProps, IState> { | |||
| private roomView = createRef<HTMLElement>(); | |||
| private searchResultsPanel = createRef<ScrollPanel>(); | |||
| private messagePanel: TimelinePanel; | |||
| private messageComposer: MessageComposer; | |||
There was a problem hiding this comment.
I think that createRef() should be sufficient in this case
There was a problem hiding this comment.
Woo! Attachments!
Screenshots/gifs of this in action would be appreciated to make design/product review a bit easier.
The code overall is a bit stringy, but I think that's because the composer code is a bit stringy from having to do so many things. The outstanding comments from Simon I think will at least help with readability and thus make it easier to follow.
I've not done a super in-depth review on this just yet as it'll need to go through product and design review, which will probably end up changing how the thing works and thus changing the code we end up with :)
| ): void { | ||
| const attachContent = { | ||
| 'm.relates_to': { | ||
| 'rel_type': 'org.matrix.msc2881.m.attachment', |
There was a problem hiding this comment.
throughout we generally prefer to use UnstableValue from the js-sdk for identifiers like this. It makes for transitioning a bit easier, particularly when/if the MSC becomes stable.
There should be some example usages in the code somewhere: look for new UnstableValue
|
I think of implementing matrix-org/matrix-spec-proposals#3051 support first, as it's necessary for supporting image replies/multiple attachments. Single relation is pretty limiting here. Hence, I'll convert this into a draft PR for now. |
|
I'm going to unassign from Design and Product for now until there are some screenshots/gifs to review |
Type: enhancement
See matrix-org/matrix-spec-proposals#2881
Ideally, m.relates_to needs to be an array for multiple attachments to work, and to implement file/image replies properly, but that would require way more changes (including in Synapse) to make all relations code work with arrays. Currently, I just show "In reply to" without the message actually being a reply.
In this implementation, if any text is already in the composer during upload, files get sent as an attachment (and after they finish uploading, text gets sent immediately). Naturally, a better UI will be needed.
Here's what your changelog entry will look like:
✨ Features
Preview: https://612723cb1e11b2602d882f7e--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.