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

MSC2781: Remove the reply fallbacks from the specification #2781

Open
wants to merge 27 commits into
base: old_master
Choose a base branch
from

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented Sep 18, 2020

Since I hit another fallback bug today, I thought I should finally propose this. Let's see, how this goes.

Rendered

Implementations:

See also:

fixes matrix-org/matrix-spec#368

fixes matrix-org/matrix-spec#350 ?

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.de


FCP tickyboxes

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels Sep 18, 2020
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
…possible)

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
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.

I understand the controversy but I really think that fallbacks bring more problems (especially in terms of the content getting stuck in messages not belonging to authors) than solutions to those who try to stay as simple as possible. The requirement to strip the fallbacks in replies raises the bar of implementing them to the point where half of the ecosystem chooses not to deal with that. Besides, the fallbacks are a huge HTML foot in the door where plaintext messengers have literally no chance of compliance. So yes, please, let's go with it.

proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the glory of the Emperor: Down with mx-reply!

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@aaronraimist
Copy link
Contributor

Implementation: matrix-org/matrix-react-sdk#6964

@turt2live turt2live changed the title MSC2781: Deprecate the (reply) fallbacks in the Matrix specification MSC2781: Deprecate edit & reply fallbacks in the Matrix specification Jan 14, 2022
@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label May 13, 2024
@ara4n
Copy link
Member

ara4n commented May 13, 2024

fwiw, the thing that swung me on this is the t&s impact of leaking contents of deleted in replies.

@clokep
Copy link
Member

clokep commented May 14, 2024

Overall I'm happy with this, but I think the text is a bit confusing. @turt2live and I left a bunch of comments which should help with that.

@mscbot concern General clarity

@dbkr
Copy link
Member

dbkr commented May 14, 2024

I think this is going in the right direction but already has appropriate concerns, and I agree with them. For future reference, some examples would make this a lot easier to read.

@deepbluev7
Copy link
Contributor Author

Thank you all very much for the feedback, I'll address it in the next few days!

Of the 23 clients listed in the [matrix client matrix](https://matrix.org/clients-matrix)
16 are listed as not supporting replies (updated January 2022):

- Element Android: Relies on the reply fallback.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to this, Element Android actually seems to completely blank the message if the rich reply is there, but not the reply fallback (new Beeper Android and some mautrix bridges have done this for example)

It also seems a little unlikely that this will be fixed element-hq/element-android#8460

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of my hopes with this MSC was to encourage clients to implement proper reply support. Element Android in particular has been a problem for me, since it sends invalid fallbacks in many cases. While I do agree, that it is unlikely this will actually be fixed in Element Android, considering how problematic the reply implementation in Element Android is, I don't know if that should really be a blocker on this MSC. Stripping out its current reply handling and implement this MSC is probably easier than making Element Android actually compliant with the current specification for replies (from before this MSC).

This also addresses several review comments from clokep and Travis.
@clokep
Copy link
Member

clokep commented Jun 5, 2024

@deepbluev7 Thanks for your effort on clarifying and improving this MSC. I think it is much easier to read now and clearer what the proposed changes are! 🎉

@mscbot resolve General clarity

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some very minor editorial points, and a question.

proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
16 are listed as not supporting replies (updated January 2022):

- Element Android: Relies on the reply fallback.
- Element iOS: [Does not support rich replies](https://github.com/vector-im/element-ios/issues/3517)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and therefore relies on the reply fallback? So is the same as Element Android? or different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When that was written, yes. Although I didn't investigate more since I don't own any iOS device. But it seems Element iOS might have some support now based on element-hq/element-ios#6155 ?

deepbluev7 and others added 3 commits June 25, 2024 17:44
Thanks dbkr, richvdh and clokep!

Co-authored-by: David Baker <dbkr@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved
proposals/2781-down-with-the-fallbacks.md Outdated Show resolved Hide resolved

## Proposal

Remove the [rich reply fallback from the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of those examples are from a time before the deprecation policy was enacted in Matrix 1.1, fwiw. This MSC introduces a technically breaking change to clients, which necessitates deprecation first at a minimum. The spec instructs clients which do support rich replies on how to handle the fallback (strip it), and implies that the module is optional. By removing the fallback, we're saying that clients must support rich replies in order to maintain conversation context - something which is currently optional (and not even a SHOULD optional).

The MSC addresses the context piece a bit, but doesn't address the breaking change. As such, this needs to go through deprecation first to manage the breaking change, then removal at least 1 version later as per the policy. This does add additional overhead - feedback regarding that is best placed in a dedicated MSC which aims to change the deprecation policy.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seemingly my github tabs got confused and submitted my review without asking the status associated with it.

Apologies for the slow review here - authenticated media has been taking up all my time.

@turt2live
Copy link
Member

@mscbot resolve Mention intentional mentions

deepbluev7 and others added 2 commits October 1, 2024 16:48
Co-authored-by: Travis Ralston <travpc@gmail.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. unresolved-concerns This proposal has at least one outstanding concern
Projects
Status: Ready for FCP ticks
Development

Successfully merging this pull request may close these issues.