Implement MSC2530: render caption for image and video if available and add white backround to images in timeline. by jmartinesp · Pull Request #2570 · element-hq/element-x-android

jmartinesp · 2024-03-19T16:03:57Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Based on #2522 (thanks @surakin!) .

Adds a caption to image and video content when filename != null && body != filename.
Displays the caption below the image or video content in the timeline.
Changes how images are displayed, following what we suggested some time ago:
- Behind the image and up until it covers the min size of a bubble a blur hash will be displayed.
- Over it the image will be scaled until it fits with its aspect ratio, and centered in the available space.
- We add a white background behind the image to make sure images with alpha channel will display their contents fine.

You can review only my commits, starting from 6cfe0c8.

Motivation and context

Closes #2521.

Screenshots / GIFs

Image	Video

Tests

You'll need a room where images or videos with captions have been sent.

Open the room.
Make sure the images and videos both using and not using captions are displayed fine.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

added video captions Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

…2530

Implement MSC2530

github-actions · 2024-03-19T16:11:22Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/edSPw2

surakin · 2024-03-19T16:15:19Z

This does look better :D

codecov · 2024-03-19T16:23:07Z

Codecov Report

Attention: Patch coverage is 78.62595% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 73.16%. Comparing base (5cc5a0b) to head (ecab819).
Report is 3 commits behind head on develop.

Files	Patch %	Lines
.../designsystem/components/blurhash/BlurHashImage.kt	35.29%	6 Missing and 5 partials ⚠️
...timeline/components/event/TimelineItemImageView.kt	78.94%	3 Missing and 5 partials ⚠️
...timeline/components/event/TimelineItemVideoView.kt	85.71%	2 Missing and 4 partials ⚠️
.../components/blurhash/BlurHashBackgroundModifier.kt	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2570      +/-   ##
===========================================
+ Coverage    73.14%   73.16%   +0.01%     
===========================================
  Files         1408     1410       +2     
  Lines        34095    34183      +88     
  Branches      6617     6638      +21     
===========================================
+ Hits         24939    25009      +70     
- Misses        5699     5709      +10     
- Partials      3457     3465       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmarty

LGTM, some remarks

bmarty · 2024-03-20T10:42:28Z

Suspicious change, as if a filter was applied to the image

It's only alpha being 0.8f instead of 1f. Otherwise the image itself might not be distinguishable from the blurhash behind it (this happened with a mostly black image I was testing).

bmarty · 2024-03-20T10:47:13Z

