Skip to content
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

Do not display negative time #2821

Closed
wants to merge 1 commit into from

Conversation

forbesjo
Copy link
Contributor

Sometimes at the end of an HLS video the remaining time is calculated as -1 (current time is greater than duration by a hundredth of a second, rounded to 1 second) causing the remaining time display to show "--1:0-1"

This PR sets negative seconds to 0 so that only a valid time or dashed time is displayed.

@gkatsev
Copy link
Member

gkatsev commented Nov 17, 2015

Makes sense to me. LGTM

@pam
Copy link

pam commented Nov 17, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 1556bfb
Build details: https://travis-ci.org/pam/video.js/builds/91626134

(Please note that this is a fully automated comment.)

@gkatsev
Copy link
Member

gkatsev commented Nov 24, 2015

I just realized that remaining time uses this as well:

const formattedTime = formatTime(this.player_.remainingTime());

We probably want a 3rd parameter to the function that determines whether it clips at 0 or not that we can use in duration and current time but it defaults to current functionality so it also isn't a breaking change.

@forbesjo
Copy link
Contributor Author

Remaining time is is a positive timestamp (duration - currentTime) with a '-' appended. I don't think formatTime is ever given a negative number because it would never return a correctly formatted timestamp.

@gkatsev
Copy link
Member

gkatsev commented Nov 24, 2015

oh, I see. Missed that.

@gkatsev gkatsev added the minor This PR can be added to a minor release. It should not be added to a patch release. label Nov 25, 2015
@gkatsev gkatsev closed this in 8acd28c Nov 25, 2015
@gkatsev gkatsev mentioned this pull request Nov 25, 2015
jgubman added a commit to jgubman/video.js that referenced this pull request Jan 27, 2016
* upstream/stable: (479 commits)
  v5.4.4
  @gkatsev switched to use custom vtt.js from npm. closes videojs#2905
  v5.4.3
  @gkatsev updated options customizer and github-release options. closes videojs#2903
  v5.4.2
  @gkatsev updated grunt-release config. closes videojs#2900
  v5.4.1
  @gkatsev added chg- and github- release for next releases. closes videojs#2899
  v5.4.0
  @gkatsev added ability to release next tag from master. closes videojs#2894
  @gkatsev added nullcheck for cues in updateForTrack. Fixes videojs#2870. closes videojs#2896
  @chemoish emulated HTMLTrackElement to enable track load events. closes videojs#2804
  @gkatsev added a Player#reset method. Fixes videojs#2852. closes videojs#2880
  @nick11703 changed multiline comments in sass with single-line comments. closes videojs#2827
  @gkatsev added Player#tech. Fixes videojs#2617. closes videojs#2883
  @misteroneill updated videojs-ie8 to 1.1.1. closes videojs#2869
  v5.3.0
  @imbcmdth added sourceOrder option for source-first ordering in selectSource. closes videojs#2847
  @forbesjo updated formatTime to not go negative. closes videojs#2821
  v5.2.4
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor This PR can be added to a minor release. It should not be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants