#429 ✓ staged
dseif

Allow content to remain after ended

Reported by dseif | March 18th, 2011 @ 03:51 PM | in 1.2 (closed)

It seems that the only way currently to keep the content on the page after a video has finished, is to set end to be > the length of the video. I don't know if it would be better if there were a keyword that could be used, something like end: never (as humph suggested).

Comments and changes to this ticket

  • Rick
  • dseif

    dseif May 10th, 2011 @ 02:17 PM

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

    Should this ticket be put up for review now? Or has it been staged already?

  • annasob

    annasob May 10th, 2011 @ 02:23 PM

    • State changed from “new” to “assigned”
    • Milestone set to 0.6
    • Assigned user set to “dseif”
    • Milestone order changed from “50” to “0”

    dseif feel free to review and see if it does what you want it to do.

  • dseif
  • Scott Downe

    Scott Downe May 10th, 2011 @ 03:03 PM

    To me, this looks like a problem with duration, same issue as ticket #432 maybe?

  • Scott Downe

    Scott Downe May 10th, 2011 @ 03:08 PM

    This is deficiencies in duration. I see what is happening in Rick's patch. Another idea is to use Number.MAX_VALUE if duration is not there.

  • annasob

    annasob May 10th, 2011 @ 11:21 PM

    • Assigned user changed from “dseif” to “David Seifried”
  • Scott Downe

    Scott Downe May 11th, 2011 @ 09:06 AM

    So I thought this over, and I think I am sold on end: false to function as not ending the track. Where as end not being defined means default to the end of the video, but still call the end track, but call it at the end.

    So:

    end: false // never calls end
    end: undefined // calls end at end of video
    

    I was just being stubborn on allowing attributes in options being polymorphic because I wanted to make sure we only did it for good reason.

  • David Seifried

    David Seifried May 11th, 2011 @ 10:02 AM

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

    Added changes so a track wont disappear now when you specify end: false. As far as leaving end: undefined, it will be solved in 432.

    Commit: https://github.com/dseif/popcorn-js/commit/23e616fe68aa69caaf1a585d...

  • Rick

    Rick May 13th, 2011 @ 05:49 PM

    dseif, I patched this back in march, see first comment.

  • Rick

    Rick May 13th, 2011 @ 06:13 PM

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

    Rick May 17th, 2011 @ 11:36 AM

    • Milestone changed from 0.6 to 0.7
  • Rick

    Rick June 18th, 2011 @ 11:39 AM

    • Milestone changed from 0.7 to 0.8
  • Rick

    Rick August 23rd, 2011 @ 12:47 PM

    • Milestone changed from 0.8 to 0.9
  • Rick

    Rick September 27th, 2011 @ 12:03 PM

    • State changed from “under-review” to “hold”
    • Milestone changed from 0.9 to 1.0 Release
    • Milestone order changed from “2” to “0”

    Using either David's or my own approach causes problems in varying tests, I'm putting this on hold for the time being

  • David Seifried

    David Seifried November 17th, 2011 @ 04:53 PM

    • Milestone changed from 1.0 Release to 1.2
    • Milestone order changed from “41” to “0”
  • Scott Downe

    Scott Downe January 31st, 2012 @ 05:28 PM

    Since ticket #481 landed, this is hard to reproduce, but given a end time that is === to the video's duration, the content still leaves.

    I think I have a super simple way to solve this: https://github.com/ScottDowne/popcorn-js/commit/9d9ba8d3ebeb87e9854...

    I have some trivial tests failing, but that can be sorted out.

    Mind if I take this Rick? To give it a go?

  • Rick

    Rick January 31st, 2012 @ 05:40 PM

    I have a pending patch for this... it's probably rotted beyond belief, but take a look at it anyway, I think you're solution is actually not going to work - you cant change the current semantics, only add to them by proactively preventing the end method from being called.

    https://github.com/rwldrn/popcorn-js/commit/69911578a477789c2e4d632...

  • Scott Downe

    Scott Downe January 31st, 2012 @ 07:57 PM

    Regarding both Rick and Dave's patches, I'm not sold on end: false

    It always felt weird to me, even at the start of this ticket I have been on the fence about it. :S

    It is also adding API features for a pretty trivial use case.

    I don't think preventing the end from firing is the solution. I just think we went about it wrong in the first place, it's probably more intuitive for the range to be inclusive on both ends. It solves the initial problem organically and intuitively. But... I agree, I was wrong in thinking we can just change the semantics; that is no different than changing the API. Good call.

    I vote we leave this as is for the duration of 1.0, and consider my requested change for 2.0.

    Definitely should keep this on hold pending more discussion.

  • Rick

    Rick January 31st, 2012 @ 08:37 PM

    We need three things:

    1. End fires at time (explicit)
    2. End fires at end of video (implied by omission of end property or by explicitly setting to the duration)
    3. End never fires (explicit)

    Today, we can do 1 & 2. My original patch provides 3, I don't think it's an edge case at all, I've had to work around it using the code plugin in the past. The idea of setting foo to "false" to prevent foo from happening is intuitive - this is actually how native DOM and other libs achieve similar behaviors.

    We can't do this by checking if the end time is greater then the duration, because very soon the Media Stream and Media Source APIs will eradicate the concept of a set duration.

    I'm not sure how else to explain this, considering I also don't think it's nearly as big a deal as its been made out to be.

    Also, we need to be more mindful of "2.0" and what that implies.

  • Scott Downe

    Scott Downe February 2nd, 2012 @ 09:29 PM

    I am just exploring other options, while I do consider end: false to be an option, I am not convinced yet enough have been explored.

    It is also not a huge deal, but it is a subtle, opinionated and complex problem of what is intuitive.

    I'm really only bringing this back up because I had time to think, and came up with another option.

    Hear me out.

    End fires at time (explicit) // still the supported expected behaviour.
    End fires at end of video (explicit by setting to the duration) // still the supported expected bahaviour
    End never fires (implied by omission of end property or by explicitly setting to false) // this is the only change.
    

    I would like to point out that after #481 landed in 0.7, my above change can sometimes, given a video with no duration, be the current behaviour. Maybe #481 is a mistake, and I am more than willing to explore that.

    Also, I would be curious to know if we documented what happens with end: undefined.

  • Rick

    Rick February 3rd, 2012 @ 09:59 AM

    The problem I have with end: undefined is that it has a meaning that I don't think matches what we're trying to express, see: http://es5.github.com/#x4.3.9

    undefined value - primitive value used when a variable has not been assigned a value.

    With regard to our decision, I'm trying to think of this as a sentence or story:

    1. "Should the end method fire? Yes. What time? 2"

    2. "Should the end method fire? Yes. What time? It doesn't matter, so defer to the end of the video"

    3. "Should the end method fire? No, never"

    As for your question about what does it do... currently it throws an exception:

    Uncaught TypeError: Cannot call method 'split' of undefined

    But that would be the case for anything we don't have explicit handling for, eg

    Uncaught TypeError: Object false has no method 'split'

    http://jsfiddle.net/rwaldron/UGU6w/2/

  • Scott Downe

    Scott Downe February 3rd, 2012 @ 12:03 PM

    Sorry, what I meant by what it does currently, I mean this line: https://github.com/cadecairos/popcorn-js/blob/develop/popcorn.js#L1407

          if ( !( "end" in options ) ) {
            options.end = options[ "out" ] || this.duration() || Number.MAX_VALUE;
          }
    

    Our current bahaviour makes it possible to default a non existing end to much after the end of the video ( Number.MAX_VALUE ). Which will never be fired.

    Yes, explicit undefined and false are not handled. Maybe they should be. I am ok with that. But I think we should consider anything falsy other than 0 to be "never play the end". Also, looking at your patch, you have logic for not playing an end. This might be better than the Number.MAX_VALUE.

    So, I think my suggested fix for this has evolved. It's not end: false I don't like. It is a non existing end being different than end: false. It is just not needed. We can be much simpler.

    We can still cover the three use cases you've laid out.

    We can fix the Number.MAX_VALUE hack with some logic for anything falsy but not 0. To clarify, we never need to rely on duration existing at this point, because we have something more explicit we can do. Non existing end, end: undefined, end: false, end: NaN, infinity, etc all never fire.

    If the user still wants it to end at end of video, they should explicitly enter the video's duration. It will still fire as expected.

  • Rick

    Rick February 3rd, 2012 @ 03:49 PM

    No matter what, it can't be a number, anything that JavaScript thinks is a number (NaN or Infinity) or even just relying on some kind of conditional comparison.

    In my patch you'll see that I had to create an actual hash to store "tracks that need to have their end event method prevented". It's just not as simple as making the end value something different - there has to be special case handling that knows how to prevent a track event from firing its end function, independently of the time and timeUpdate loop's conditions (however, that is where the logic of preventing the execution actually has to be).

    I suggest false because it's a logically observable value that is distinctly not "falsy" (falsy means things that are not actual booleans but will be evaluated the same as false: "", 0, NaN, null, undefined ) and not in the number family ( NaN, 0 - Number.MAX_VALUE, Infinity)

  • Scott Downe

    Scott Downe February 3rd, 2012 @ 04:42 PM

    Another simple option is changing:

          if ( !( "end" in options ) ) {
            options.end = options[ "out" ] || this.duration() || Number.MAX_VALUE;
          }
    

    To:

          if ( !options.end && options.end !== 0 ) {
            options.end = options[ "out" ] || Number.MAX_VALUE;
          }
    

    this.duration() is pretty unreliable for us anyway, and I've always wanted a better solution.

    This way our end will never be fired for both undefined and false. Which is the current case for a failed attempt at duration anyway.

    0 still works, not that 0 is a valid case, as it means the event will do nothing, but that should be the expected behaviour for 0.

    We also won't have any overhead with logic in the update code.

    If a user really wants a video to end at the end of the video, they should explicitly enter the desired time.

    Simple is my goal.

    I do have to say after each comment, my solution and options evolve, so on my end, I am finding this debate helpful.

  • Rick

    Rick February 3rd, 2012 @ 05:03 PM

    My concern here is that it changes an expected behaviour. Can you create a branch that actually uses the approach you last illustrated and has a couple of tests - you can make a standalone runner for this, if it's easier. Let's see how it works :)

  • Scott Downe

    Scott Downe February 3rd, 2012 @ 11:29 PM

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

    code and test.

    Note, I found a bug with my new test for this: #907. I will block this on that, but for example purposes, I included the fix in the above patch.

    This is a feedback requested, not a full review.

  • Rick

    Rick February 4th, 2012 @ 02:21 PM

    I've made some line comments on the commit at github.

    Aside from that, a few failures here, but they're in Base Player and Parser:

  • Scott Downe

    Scott Downe February 8th, 2012 @ 09:34 PM

    Sorry for the delay, been slowed down by a nasty cough. On it now though.

  • Scott Downe

    Scott Downe February 8th, 2012 @ 11:04 PM

    Interesting...

    I get that same baseplayer failure on develop.

    I took a quick look, and noticed Baseplayer's play method is not being called anymore.

  • Scott Downe

    Scott Downe February 9th, 2012 @ 10:57 AM

    Interesting, only seems to fail on develop when run alone.

  • Scott Downe

    Scott Downe February 9th, 2012 @ 11:08 AM

    If you comment out all tests above "Popcorn.disable/enable/toggle (timeupdate)" and below "Base player functionality" the baseplayer will pass, if you comment out "Popcorn.disable/enable/toggle (timeupdate)" it fails. I think I'm going to throw this in a new ticket.

  • Scott Downe

    Scott Downe February 9th, 2012 @ 11:11 AM

    In my patch, I added a test after "Popcorn.disable/enable/toggle (timeupdate)" which would make sense now why it is all of a sudden failing.

  • Scott Downe

    Scott Downe February 9th, 2012 @ 11:49 AM

    • State changed from “hold” to “blocked”

    Fixed the whitespace: https://github.com/ScottDowne/popcorn-js/commits/t429

    Sorry about that, I was not working on my own machine or my preferred editor that day.

    Parser tests were failing because of fragments left from the baseplayer test not passing.

    The baseplayer fail is fixed by #904, I will block this on that, as well as #907. See #910 as to why baseplayer was failing.

  • Rick

    Rick February 9th, 2012 @ 12:45 PM

    Sound good - glad you were able to get to the bottom of that. I'll review this once those other issues are resolved.

  • Rick

    Rick February 26th, 2012 @ 10:33 PM

    When this is unblocked, it is PR+

  • Jon Buckley

    Jon Buckley March 2nd, 2012 @ 05:08 PM

    • State changed from “blocked” to “super-review-requested”
    • Assigned user changed from “Rick” to “David Seifried”

    All three tickets are staged or review looks good, so this can be reviewed.

  • David Seifried

    David Seifried March 2nd, 2012 @ 05:30 PM

    The changes made here all look good, so SR+ from me once the other tickets land and I can run the test's with them passing.

  • David Seifried

    David Seifried March 4th, 2012 @ 12:01 PM

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

    The tests hang on the in/out alias' test, mind taking a look into this scott?

  • David Seifried

    David Seifried March 4th, 2012 @ 01:27 PM

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

    Jon can you give this a review? I'm seeing a weird hang on Chrome so it might be my chrome that is screwed.

  • Jon Buckley

    Jon Buckley March 4th, 2012 @ 02:07 PM

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

    I'm hanging on in/out alias with Chrome as well :/

  • Rick

    Rick March 4th, 2012 @ 04:19 PM

    Which Chrome are you using? Looking good on my end:

  • Scott Downe

    Scott Downe March 4th, 2012 @ 04:30 PM

    Odd. The thing they have in common is they are on a Mac.

    Do you not get failures when using develop?

  • Scott Downe

    Scott Downe March 4th, 2012 @ 04:31 PM

    I am on Chrome 17.0.963.56 m

  • Rick

    Rick March 4th, 2012 @ 05:23 PM

    I'm also on Chrome 17 - on a Mac

  • David Seifried

    David Seifried March 5th, 2012 @ 11:55 AM

    Rick are you running Lion? ( Jon and I are )

  • Rick

    Rick March 5th, 2012 @ 03:45 PM

    Nope, I'm on snow leopard. I can't imagine chrome having some massive difference by OS (especially when the OS is the same family, different release)

  • Jon Buckley

    Jon Buckley March 5th, 2012 @ 04:21 PM

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

    This is just Chrome being Chrome. If you manually pause and play the videos during the tests, all of the tests will pass. This gets an SR+ from Dave and I.

  • cadecairos

    cadecairos March 5th, 2012 @ 04:39 PM

    • State changed from “review-looks-good” to “staged”
    • Assigned user changed from “cadecairos” to “Scott Downe”
  • Rick
  • 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

Referenced by

Pages