Media navigation with swipe gesture #4161

bmarty · 2025-01-17T11:44:35Z

Content

Add ability to swipe between media (images and videos) and files (files, audio and voice message) when navigating from the gallery.

Note: for now, when opening a media from the room, swipe is not supported. This support will come later.

Motivation and context

Closes #4174
Related to #1484

Screenshots / GIFs

SwipeMedia.mp4

Tests

Open a room and navigate to settings/Media and files
Open an image
Swipe left and right
press back
open the files tab
open a file
Swipe left and right

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 24
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 title will be used in the release note, it clearly define what will change for the user
Pull request includes screenshots or videos if containing UI changes
You've made a self review of your PR

github-actions · 2025-01-17T11:55:58Z

📱 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/9Xh75M

bmarty · 2025-01-22T06:46:09Z

...uitests/src/test/snapshots/images/libraries.mediaviewer.impl.viewer_MediaViewerView_3_en.png

Note to myself: why the caption has vanished here?

Fixed in 4f4c994

codecov · 2025-01-22T07:07:49Z

Codecov Report

Attention: Patch coverage is 91.27273% with 48 lines in your changes missing coverage. Please review.

Project coverage is 83.35%. Comparing base (e4768a8) to head (da22758).
Report is 44 commits behind head on develop.

Files with missing lines	Patch %	Lines
...braries/mediaviewer/impl/viewer/MediaViewerView.kt	91.33%	7 Missing and 4 partials ⚠️
...l/timeline/model/event/TimelineItemEventContent.kt	0.00%	5 Missing ⚠️
...aries/mediaviewer/impl/gallery/MediaGalleryView.kt	75.00%	3 Missing and 2 partials ⚠️
...diaviewer/impl/viewer/MediaViewerFlickToDismiss.kt	88.09%	2 Missing and 3 partials ⚠️
...aries/mediaviewer/impl/gallery/EventItemFactory.kt	50.00%	0 Missing and 4 partials ⚠️
...s/mediaviewer/impl/viewer/MediaViewerDataSource.kt	96.29%	2 Missing and 1 partial ⚠️
...s/mediaviewer/impl/DefaultMediaViewerEntryPoint.kt	0.00%	2 Missing ⚠️
...aries/mediaviewer/impl/gallery/ui/VoiceItemView.kt	0.00%	0 Missing and 2 partials ⚠️
...ies/mediaviewer/impl/local/audio/MediaAudioView.kt	60.00%	2 Missing ⚠️
...ies/mediaviewer/impl/local/video/MediaVideoView.kt	50.00%	2 Missing ⚠️
... and 6 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4161      +/-   ##
===========================================
+ Coverage    83.31%   83.35%   +0.03%     
===========================================
  Files         1881     1886       +5     
  Lines        49101    49403     +302     
  Branches      5769     5804      +35     
===========================================
+ Hits         40910    41180     +270     
- Misses        6116     6134      +18     
- Partials      2075     2089      +14

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

…enter.

jmartinesp

There are a few nits that you can decide to change or not and some more docs in the new classes/methods would be nice, but mainly I think we need to fix backpagination with either the fix discussed privately or something similar, otherwise the gallery seems to get stuck.

jmartinesp · 2025-01-20T11:33:39Z

...impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerView.kt

