-
Notifications
You must be signed in to change notification settings - Fork 379
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
MSC2589: Improve replies #2589
MSC2589: Improve replies #2589
Conversation
```json | ||
{ | ||
"m.relationship": { | ||
"rel_type": "m.reference", |
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.
Why m.reference
and not something like m.reply
? Reference seems kinda too generalized, an edit is a reference, a reaction is a reference, you might be referencing multiple things in your content but only replying to one, etc.
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.
Stolen as-defined from 1849. That's about the end of my justification 😄
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 want to trigger bikeshedding but I'm also not entirely convinced by the relationship name and I'd prefer to have m.reply
even if it happens to be unnecessarily specific later on (which I believe is unlikely; see also what happened to In-Reply-To
and References
in e-mail).
MSC1849 mentions threading as a possible reason for the name; but label-based threading proposed by #2326 doesn't use m.reference
(and, arguably, m.reference
would anyway look alien there 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.
Let's move the bikeshed to 1849, which this MSC can follow along with. If this MSC somehow gets closer to FCP than 1849, we can do some course correction.
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 really happy to see something in this direction (I sadly never managed to write an MSC for this myself). I would still prefer to go with the alternative and I added a few comments to explain my stance, but anything that makes reply handling easier is very welcome in my opinion!
## Potential issues | ||
|
||
Events will be larger as a result, though typical messaging scenarios hardly get anywhere near the | ||
event size limit. |
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.
While I don't think the event size limit is an issue, replies currently already have quite a bit of overhead already and this would do not that much to decrease it. If someone sends the draft for an MSC for example, replying to it basically sends the same message again in size. Since Riot is already a considerable factor of my mobile data usage, I think the alternative you propose is a better solution for at least the bloat issue.
|
||
Instead of having to parse the fallback to get the reply, clients MUST send a `reply_body` | ||
(and optionally a `reply_formatted_body`) in the event content to represent what the user is | ||
replying with. |
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.
How would this relate to edits, which also send a new_content, effectively duplicating the body
and formatted_body
? If you don't want to go with your proposed alternative, would I need to send something like this?:
{
"content": {
"body": "...",
"formatted_body": "...",
"reply_body": "...",
"reply_formatted_body": "..."
},
"new_content": {
"body": "...",
"formatted_body": "...",
"reply_body": "...",
"reply_formatted_body": "..."
}
}
That's a bit ridiculous, I think. You may be able to drop the reply_*
parts in new_content
though, but I would still prefer to not duplicate content unnecessarily.
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.
Edits are a problem for 1849 entirely. Whichever MSC lands first will force the other to figure it out :)
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.
While that is true, I think it is a valid concern, that we do have a somewhat exponential trend of message contents here :D
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 simplicity I'd be inclined to say that edits, under 1849, would include the fallback and other properties so clients can simply map it over top and not worry about it.
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.
The m.new_content
in edits currently doesn't include a reply fallback at all ftr
so with no referencing context. For example, "does anyone know how to fix this?" means nothing if | ||
the original event's context isn't presented. This is largely an issue for bridges and other simple | ||
clients, though many projects which realize the reply fallback is not perfect tend to take action | ||
to support replies properly. |
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 I would prefer this. You don't lose that much context, if you are replying without a large gap to a message and if you do, you may want to use a client, that supports replies. Many bridges already need to deal with replies too and for at least a few of them, the fallback is actually a problem and most of them strip the fallback to replace it with their own. The IRC bridge for example would do much better to send their own fallback, so that messages don't become too long, which many IRC users dislike. Other platforms like WhatsApp for example do replies differently, so the fallback provides no value. It is far easier to add a fallback in my experience, than to strip it.
I'd also argue, that Riot/Android or RiotX would have implemented proper replies by now, if there was no reliable fallback. Usually the experience of proper replies is far better than relying on the fallback and I think client devs should be nudged to provide the better experience, when possible.
Stripping the fallback also takes quite a bit more effort than adding it, if you don't want to implement replies.
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.
You don't have to strip the fallback with this proposal. The IRC bridge also already adds a fallback itself, in the form of supporting replies properly by requesting the event it references.
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.
After re-reading relevant parts of #1849 and the discussion around them I tend to agree that we have to ditch the reply fallback in the form it exists now. Expecting too much from clients, the fallback text construction rules ended up being the second-class citizen and slip in all kinds of holes with respect to edits, redactions, E2EE; and worst of all, the list of holes is open to future additions. From the client perspective, once we moved to all these additional things that have to interact with replies, I see no way to provide consistent fallback text experience to clients that don't know about replies (meaning, specifically: neither parse the reply text, nor support replies through requesting the event replied to). Clients that do know about replies, do either of these two things anyway.
In order to give at least some help on "what was your comment about" thing, we could probably leave "In reply to <a href="https://matrix.to/#/$eventid">event</a>"
as the least troublesome part of the reply, in formatted_body
; and maybe Replying to @mxid:example.org:\n
prepended to body
. No body pieces of the message replied to, and no display names; @mxid:example.org
can still be abusive though, so I'm not certain about the plain text fallback.
I cry every time I think about Replies and the fallbacks I designed for them, I would love it if we can find a way to ditch them all together as suggested under |
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.
Next to other community members, I have reservations about the fallback; and also have a few easier comments.
```json | ||
{ | ||
"m.relationship": { | ||
"rel_type": "m.reference", |
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 want to trigger bikeshedding but I'm also not entirely convinced by the relationship name and I'd prefer to have m.reply
even if it happens to be unnecessarily specific later on (which I believe is unlikely; see also what happened to In-Reply-To
and References
in e-mail).
MSC1849 mentions threading as a possible reason for the name; but label-based threading proposed by #2326 doesn't use m.reference
(and, arguably, m.reference
would anyway look alien there as well).
so with no referencing context. For example, "does anyone know how to fix this?" means nothing if | ||
the original event's context isn't presented. This is largely an issue for bridges and other simple | ||
clients, though many projects which realize the reply fallback is not perfect tend to take action | ||
to support replies properly. |
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.
After re-reading relevant parts of #1849 and the discussion around them I tend to agree that we have to ditch the reply fallback in the form it exists now. Expecting too much from clients, the fallback text construction rules ended up being the second-class citizen and slip in all kinds of holes with respect to edits, redactions, E2EE; and worst of all, the list of holes is open to future additions. From the client perspective, once we moved to all these additional things that have to interact with replies, I see no way to provide consistent fallback text experience to clients that don't know about replies (meaning, specifically: neither parse the reply text, nor support replies through requesting the event replied to). Clients that do know about replies, do either of these two things anyway.
In order to give at least some help on "what was your comment about" thing, we could probably leave "In reply to <a href="https://matrix.to/#/$eventid">event</a>"
as the least troublesome part of the reply, in formatted_body
; and maybe Replying to @mxid:example.org:\n
prepended to body
. No body pieces of the message replied to, and no display names; @mxid:example.org
can still be abusive though, so I'm not certain about the plain text fallback.
Maybe we can merge current replies improvement MSC with my message attachments feature request https://github.com/matrix-org/matrix-doc/issues/2289? For implement this, we can convert
What do you think about this? |
Ideally we don't increase the scope of this beyond what it currently is. |
I second @turt2live - let's rather track these individually, otherwise we risk to not get either of them to the merge. |
Closing in favour of #2781 |
Relevant issues (see MSC for details):
Rendered