Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Increase blur intensity of spoilers #5646

Closed

Conversation

jaiwanth-v
Copy link
Contributor

@jaiwanth-v jaiwanth-v commented Feb 15, 2021

Fixes element-hq/element-meta#1545
image

Signed-off-by: Jaiwanth jaiwanth2011@gmail.com


Here's what your changelog entry will look like:

✨ Features

Signed-off-by: Jaiwanth <jaiwanth2011@gmail.com>
@jryans jryans requested a review from a team February 22, 2021 10:18
Copy link
Contributor

@nadonomy nadonomy left a comment

Choose a reason for hiding this comment

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

Hi @jaiwanth-v— thanks for the contribution! I have a couple of questions:

  1. I'd like to get a better shared understanding of the context and motivation for this change, can you elaborate? Has there been common or multiple points of feedback that the previous blur setting is too leaky? If so, please can you add some links to this PR (GitHub issues, matrix.to links to matrix messages, etc).

  2. The increase in blur size seems to be causing, or exaggerating a hard clip at the top or the bottom which doesn't meet our visual bar. Do you know what's causing it?

@jaiwanth-v
Copy link
Contributor Author

@nadonomy

  1. I have already linked the issue for this change.
  2. No. I'm not sure

@shmerl
Copy link

shmerl commented Feb 25, 2021

To add to the first question. I think text blurring was OK, but blurring for images was insufficient. See: element-hq/element-meta#1545

@jryans jryans requested a review from nadonomy February 27, 2021 15:20
@niquewoodhouse
Copy link
Contributor

To add to the first question. I think text blurring was OK, but blurring for images was insufficient. See: vector-im/element-meta#1545

I also think the text blurring was OK, it actually looked roughly like words, so I could understand it to be hidden words. In the context of a timeline, I think that's important. The after screenshot blur looks alien to the concept of words, so I think it might actually make things a bit harder to understand that it's text on the timeline?

For the image spoiler not being blurred enough, I don't think it's solved by this.

Before
image

After
image

Someone showed me this the other week - using blurhash for media placeholders. I wonder if there's a way to use styling like this for the image spoiler?

@shmerl
Copy link

shmerl commented Apr 22, 2021

Yeah, that's exactly what I meant about images. That kind of blurring is insufficient.

@jaiwanth-v
Copy link
Contributor Author

jaiwanth-v commented Apr 22, 2021

@niquewoodhouse Yes, using blurhash for rendering initial spoiler preview would be a great idea. For now, increasing the intensity of spoiler images looks good. The result would look something like this:
ezgif com-gif-maker (10)
What do you think?

@shmerl
Copy link

shmerl commented Apr 22, 2021

The maxed out blur is for sure much better than before. Still not ideal though, since you can make out some shapes.

@jaiwanth-v
Copy link
Contributor Author

It can be further increased. There's no upper bound.

@robintown
Copy link
Member

IMO increasing the blur intensity of all spoilers isn't the right way to approach the problem, since currently images can only be spoilered by inserting them inline in a textual message, which is somewhat of a hack. I expect that a spec change will be needed to properly support image spoilers, at which point it will hopefully be easy to handle images separately from text spoilers (e.g. by showing a blurhash)

@jaiwanth-v
Copy link
Contributor Author

Actually, images inside spoilers can be targeted and can be given a different blur. That is what I did in this comment. That way we won't disturb the other text spoilers.

@MadLittleMods MadLittleMods added Z-Community-PR Issue is solved by a community member's PR T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements labels Jun 2, 2022
@andybalaam
Copy link
Member

Had a conversation about this today - we think image spoilers should be implemented using blurhash instead of this way. We would very happily accept PRs for that, but I now think this PR is irrelevant, so closing it.

Thanks very much for your contribution!

@andybalaam andybalaam closed this Jul 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase blurring for images in spoilers
7 participants