+            AsyncImage(
+                modifier = Modifier
+                    .fillMaxWidth()
+                    .then(if (isLoaded) Modifier.background(Color.White) else Modifier),


I know this has been reported internally, but I am pretty sure adding a white bg can cause more troubles than fixes (rendering in Dark theme for instance), but let's see.
FTR last time I checked, Element Web does not add white BG to images.

Is the white background the cause of these strange artifacts on the border/corners?

FTR last time I checked, Element Web does not add white BG to images.

Yes, but Matthew said it was a mistake. i.e. png images with no background and black letters were impossible to read in dark mode.

Is the white background the cause of these strange artifacts on the border/corners?

Sadly, I think that's the case...

png images with no background and black letters

OK, so with a white background, how a png image with no background and white letters will render?

I'd say this is the problem for only a minority of files, probably built just to make applications fail (not only Element).

Adding a background to image with transparency is like not supporting image with transparency, this is a shame. I still believe we should not add a white background. This degrades the user experience (stickers, etc.)

You might be right. If the users find it annoying we can always remove this white background.

This degrades the user experience (stickers, etc.)

Stickers use a different view, so they won't have this problem.

Yes, but Matthew said it was a mistake. i.e. png images with no background and black letters were impossible to read in dark mode.

Could we only add the background if we detect that the image is both transparent, and has a specific contrast ratio relative to the app colour scheme?

To check if an image is transparent we'd have to load it twice in memory if I'm not mistaken, which isn't really efficient, or add some transformation when Coil loads them, but it seems like it could affect animated images (GIFs, WebP...), so I don't think it's something we want to invest too much time on.

bmarty · 2024-03-20T10:54:32Z

+                content.formatted?.body?.takeIf { content.formatted.format == MessageFormat.HTML } ?: SpannedString(content.caption)
+            }
+            Box {
+                EditorStyledText(


I think you need to provide a LocalContentColor, as per this place, this will fix the fact that color of caption is not the same for my captions and for other people captions. (see the recorded screenshot)

Nice catch!

bmarty · 2024-03-20T10:55:51Z

+@Composable
+internal fun TimelineImageWithCaptionRowPreview() = ElementPreview {
+    Column {
+        sequenceOf(false, true).forEach {


Maybe name it for clarity:

Suggested change

sequenceOf(false, true).forEach {

sequenceOf(false, true).forEach { isMine ->

(do not apply suggestion, it will not compile!)

Note that this is not done everywhere, so maybe an opportunity to replace all occurences of isMine = it in the codebase :)

bmarty · 2024-03-20T11:09:06Z

Please see my comment about the difference between the caption colors.

sonarqubecloud · 2024-03-20T12:22:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

bmarty · 2024-03-20T13:16:44Z

(updating the title for further reference when searching for this PR)

pixlwave · 2024-10-02T08:44:22Z

@jmartinesp Given this is part of the spec, is there a way we could get the event logic for this in the SDK so that iOS could reuse it? 😇

jmartinesp · 2024-10-02T08:52:24Z

@jmartinesp Given this is part of the spec, is there a way we could get the event logic for this in the SDK so that iOS could reuse it? 😇

Sure, feel free to open a PR 😉 .

pixlwave · 2024-10-02T08:55:14Z

Is the logic literally just

fn caption() -> Option<String> {
    if filename != None && body != filename {
        body
    } else {
        None
    }
}

Or is there more to this?

jmartinesp · 2024-10-02T08:59:54Z

I think that should be it, based on the implemented MSC contents.

frebib · 2024-10-02T09:51:35Z

Would it make sense to have a similar function for the filename()? That's arguably the more confusing one, because it can come from either filename or body

pixlwave · 2024-10-02T16:16:18Z

Let's see ruma/ruma#1919 🤞

Would it make sense to have a similar function for the filename()? That's arguably the more confusing one, because it can come from either filename or body

I wondered about this too, wasn't really sure what to call it though.

bmarty · 2024-10-03T08:14:41Z

FWIW, I tried to mitigate the confusion in #3574 but it's still in discussion.

pixlwave · 2024-10-04T08:59:52Z

FWIW, I tried to mitigate the confusion in #3574 but it's still in discussion.

Have opened ruma/ruma#1921 to try address this in Ruma and then we can use it in the SDK.

surakin and others added 14 commits March 10, 2024 15:02

Implement MSC2530

188d795

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

Merge remote-tracking branch 'upstream/develop' into msc2530

ec597cb

Fix caption width to match the image maximum

0fd09fc

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

empty

6060703

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

fixed tests

7f5c5fc

added video captions Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

remove redundant semicolons

2396829

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

fix video composable main modifier warning

0430c9d

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

code quality fixes

a54ac5c

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

code quality fixes

f61e308

Signed-off-by: Marco Antonio Alvarez <surakin@gmail.com>

Merge branch 'feature/add-caption-to-image-and-video-events' into msc…

8fca7db

…2530

Merge pull request #2522 from surakin/msc2530

5b2a45f

Implement MSC2530

Some layout improvements for images and videos with captions

6cfe0c8

Fix tests and lint issues

5c36378

Update screenshots

e2a25dc

jmartinesp requested a review from a team as a code owner March 19, 2024 16:03

jmartinesp requested review from bmarty and removed request for a team March 19, 2024 16:03

bmarty approved these changes Mar 20, 2024

View reviewed changes

jmartinesp added 2 commits March 20, 2024 12:56

Fix caption default colors and typography

2360044

Replace it in several previews with isMine

8cff987

jmartinesp added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Mar 20, 2024

github-actions Bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Mar 20, 2024

ElementBot and others added 2 commits March 20, 2024 12:08

Update screenshots

318b72d

Fix lint issues

ecab819

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Mar 20, 2024

github-actions Bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Mar 20, 2024

jmartinesp merged commit 6aa84d6 into develop Mar 20, 2024

jmartinesp deleted the feature/add-caption-to-image-and-video-events branch March 20, 2024 12:58

bmarty changed the title ~~Implement MSC2530~~ Implement MSC2530: render caption for image and video if available Mar 20, 2024

bmarty changed the title ~~Implement MSC2530: render caption for image and video if available~~ Implement MSC2530: render caption for image and video if available and add white backround to images in timeline. Mar 20, 2024

frebib mentioned this pull request Sep 16, 2024

Render image captions as per MSC2530 element-hq/element-x-ios#3285

Closed

bmarty mentioned this pull request Oct 3, 2024

Clarify model for Event with attachment #3574

Merged

10 tasks

	sequenceOf(false, true).forEach {
	sequenceOf(false, true).forEach { isMine ->

Uh oh!

Conversation

jmartinesp commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

github-actions Bot commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

surakin commented Mar 19, 2024

Uh oh!

codecov Bot commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Mar 20, 2024

Quality Gate passed

Uh oh!

bmarty commented Mar 20, 2024

Uh oh!

pixlwave commented Oct 2, 2024

Uh oh!

jmartinesp commented Oct 2, 2024

Uh oh!

pixlwave commented Oct 2, 2024

Uh oh!

jmartinesp commented Oct 2, 2024

Uh oh!

frebib commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pixlwave commented Oct 2, 2024

Uh oh!

bmarty commented Oct 3, 2024

Uh oh!

pixlwave commented Oct 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmartinesp commented Mar 19, 2024 •

edited

Loading

github-actions Bot commented Mar 19, 2024 •

edited

Loading

codecov Bot commented Mar 19, 2024 •

edited

Loading

frebib commented Oct 2, 2024 •

edited

Loading