+        }
+        HorizontalPager(
+            state = pagerState,
+            modifier = Modifier,


This is probably not needed?

Yes, removed.

jmartinesp · 2025-01-23T10:09:52Z

.../main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryDataSource.kt

+    fun start() {
+        if (!isStarted.compareAndSet(false, true)) {
+            return
+        }
+        flow {


Do we have a way to stop this flow, i.e. when the user leaves the gallery screen? Or is it ok if the gallery keeps loading in bg while the user is checking the room?

There is no way so far, but the flow will complete when the room will be destroyed.
The idea is to share the flow between the gallery and the media viewer with swipe. This will have to be reworked a bit when we will open the viewer from the timeline.

jmartinesp · 2025-01-23T10:27:09Z

...src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenter.kt

+        .takeIf { it != -1 }
+        ?: 0


I think this could be .coerceAtLeast(0).

jmartinesp · 2025-01-23T10:29:59Z

...impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerView.kt

+                        onDismiss = {
+                            onBackClick()
+                        },


Nit: this can probably be just onDismiss = onBackClick,. Same below.

jmartinesp · 2025-01-23T10:42:28Z

...rc/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt

+            .takeIf { it != -1 }
+            ?: 0


coerceAtLeast(0) here too?

jmartinesp · 2025-01-23T11:03:25Z

...pl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/ui/VoiceItemView.kt

@@ -115,7 +115,7 @@ private fun VoiceInfoRow(
        }
        Spacer(Modifier.width(8.dp))
        Text(
-            text = if (state.progress > 0f) state.time else voice.duration ?: state.time,
+            text = if (state.progress > 0f) state.time else voice.mediaInfo.duration ?: state.time,


voice.mediaInfo.duration?.takeIf { state.progress > 0f } ?: state.time maybe?

The correct alternative would be
voice.mediaInfo.duration?.takeIf { state.progress == 0f } ?: state.time
and I prefer not compare float using equality.

jmartinesp · 2025-01-23T11:05:01Z

...src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenter.kt

+            .takeIf { it != -1 }
+            ?: 0


I keep writing the coerceAtLeast(0) suggestion and the function keeps moving 😅 .

jmartinesp · 2025-01-23T11:09:18Z

...impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerView.kt

+                        onDismiss = {
+                            onBackClick()
+                        },


This can probably be inlined as some other previous cases.

jmartinesp · 2025-01-23T11:09:58Z

...ain/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerFlickToDismiss.kt

jmartinesp · 2025-01-23T11:15:50Z

...pl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryView.kt

+        loadingItem?.let { item ->
            LaunchedEffect(item.timestamp) {
                state.eventSink(MediaGalleryEvents.LoadMore(item.direction))


Nit: if (loadingItem != null) and use loadingItem inside maybe? It's easier to step the debugger this way.

bmarty · 2025-01-23T14:45:57Z

Thanks for the review @jmartinesp . I (think I) have fixed all the remarks except the main one. I am working on it while the CI is recording the screenshots. I want to add unit test on this part.

bmarty · 2025-01-23T16:51:11Z

@jmartinesp I have fixed the pagination issue and covered it by a unit test in da22758

Let me know if it's OK for you!

jmartinesp

Thanks for addressing the previous nits, with the back pagination issue fixed it should be good to merge!

sonarqubecloud · 2025-01-23T17:01:18Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

bmarty added PR-Feature For a new feature Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. labels Jan 20, 2025

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

bmarty commented Jan 22, 2025

View reviewed changes

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 22, 2025

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

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 22, 2025

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

bmarty and others added 20 commits January 22, 2025 16:21

Create MediaGalleryDataSource and extract logic from MediaGalleryPres…

35cc5df

…enter.

Let MediaGalleryDataSource be an interface

cbf7bf0

Remove RetryLoading (use LoadMedia)

a06607f

Suppress Detekt false positive (?)

32db42a

Add support for files navigation (when coming from the gallery)

7df65b0

Open in SingleMedia mode when coming from the timeline

ca5c06f

If not displayed, make sure to pause the audio / video

8d6550b

media viewer : create MediaViewerDataSource

98a786b

Provide duration

d26414f

Remove unused import

bad4566

media viewer : use collectAsState in the DataSource

3248041

Fix and write tests

c0542c8

Improve loading state and add preview.

d6ebec8

Add exception for Konsist

6c904a4

sync strings

aa68a35

Restore caption rendering

a4cf1d7

Small cleanup

f286f6d

Add test on SingleMediaGalleryDataSource

428b81c

Add test on MediaViewerDataSource

18c3b5b

MediaViewer: add error case in the UI.

621558a

bmarty and others added 2 commits January 22, 2025 16:24

Fix tests.

b72f4bb

Update screenshots

74be5a5

bmarty force-pushed the feature/bma/mediaNavigation branch from 1d87a2b to 74be5a5 Compare January 22, 2025 15:25

bmarty added 2 commits January 23, 2025 09:29

Ensure gallery is paginating to get new items.

1580c73

Fix formatting.

77887f9

bmarty changed the title ~~WIP media navigation with swipe~~ Media navigation with swipe gesture Jan 23, 2025

bmarty marked this pull request as ready for review January 23, 2025 09:54

bmarty requested a review from a team as a code owner January 23, 2025 09:54

bmarty requested review from ganfra and removed request for a team January 23, 2025 09:54

jmartinesp requested changes Jan 23, 2025

View reviewed changes

bmarty added 9 commits January 23, 2025 14:49

Remove useless parameter

dff9005

Simplify with code coerceAtLeast(0)`

54182b0

Simplify

2fde015

Add documentation on buildMediaViewerPageList.

afd8161

Add name to argument for clarity.

a5515b0

Use Black for code clarity.

7dd797b

Fix color for media viewer according to Figma.

beb835c

Cleanup

05660fb

Improve code clarity

4d2edfa

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Jan 23, 2025

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

ElementBot and others added 2 commits January 23, 2025 14:56

Update screenshots

ba0502c

Fix pagination restart issue and cover by unit test.

da22758

jmartinesp approved these changes Jan 23, 2025

View reviewed changes

bmarty merged commit 3668e86 into develop Jan 23, 2025
28 checks passed

bmarty deleted the feature/bma/mediaNavigation branch January 23, 2025 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media navigation with swipe gesture #4161

Media navigation with swipe gesture #4161

bmarty commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

bmarty Jan 22, 2025

bmarty Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

jmartinesp left a comment

jmartinesp Jan 20, 2025

bmarty Jan 23, 2025

jmartinesp Jan 23, 2025

bmarty Jan 23, 2025

jmartinesp Jan 23, 2025

bmarty Jan 23, 2025

jmartinesp Jan 23, 2025

bmarty Jan 23, 2025

jmartinesp Jan 23, 2025

jmartinesp Jan 23, 2025

bmarty Jan 23, 2025

jmartinesp Jan 23, 2025

jmartinesp Jan 23, 2025

bmarty Jan 23, 2025

jmartinesp Jan 23, 2025

jmartinesp Jan 23, 2025

bmarty commented Jan 23, 2025

bmarty commented Jan 23, 2025

jmartinesp left a comment

sonarqubecloud bot commented Jan 23, 2025

Media navigation with swipe gesture #4161

Media navigation with swipe gesture #4161

Conversation

bmarty commented Jan 17, 2025 • edited Loading

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

jmartinesp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Jan 23, 2025

bmarty commented Jan 23, 2025

jmartinesp left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 23, 2025

Quality Gate passed

bmarty commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading