#95 ✓ checked-in
Nick Cammarata

Multiple Popcorn Instances

Reported by Nick Cammarata | August 4th, 2010 @ 11:42 AM | in 0.4

Multiple popcorn instances don't work correctly.

Comments and changes to this ticket

  • annasob

    annasob October 14th, 2010 @ 06:23 PM

    Whats the use case for having multiple popcorn instances? Do you have an example. We are working on a wrapper function that will enclose more than one video elements but will still only have one popcorn instance, will this solve this problem. Check out ticket #97

  • annasob
  • annasob

    annasob November 3rd, 2010 @ 11:36 AM

    • Milestone set to 0.2 release
    • Milestone order changed from “1” to “0”
  • Scott Downe

    Scott Downe November 19th, 2010 @ 11:44 AM

    • Milestone changed from 0.2 release to 0.3 release date
    • Milestone order changed from “10” to “0”
  • annasob

    annasob January 12th, 2011 @ 04:02 PM

    • Tag set to starter
    • Milestone order changed from “1” to “0”

    We should test to see if this is still an issue. Marking as starter

  • Scott Downe

    Scott Downe January 13th, 2011 @ 10:37 AM

    • Assigned user cleared.

    I noticed this is marked as a starter, so I will put it back into the pool.

  • Sonnilion

    Sonnilion January 20th, 2011 @ 11:12 AM

    • State changed from “new” to “assigned”
    • Assigned user set to “Sonnilion”
  • Scott Downe

    Scott Downe January 31st, 2011 @ 12:03 PM

    Hey Sonnilion, I was wondering if you could make a way to track instances of Popcorn from the Popcorn object in this ticket? Oh, and writing unit tests for multiple instances.

  • Rick

    Rick January 31st, 2011 @ 04:31 PM

    Popcorn will always return a unique instance when Popcorn() is called.

    @Scott, Popcorn can easily track when a new instance is created, however it won't know if one has been destroyed.

  • Sonnilion

    Sonnilion February 8th, 2011 @ 08:44 AM

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

    Tell me if this is how you wanted the instances set up.
    https://github.com/sonnilion/popcorn-js/tree/bug95

  • Rick

    Rick February 8th, 2011 @ 12:03 PM

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

    Rick February 8th, 2011 @ 12:26 PM

    • State changed from “under-review” to “review-needs-work”

    Looks good, but needs the following:

    Please add (above-the-line) comments to your additions -- explain the "why".

    Everything is very crowded, which is unkind to the next developer. We have a build process that will compress Popcorn.js source, so don't be afraid to be liberal with whitespace and comments.

    Based on #2, please review the style guide and apply any necessary changes: https://gist.github.com/793649

    Popcorn.removeInstance() should check if Popcorn.instances has any length before it splice's, if it does not, do an early return...

    
    Popcorn.removeInstance = function( id ) {
    
      //  If called prior to any instances being created
      //  Return early to avoid splicing on nothing
      if ( !Popcorn.instances.length ) {
        return;
      }
      
      Popcorn.instances.splice( Popcorn.instanceIds[ id ], 1 );
    
      delete Popcorn.instanceIds[ id ];
    };
    

    Popcorn.addInstance() has some heavy duty evaluation, that I'm sure isn't necessary:

    
    Popcorn.addInstance = function( popcornInstance ) {
    
      //  Currently looks like:
      //  if ( popcornInstance.video.id === "undefined" || !popcornInstance.video.id.length ) {
      
    
      // Does the same thing:
      if ( !popcornInstance.video.id ) { 
    
        popcornInstance.video.id = "__popcorn" + Popcorn.instances.length;
    
      }
    
      Popcorn.instanceIds[ popcornInstance.video.id ] = Popcorn.instances.length;
    
      Popcorn.instances.push(popcornInstance);
    
    };
    
  • Rick

    Rick February 8th, 2011 @ 12:39 PM

    Additionally, I wrote these unit tests:

    
    test("Instances", function() {
      expect(8);
      
      equals( typeof Popcorn.addInstance, "function" , "Popcorn.addInstance is a provided utility function");
      equals( typeof Popcorn.removeInstance, "function" , "Popcorn.removeInstance is a provided utility function");
      equals( typeof Popcorn.getInstanceById, "function" , "Popcorn.getInstanceById is a provided utility function");
      equals( typeof Popcorn.instanceIds, "object" , "Popcorn.instanceIds is a provided cache object");
      ok( "length" in Popcorn.instances && "join" in Popcorn.instances, "Popcorn.error is a provided cache array");  
      
      var instance;
      
      for ( var prop in Popcorn.instanceIds ) {
        
        instance = Popcorn.getInstanceById( prop );
        
        ok( instance.video, "Stored instance as a `video` property" );
        ok( instance.data, "Stored instance as a `data` property" );
        ok( instance instanceof Popcorn, "instance instanceof Popcorn" );
      
      }
    
    });
    

    I added them to popcorn.unit.js just after the "Utility" test block. They do not require any changes to the test/index.html.

    This is just a starting point - there are plenty of additional aspects that need to be tested.

  • annasob

    annasob February 8th, 2011 @ 05:33 PM

    • Milestone changed from 0.3 release date to 0.4
    • Milestone order changed from “3” to “0”
  • Sonnilion

    Sonnilion February 11th, 2011 @ 12:52 PM

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

    I made the requested changes hope you like it :D

  • Rick

    Rick February 11th, 2011 @ 02:38 PM

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

    So close - can you take a look at the style guide and make sure that the white space is correct.

    Thanks!

  • Sonnilion

    Sonnilion February 11th, 2011 @ 05:46 PM

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

    Hope this is better :D
    I read the style guide more throughly.

  • Rick

    Rick February 12th, 2011 @ 03:20 PM

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

    I'm sorry to do this to you, but if I'm not tough then the source will become neglected and I can't allow that.

    
    //Program comments that look like this
      ^ 
    
    //  Should look like this. Crowding makes them hard to read.
      ^ 
    
    //debugging related comments MUST be left like this
      ^
    

    In test/popcorn.unit.js, can you please limit the var to one per test.

    Currently:

    
      var popped =  Popcorn("#video");
      var popped2 = Popcorn("#video");
      var popped3 = Popcorn("#video");
      var expects = 9, 
          count   = 0;
    
      // later in the test...
    
      var instance;
      instance = Popcorn.getInstanceById("video");
    

    Should be:

    
      var popped = Popcorn("#video"), 
          popped2 = Popcorn("#video"), 
          popped3 = Popcorn("#video"), 
          expects = 9, 
          count = 0, 
          instance;
    
      // later in the test...
    
      instance = Popcorn.getInstanceById("video");
    

    .. Even still I don't think there is a reason to create 3 new instances when 1 is sufficient - I only mention this because I'm seeing a failure in this test:

    
      ok( 3 === Popcorn.instances.length, "There are the correct number of Popcorn instances" );
      plus();
    

    Which should also be changed to:

    
      equal( Popcorn.instances.length, 3, "There are the correct number of Popcorn instances" );
      plus();
    

    ... So that when it fails it will tell me what was wrong with the expected value.

  • Sonnilion

    Sonnilion February 25th, 2011 @ 02:22 PM

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

    Fixed sorry for delay school got hectic.

  • Rick

    Rick February 25th, 2011 @ 02:47 PM

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

    I'm going to review this again, mostly because I'm worried that you'll need to update it to be current with all the latest changes.

    EDIT.

    I just pulled your bug95 branch into a local branch with the latest 0.4 code and stopped by merge conflicts, so you'll need to resolve these.

    Keep up the good work!

  • Sonnilion

    Sonnilion March 1st, 2011 @ 04:10 PM

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

    I merged the fix with 0.4 code and fixed the conficts

  • Rick

    Rick March 1st, 2011 @ 04:13 PM

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

    Rick March 1st, 2011 @ 04:37 PM

    • State changed from “under-review” to “super-review-requested”
    • Assigned user changed from “Sonnilion” to “annasob”

    PR +1
    Tested in FF 3.6, 4bX. Chrome 9, 11-dev
    Tested with Popcorn Test Suite & Instance Test Suite

  • Scott Downe

    Scott Downe March 2nd, 2011 @ 09:57 AM

    • State changed from “super-review-requested” to “under-review”
    • Assigned user changed from “annasob” to “Scott Downe”
  • Scott Downe

    Scott Downe March 2nd, 2011 @ 10:33 AM

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

    What is the purpose of instanceTester.html? Is that testing something that is impossible to test in the test suite?

    I don't like the idea of extra test files to run after a patch, especially if it is related to the core code.

    Try and move that test case into the core unit tests.

    Your current unit tests are good though, so I don't even thing the instanceTester is needed.

    If it is testing something already covered by the unit tests, move it into the demos folder under a sub directory called instance demo, or something.

    I don't like this:

    popcornInstance.video.id = "__popcorn" + Popcorn.instances.length;
    

    Getting the id from the video is the right thing to do, if it exists, and generating one for them if it does not, is also the right thing to do.

    But, I think that id should then be stored on the instance itself, and not the video. This way we have control and assurance that bugs won't creep in because someone else programmatically adds an id to the element they thought had none.

    Only one more thing, and it is more of an idea than a request.

    You have an addInstance and removeInstance, but add instance takes an actual popcorn instance, removeInstance take an id. We should probably change it to removeInstanceById. This bring me to my idea. Add both a removeInstance( instance ) and a removeInstanceById( id ) you can easily make removeInstance call removeInstanceById, passing in the id of the instance.

  • Sonnilion

    Sonnilion March 7th, 2011 @ 03:43 PM

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

    Rick March 8th, 2011 @ 12:36 PM

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

    There are still some formatting issues, here are two of them to give you an idea of what needs to be changed:

    
    // Line 29
    Popcorn.removeInstance = function( id ) {
                                      ^  ^ Spaces here
    
    // Line 31
    Popcorn.instances.splice( Popcorn.instanceIds[id], 1 );
                             ^                          ^
    

    For review: https://gist.github.com/793649

    I'm confident you'll get the hang of it, don't get discouraged :D

  • Sonnilion

    Sonnilion March 8th, 2011 @ 01:04 PM

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

    I think you are looking at an old code base. removeInstance looks like this now:

    Popcorn.removeInstance = function( popcornInstance ) {

    The new link is here:https://github.com/sonnilion/popcorn-js/tree/bug95fix

  • Rick

    Rick March 8th, 2011 @ 01:14 PM

    • State changed from “peer-review-requested” to “review-looks-good”
    • Assigned user changed from “Sonnilion” to “annasob”

    I was, sorry about that.

  • Sonnilion

    Sonnilion March 8th, 2011 @ 01:26 PM

    It's cool thanks for the review :D

  • annasob

    annasob March 8th, 2011 @ 01:49 PM

    • State changed from “review-looks-good” to “staged”

    Staged in annasob/popcorn-js commit

  • annasob

    annasob March 21st, 2011 @ 02:47 PM

    • State changed from “staged” to “checked-in”
    • Milestone order changed from “1” to “0”
  • annasob

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

Tags

Referenced by

Pages