-
Notifications
You must be signed in to change notification settings - Fork 373
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
MSC2299: Proposal to add m.textfile msgtype #2299
base: old_master
Are you sure you want to change the base?
Conversation
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 the need for the ability to send files over Matrix and have them preview-able inline is useful, but I think this proposal is the wrong approach. This will require a new message type which will replace functionality in an existing type and break client support.
Furthermore, I believe a lot of the issues raised in this PR are issues with Riot rather than issues with the specification. The authors concerns could actually be largely solved with the ability to save codeblocks in Riot for example.
My counter solution that would not break existing functionality in clients but still allow a preview would just to add a preview field to the existing file type.
Honestly, I believe this proposal is glaring proof for the need of extensible events.
|
||
Let's say you are helping someone out with code and just want to look at their code. Currently, you have to save the file | ||
to your desktop and then view it in whichever viewer you want. Or you are helping someone configure something, if they | ||
send the config file, you'll have to save it and view it externally. Instead it would help greatly to display the content |
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.
Currently, you have to save the file to your desktop and then view it in whichever viewer you want. Or you are helping someone configure something, if they send the config file, you'll have to save it and view it externally.
This is not true. You are able to send markdown formatted blocks of code absolutely fine, and the Matrix community have been doing this for a while. It's more acceptable to say that this might not be an adequate solution to the problem of sharing code snippets, but it's definitely possible with today's spec.
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.
Depending on usecase it can be way more convenient to send the file instead of copypasting the correct snippets out of it.
Let's say you are helping someone out with code and just want to look at their code. Currently, you have to save the file | ||
to your desktop and then view it in whichever viewer you want. Or you are helping someone configure something, if they | ||
send the config file, you'll have to save it and view it externally. Instead it would help greatly to display the content | ||
of these files inline. To not clog up vertical space and to save bandwidth a small preview could be attached to text files |
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.
To not clog up vertical space and to save bandwidth a small preview could be attached to text files
in order to pitch the file to the user.
This is a client concern, and as it stands we already limit the size of inline text blocks in Riot. Other clients can apply their own limits here if this is a concern. This spec will not solve that problem.
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.
Fiar point
|
||
## Proposal | ||
|
||
This proposal adds a new `m.textfile` msgtype to messages with type `m.room.message`. It would accompany the msgtypes |
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.
One thing I am concerned about here is the need for a new type. I would rather see clients pick off the mimetype from a m.file
and decide for themselves if it can be rendered inline.
My favoured solution to this problem is a hint flag like m.preview
: "" on the existing m.file
which provides several benefits:
- Primarily, this requires no changes on any client rendering or sending code. It's an opt-in feature.
- Clients which do support this feature can just read the preview key, or not show a preview.
- This allows for a broader range of previews, not limited to just textfiles but indeed any file which might want a text based preview format.
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.
Then m.image
, m.video
and m.audio
are also unneeded as the client could just go via the specified mimetype to display images/videos/audio inline. Soru followed the existing pattern here to use different msgtypes for different kind of previews.
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'm more in favour of clients starting to pick off the type instead. We already don't want to keep adding msgtypes in favour of new events, and a text file event sounds irresponsible considering that's exactly what a room message event is currently. The client can also pick apart the event for any type it sees: if it sees a PDF, it could render a preview - we shouldn't keep adding msgtypes for things clients want to render, and instead encourage clients to just render them.
When we finally get extensible events, this could be represented a lot better as well.
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.
Are you saying that m.image, m.video and m.audio should be obsoleted, then? As with this MSC soru merely followed the pattern of having different msgtypes for different previews
@Sorunome the eventual plan is to do that, yes. For the time being we're mostly interested in not making the problem worse.
(also, should be able to reply here - the review comment was on a thread, so the original thread has to be found instead of replying to the review)
send the config file, you'll have to save it and view it externally. Instead it would help greatly to display the content | ||
of these files inline. To not clog up vertical space and to save bandwidth a small preview could be attached to text files | ||
in order to pitch the file to the user. | ||
Furthermore, other networks (such as slack) already do this, which would help bridging networks together. |
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.
In the slack bridge, we already render it inline into markdown to provide a preview. I guess the ability to save the file would be useful, so I refer to the concern below.
|
||
## Proposal | ||
|
||
This proposal adds a new `m.textfile` msgtype to messages with type `m.room.message`. It would accompany the msgtypes |
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'm more in favour of clients starting to pick off the type instead. We already don't want to keep adding msgtypes in favour of new events, and a text file event sounds irresponsible considering that's exactly what a room message event is currently. The client can also pick apart the event for any type it sees: if it sees a PDF, it could render a preview - we shouldn't keep adding msgtypes for things clients want to render, and instead encourage clients to just render them.
When we finally get extensible events, this could be represented a lot better as well.
@turt2live somehow unable to reply to your comment directly, so here we go Are you saying that |
Rendered