Feat: Forward/Rewind #40

thalysmcarrara · 2023-06-06T13:16:41Z

Description

implement forward/rewind functionality

Related Issues

Closes Implement Double Tap to FF or RWD #15

Progress

Pull request checklist

Tests: This PR includes tests for covering the features or bug fixes (if applicable).
Docs: This PR updates/creates the necessary documentation.
CI: Make sure your Pull Request passes all CI checks. If not, clarify the motif behind that and the action plan to solve it (may reference a ticket)

How to test it

Visual reference

Screenrecorder-2023-06-08-15-20-30-642.mp4

aelnonym · 2023-06-07T13:29:01Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -2,31 +2,48 @@ package com.profusion.androidenhancedvideoplayer.components

 import android.content.res.Configuration.ORIENTATION_LANDSCAPE
 import android.net.Uri
-import androidx.compose.foundation.clickable
-import androidx.compose.foundation.interaction.MutableInteractionSource
+import androidx.compose.animation.* // ktlint-disable no-wildcard-imports


I think we will have to disable this rule to reduce the number of imports, this file tends to be very large and therefore the number of imports too. in anova project we disabled this rule.

I agree with you @thalysmcarrara, but if you want to disable this rule, I'd prefer to disable it for the entire project

i can create a a new issue for that, because its not part of the current issue

I'm outvoted, but I'd vouch for us keep following google's code-style convention: https://developer.android.com/kotlin/style-guide#import_statements

You can disable it on the IDE: Settings > Editor > Code Style > Kotlin > Imports

It's not part of the current issue to add // ktlint-disable as well. We are not disabling rules in any part of the codebase. So, you either add a one-line configuration to the .editorconfig and remove the // ktlint-disable or adopt the ktlint rules of the project.

I added the lint rule in a .editorconfig file, I thought it would be more work, but it is a simple modification

androidenhancedvideoplayer/src/main/res/values-pt-rBR/controls_strings.xml

ricardodalarme · 2023-06-07T21:05:38Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -2,31 +2,48 @@ package com.profusion.androidenhancedvideoplayer.components

 import android.content.res.Configuration.ORIENTATION_LANDSCAPE
 import android.net.Uri
-import androidx.compose.foundation.clickable
-import androidx.compose.foundation.interaction.MutableInteractionSource
+import androidx.compose.animation.* // ktlint-disable no-wildcard-imports


I agree with you @thalysmcarrara, but if you want to disable this rule, I'd prefer to disable it for the entire project

...r/src/main/java/com/profusion/androidenhancedvideoplayer/repository/VideoPlayerRepository.kt

.../java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekClickableArea.kt

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/PlayerIcons.kt

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

...rc/androidTest/java/com/profusion/androidenhancedvideoplayer/test/EnhancedVideoPlayerTest.kt

.../java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekClickableArea.kt

...eoplayer/src/main/java/com/profusion/androidenhancedvideoplayer/utils/ExecuteAfterTimeout.kt

.../java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekClickableArea.kt

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekHandler.kt

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/PlayerIcons.kt

...rc/androidTest/java/com/profusion/androidenhancedvideoplayer/test/EnhancedVideoPlayerTest.kt

alanpazetto · 2023-06-08T13:17:03Z

.../java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekClickableArea.kt

