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

MSC2589: Improve replies #2589

Closed
wants to merge 3 commits into from
Closed

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented May 29, 2020

@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels May 29, 2020
@turt2live turt2live changed the title Proposal to improve replies MSC2589: Improve replies May 29, 2020
```json
{
"m.relationship": {
"rel_type": "m.reference",
Copy link
Contributor

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.

Copy link
Member Author

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 😄

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Contributor

@deepbluev7 deepbluev7 left a 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.
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Member Author

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 :)

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member

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.
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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.

@t3chguy
Copy link
Member

t3chguy commented May 29, 2020

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

@matrix-org matrix-org deleted a comment from deepbluev7 May 30, 2020
@matrix-org matrix-org deleted a comment from tulir May 30, 2020
Copy link
Member

@KitsuneRal KitsuneRal left a 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.

proposals/2589-reply-improvements.md Show resolved Hide resolved
```json
{
"m.relationship": {
"rel_type": "m.reference",
Copy link
Member

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.
Copy link
Member

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.

@MurzNN
Copy link
Contributor

MurzNN commented Jun 15, 2020

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 m.relationship to array m.relationships (or use "m.relations" like in #1849) and add new rel_type types:

  • m.reference (or better m.reply) for replied message
  • m.forward for forwarded messages
  • m.attachment for attachments
  • m.annotation for reactions

What do you think about this?

@turt2live
Copy link
Member Author

Ideally we don't increase the scope of this beyond what it currently is.

@KitsuneRal
Copy link
Member

I second @turt2live - let's rather track these individually, otherwise we risk to not get either of them to the merge.

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@turt2live
Copy link
Member Author

Closing in favour of #2781

@turt2live turt2live closed this Jan 14, 2022
@turt2live turt2live deleted the travis/msc/better-replies branch January 14, 2022 03:33
@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed proposal-in-review labels Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants