#641 ✓ staged
brianchirls

.enable and .disable should force immediate update of displayed tracks

Reported by brianchirls | June 30th, 2011 @ 03:29 PM | in 1.0 Release (closed)

If .enable or .disable is called while the media is paused or in the middle of a long track event, the state will not be reflected in the display until the media advances (or rewinds) far enough to be outside of the event, in the case of .disable, or inside the next event, in the case of .enable.

Currently, the event loop is only called by a "timeupdate" event, and the loop is skipped if the media currentTime has not changed. This will require allowing .enable, .disable and potentially other methods to call the update function directly and to force a refresh even when the media has not advanced.

Comments and changes to this ticket

  • Rick

    Rick July 1st, 2011 @ 01:03 PM

    • Milestone set to 0.8
    • State changed from “new” to “assigned”
    • Assigned user set to “Rick”
    • Milestone order changed from “70” to “0”

    I think I can make this work fairly painlessly. Patch & tests to follow

  • brianchirls

    brianchirls July 7th, 2011 @ 12:32 PM

    Excellent. Thank you. Looking forward to it.

  • Rick

    Rick July 8th, 2011 @ 02:02 PM

    (from [2ac9744d00406640b13aeac71523e542a8a72a7f]) [#641] trigger timeupdate event when enable/disable are called, include data about which was called in customdata. https://github.com/rwldrn/popcorn-js/commit/2ac9744d00406640b13aeac...

  • Rick

    Rick July 8th, 2011 @ 02:04 PM

    Ugh, it looks like my auto-whitespace-fix hook went bananas on this commit.

  • Rick

    Rick July 8th, 2011 @ 02:09 PM

    • State changed from “assigned” to “blocked”

    Blocked by #646

  • brianchirls

    brianchirls July 9th, 2011 @ 02:00 PM

    Thanks for jumping on this Rick. This will make Popcorn slightly more responsive, but it does not solve the problem. Two issues remain:

    1) If the video is paused, the timeupdate event will not do anything because previousTime < currentTime (as well as previousTime > currentTime) will evaluate to false.

    2) Popcorn will not enable/disable any event that is currently between start and end. This will require a fair bit of work.

    First, tracks.endIndex and tracks.startIndex need to back up to review any events that they missed. Second, the test conditions in those loops need to be changed to test whether each event is disabled.

    I'd suggest doing this whole process in .disable and .enable so as to avoid burdening the main loop with testing all this every time, when it should be a relatively rare event.

    To demonstrate, I've put together a quick and dirty test:
    http://code.chirls.com/popcorn-tests/641/

  • Rick

    Rick July 9th, 2011 @ 04:47 PM

    Thanks for the further detail, I'll work on this over the weekend

  • Rick

    Rick July 20th, 2011 @ 02:09 PM

    • State changed from “blocked” to “assigned”
  • Rick
  • Rick

    Rick July 20th, 2011 @ 04:59 PM

    Not quite, no review requested

  • Rick
  • Rick

    Rick July 20th, 2011 @ 05:35 PM

    • State changed from “assigned” to “blocked”

    Blocked #659

  • Scott Downe

    Scott Downe August 26th, 2011 @ 04:48 PM

    • Milestone changed from 0.8 to 0.9
    • Milestone order changed from “3” to “0”
  • Rick

    Rick August 31st, 2011 @ 12:45 PM

    • State changed from “blocked” to “assigned”
  • Rick

    Rick August 31st, 2011 @ 12:57 PM

    (from [8fcf2afcb1f25ffab5de5d945c2cbfea828906ab]) [#641] trigger timeupdate event when enable/disable are called, include data about which was called in customdata https://github.com/rwldrn/popcorn-js/commit/8fcf2afcb1f25ffab5de5d9...

  • Rick

    Rick August 31st, 2011 @ 01:01 PM

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

    brianchirls September 2nd, 2011 @ 10:24 AM

    We're almost there. But it looks like the update still won't happen if the video is paused because timeUpdate checks previousTime < currentTime or previousTime > currentTime.

    There needs to be some exception to this rule in timeUpdate. Perhaps a third parameter force or something attached to the event would do. I suspect we'll need a third section in that if chain because an iteration of timeUpdate when the play head is not moving would need to leave tracks.startIndex and tracks.endIndex unchanged.

  • David Seifried

    David Seifried September 6th, 2011 @ 09:25 AM

    Before the previous timeUpdate code landed we had a check in for if the previousTime === currentTime, but we removed it as it was a little out of scope for pulling out the timeUpdate function. We should probably create another ticket for adding that check back into timeUpdate.

    Also Rick, am I reviewing t641 ( no 641b or 641c on github ) ?

  • brianchirls

    brianchirls September 6th, 2011 @ 10:30 AM

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

    For the record, I don't see the point of making another ticket, since this is the very subject of this one and is mentioned explicitly in the request. I mean, log it however you want, but let's be careful not to let this one slip.

  • David Seifried

    David Seifried September 23rd, 2011 @ 11:46 AM

    • State changed from “review-needs-work” to “assigned”
    • Assigned user changed from “David Seifried” to “brianchirls”

    Brian, now that #708 has landed, is this still an issue?

  • brianchirls

    brianchirls September 23rd, 2011 @ 11:52 AM

    Thanks for checking in. Let me have a look at it and get back on it.

  • David Humphrey

    David Humphrey September 26th, 2011 @ 03:43 PM

    Brian, any update? We need to shut this release down asap.

  • brianchirls

    brianchirls September 26th, 2011 @ 03:53 PM

    Will run some tests right now.

  • brianchirls

    brianchirls September 26th, 2011 @ 07:11 PM

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

    Ok, done. Thanks for pestering me about this, humph.

    The proposed solution doesn't quite get there because the timeUpdate event hasn't been "listened" through Popcorn's event system, and the index needs to back up to look at old events that may be active.

    So here's an alternative solution:
    https://github.com/brianchirls/popcorn-js/commit/c572d943e300612fe2...

    You see it in action it here:
    http://code.chirls.com/mozilla/641.html

    Unfortunately, there doesn't appear to be any way to avoid looping through avg O(n/2) of the track events. I suppose you could check whether byEnd is shorter and decide which array to loop through and save a few instructions, but it's probably more code than it's worth.

    I switched <= back to < in the currentTime check. It solved #708, but it did not address this one, and it was causing timeUpdate to run every 16ms even if the video was paused and there were no updates. Now it should only run when something changes.

    I have this running in addition to the existing loop, not instead of it, because it's possible that some code could (enable|disable|addTrackEvent) and then update the time all in one function before the usual events call timeUpdate. So i wanted to make sure no events were missed.

    This code fails the "Popcorn Compose" test, but it appears to be because that test doesn't take #647 into account. A few other tests fail, but they pass when you run them individually.

    Looking forward to your feedback.

  • Scott Downe

    Scott Downe September 27th, 2011 @ 09:43 AM

    "I switched <= back to < in the currentTime check. It solved #708, but it did not address this one, and it was causing timeUpdate to run every 16ms even if the video was paused and there were no updates. Now it should only run when something changes."

    Is this problem when using timeUpdate and rAF? I suspect that is only an issue in rAF, because if the video is paused, the timeUpdate function should only get called if it is being called manually. I want to try hard to keep the timeUpdate signature the same.

    Any failing tests will need to be updated and fixed inside this ticket, unfortunately.

  • brianchirls

    brianchirls September 27th, 2011 @ 10:08 AM

    Yes, you're right. This has been bugging me, actually, because, while the whole loop is necessary for enable/disable, it's not necessary for addTrackEvent. And we need addTrackEvent to be as fast as possible for real-time editing in Popcorn Maker, because Butter deletes and re-creates an event every time it's added.

    So here's what I'm gonna do. I'll put back Dave's <= and change addTrackEvent back so it doesn't do the whole loop, and I'll change animate so it only calls timeUpdate when the time has changed. enable/disable will still do the whole loop. Any objections?

    The rest of those tests fail on the develop branch as well, so that's outside the scope of this ticket.

  • Rick

    Rick September 27th, 2011 @ 10:11 AM

    @brianchirls can you screenshot the tests that fail on the develop branch (without any changes) thanks

  • brianchirls
  • Rick

    Rick September 27th, 2011 @ 11:03 AM

    I wonder if #739 fixes those

  • Scott Downe

    Scott Downe September 27th, 2011 @ 11:04 AM

    "So here's what I'm gonna do. I'll put back Dave's <= and change addTrackEvent back so it doesn't do the whole loop, and I'll change animate so it only calls timeUpdate when the time has changed. enable/disable will still do the whole loop. Any objections?"

    Yeah, that sounds right. No objections from me.

  • David Seifried

    David Seifried September 27th, 2011 @ 11:08 AM

    Brian, #739 has been staged and should fix the fails you are seeing. Try running off of cadecairos' current develop branch. Im getting full pass on FF6 and Chrome 14.

  • David Seifried

    David Seifried September 27th, 2011 @ 11:09 AM

    Also with regards to the proposed fix, sounds good to me as well.

  • brianchirls

    brianchirls September 27th, 2011 @ 11:16 AM

    Well, that didn't work. It turns out simply running timeUpdate once from addTrackEvent with the <= in place doesn't always force an update, since startIndex is not always so predictable. I'm not gonna check this in, but if you want, you can see what I'm talking about here:

    http://code.chirls.com/mozilla/641.html
    (Make sure you shift-reload.)

    Another option is to make addTrackEvent do all the heavy lifting itself and skip timeUpdate altogether. That would mean calling .start and .frame and adding the event to the animating queue when appropriate. addTrackEvent knows exactly which event needs updating, so it's not really necessary to make timeUpdate go through the trouble of finding it again.

    It's a trade-off between a small performance gain and keeping the code simple and all in one place. It's a judgement call, and I'm agnostic about it, so which do you guys prefer?

  • brianchirls

    brianchirls September 27th, 2011 @ 11:24 AM

    Sorry, I forgot to describe how to replicate the problem on that latest example.

    1. Go here and shift-reload to clear cache: http://code.chirls.com/mozilla/641.html
    2. Play to about 4 seconds and then pause.
    3. Click "Add Words" button.

    Nothing happens.

  • David Seifried

    David Seifried September 27th, 2011 @ 11:29 AM

    Looking at this stuff now brian, thanks for the details on how to replicate.

  • brianchirls

    brianchirls September 27th, 2011 @ 11:31 AM

    Yup, merging in cadecairos/develop fixed the unit tests. Thanks guys.

  • Scott Downe

    Scott Downe September 27th, 2011 @ 01:47 PM

    That's a very odd bug.

    Putting this into firebug console works:

    var popcorn = Popcorn('#video');
    popcorn.words({
      start: Math.max(0, popcorn.currentTime() - 3),
      end: popcorn.currentTime() + 3,
      target: "target",
      text: "hello"
    });
    
  • Scott Downe

    Scott Downe September 27th, 2011 @ 01:52 PM

    So, I see a working case, and a bugged case. We need to find out what is different between the two and eliminate the differences until we have both working, or both failing.

  • brianchirls

    brianchirls September 27th, 2011 @ 02:09 PM

    The checked in code works.

  • Scott Downe

    Scott Downe September 27th, 2011 @ 02:12 PM

    "The checked in code works."

    I am not sure what you mean by this :(

  • brianchirls

    brianchirls September 27th, 2011 @ 02:30 PM

    It means the code on my 641 repo works.

    Please review my 11:16am comment about doing addTrackEvent separately.

  • Rick

    Rick September 27th, 2011 @ 03:40 PM

    I just tried to pull your 641 and hit merge conflicts:

    
    remote: Counting objects: 9, done.
    remote: Compressing objects: 100% (3/3), done.
    remote: Total 6 (delta 4), reused 5 (delta 3)
    Unpacking objects: 100% (6/6), done.
    From git://github.com/brianchirls/popcorn-js
     * branch            641        -> FETCH_HEAD
    CONFLICT (delete/modify): players/sequence deleted in HEAD and modified in e8960d86728067d0c27a713bf37b97981aa34202. Version e8960d86728067d0c27a713bf37b97981aa34202 of players/sequence left in tree.
    Auto-merging popcorn.js
    CONFLICT (content): Merge conflict in popcorn.js
    Recorded preimage for 'popcorn.js'
    Automatic merge failed; fix conflicts and then commit the result.
    
  • brianchirls

    brianchirls September 27th, 2011 @ 03:44 PM

    What are the conflicts in popcorn.js?

    I'm not sure what's up with players/sequence. That showed up in some other merge along the way somewhere. I think it's supposed to be somewhere else though. Any ideas what that is?

    Also, I forgot to push a commit to the server a few hours ago. I think it's the one that merges cadecairos's develop branch. I'll do that now.

  • Rick

    Rick September 27th, 2011 @ 03:50 PM

    We moved the sequence module from players/ to modules/, you're branch is out of date. The conflict is minor, but still requires you to rebase your branch, because your changes are out of sync

    <<<<<<< HEAD
    
        this.timeUpdate( obj, null );
    =======
        
        this.timeUpdate( obj, null, true );
    >>>>>>> e8960d86728067d0c27a713bf37b97981aa34202
    
  • David Seifried

    David Seifried September 27th, 2011 @ 03:54 PM

    Brian, im trying to reproduce what you are seeing without using your words plugin and I can't seem to do it. Ive created a simplified test file outlining the same functionality that you used and it seems to be working fine.

    Take a look here http://scotland.proximity.on.ca/dseif/popcorn-js/asdf.html

    I dont know why this is the case, but I cant reproduce it other than on your file.

  • Rick

    Rick September 27th, 2011 @ 04:00 PM

    I cleared the conflict just to get the branch working and saw the Popcorn.compose errors you had mentioned but those could be updated... Aside from that, no other errors. Despite a few style nitpicks and ideally some kind of perf test to see how much we are wagering with performance, I think this could be viable.

  • brianchirls

    brianchirls September 27th, 2011 @ 04:04 PM

    It seem to have caused some confusion here. I will clarify. Two examples below.

    http://code.chirls.com/mozilla/641.html
    This one now works. It should be the same as what's in my github repo, 641 branch.

    http://code.chirls.com/mozilla/641-bad.html
    This one does not work. It's similar to what's above, except that addTrackEvent does not set the third argument (force) to timeUpdate, and timeUpdate has previousTime <= currentTime, per dseif's #708 fix. This is just here to demonstrate the point. It's not in my repo. The page has instructions on how to replicate the problem.

  • brianchirls

    brianchirls September 27th, 2011 @ 04:08 PM

    @Rick, thanks for your tips about the branches. I'm still learning this part. I will look into figuring out how to rebase.

    Can you tell me exactly which repo I should be using as my base for the "develop" branch? I was using jbuck, but it didn't have the #739 fix.

    In the merge conflict, this is the correct line:

    this.timeUpdate( obj, null, true );
    
  • David Seifried

    David Seifried September 27th, 2011 @ 04:09 PM

    Thanks for clarifying that Brian, but im still wondering what exactly is causing this. Using ( for example ) the footnote plugin and the current develop branch, I created a similar scenario to yours and was unable to reproduce it. Is there something that i should be doing in my demo http://scotland.proximity.on.ca/dseif/popcorn-js/asdf.html in order to reproduce what you have done.

    I just want to be able to get a concrete example of what is going on here by creating another test to show what is wrong.

  • Rick

    Rick September 27th, 2011 @ 04:13 PM

    @brian, Chris De Cairos' repo is our dev upstream, this will make adding it easy (will remove your old upstream and add new)...

    git remote rm upstream && git remote add upstream git://github.com/cadecairos/popcorn-js.git

    Confirmed, that is the line i chose to resolve the conflict with

  • brianchirls

    brianchirls September 27th, 2011 @ 04:16 PM

    Regarding the perf test, I don't have time to run that today - maybe tomorrow. It'd be hard to do accurately without recreating the whole loop in jsperf.

    But I did look around at some existing tests looping through arrays, and they seem to run on the order of several hundred thousand or a million operations (read, compare, increment) per second, which at first glance seems fast enough for interactivity. In a worst-case scenario, where you are looping through the entire queue of events without updating any of them, you're not running any user code at all, any DOM operations or any network/disk/device I/O - just going through our own index array.

    Still, I'd like to run a test. I'm open to any suggestions on that.

  • Scott Downe

    Scott Downe September 27th, 2011 @ 09:31 PM

    Ideally (to me), the loop should only happen if the video has changed, or one pass if timeUpdate was explicitly called, including when rAF calls it, without having a state variable being passed in (this is a last resort).

    Would it be possible to keep the <= check inside timeupdate, and before timeUpdate is called inside rAF, do a quick check if the time has changed?

  • brianchirls

    brianchirls September 27th, 2011 @ 11:23 PM

    Yes, I think you're right that the loop should only happen when absolutely necessary.

    I put this stuff in timeUpdate for the purpose of keeping all the timeline code together, but I think it's not really appropriate. So here's what I'd like to do.

    1. Revert timeUpdate back to the way it was before.
    2. Move all the "force" loop stuff into its own internal/private function that can be called by disable, enable and any function that needs it in the future.
    3. Make addTrackEvent directly update only the newly created track.

    As I think about it, there's no real need to put any of that new stuff inside timeUpdate, because it never actually needs to run at the same time as the rest of what's already in there.

    And we don't need <= in timeUpdate because it doesn't solve the problem in all cases. Organized this way, timeUpdate will really be specifically for updating the time.

    I'll code this up and submit it to you guys for review in the morning. Thanks everyone for the helpful feedback.

  • David Seifried

    David Seifried September 28th, 2011 @ 03:23 PM

    Any update on this brian? Ideally wed like to wrap up everything in this release today if possible and begin testing tomorrow.

  • brianchirls

    brianchirls September 28th, 2011 @ 03:43 PM

    Yup. The bulk of the code is done. I'm stepping through some unit tests right now. Hope to commit soon.

  • cadecairos

    cadecairos September 29th, 2011 @ 11:08 AM

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

    David Seifried September 29th, 2011 @ 01:14 PM

    Brian, is this going to be done soon? We are starting to test the release now, and if this is going to land its going to have to be by end of day today.

  • brianchirls

    brianchirls September 29th, 2011 @ 04:49 PM

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

    Hi everyone. Sorry for the delay. I had a hell of a time tracking down some unit test failures.

    The latest code is here:
    https://github.com/brianchirls/popcorn-js/commit/93fc98bad7cd7a1cb4...

    This does everything I described in my post yesterday. To refresh your memory:
    1. Revert timeUpdate back to the way it was before.
    2. Move all the "force" loop stuff into its own internal/private function that can be called by disable, enable and any function that needs it in the future.
    3. Make addTrackEvent directly update only the newly created track.

    Some notes on the unit tests...

    Popcorn Compose is failing. As I said before, I think this has to do with #647.

    Update Timer (frameAnimation) is timing out. I'm having a bit of trouble tracking down exactly what the cause is here, because it passes when I run it alone or when I step through it with a debugger. I think it's a separate issue from this one and probably has something to do with when timeUpdate is called, because the queues and indexes all seem to be in the right place. There are two versions of this test - one for frameAnimation and one for the regular timeupdate event. If I switch the order around, whichever one runs second is the one that fails.

    A whole bunch of tests that fail after this one. As far as I can tell, they all fail because Update Timer doesn't clean out its registry if it times out. I submitted ticket #749 about this one.

  • David Seifried

    David Seifried September 29th, 2011 @ 05:37 PM

    • Assigned user changed from “brianchirls” to “David Seifried”
  • Rick

    Rick September 30th, 2011 @ 12:01 AM

    Are there any tests to support all of these changes? When I comment out all of the code you added to Popcorn.addTrackEvent() and restore its this.timeUpdate( obj, null, true ); call, the test suite passes completely.

  • brianchirls

    brianchirls September 30th, 2011 @ 12:07 AM

    That's a good idea. It's helpful to know that restoring that code fixes the tests, but it doesn't fix the functionality. I'll see what I can do about converting my demo into a unit test.

  • David Seifried

    David Seifried September 30th, 2011 @ 01:26 AM

    After reviewing this over a bit, a few things are sticking out so far:

    • As Rick mentioned we need to get the tests back in working order before we continue along with this. We cant push this along any further in the review process as long as they are failing, so that is going to need to be addressed.
    • There are a few style problems with spacing and indentation that can be fixed ( 795 & 802 braces need to be indented more, 798 can use some white space padding around the if, and a few more like this )
    • There are alot of object properties that are being accessed > 1 time. We should store a reference in cases like this for a nice performance gain.
    • A lot of the functionality that you added seems to be pretty much identical to what was in timeUpdate, i dont see the reason to be doing this vs calling timeUpdate itself
    • You will need to update the index again after stating that a trackEvent is running and what not, we do this at the bottom of timeUpdate
    • RefreshTimeline seems to be a lot of the same logic that was in addTrackEvent and timeUpdate reused again. End and byEnd are decalared but never used. Seems like the logic for end maybe never got added? If this is the case this will be looking more and more familiar to what timeUpdate is like currently.
    • Looks like players/sequence has been revived somehow and got dragged along with your commit

    I think figuring out the unit test failures and getting those back up to par should be our main priority for now.

  • brianchirls

    brianchirls September 30th, 2011 @ 01:37 AM

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

    Excellent feedback, Dave. Thanks.

    Honestly, I just need a few hours away from this to come back fresh and figure out the unit tests. I'll make it happen.

    I will clean up the white space and the unused variables. That's easy enough.

    Yes, at first glance, there appears to be a lot of duplicated logic - that's the approach reflected in my 641 branch (not to be confused with 641a). But there are subtle differences in all these functions, which are necessary for a) covering all the weird cases and b) avoiding unnecessary loops and searches.

    Will first get the bugs fixed and then optimize object references. But good eye here.

    You will need to update the index again after stating that a trackEvent is running and what not, we do this at the bottom of timeUpdate

    Yes, the indexes (indices?) should all work out at the end, but I see that I'm doing it in the wrong order - updating the index BEFORE running .start (and .frame), when it should be the other way around. I was used to the old timeUpdate that updated the index a bit at a time, rather than all at once at the end. I wouldn't be surprised if this was the cause of the problem.

    Will see what I can do about it tomorrow.

  • brianchirls

    brianchirls September 30th, 2011 @ 01:38 AM

    re: white space, do you guys have some kind of parser for this? Or do you just eyeball it?

  • Rick

    Rick September 30th, 2011 @ 09:25 AM

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

    with regard to whitespace, we're just following the styleguide here https://webmademovies.lighthouseapp.com/projects/63272/styleguide but yes, eyeballs and fingers

  • David Seifried

    David Seifried September 30th, 2011 @ 11:53 AM

    • Milestone changed from 0.9 to 1.0 Release
    • Milestone order changed from “8” to “0”

    This is going to have to be pushed to 1.0, as we can't delay the release any longer.

  • cadecairos

    cadecairos October 24th, 2011 @ 02:56 PM

    How's this coming along Brian?

  • brianchirls

    brianchirls October 26th, 2011 @ 04:05 PM

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

    Ok, made some progress here. Started a new branch (641b), based on cadecairos/develop to make sure I didn't miss any of the 0.9 updates.

    Latest code is here:
    https://github.com/brianchirls/popcorn-js/commit/233c0a75edd01f818e...

    I still need to convert the example on my site to a unit test, but have to get some other things done first.

    Update Timer test is fixed, but we have a new problem with Base Player Functionality. It's running old code from a prior test that didn't destroy its popcorn instance. Index Integrity has an exec at 42 seconds, which runs again later in the test. That part actually makes sense. What's weird is that the base player test should be setting currentTime to 3 seconds, but the player doesn't actually respond to the base player commands. If you watch the test, the video just keeps playing past 40s.

    You can see what I mean more clearly if you try to run the Popcorn Player tests by themselves - the video never plays. I don't know the base player stuff at all, so I can't debug this.

    Little help?

  • Rick

    Rick October 26th, 2011 @ 06:30 PM

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

    I've made some line comments on the commit, but I want Scott to take a look at the approach as well

  • brianchirls

    brianchirls October 26th, 2011 @ 06:42 PM

    Thanks for pointing those out. I thought I had caught most of the whitespace points, but I guess not.

    Updated here:
    https://github.com/brianchirls/popcorn-js/commit/d924a487ef7ffc9381...

  • Rick
  • Scott Downe

    Scott Downe October 28th, 2011 @ 01:02 PM

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

    Can we make refresh internal and possibly expose it after more thought after 1.0? That's what my instincts tell me.

    Does it have any external use cases?

    I also want to explore making timeupdate handle this logic, after 1.0.

    Also, you pasted your commit, not the branch. You have three branches, 641, 641a and 641b. For next time can you add the branch in the review requested message: https://github.com/brianchirls/popcorn-js/commits/641b when you have multiple possible branches that it could be. 641b is the branch.

    I really just don't want to add something to the API this close. But, I feel we can safely make the functionality internal, and expose it later.

  • brianchirls

    brianchirls October 28th, 2011 @ 01:45 PM

    Yes, yes. It's better internal. I don't see how you'd need it to be exposed, since calling enable/disable or adding an event should cover it. I'm not sure how that got in there. I'll fix it.

    We already tried making timeupdate do it, but it was inefficient or ineffective. But feel free to take another crack at it.

    Yes, 641b. I'll post the branch links from now on.

  • Scott Downe

    Scott Downe October 28th, 2011 @ 03:11 PM

    "But feel free to take another crack at it."

    Yeah, once 1.0 settles a bit, and once I settle a bit, I'm going to try to do a bunch of housekeeping on timeupdate.

  • brianchirls

    brianchirls October 28th, 2011 @ 05:05 PM

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

    Moved 'refresh' into a private function so it's no longer exposed on the global Popcorn object.

    https://github.com/brianchirls/popcorn-js/commits/641b

  • Scott Downe

    Scott Downe October 29th, 2011 @ 05:08 PM

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

    Everything looks good now.

    Passing lint.

    Passing tests.

  • cadecairos

    cadecairos October 30th, 2011 @ 07:42 PM

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

    I'm seeing failures in the unit tests in Firefox 7, Chrome 15 and Safari 5.1:

    test 44: Popcorn Compose:

    8: one compose running, expected: 1 result: 0, diff: 1 0
    9: one effect running, expected: 1 result: 0, diff: 1 0

    test 67: Popcorn Player: Base Player functionality:

    2: $pop.data.trackEvents.byStart.length is 4 - after play, before removeTrackEvent, expected: 4 result: 3, diff: 4 3
    3: $pop.data.trackEvents.startIndex is 2 - after play, before removeTrackEvent, expected: 2 result: 1, diff: 2 1
    4: $pop.data.trackEvents.endIndex is 2 - after play, before removeTrackEvent, expected: 2 result: 1, diff: 2 1

    5: $pop.data.trackEvents.startIndex is 1 - after removeTrackEvent, expected: 1 result: 0, diff: 1 0
    6: $pop.data.trackEvents.endIndex is 1 - after removeTrackEvent, expected: 1 result: 0, diff: 1 0

  • brianchirls

    brianchirls October 30th, 2011 @ 08:11 PM

    Yes, please see my notes on these above.

    I will take another look at Compose. But I need to talk through the base player issue with someone because I don't understand what base player is or what it does, and there's some weirdness happening. Unless someone can point me to some documentation on that.

  • cadecairos

    cadecairos October 30th, 2011 @ 09:08 PM

    #647 has landed, and the test for it is still passing, I think it's coincidental that those compose tests are failing (they start at 0)

    The baseplayer creates an object that keeps time, so that Popcorn can trigger events using it. It pretty much "fakes" a media element.

    Scott wrote the baseplayer and its tests, he's the best person to ask about how his tests work.

  • cadecairos

    cadecairos October 30th, 2011 @ 09:16 PM

    hmm those fails in baseplayer are leaking from another test: Index Integrity ( timeUpdate )

  • Jon Buckley

    Jon Buckley October 31st, 2011 @ 01:58 PM

    I probably fixed the compose plugin fails in #799. Chris, can you verify?

  • Jon Buckley

    Jon Buckley November 1st, 2011 @ 09:39 AM

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

    cadecairos November 1st, 2011 @ 11:48 AM

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

    Alright with #799 and #800 All tests pass no problem.

    Core tests Passing in:

    Firefox 7, Chrome 15, Safari 5.1, Opera 11.52 ( ignoring known locale and position fails)

    SR+

  • cadecairos

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