#708 ✓ staged
David Seifried

timeUpdate function should be able to handle conditions when the video is paused

Reported by David Seifried | September 6th, 2011 @ 09:38 AM | in 0.9 (closed)

Originally before #659 got staged we had code in there to check if the currentTime and previousTime were equal to each other and if they were, act accordingly. We decided that this functionality would be better handled in another ticket, as #659 was more about pulling out the function.

This also fixes issues in Popcorn maker about events being fired when the video is paused as well as #641 with .enable and .disable.

Comments and changes to this ticket

  • David Seifried

    David Seifried September 7th, 2011 @ 01:27 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “Rick”
  • Rick

    Rick September 7th, 2011 @ 02:54 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “Rick” to “cadecairos”

    This is awesome! PR+

    (We also tried a few tweaks to see what we could do with calling trigger("timeupdate") )

    Tested: core test suite;

    Passing in:

    • FF 3.6 (stable), 6.x (stable), 7.x (Aurora), 8.x (Nightly)

    • Chrome 13 (Stable), 14 (Beta), 15 (Canary)

    Lint Passes

  • cadecairos

    cadecairos September 7th, 2011 @ 03:42 PM

    • State changed from “super-review-requested” to “review-needs-work”

    Dave is looking into the +0.25 on this line to see if we can get rid of it.

  • cadecairos

    cadecairos September 7th, 2011 @ 03:47 PM

    • Assigned user changed from “cadecairos” to “David Seifried”

    setting to dseif..

  • David Seifried

    David Seifried September 7th, 2011 @ 05:59 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Alright I think I have a better fix for this than the 0.25 second addition. It was wrong of us to modify the core like that, as the problem lied in the unit tests. It seemed that it was playing, then we were going through all of that code, and then we reset the time back to 0 to actually get those execs to be fired. When we didnt pause it the first time around we left ourselves open to possibly having those execs fire, then we reset the time, then them firing again. This was the case ( or from what I can tell at least ). Lemmie know if this fix seems correct to you guys.

    https://github.com/dseif/popcorn-js/tree/708

  • Rick

    Rick September 7th, 2011 @ 06:05 PM

    • State changed from “peer-review-requested” to “super-review-requested”

    YES! So glad you insisted on revisiting this. Just ran the test suite - works like a charm. PR+ still stands

  • David Seifried

    David Seifried September 8th, 2011 @ 10:03 AM

    • Assigned user changed from “cadecairos” to “Scott Downe”

    Assigning to scott as he's got the whole day to review stuff.

  • Scott Downe

    Scott Downe September 8th, 2011 @ 01:22 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “Scott Downe” to “David Seifried”

    So far, my only complaint is the location of the tests.

    The check for timeUpdate's existence should be in "2. Popcorn API: Popcorn.* Static Methods" and testing the functionality of the === case should be in "Popcorn Plugin: Update Timer" tests. I am also open to the option of doing the === test in a new module like it is, but "timeUpdate functionality" is a bad name.

  • David Seifried

    David Seifried September 13th, 2011 @ 12:08 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “David Seifried” to “Scott Downe”

    Alright fixed like you asked scott.

    https://github.com/dseif/popcorn-js/tree/708

  • David Seifried

    David Seifried September 13th, 2011 @ 12:11 PM

    • State changed from “peer-review-requested” to “super-review-requested”
  • cadecairos

    cadecairos September 16th, 2011 @ 10:45 AM

    • Assigned user changed from “Scott Downe” to “cadecairos”
  • cadecairos

    cadecairos September 16th, 2011 @ 01:01 PM

    • State changed from “super-review-requested” to “review-looks-good”

    Okay, lets get this moving!

    Code looks great, unit tests make sense.

    Tested on Firefox 6,7,8 and Chrome 13, all passing.

    Passes lint check.

    SR+

  • cadecairos

    cadecairos September 16th, 2011 @ 01:28 PM

    • State changed from “review-looks-good” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “David Seifried”

    Dave can yo rebase this off of the latest develop branch :D

  • David Seifried

    David Seifried September 22nd, 2011 @ 03:15 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Alright chris this should be fine to merge in now.

    Maybe just take another look over it just to be safe.

  • cadecairos

    cadecairos September 22nd, 2011 @ 04:03 PM

    • State changed from “super-review-requested” to “staged”

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages