#654 ✓ staged
brianchirls

Popcorn should give access to `paused` property of media

Reported by brianchirls | July 18th, 2011 @ 08:02 PM | in 0.8 (closed)

This is an easy fix. In Popcorn.extend, just add paused to methods.

Comments and changes to this ticket

  • Rick

    Rick July 18th, 2011 @ 09:50 PM

    Popcorn.extend is a generic method, do you mean a specific instance of the method being called? Also, paused makes for an awkward method... I think this adds additional credence to my idea of creating a Popcorn media-element-specific attr() method

  • Rick

    Rick July 18th, 2011 @ 09:51 PM

    • State changed from “new” to “blocked”

    Blocked by #648

  • brianchirls

    brianchirls July 18th, 2011 @ 09:57 PM

    Sorry, my bad. The change goes right here: [https://github.com/jbuck/popcorn-js/blob/develop/popcorn.js#L436]

    Yes,I agree that paused is a weird name, but it is part of the HTML5 spec as a property on all media elements.

  • Rick

    Rick July 18th, 2011 @ 10:44 PM

    Right, I'm very well aware that it's part of the spec, but it's a property "getter" only and offers no setter value otherwise. It makes more sense to make all properties and attributes accessible with a single point method.

  • brianchirls

    brianchirls July 19th, 2011 @ 09:58 AM

    I can definitely see the value of giving read-only attributes a separate interface. But you've already got duration in there, which is also read-only.

    One way or another, it should be accessible. Popcorn has gone down the road of being something of a player abstraction layer, and paused is a pretty useful piece of information to have as well as one that's likely to be available in other players with varying syntax.

    I have mixed feelings about the attr() method idea. On one hand, I'm all in favor of being helpful. But on the other hand, if it's media-element-specific, we lose the value of the abstraction layer. There's even a small danger, as I've seen a number of inexperienced programmers (at Buttercamp and in Ben's ITP class, for example) using Popcorn merely as a video controller when they're not even firing timed events, and I fear that extra syntactic sugar might encourage that sort of confusion.

    I suppose I would be convinced if:

    • There were some reason that the instance and type of media element were likely enough to be unpredictable that the .media property became unreliable, or...
    • attr() standardized interfaces to different media, rather than being media-element-specific, and adding to the value of the abstraction layer.
  • Rick

    Rick July 19th, 2011 @ 10:46 AM

    • State changed from “blocked” to “wont-fix”

    You're right, duration also doesn't make sense as a method and we should move to deprecate it before 1.0 - it's too confusing to have per instance proto methods that can "get" values but cannot "set" values. Also, good points about the .media property and it's own implications - forced me to review the objects created by the non-HTML5 players (ie. Youtube, Vimeo, Soundcloud) and these are all required to implement a bridge to the .media property.

    There's even a small danger, as I've seen a number of inexperienced programmers (at Buttercamp and in Ben's ITP class, for example) using Popcorn merely as a video controller when they're not even firing timed events, and I fear that extra syntactic sugar might encourage that sort of confusion.

    The power of Popcorn.js is that it's not limited to programming timed events, in fact it was designed to encourage the type of programming that you're calling a "small danger"...

    Popcorn.js was built with the idea that we shouldn't limit its capability to timeline synchronized playbacks, but instead to provide a very simple API to program expressive, highly interactive, event/interaction driven media experiences.

    While I understand your last bullet point and the concern therein, Popcorn.js core has a project precedent that saves it from being required to normalize for non-HTML5 media (eg. player plugins) to avoid bloat.

    Lastly, thanks for your input - it will serve as a catalyst for improved usability of the library

  • brianchirls

    brianchirls July 19th, 2011 @ 10:57 AM

    • State changed from “wont-fix” to “blocked”

    Great, consistency is good. And the .media thing is worth exploring.

    I'm curious, what's the precedent?

    What is Popcorn's case for an interactive event/interaction driven media experience that doesn't involve timeline-synchronized playback?

    Anyway, I'm talking about situations where someone was not using any Popcorn features at all other than pass-through player functions. It confuses people just as they're starting to learn to program, not to mention it adds an unnecessary extra source of bloat and bugs.

  • Rick

    Rick July 19th, 2011 @ 12:31 PM

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

    After a really productive brainstorm with Humph, we both agree that this is actually the way to go and even more so, we want to expose all of the IDL attributes as getters for readonly and getter/setter for writable.

    Brian, thanks for your patience on this ticket.

  • Rick

    Rick July 19th, 2011 @ 12:50 PM

    (from [8678c2c1597f17e49f4fa1f04fbbd14ce573ae73]) [#654] Implements new api pass-through methods: preload playbackRate autoplay loop controls muted buffered readyState seeking paused played seekable ended https://github.com/rwldrn/popcorn-js/commit/8678c2c1597f17e49f4fa1f...

  • brianchirls

    brianchirls July 19th, 2011 @ 12:53 PM

    Patience indeed. That's a mighty quick turnaround. Excellent work, and thank you.

  • Rick

    Rick July 19th, 2011 @ 12:54 PM

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

    Brian, this stuff is full of win. Docs to come soon

  • Scott Downe

    Scott Downe July 22nd, 2011 @ 11:01 AM

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

    Scott Downe July 22nd, 2011 @ 11:26 AM

    • Assigned user cleared.
    • State changed from “under-review” to “super-review-requested”

    Awesome, the functionality was already there!

    Adding duration, we can now get the duration using p.duration(). If we do p.duration( 100 ) the p instance is returned instead of the duration, and then p.duration() it will not be 100, so it has not changed.

    I think this is correct.

    tests pass.

  • Scott Downe

    Scott Downe July 22nd, 2011 @ 11:30 AM

    • Assigned user set to “cadecairos”
  • Rick

    Rick July 22nd, 2011 @ 12:17 PM

    @Scott, yes several of these are passthroughs "readonly" IDL attributes, duration(), paused(), buffered(), played(), seekable() (i tihnk?) are some examples

  • cadecairos

    cadecairos July 22nd, 2011 @ 01:21 PM

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

    Really cool stuff!

    One problem I have noticed is that you cannot set any boolean values to false because of line 453.

    if ( arg !== false && arg !== null && typeof arg !== "undefined" ) {
    
      this.media[ name ] = arg;
    
      return this;
    }
    

    edit: to clarify, doing this:

    var p = new Popcorn( "#video" );
    
    p.loop( true );
    p.loop( false );
    
    console.log(p.loop() );
    //logs true
    
  • Scott Downe

    Scott Downe July 22nd, 2011 @ 01:43 PM

    I think we can safely remove the !== false check from the logic, as it is a valid settable value.

  • Rick

    Rick July 22nd, 2011 @ 06:20 PM

    Totally. Also going to reduce it to != null which will check for both null and undefined, but all false, 0 and ""

  • Rick
  • Rick

    Rick July 22nd, 2011 @ 06:21 PM

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

    Scott Downe July 23rd, 2011 @ 06:25 PM

    Fantastic.

    I always forget about that technique.

  • Rick

    Rick July 23rd, 2011 @ 07:22 PM

    @Scott, the best commits are definitely the ones that remove code :)

  • cadecairos

    cadecairos July 26th, 2011 @ 10:47 AM

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

    Sorry for the wait in reviewing this.

    I'm playing around a bit with this and one thing I noticed, and I'm not sure if this is a problem, is that when undefined, or anything with the value undefined is passed into the methods, it doesn't return the popcorn object, it returns the value. for example:

    var pop = new Popcorn( "#video" ),
        nothing;
    
    pop.loop(); 
    // returns undefined
    
    pop.loop( true ); 
    // returns [popcorn Object]
    
    pop.loop(); 
    // returns true
    
    pop.loop( false ); 
    // returns [popcorn Object]
    
    pop.loop(); 
    // returns false
    
    pop.loop( undefined );
    // false (shouldn't it return the popcorn instance?)
    
    pop.loop( true ); 
    // returns [popcorn Object]
    
    pop.loop ( nothing ); // 'nothing' is undefined
    // true (this is not good imho)
    
  • Rick

    Rick July 26th, 2011 @ 01:00 PM

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

    Passing an undefined variable is evaluated the same way as passing nothing at all - there is no way around that.

    false - is a valid value, null/undefined are not

    So..

    
    pop.loop( undefined );
    // false (shouldn't it return the popcorn instance?)
    

    Is the same as:

    
    pop.loop();
    // false (shouldn't it return the popcorn instance?)
    

    So, I'll work through the last one...

    
    pop.loop ( nothing ); // 'nothing' is undefined
    // true (this is not good imho)
    
  • Rick

    Rick July 26th, 2011 @ 01:14 PM

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

    Rick July 26th, 2011 @ 02:10 PM

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

    There is no efficient way around this. I think counting arguments is too messy and unpredictable. If the argument is named, but undefined, it will be evaluated as undefined and the "getter" logic will be executed

  • cadecairos

    cadecairos July 27th, 2011 @ 10:56 AM

    • Assigned user changed from “cadecairos” to “Jon Buckley”
    • State changed from “super-review-requested” to “review-looks-good”

    Tested The Core Test suite on:

    Firefox 5, 8
    Chrome 13

    All tests passed

    code looks good.

    SR+!

  • Jon Buckley

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