+) {
+    val isTapCountGreaterThanZero = tapCount > 0
+    Box(
+        modifier = modifier


this code adds fillMaxHeight, indication, etc to modifier argument. However, this couldn't be the desired behavior...
in order to combine the modifiers, it should be something like this:

Box( modifier = Modifier. <do the stuff> .then(modifier)

hspedro · 2023-06-08T13:10:42Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -2,31 +2,48 @@ package com.profusion.androidenhancedvideoplayer.components

 import android.content.res.Configuration.ORIENTATION_LANDSCAPE
 import android.net.Uri
-import androidx.compose.foundation.clickable
-import androidx.compose.foundation.interaction.MutableInteractionSource
+import androidx.compose.animation.* // ktlint-disable no-wildcard-imports


I'm outvoted, but I'd vouch for us keep following google's code-style convention: https://developer.android.com/kotlin/style-guide#import_statements

You can disable it on the IDE: Settings > Editor > Code Style > Kotlin > Imports

hspedro · 2023-06-08T13:12:06Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -2,31 +2,48 @@ package com.profusion.androidenhancedvideoplayer.components

 import android.content.res.Configuration.ORIENTATION_LANDSCAPE
 import android.net.Uri
-import androidx.compose.foundation.clickable
-import androidx.compose.foundation.interaction.MutableInteractionSource
+import androidx.compose.animation.* // ktlint-disable no-wildcard-imports


It's not part of the current issue to add // ktlint-disable as well. We are not disabling rules in any part of the codebase. So, you either add a one-line configuration to the .editorconfig and remove the // ktlint-disable or adopt the ktlint rules of the project.

hspedro · 2023-06-08T13:17:30Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -64,7 +75,8 @@ fun EnhancedVideoPlayer(
        soundOff = soundOff,
        currentTimeTickInMs = currentTimeTickInMs,
        controlsCustomization = controlsCustomization,
-        settingsControlsCustomization = settingsControlsCustomization
+        settingsControlsCustomization = settingsControlsCustomization,
+        transformSeekIncrementRatio = { transformSeekIncrementRatio(it) }


Suggested change

transformSeekIncrementRatio = { transformSeekIncrementRatio(it) }

transformSeekIncrementRatio = transformSeekIncrementRatio

hspedro · 2023-06-08T13:19:15Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -113,6 +125,10 @@ fun EnhancedVideoPlayer(
        )
    }

+    fun setIsControlsVisible(value: Boolean) {


nit: wrong english for isControls (plural). I'd rename to setControlsVisibility(visible: Boolean)

hspedro · 2023-06-08T13:21:50Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -113,6 +125,10 @@ fun EnhancedVideoPlayer(
        )
    }

+    fun setIsControlsVisible(value: Boolean) {
+        isControlsVisible = value


oh, the variable name seems wrong as well. Nevermind

hspedro · 2023-06-08T13:24:53Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -166,6 +177,15 @@ fun EnhancedVideoPlayer(
                }
            }
        )
+
+        SeekHandler(
+            toggleIsControlsVisible = { isControlsVisible = !isControlsVisible },


Some functions you are defining in the composable call and others in the EnhancedVideoPlayer component: toggleIsControlsVisible and setIsControlsVisible for example.

Is there a specific reason for that? I'd vouch for us to pick one of them

I created the two functions because there are cases where I just want to toggle the variable value and there are cases where I explicitly need to set the variable value to false, I need to make a decision or pass the current value of the variable and add the logic inside the child component or create a function that already does this in the parent element.

So it's better to pass just the setIsControlsVisible and create a function with the toggle logic in the SeekHandler component

hspedro · 2023-06-08T13:25:25Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

+            exoPlayer = exoPlayer,
+            controlsCustomization = controlsCustomization,
+            setIsControlsVisible = ::setIsControlsVisible,
+            transformSeekIncrementRatio = { transformSeekIncrementRatio(it) }


Suggested change

transformSeekIncrementRatio = { transformSeekIncrementRatio(it) }

transformSeekIncrementRatio = transformSeekIncrementRatio

hspedro · 2023-06-08T13:30:35Z

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekHandler.kt

+            seekIcon = { controlsCustomization.rewindIconContent(it) }
+        )
+        Spacer(modifier = Modifier.weight(0.2f))
+        SeekClickableArea(


I'd add a parameter to disable or do not render this composable for the fast-forward if the playback has ended

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/PlayerIcons.kt

ricardodalarme · 2023-06-08T15:04:53Z

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt

@@ -166,6 +177,15 @@ fun EnhancedVideoPlayer(
                }
            }
        )
+
+        SeekHandler(
+            toggleIsControlsVisible = { isControlsVisible = !isControlsVisible },


So it's better to pass just the setIsControlsVisible and create a function with the toggle logic in the SeekHandler component

ricardodalarme · 2023-06-08T15:17:48Z

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekHandler.kt

+
+    val transition = rememberInfiniteTransition(TRANSITION_LABEL)
+
+    val scale = transition.animateFloat(


ditto about the label

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekHandler.kt

...hancedvideoplayer/src/main/java/com/profusion/androidenhancedvideoplayer/utils/JobsHolder.kt

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekHandler.kt

...hancedvideoplayer/src/main/java/com/profusion/androidenhancedvideoplayer/utils/JobsHolder.kt

Closes #15

ricardodalarme · 2023-06-08T17:19:20Z

.../java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/SeekClickableArea.kt

+                    onTap = if (isTapCountGreaterThanZero) {
+                        if (disableSeekClick) {
+                            null
+                        } else { { onSeekSingleTap() } }
+                    } else {
+                        { checkIfCanToggleIsControlsVisible() }
+                    },
+                    onDoubleTap = if (isTapCountGreaterThanZero) { null } else {
+                        if (disableSeekClick) {
+                            null
+                        } else {
+                            { onSeekDoubleTap() }
+                        }
+                    }
+                )


nit: now you have more than one state, so it would be better to use when instead of if 😅

now i need the if because this additional state needs to be nested 🤣

note that the check need to happen only when the isTapCountGreaterThanZero is true

Closes #15

* disable no-wildcard-imports rule * add insert_final_newline = true Closes #15

thalysmcarrara changed the base branch from main to feat/create-custom-scaffold June 6, 2023 13:17

ricardodalarme marked this pull request as draft June 6, 2023 13:20

thalysmcarrara self-assigned this Jun 6, 2023

thalysmcarrara force-pushed the feat/create-custom-scaffold branch from 06b49d9 to 68404bf Compare June 6, 2023 13:25

Base automatically changed from feat/create-custom-scaffold to main June 6, 2023 13:28

thalysmcarrara force-pushed the feat/forward-backward branch from 3022035 to b8f1a89 Compare June 7, 2023 12:58

thalysmcarrara changed the title ~~WIP~~ Feat: Forward/Rewind Jun 7, 2023

thalysmcarrara added goal Feature to be delivered by the end of the sprint enhancement New feature or request labels Jun 7, 2023

thalysmcarrara added this to the S04 milestone Jun 7, 2023

thalysmcarrara marked this pull request as ready for review June 7, 2023 13:13

aelnonym reviewed Jun 7, 2023

View reviewed changes

thalysmcarrara force-pushed the feat/forward-backward branch 7 times, most recently from d8f6129 to 7dabe86 Compare June 7, 2023 21:14

ricardodalarme reviewed Jun 7, 2023

View reviewed changes

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt Outdated Show resolved Hide resolved

ricardodalarme reviewed Jun 7, 2023

View reviewed changes

...yer/src/main/java/com/profusion/androidenhancedvideoplayer/components/EnhancedVideoPlayer.kt Outdated Show resolved Hide resolved

ricardodalarme reviewed Jun 7, 2023

View reviewed changes

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/PlayerIcons.kt Outdated Show resolved Hide resolved

...c/main/java/com/profusion/androidenhancedvideoplayer/components/playerOverlay/PlayerIcons.kt Outdated Show resolved Hide resolved