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

MSC3286: Media spoilers #3286

Closed
wants to merge 4 commits into from

Conversation

robintown
Copy link
Member

Signed-off-by: Robin Townsend <robin@robin.town>
Signed-off-by: Robin Townsend <robin@robin.town>
@turt2live turt2live added client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Jul 18, 2021
Signed-off-by: Robin Townsend <robin@robin.town>
Signed-off-by: Robin Townsend <robin@robin.town>
"body": "screenshot.png",
"info": {
"mimetype": "image/png",
"size": 123456,
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to require or at least suggest, that clients should send a blurhash for spoilers (of image like files), that the client can display in place of the actual image. Might make not much sense to include in the MSC, since #2448 is still an MSC too, but it would probably make the spoiler experience much nicer!

Copy link
Member Author

Choose a reason for hiding this comment

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

There are plenty of ways clients might render spoilers without using blurhashes, so I don't think it makes sense to require them. Plus, if a client can render blurhashes and wants to use them for media spoilers, it's very likely to already have a local blurhash encoding implementation that it can use anyways, so I think blurhashes deserve a tangential mention here at most.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to be able to always show a blurhash, but I guess you can't anyway, so alright, makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

There are plenty of ways clients might render spoilers without using blurhashes, so I don't think it makes sense to require them. Plus, if a client can render blurhashes and wants to use them for media spoilers, it's very likely to already have a local blurhash encoding implementation that it can use anyways, so I think blurhashes deserve a tangential mention here at most.

While there are other ways I think a blurhash would be save more resources here. So the Client doesnt need to download the thumbnail files until a User interaction happened.

@@ -0,0 +1,79 @@
# MSC3286: Media spoilers

Choose a reason for hiding this comment

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

I actually feel like this could be made a lot more generic. There are various reasons why one may choose to hide something by default including: spoilers, nsfw content, graphic content or medical reasons (like epilepsy). For the most part, the client wouldn't need to tell a difference, but in some cases (like NSFW), it would be useful for the client to know as it could then have a global setting to permanently hide such posts. The lack of this has also been the reason for implementing a basic description-based filtering in Element Android, but that could be replaced with this.

I would suggest spoiler be replaced with something like hidden or hidden_by_default with the subkeys reason and description, the former being machine-readable (m.nsfw, m.spoiler, m.graphic, m.medical for instance) and latter, human-readable written by the sender (like, can cause epileptic seisures).

If this is too generic or out-of-scope for this MSC, let me know, but I figured it wouldn't hurt to suggest.

"info": {
"mimetype": "video/mp4",
"size": 123456,
"spoiler": ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you thought of doing "spoiler": true for spoilers without a reason? That might choke on some languages due to the spoiler attribute being potentially a string or a boolean.

Choose a reason for hiding this comment

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

I prefer this method.

  1. Easier for statically typed languages to manage.
  2. "" is a natural value for "no reason" since the reason is empty.
  3. If spoiler: true is allowed then you still need to support spoiler: "" unless we disallow it. However disallowing it adds complexity on the clients because now they have to map no user reason to true, mapping to "" probably happens natural in most implementations..
  4. It raises questions about what spoiler: false means. (Presumably it means the same as null or missing, but now we have an extra representation for the same information).

Copy link
Contributor

Choose a reason for hiding this comment

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

By that argument, I would agree types shouldn't get mixed, but the natural way to express "no reason" would be... to not supply this optional field (which is equal to setting null). A reason of "" makes little sense to have, I would consider it bad style by the sending client and leave it to the receiving client whether to handle it specially or show an empty string.

Choose a reason for hiding this comment

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

Doesn't null represent "not a spoiler" but "" represent "is a spoiler, but no rationale provided"?

Maybe a better schema is making this an object with a reason_text attribute and maybe can have more structured attributes in the future (translations, structured info on what the spoiler is spoiling...). That way {} is a spoiler is no rationale provided and {reason: "Harry Potter 4 ending"} is a spoiler with rationale. And in the future we could have {subjects: ["Harry Potter 3"]} for a spoiler about a well specified topic but no rationale (maybe I have seen it so can tell my client to always reveal thins about this show).

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't null represent "not a spoiler" but "" represent "is a spoiler, but no rationale provided"?

Sorry, you're right.

better schema is making this an object

I think that is what I really had in mind, so I would agree. In short, empty string feels like a hack when we could use proper structured data, even if the only way to use it remains the reason field.

Copy link
Contributor

Choose a reason for hiding this comment

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

#3725 does pretty much this :)

@robintown
Copy link
Member Author

This has been superseded by #3725, which incorporates the feedback from @0x1a8510f2.

@robintown robintown closed this Feb 14, 2022
@turt2live turt2live added obsolete A proposal which has been overtaken by other proposals and removed proposal-in-review labels Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff 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.

None yet

9 participants