Video player controller #3959

bmarty · 2024-11-27T15:26:33Z

Content

Implement a new video player controller, instead of using the one provided by exoplayer.

Note that the module would need some cleanup, in particular some code is in the api module and should be moved to the impl module.

I prefer to merge this PR first and do the rework after, in a dedicated PR.

Motivation and context

Be able to render caption, see https://www.figma.com/design/Ni6Ii8YKtmXCKYNE90cC67/Timeline-(new)?node-id=1727-5255&node-type=instance&m=dev.

Screenshots / GIFs

Before	After

Tests

Play a video from a room and observe the new controller

Tested devices

Physical
Emulator
OS version(s):

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 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
Pull request includes a sign off
You've made a self review of your PR

bmarty · 2024-11-27T15:28:04Z

...sts/src/test/snapshots/images/libraries.designsystem.theme.components_Sliders_Sliders_en.png

Note: light mode should not be used for now...

github-actions · 2024-11-27T15:32:30Z

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

codecov · 2024-11-27T15:48:05Z

Codecov Report

Attention: Patch coverage is 59.57447% with 95 lines in your changes missing coverage. Please review.

Project coverage is 83.01%. Comparing base (8d26af5) to head (5686b77).
Report is 15 commits behind head on develop.

Files with missing lines	Patch %	Lines
.../libraries/mediaviewer/api/local/LocalMediaView.kt	0.00%	82 Missing ⚠️
.../libraries/designsystem/theme/components/Slider.kt	84.44%	4 Missing and 3 partials ⚠️
...ediaviewer/api/player/MediaPlayerControllerView.kt	93.33%	4 Missing and 1 partial ⚠️
...aries/mediaviewer/api/local/LocalMediaViewState.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3959      +/-   ##
===========================================
- Coverage    83.07%   83.01%   -0.06%     
===========================================
  Files         1791     1795       +4     
  Lines        45335    45533     +198     
  Branches      5348     5365      +17     
===========================================
+ Hits         37661    37800     +139     
- Misses        5799     5854      +55     
- Partials      1875     1879       +4

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

jmartinesp

It LGTM overall, just a couple of questions about the colors used and a suggestion for the 'legacy' slider component.

jmartinesp · 2024-11-27T15:37:55Z

...tter/api/src/main/kotlin/io/element/android/libraries/dateformatter/api/DurationFormatter.kt

+    val minutes = (inSeconds % 3_600) / 60
+    val seconds = inSeconds % 60
+    return if (hours > 0) {
+        String.format(Locale.US, "%d:%02d:%02d", hours, minutes, seconds)


I was going to suggest only calculating the hours if inSeconds > 3600 but it seems like an overkill to just avoid a single allocation and a division 😅 .

jmartinesp · 2024-11-27T15:45:41Z

...nsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/Slider.kt

@@ -32,8 +45,20 @@ fun Slider(
    steps: Int = 0,
    onValueChangeFinish: (() -> Unit)? = null,
    colors: SliderColors = SliderDefaults.colors(),
-    interactionSource: MutableInteractionSource = remember { MutableInteractionSource() }
+    interactionSource: MutableInteractionSource = remember { MutableInteractionSource() },
+    useCustomLayout: Boolean = false,


This feels a bit weird, to be honest. I think we should either split this component into two (one for the new M3 look and feel and another for the old one) or rename/clarify a bit what useCustomLayout does. Maybe rename it to something like useLegacyMaterialStyle?

Yes, it's not ideal. I'll confirm with Aaron that the design is up to date for Android first.

jmartinesp · 2024-11-27T15:46:29Z

...nsystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/Slider.kt

+                        activeTrackColor = Color(0x66E0EDFF),
+                        inactiveTrackColor = Color(0x66E0EDFF),


Don't we have either semantic or core token colors for these?

jmartinesp · 2024-11-27T15:49:12Z

...main/kotlin/io/element/android/libraries/mediaviewer/api/player/MediaPlayerControllerView.kt

+    ) {
+        Box(
+            modifier = Modifier
+                .background(color = Color(0x99101317))


I guess we also don't have any tokens for this?

ganfra · 2024-11-27T15:56:05Z

...pi/src/main/kotlin/io/element/android/libraries/mediaviewer/api/local/LocalMediaViewState.kt

@@ -30,7 +30,10 @@ sealed interface PlayableState {
    data object NotPlayable : PlayableState
    data class Playable(
        val isPlaying: Boolean,
-        val isShowingControls: Boolean
+        val progressInMillis: Long,


This is what we want to expose to the outer world, so I'm not sure this is the right place to add here
I'd have added all those values to the MediaPlayerControllerState instead and let the Playable derive some values from that

…onents.

sonarcloud · 2024-11-28T15:34:21Z

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 and others added 5 commits November 27, 2024 16:05

Add a duration formatter.

98875a6

Slider: import design.

7a8b942

Add custom video player controller

9c1ccdf

Cleanup

28ed092

Update screenshots

c8519b3

bmarty requested a review from a team as a code owner November 27, 2024 15:26

bmarty requested review from jmartinesp and removed request for a team November 27, 2024 15:26

bmarty commented Nov 27, 2024

View reviewed changes

bmarty added the PR-Feature For a new feature label Nov 27, 2024

jmartinesp approved these changes Nov 27, 2024

View reviewed changes

ganfra reviewed Nov 27, 2024

View reviewed changes

bmarty added 2 commits November 28, 2024 14:38

PlayableState.Playable should only contain data useful for other comp…

6213402

…onents.

Improve preview

9a6b398

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label Nov 28, 2024

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

Update screenshots

5686b77

bmarty merged commit 31f9fa2 into develop Nov 28, 2024
23 of 25 checks passed

bmarty deleted the feature/bma/videoPlayer branch November 28, 2024 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Video player controller #3959

Video player controller #3959

bmarty commented Nov 27, 2024

bmarty Nov 27, 2024

github-actions bot commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

jmartinesp left a comment

jmartinesp Nov 27, 2024

jmartinesp Nov 27, 2024

bmarty Nov 28, 2024

jmartinesp Nov 27, 2024

jmartinesp Nov 27, 2024

ganfra Nov 27, 2024

bmarty Nov 28, 2024

sonarcloud bot commented Nov 28, 2024

		activeTrackColor = Color(0x66E0EDFF),
		inactiveTrackColor = Color(0x66E0EDFF),

Video player controller #3959

Video player controller #3959

Conversation

bmarty commented Nov 27, 2024

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

github-actions bot commented Nov 27, 2024 • edited Loading

codecov bot commented Nov 27, 2024 • 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

sonarcloud bot commented Nov 28, 2024

Quality Gate passed

github-actions bot commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading