Fix scaling of timeline images #1941

jmartinesp · 2023-12-01T12:05:19Z

Type of change

Feature
Bugfix
Technical
Other :

Content

The scaling behaviour now matches iOS:

Fit content scaling, not Crop.
Min/max height, not min/max aspect ratio. Changes the aspect ratio to take into account width, then height.

Motivation and context

Fixes #1940.

Screenshots / GIFs

In the PR.

Tests

Send an image with long height.
Check how it looks in the timeline.

Tested devices

Physical
Emulator
OS version(s): 11

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/vector-im/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/vector-im/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

github-actions · 2023-12-01T12:12:28Z

📱 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/fWw74w

codecov · 2023-12-01T12:18:05Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7283732) 67.60% compared to head (f7e03f8) 67.60%.
Report is 4 commits behind head on develop.

Files	Patch %	Lines
...ine/components/event/TimelineItemAspectRatioBox.kt	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1941   +/-   ##
========================================
  Coverage    67.60%   67.60%           
========================================
  Files         1349     1349           
  Lines        33786    33784    -2     
  Branches      7261     7261           
========================================
  Hits         22840    22840           
+ Misses        7496     7495    -1     
+ Partials      3450     3449    -1

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

ganfra · 2023-12-01T12:42:09Z

This doesnt work nicely for very large or very high images...

jmartinesp · 2023-12-01T12:43:29Z

Could we test those in the iOS app, to compare what they look like? Maybe I messed up the implementation, or maybe that's just what their algorithm does.

ganfra · 2023-12-01T12:45:21Z

Could we test those in the iOS app, to compare what they look like? Maybe I messed up the implementation, or maybe that's just what their algorithm does.

Honestly I think we should keep cropping, thats what other messenger apps are doing. We could just have a bigger maxHeight maybe. @VolkerJunginger

jmartinesp · 2023-12-01T12:51:00Z

This is what something similar looks like on iOS:

Details

This is what it looks like on Android with these changes:

Details

And this is what it looks like in the app at the moment (it looks empty, but it's actually cropping the center of both images):

Details

And this is what Whatsapp does with them (thanks @ganfra):

Details

In Telegram it seems like they just refuse to display them as actual images 😅 :

Details

So it seems like there isn't a 'fits all' solution for this...

Maybe we should keep this new strategy for images with normal aspect ratios and use the cropping one for images with extreme aspect ratios, such as these? Maybe we could add some kind of indicator to mark them as cropped, so the user knows it can be expanded, or use the telegram alternative, but that makes things way more complex.

seankhl · 2023-12-02T03:36:32Z

Just my two cents as a user:

I think the WhatsApp and pre-PR solutions are worse, because they are misleading about the contents of the image. It's not clear that I wasn't just sent a black square, whereas at least with the element-x iOS and telegram solutions I can be like okay, there's more detail than I can easily see from the timeline image, let me download it or click and zoom.

The new element-x android solution is okay, but the background color being much larger than the image and the image not being centered in it feels unnecessary and is a bit visually awkward and distracting. In that sense maybe it could be made to conform more to the element-x iOS version.

The final note I'd like to make that the rounded corners of the messages would have a larger negative impact on these very narrow images if the background is not present, as shown in the iOS version. Maybe it would be good to make the background just wide or tall enough to capture the rounded corner, so the image can remain rectilinear while providing the least distracting background? Not sure the exact ideal solution to that, but the rounded corner cutting into the image and obscuring some of it is worth avoiding or at least considering in the design.

jmartinesp · 2023-12-02T14:18:35Z

I was using Telegram desktop today and I saw another possible solution, something like they do here:

We could overlay the blurhash image and the actual loaded image: this way, the bubble still looks like a bubble and the actual image is displayed. I don't know how it'll look with tiny images though.

seankhl · 2023-12-03T08:45:17Z

The blurhash as background looks good and I like how the rounded corners aren't impacting the image.

I also like how there is no timestamp overlapping with the image in the Telegram desktop version. I think this aspect is worth considering for Element, too. I see no reason why a design couldn't be made that avoids overlapping with the image while being as good as the current one in all other respects. Not trying to criticize, just sharing my opinion that keeping the image completely unmodified and unobscured seems strictly better.

jmartinesp · 2023-12-11T08:41:42Z

I was on holidays last week, I'm continuing with this work now. @VolkerJunginger , @amshakal what do you think about the issue found above and the possible solutions? To me, the blurhash one looks like the best option, and as a second option the telegram Android one where images with a width or height smaller than a min value don't actually show a preview (last spoiler tag here.

In any case, doing any of these changes would mean doing the equivalent on iOS. I think the blurhash one should be fairly easy to do since all we'd had to do is not replace the loading blurhash background with the loaded image, but overlay them.

… the screenshot

Always display a white background for them.

bmarty

Thanks for the update. There is an issue regarding the background color, else I think it's fine.
It's a shame that we do not have a preview with the image and the blurhash behind. Not a P1 to investigate this thought.

bmarty · 2023-12-15T08:28:32Z

...ntRowForDirectRoom_null_TimelineItemEventRowForDirectRoom-Day-17_17_null,NEXUS_5,1.0,en].png

I think we need to have a background color, else the bubble does not render nicely...

This is what iOS looks like and what we've decided to use for now, given the deadline, since otherwise it would mean extra work for the iOS team, although this will probably be revisited soon. I took into account the suggestions to have a white background so images with transparency render nicely.

Maybe we could add a subtle border radius for these images so the bubble keeps its shape, something like this?

Screenshot

Can we have the bubble background color and white background for the image only (and maybe black background on dark theme)?
Like this this is not OK I think, but yes, definitely better with the bubble border.

With the bubble color I've got this:

Light Dark

And I think it's fine.

We may want to improve the rendering of images with transparency in another PR, since applying a white background should be done to all images generated by users: room avatars, user avatars, images in timeline, etc. But note that Element Web done not add background to the images. They just let see the current background if they have an alpha layer.

@amshakal do you agree?

Yes, this makes sense to me. :)

bmarty · 2023-12-15T08:29:04Z

...RowForDirectRoom_null_TimelineItemEventRowForDirectRoom-Night-17_18_null,NEXUS_5,1.0,en].png

... and this will also fix this issue.

bmarty · 2023-12-15T08:29:56Z

...n/kotlin/io/element/android/features/messages/impl/timeline/components/MessageEventBubble.kt

-    } else {
-        ElementTheme.colors.messageFromOtherBackground
+    val backgroundBubbleColor = when {
+        !state.displayBackground -> Color.White


You should use background color of the bubble, see my remark on the screenshots.

…t images will be rendered with the default bubble background color.

sonarqubecloud · 2023-12-15T12:12:49Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

jmartinesp requested a review from a team as a code owner December 1, 2023 12:05

jmartinesp requested review from ganfra and removed request for a team December 1, 2023 12:05

jmartinesp added the X-Needs-Product Issue needs input from Product team label Dec 12, 2023

manuroe mentioned this pull request Dec 13, 2023

Android: Don't crop pictures in the timeline element-hq/element-meta#2214

Closed

3 tasks

jmartinesp removed the X-Needs-Product Issue needs input from Product team label Dec 14, 2023

ganfra approved these changes Dec 14, 2023

View reviewed changes

jmartinesp added 4 commits December 15, 2023 08:12

Fix scaling of timeline images

07ab5dc

Fix timeline screenshot tests so the images don't take almost half of…

d9d8efa

… the screenshot

Fix lint issue

b4ce316

Center images in the message bubble.

2b76aeb

Always display a white background for them.

jmartinesp force-pushed the fix/jme/align-timeline-images-scaling-with-ios branch from af29f9f to 2b76aeb Compare December 15, 2023 07:55

jmartinesp added Run-Maestro Starts a Maestro Cloud session to run integration tests Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. and removed Run-Maestro Starts a Maestro Cloud session to run integration tests labels Dec 15, 2023

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Dec 15, 2023

Update screenshots

307a0d1

bmarty requested changes Dec 15, 2023

View reviewed changes

bmarty and others added 2 commits December 15, 2023 12:41

Do not apply white background on images (parity with Web). Transparen…

d83d64a

…t images will be rendered with the default bubble background color.

Update screenshots

fbec6b4

Cleanup

f7e03f8

bmarty merged commit 7ed460b into develop Dec 15, 2023
14 checks passed

bmarty deleted the fix/jme/align-timeline-images-scaling-with-ios branch December 15, 2023 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scaling of timeline images #1941

Fix scaling of timeline images #1941

jmartinesp commented Dec 1, 2023

github-actions bot commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading

ganfra commented Dec 1, 2023

jmartinesp commented Dec 1, 2023

ganfra commented Dec 1, 2023

jmartinesp commented Dec 1, 2023 •

edited

Loading

seankhl commented Dec 2, 2023

jmartinesp commented Dec 2, 2023 •

edited

Loading

seankhl commented Dec 3, 2023 •

edited

Loading

jmartinesp commented Dec 11, 2023

bmarty left a comment

bmarty Dec 15, 2023

jmartinesp Dec 15, 2023

bmarty Dec 15, 2023

bmarty Dec 15, 2023 •

edited

Loading

amshakal Dec 15, 2023

bmarty Dec 15, 2023

bmarty Dec 15, 2023

sonarqubecloud bot commented Dec 15, 2023

Fix scaling of timeline images #1941

Fix scaling of timeline images #1941

Conversation

jmartinesp commented Dec 1, 2023

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Dec 1, 2023 • edited Loading

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

ganfra commented Dec 1, 2023

jmartinesp commented Dec 1, 2023

ganfra commented Dec 1, 2023

jmartinesp commented Dec 1, 2023 • edited Loading

seankhl commented Dec 2, 2023

jmartinesp commented Dec 2, 2023 • edited Loading

seankhl commented Dec 3, 2023 • edited Loading

jmartinesp commented Dec 11, 2023

bmarty left a comment

Choose a reason for hiding this comment

bmarty Dec 15, 2023

Choose a reason for hiding this comment

jmartinesp Dec 15, 2023

Choose a reason for hiding this comment

bmarty Dec 15, 2023

Choose a reason for hiding this comment

bmarty Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

amshakal Dec 15, 2023

Choose a reason for hiding this comment

bmarty Dec 15, 2023

Choose a reason for hiding this comment

bmarty Dec 15, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 15, 2023

Quality Gate passed

github-actions bot commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 1, 2023 •

edited

Loading

jmartinesp commented Dec 1, 2023 •

edited

Loading

jmartinesp commented Dec 2, 2023 •

edited

Loading

seankhl commented Dec 3, 2023 •

edited

Loading

bmarty Dec 15, 2023 •

edited

Loading