#659 ✓ staged
David Seifried

create timeUpdate function

Reported by David Seifried | July 20th, 2011 @ 04:07 PM | in 0.8 (closed)

Currently, when the timeupdate listener is being triggered, an anonymous function is being called in order to run this code. This makes it difficult to explicitly call that functionality when needed, and may server better as its own Popcorn function, that we can access both internally and externally. The patch for this is included and I also changed one of the checks to be previousTime <= currentTime instead of just < in order for timeUpdates to trigger added track events when the video is paused. This will allow trackEvents that are added as the video is paused to be shown right away ( This is mainly needed for Butter). Prior to this we were setting currentTime += 0.000001 or something like that in order to trigger a time update, but we should not be using this for obvious reasons.

Tree: https://github.com/dseif/popcorn-js/tree/timeUpdate

Comments and changes to this ticket

  • Rick

    Rick July 20th, 2011 @ 04:59 PM

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

    Per discuss in IRC, let's keep this moving towards implementation

  • Scott Downe

    Scott Downe July 20th, 2011 @ 05:14 PM

    We need to revert the

    previousTime >= currentTime
    

    back to

    previousTime > currentTime
    

    , and add a third case, where

    previousTime === currentTime
    

    This case will be used to check if there is an event, that is not fired, that lives on the currentTime, without updating the internal indices.

    Three cases, forward, backward, stay the same. Only real difference, is the internal indices change in the direction we're moving.

  • David Seifried

    David Seifried July 21st, 2011 @ 11:41 AM

    Alright made the requested changes, heres the branch again https://github.com/dseif/popcorn-js/tree/timeUpdate

  • David Seifried

    David Seifried July 21st, 2011 @ 11:47 AM

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

    Rick July 21st, 2011 @ 12:05 PM

    • State changed from “peer-review-requested” to “under-review”
  • Scott Downe

    Scott Downe July 21st, 2011 @ 02:29 PM

    I noticed you still have "if ( previousTime <= currentTime ) {"

  • Scott Downe

    Scott Downe July 21st, 2011 @ 02:33 PM

    You're only checking for start in the ===. You should also check to see if end should be called.

  • David Seifried
  • Scott Downe

    Scott Downe July 21st, 2011 @ 02:38 PM

    We noticed teardown will be called in all cases where end my be needed. So I hardly doubt anything will be noticeable if end does not get called, but, to have a complete and consistent solution, I still think end should be called. We cannot make assumptions as to what they are using end for. In most cases end is used to remove visuals, which teardown will also do, but it's not up to us to decide that.

  • Scott Downe

    Scott Downe July 21st, 2011 @ 03:04 PM

    Another thing, I noticed you're copying the seek backwards logic, where start is checked to call end, and end is checked to call start.

    The forward logic is reversed.

    I think what you have to do here is assume the index pointers are wrong, tracks have changed, we can no longer trust their previous positions. Unless we can think of something, I think we have to set them to 0, and search our way through the array until we hit something greater, see if that event needs to be fired, and put the index on that position.

    To sum that up, we're using the forward logic on a standstill, because we're back at 0 index.

  • Scott Downe

    Scott Downe July 21st, 2011 @ 03:32 PM

    • Assigned user changed from “Rick” to “David Seifried”
    • State changed from “under-review” to “review-needs-work”

    I think I am hijacking this review. I'm going to r- it because of the above concerns.

    Rick, if you're in mid review, find anything else, or have better solutions to my above problems, note it and we'll add it to the list of things it needs.

  • David Seifried

    David Seifried July 21st, 2011 @ 05:47 PM

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

    Alright updated again, made requested changes
    https://github.com/dseif/popcorn-js/tree/timeUpdate

  • Rick

    Rick July 21st, 2011 @ 07:02 PM

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

    This is starting to take shape... A couple nit picks:

    Line #836 is indented too far

    Line #939 needs 2 spaces indent

    I diffed Line #839-870 and to #913-928: http://gyazo.com/6dcc9c997630b8fbbb7458bbeead2dea.png

    They are identical with the exception of 2 characters and a line of whitespace - that's a lot of code to carry dups of - we need to address this otherwise the code cannot land

    Additionally, the newly introduced proto method "timeUpdate" needs tests (the rest just needs to pass the existing tests)

  • David Seifried

    David Seifried July 22nd, 2011 @ 10:36 AM

    Alright cool thanks Rick, gonna make the changes now and put up for review shortly

  • David Seifried

    David Seifried July 22nd, 2011 @ 01:56 PM

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

    Alright got some unit tests now as well as made the requested changes
    https://github.com/dseif/popcorn-js/tree/timeUpdate

  • Scott Downe

    Scott Downe July 22nd, 2011 @ 03:22 PM

    • Assigned user changed from “Rick” to “Scott Downe”
    • State changed from “peer-review-requested” to “under-review”
  • Scott Downe

    Scott Downe July 29th, 2011 @ 02:07 PM

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

    Two failures in unit tests:

    p.data.trackEvents.endIndex is 0, expected: 0 result: 1, diff: 0 1
    p.data.trackEvents.startIndex is 0, expected: 0 result: 1, diff: 0 1

    and

    toggle() plugin: toggler is re-enabled

  • David Seifried

    David Seifried August 3rd, 2011 @ 03:08 PM

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

    Alright, changed the unit tests a bit to reflect the changes being made. Since a timeUpdate is being called now with each trackEvent added, some things like the pointer shifted without the time being updated (which is now expected behaviour after my changes). This also affected the toggler, so I added another test to ensure everything was working as expected.

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

  • Rick

    Rick August 4th, 2011 @ 12:34 AM

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

    Why did you change the toggle tests to do exactly the opposite of what they were currently testing? Whatever changes you make to the core need to pass the existing tests as they are, otherwise we risk breaking code in the wild. Please revert the test changes and correct your implementation as needed.

  • Rick

    Rick August 4th, 2011 @ 12:37 AM

    I'm not sure about the details of the other tests that were changed, but you dodnt update the messages in the tests, so they still print out the previous assertion message

  • David Seifried

    David Seifried August 5th, 2011 @ 12:32 PM

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

    Right, dont know what I was thinking about changing the toggle tests, my bad. Tho apparently, after adding my code that exec statement in toggle gets executed before

    
    $pop.exec( 40, function() {
    
        //  make sure toggler never happened
        // look for: "toggler-test"
    
        ok( !document.getElementById("toggler-test"), "No toggler container, disabled toggler plugin correctly never ran" );
        plus();
    
        $pop.toggle( "toggler" );
        ok( $pop.data.disabled.indexOf("toggler") === -1, "toggle() plugin: toggler is disabled" );
        plus();
    
      });
    

    gets executed before

    
      $pop.toggler({
        start: 40,
        end: 50
      });
    

    Where it did not happen like this before. I figure this is because of the added tests to Index integrity and the fact that every track event that is added is now calling timeUpdate. This is just a guess, but I compared with and without my code being there and that seems to be the difference. To stop that from happening, at the beginning of the toggler tests, I set the current time to 39 seconds, 1 second before all of the toggler tests should be run, I figure this should be more than enough time and seems to do the trick. Let me know if this is frowned upon in some way, or I am over looking something.

    As far as the messages for the other tests, I have changed them to what they should be.

    Tree: https://github.com/dseif/popcorn-js/tree/timeUpdate

  • Rick

    Rick August 10th, 2011 @ 01:12 PM

    • State changed from “peer-review-requested” to “under-review”
  • David Seifried

    David Seifried August 10th, 2011 @ 03:16 PM

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

    Alright, Rick helped me find a few issues that arose and came up with some fixes for them. Renamed some variables, giving them more meaningful names and fixed a few styling issues.

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

  • Rick

    Rick August 10th, 2011 @ 04:05 PM

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

    Great work! PR+

    Tested: ALL test suites;

    Passing in:

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

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

    Lint Passes

  • David Seifried

    David Seifried August 24th, 2011 @ 01:04 PM

    As per Scott and I's conversation, I removed the functionality to handle when the video is paused so currentTime and previousTime are equal, and just left in the part where I pulled out everything and put it in a seperate function. It is now on a new branch:

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

  • Scott Downe

    Scott Downe August 24th, 2011 @ 01:41 PM

    Yeah, I think we can break up the functionality into two tickets. Doing the === block of code in another ticket.

  • Jon Buckley

    Jon Buckley August 25th, 2011 @ 11:46 AM

    • Milestone order changed from “36” to “0”

    dseif, you need to rebase your patch, otherwise we'll be losing changes that Rick made in #675

  • Scott Downe

    Scott Downe August 25th, 2011 @ 04:19 PM

    • Assigned user changed from “Scott Downe” to “Jon Buckley”

    uh, some git shenanigans.

    This should be a merged branch, with both rick and dave's commits in history.

    https://github.com/ScottDowne/popcorn-js/commits/timeUpdate2

  • cadecairos

    cadecairos August 25th, 2011 @ 04:43 PM

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

    I checked out Scotts branch, it looks good. The changes from Rick's patch are included in Daves.

    All Core unit tests are still passing in Firefox 6 and Chrome 12.

  • Rick

    Rick August 25th, 2011 @ 06:08 PM

    Scott, you're smooth criminal.

    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

  • Jon Buckley
  • Rick

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

Pages