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

MSC2299: Proposal to add m.textfile msgtype #2299

Open
wants to merge 1 commit into
base: master
from

Conversation

@Sorunome
Copy link
Contributor

commented Sep 21, 2019

Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Sep 21, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Sorunome

Sorunome Sep 21, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Sep 21, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Sorunome

Sorunome Sep 21, 2019

Author Contributor

Fiar point


## Proposal

This proposal adds a new `m.textfile` msgtype to messages with type `m.room.message`. It would accompany the msgtypes

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Sep 21, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Sorunome

Sorunome Sep 21, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@turt2live

turt2live Oct 7, 2019

Member

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.

This comment has been minimized.

Copy link
@turt2live

turt2live Oct 9, 2019

Member

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

re #2299 (comment)

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

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Sep 21, 2019

Contributor

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.

@turt2live turt2live changed the title MSC 2299: Proposal to add m.textfile msgtype MSC2299: Proposal to add m.textfile msgtype Sep 24, 2019
@turt2live turt2live self-requested a review Sep 24, 2019

## Proposal

This proposal adds a new `m.textfile` msgtype to messages with type `m.room.message`. It would accompany the msgtypes

This comment has been minimized.

Copy link
@turt2live

turt2live Oct 7, 2019

Member

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.

@Sorunome

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

@turt2live somehow unable to reply to your comment directly, so here we go

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.