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

Update duration and remaining time displays on durationchange #3349

Conversation

MattiasBuelens
Copy link
Contributor

Description

The duration display and remaining time displays do not update when the tech dispatches a durationchange event without a timeupdate event. This PR ensures that these displays bind to durationchange to update their content.

There is a comment in DurationDisplay saying that this change should be done once a previous bug was resolved with the way player.duration() is updated on durationchange. I believe this bug was fixed with #2552, so I think it should be safe to do this change now.

Problem

In the current version, if a Tech updates its duration and only fires a durationchange event (without firing a timeupdate event), the time displays are currently not updated properly.

Steps to reproduce

  1. Create a custom Tech, for example based on TechFaker
  2. Set the tech's featuresTimeupdateEvents to true (to prevent the player from triggering interval-based timeupdate events)
  3. Make the tech update its duration and dispatch a durationchange event
  4. Notice the duration display and remaining time displays

This page demonstrates the issue when you hit the play button: http://jsbin.com/gedoxah/edit?html,output

Expected results

  • The duration display and remaining time display update their content with the new duration.

Actual results

  • The duration display and remaining time display are not updated with the new duration.

Specific Changes proposed

  • Add durationchange listeners in DurationDisplay and RemainingTimeDisplay
  • Remove the timeupdate and loadedmetadata listeners in DurationDisplay (since they are no longer needed)

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@MattiasBuelens MattiasBuelens changed the title Listen for durationchange event to update duration display and remain… Update duration and remaining time displays on durationchange May 31, 2016
@gkatsev
Copy link
Member

gkatsev commented May 31, 2016

Yeah, I think you may be right that we can just use durationchange.
LGTM.

@gkatsev gkatsev added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels May 31, 2016
@gkatsev gkatsev added minor This PR can be added to a minor release. It should not be added to a patch release. and removed patch This PR can be added to a patch release. labels Jun 20, 2016
@brandonocasey
Copy link
Contributor

LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Jul 18, 2016
@gkatsev gkatsev closed this in e2bfe09 Jul 18, 2016
@MattiasBuelens MattiasBuelens deleted the time-display-durationchange branch July 24, 2016 10:57
gkatsev pushed a commit that referenced this pull request Oct 17, 2016
There was a potential breakage that was caused by #3349. This restores the timeupdate and loadedmetadata listeners in the duration display that were removed. As part of 6.0, they could be removed as durationchange should be the correct and only listener we need.
gkatsev added a commit that referenced this pull request Oct 18, 2016
…#3682)

There was a potential breakage that was caused by #3349. This restores the timeupdate and loadedmetadata listeners in the duration display that were removed. As part of 6.0, they could be removed as durationchange should be the correct and only listener we need.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed 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