-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add fast-forward and rewind buttons to seek video on tapping #4649
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Incremental code coverage: 68.89% |
8e05215
to
0b946cf
Compare
@@ -109,6 +111,14 @@ shaka.extern.UIVolumeBarColors; | |||
* should be part of the UI. | |||
* @property {boolean} customContextMenu | |||
* Whether or not a custom context menu replaces the default. | |||
* @property {boolean} fastForwardOnTaps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have implemented the ui.Element interface, why not configure them the way you do other elements, for example, in the controlPanelElements
array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would want to keep the fast-forward and rewind buttons linked though, wouldn't we? Technically, I think we could use a factory like how other elements are configured.
Also, I might be wrong but I do not think that will work since we need to add these elements to controlsContainer_
, yet, the parent element for the factory is controlsButtonPanel_
.
@joeyparrish @FireMasterK |
@@ -831,6 +841,44 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { | |||
svg.appendChild(spinnerCircle); | |||
} | |||
|
|||
/** | |||
* Add fast-forward button on Controls container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think "fast forward" is the right term for this.
"Fast forward" usually refers to something that changes the playback rate of the video; that's what the element that our UI controls internally refers to as fast_forward
does.
This is more often called seeking, skipping, or sometimes scrubbing (I think scrubbing is mostly used for seeking via the bar? Not 100% on that, terminology-wise).
// because of previous touch event. | ||
this.hideFastForwardButtonOnControlsContainerTimer_.stop(); | ||
this.lastTouchEventTimeSet_ = Date.now(); | ||
this.fastForwardValue_.textContent = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might look better if you add a '+' if it's >0.
Like, so it goes from '0s' to '+5s' to '+10s', to match how the opposite goes from '0s' to '-5s' to '-10s'.
this.hideFastForwardButtonOnControlsContainerTimer_.stop(); | ||
this.lastTouchEventTimeSet_ = Date.now(); | ||
this.fastForwardValue_.textContent = | ||
(parseInt(this.fastForwardValue_.textContent, 10) + 5).toString() + 's'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how this is so significantly based on reading the textContent of the element. I'd prefer it to just be tracked via a member variable of the object.
If you want to make sure the text and the variable are always the same, you could make a helper method that sets both at once. That could also handle adding the plus sign, as per my other suggestion.
* @final | ||
* @export | ||
*/ | ||
shaka.ui.HiddenFastForwardButton = class extends shaka.ui.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other button are almost the exact same code-wise. You could probably make a super-class that handles the basic functionality, and just have these classes pass in a few values: the icon code (FAST_FORWARD or REWIND), the step size (+5 or -5), and the aria label.
@@ -71,6 +71,8 @@ shaka.extern.UIVolumeBarColors; | |||
* addSeekBar: boolean, | |||
* addBigPlayButton: boolean, | |||
* customContextMenu: boolean, | |||
* fastForwardOnTaps: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like these could be an optional number rather than a boolean, and the number describes how far they go with one tap.
So like:
null
means the feature is disabled.
5
means it jumps in intervals of 5s.
10
means it jumps in intervals of 10s.
Etc etc.
You could pass this value in to the element during creation time, which should work well with the suggestion I made about the superclass.
It's relevant so that the developer can have the seeking intervals be different from each other. For example, a podcast app I use for iOS has the seeking intervals of -10s and +45s. I've also seen apps that seek forward a small amount but back a large amount.
.add('shaka-forward-rewind-on-controls-container-icon'); | ||
this.fastforwardIcon_.textContent = | ||
shaka.ui.Enums.MaterialDesignIcons.FAST_FORWARD; | ||
this.fastforwardContainer_.appendChild(this.fastforwardIcon_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These classes should probably have aria labels?
hideFastForwardButtonOnControlsContainer() { | ||
// prevent adding seek value if its a single tap. | ||
if (parseInt(this.fastForwardValue_.textContent, 10) != 0) { | ||
this.video.currentTime = this.controls.getDisplayTime() + parseInt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only seeking when the element fades away is probably the most efficient way to do this performance-wise, but I feel like every app I see that has this functionality seeks immediately on tap.
* @private | ||
*/ | ||
onFastForwardButtonClick_() { | ||
// this stores the time for first touch and makes touch valid for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an unnecessarily complicated explanation for this conditional.
I would just say something like:
// The first tap causes the element to become visible. Subsequent taps seek.
} else if (this.lastTouchEventTimeSet_+1000 > Date.now()) { | ||
// stops hidding of fast-forward button incase the timmer is active | ||
// because of previous touch event. | ||
this.hideFastForwardButtonOnControlsContainerTimer_.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling stop()
here is not necessary. When you call tickAfter()
at the end of this clause, that function also stops the current timer.
this.triggeredTouchValid_ = true; | ||
this.lastTouchEventTimeSet_ = Date.now(); | ||
this.hideFastForwardButtonOnControlsContainerTimer_.tickAfter(1); | ||
} else if (this.lastTouchEventTimeSet_+1000 > Date.now()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this.lastTouchEventTimeSet_
? When the timer ticks, after 1000ms, it causes this.triggeredTouchValid_
to be set to false. So there will never be a situation where this.triggeredTouchValid_
and also this.lastTouchEventTimeSet_+1000 < Date.now()
, right?
@FireMasterK can you review the latest comments? Thanks! |
Apologies, I will be working on this PR in a few days! |
@FireMasterK can you review it please? |
Closing due to inactivity. If you need to reopen this PR, just put @shaka-bot reopen in a comment. Thanks! |
Closes #3357
This is a rework/continuation of #3373