#461 ✓ staged
Bobby Richter

Popcorn could benefit from either suggested default values or extended manifest types

Reported by Bobby Richter | March 29th, 2011 @ 05:28 PM | in 0.8 (closed)

Trying to incorporate the Code plugin into Butter, I came across this problem: if onStart is undefined or some reasonable facsimile, an exception is thrown courtesy of this:

    if ( !options.onStart || typeof options.onStart !== 'function' ) {
      throw 'Popcorn Code Plugin Error: onStart must be a function.';
    }    

    if ( options.onEnd && typeof options.onEnd !== 'function' ) {
      throw 'Popcorn Code Plugin Error: onEnd  must be a function.';
    }    

    if ( options.onFrame && typeof options.onFrame !== 'function' ) {
      throw 'Popcorn Code Plugin Error: onFrame  must be a function.';
    }

It would be great if plugins could either suggest a default to automation programs like Butter, or if the manifest could list things like onStart as function instead of text.

Thoughts? Is there a non-slimy way around this?

Comments and changes to this ticket

  • Rick

    Rick March 29th, 2011 @ 06:14 PM

    I think the best solution to this problem is for the plugins to have their documentation filled out: http://popcornjs.org/plugins

  • Bobby Richter

    Bobby Richter March 30th, 2011 @ 10:27 AM

    @Rick: You're right about documentation: it certainly does need to be completed. But, how can programs like Butter do this automatically?

    It would be great for the plugin to be structured like this:

      {
        about: {
          name: 'Popcorn Code Plugin',
          version: '0.1',
          author: 'David Humphrey (@humphd)',
          website: 'http://vocamus.net/dave'
        },  
        options: {
          start: {elem:'input', type:'text', label:'In'},
          end: {elem:'input', type:'text', label:'Out'},
          // TODO: how to deal with functions, eval strings?
          onStart: {elem:'input', type:'function', label:'onStart'},
          onFrame: {elem:'input', type:'function', label:'onFrame'},
          onEnd: {elem:'input', type:'function', label:'onEnd'}
        }   
      });
    

    Butter could then prepare for the plugin's instantiation by correctly providing it with a function for onStart instead of a blank string, or undefined. Most plugins don't care or have a default. Butter manipulates the options of an instance of a plugin, so onStart could be an empty function to begin with.

  • Rick

    Rick March 30th, 2011 @ 10:58 AM

    "type" directly defines the input type - I'd recommend using the "placeholder" attribute as a property and creating a placeholder attr on the input that gets created.

    All the plugins would have to be updated for this.

  • Rick

    Rick March 31st, 2011 @ 09:16 PM

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

    Scott Downe April 18th, 2011 @ 06:43 PM

    consider the solution to #411, it has created a similar problem that the code plugin is facing.

    I think our two solutions are to either use the placeholder idea for all plugin, or to make all popcorn plugins create defaults for all needed variables, or both!

  • Scott Downe

    Scott Downe April 18th, 2011 @ 07:48 PM

    • Milestone changed from 0.5 to 0.6
    • Milestone order changed from “49” to “0”

    0.5 cutoff is now! :P

  • Bobby Richter

    Bobby Richter April 20th, 2011 @ 04:52 PM

    I really like the idea of filling out a manifest. It helps apps that are meant to exploit popcorn plugins (i.e. butter) know enough about them to make wise decisions about using them.

    Really, we should use both:

    • plugins should have defaults anyway, and we should enforce that they exist in test-cases where possible
    • manifests should contain more information about what the plugin expects, etc.
  • Rick

    Rick April 20th, 2011 @ 04:58 PM

    I second both points that Bobby makes

  • David Humphrey

    David Humphrey April 20th, 2011 @ 05:07 PM

    We had a long talk about this yesterday (scott/anna/steven/me), and I'm also behind this idea.

  • David Seifried

    David Seifried May 17th, 2011 @ 10:56 AM

    • Milestone changed from 0.6 to 0.7
    • Milestone order changed from “1” to “0”
  • Bobby Richter

    Bobby Richter June 7th, 2011 @ 03:56 PM

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

    I put together this small sample to take care of the problem. Does this suffice?
    https://github.com/secretrobotron/popcorn-js/tree/manifests

    If it does, I can look around finding defaults for all the plugins.

  • Rick

    Rick June 7th, 2011 @ 05:33 PM

    I guess this assumes you're not actually building the UIs from the manifests? This must've changed since you inherited the project, because input fields can't be build out of this change: type:"text" => type:"function"

    I ran the test page and it worked as expected, however I think this is a lot of code. I thought the idea was that the plugins themselves would add code to handle merging some kind of plugin-default options object on a case-by-case basis. shrugs

  • Bobby Richter

    Bobby Richter June 8th, 2011 @ 08:42 AM

    No, that hasn't changed. It's going to happen a bit differently though. Plugins will have custom editors when possible, but we still need a bit of automation. When 'type' is set to 'function', we can make a textarea instead of a textbox for now.

    I'm actually in favour of each plugin dealing with its defaults and subsequent error-handling on its own. This step, however, lets automaticly generated UI's be populated with data when possible without having to actually construct anything yet. So, we actually attack the problem of bad plugin initialization from two fronts.

    We're never going to be able to stop people entirely from screwing up when they write plugins, right? :/ But, this allows people another layer of "protection".

    Because we're decoupling the problem in future versions of Butter (making editors for plugins), I'm sort of ambivalent about this now.

    More opinions?

  • Rick

    Rick June 8th, 2011 @ 08:55 AM

    @Bobby, thanks for explaining the first part to me :)

    I agree with everything you've said flat out - I guess what I'm really wondering if there is a less code-heavy approach?

  • Bobby Richter

    Bobby Richter June 8th, 2011 @ 09:12 AM

    @Rick: you mean in core, right? The only other thing we can do is make sure plugins -- future and present -- conform, which is slowly being worked on (like #522).

    If that's what this ticket is morphing into, maybe it just means a big sweep through all plugins to make sure they do defaults and error-checking for themselves. I'd like to see some measure of formality though, so we can tell plugin-writers of the near future to make sure they have XXXXXXXXX in their code to make sure it plays nice with core and Butter. That might just mean good docs.

  • Rick

    Rick June 8th, 2011 @ 01:06 PM

    @Bobby, I like the plan that is formulating in your latest comment, I think a reduced set of minimal guidelines for viability is a great idea.

  • Rick

    Rick June 18th, 2011 @ 11:42 AM

    • Milestone changed from 0.7 to 0.8
    • Milestone order changed from “5” to “0”
  • Scott Downe

    Scott Downe July 15th, 2011 @ 01:40 PM

    I was thinking about this, and for me it would be ideal if a plugin was smart enough to not break if it was created with no parameters.

    I would be more than willing to go through all plugins and make them safe when no parameters are supplied. I would also add tests to each one, making an empty plugin, to make sure it stays that way.

  • Scott Downe

    Scott Downe July 18th, 2011 @ 09:25 AM

    • Assigned user changed from “Bobby Richter” to “Scott Downe”
  • Scott Downe

    Scott Downe July 18th, 2011 @ 09:54 AM

    Rick, to make sure I understood your idea completely, I'm going to paraphrase it.

    Popcorn.plugin.debug = true|false; // Defaults to false

    Then, on any use of a plugin's function (start, end, teardown and setup) wrap in a try catch, if debug is true, throw, if false, suppress and keep on rolling.

    Core tests for this, turning default on and off on an empty plugin that breaks.

    Then, fix all plugins to not break when completely empty, and tests for that as well.

  • Rick

    Rick July 18th, 2011 @ 10:39 AM

    Most of that matches the idea I had. I don't think we should wrap entire function bodies in try/catch statements, my thinking was more along the lines of this...

    (Assume Popcorn.plugin.debug has been set to true... )

    
    // On a case-by-case need...
    // options.foo is something that requires a value, that we cannot reasonably set a default for
    // eg. "location" in the Google Map plugin
    if ( !option.foo ) {
     
      Popcorn.plugin.debug && (
        // either throw an exception, or log the error to console.   
      );
    
    }
    
  • Scott Downe

    Scott Downe July 19th, 2011 @ 03:19 PM

    • Assigned user cleared.
    • State changed from “assigned” to “peer-review-requested”
  • Rick

    Rick July 19th, 2011 @ 04:08 PM

    Dear Scott,

    Wow.

    Sincerely,

    Rick

  • Rick

    Rick July 19th, 2011 @ 04:22 PM

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

    The only thing I can think of is that you should add:

    
    Popcorn.plugin.debug = false;
    

    To Popcorn.js core

  • Mohammed Buttu

    Mohammed Buttu July 19th, 2011 @ 04:27 PM

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

    Mohammed Buttu July 19th, 2011 @ 04:28 PM

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

    Scott Downe July 19th, 2011 @ 04:31 PM

    • Assigned user cleared.
    • State changed from “review-needs-work” to “peer-review-requested”
  • Mohammed Buttu

    Mohammed Buttu July 20th, 2011 @ 02:21 PM

    • Assigned user set to “Mohammed Buttu”
  • Mohammed Buttu

    Mohammed Buttu July 20th, 2011 @ 04:04 PM

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

    Running Mac OS X 10.6.8

    Ran unit tests on FF 5.0.1 and Chrome 12.0.742.122

    • All plug-in unit tests (eventually) passed.

    I used the current popcorn-complete in Butter to add a track event without a target

    • an exception is thrown as expected

    I created a new popcorn-complete that includes this patch and used that instead in Butter

    • I was able to create a track event without a target

    There is code that you did not touch that needs some style changes, which should be done in another ticket. Also, for the code that was modified, just fixing the style for those lines would make the style inconsistent with the rest of the file.

    However, there a few areas that could benefit from being changed now.

    Unit tests

    • There is a line similar to this in the unit tests:
    
    ok(true, 'empty event was caught by debug');
    

    It should be:

    
    ok( true, "empty event was caught by debug" );
    

    Spaces were added around the arguments, and the single quotes were changed to double quotes.

    popcorn.timeline.html

    • line 13 should just be
    
    .timeline({})
    

    popcorn.mustache.js

    • On lines 112 and 127, instead of using Popcorn.error, the error handling should be consistent with what you have been doing with the other plug-ins, i.e., throw new Error( "..." ).

    Other than that, it looks good!

  • Scott Downe

    Scott Downe July 22nd, 2011 @ 10:39 AM

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

    I filed a ticket for whitespace and quote usage here #660

    Rest fixed here: https://github.com/ScottDowne/popcorn-js/commits/t461

    Good catch on timeline.html having the empty track event.

  • Mohammed Buttu

    Mohammed Buttu July 28th, 2011 @ 02:37 PM

    Hey Scott:

    Will you be fixing the problem with the tear downs in this ticket where there is an error when it tries to get an element from the DOM that is undefined because the target was not specified?

  • David Seifried
  • David Seifried

    David Seifried August 26th, 2011 @ 02:50 PM

    Made a few changes to my develop2 branch after some comments that Rick left on github. Changed what I could from the comments he left, as there have been a bunch of changes to develop since then, so the code has changed a bunch. We probably shouldnt let a ticket sit more a month, as it seems like everything gets all out of whack when this happens and causes more issues then it should.

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

  • Mohammed Buttu

    Mohammed Buttu August 26th, 2011 @ 05:40 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “Mohammed Buttu” to “cadecairos”

    Running Mac OS X 10.6.8

    • Lint passes
    • All tests pass on Firefox 6.0
    • All tests pass on Safari 5.1 except for subtitle (test 11 times out)

    I was not able to run the most of the plug-in tests on Chrome 13.0.782.215. There are no console errors or anything, something else is going on because Jon Buckley was able to run them on his Mac.

  • cadecairos

    cadecairos August 29th, 2011 @ 10:42 AM

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

    Tested: All Plug-in Unit Tests

    Browsers: Firefox 6, Chrome 13

    All tests passing

    SR+

    dseif: In cases where a ticket like this sits for so long, it would be soooooo nice if you could cherry-pick the relevant commits for us reviewers.. just saying.

  • Jon Buckley
  • Rick
  • Rick

    Rick August 31st, 2011 @ 12:42 PM

    (from [ad03cb54d06f1b455766f3ce1611732b5df63294]) Add suggested default values and extended manifest types [#461]

    Conflicts:

    plugins/timeline/popcorn.timeline.js
    

    https://github.com/rwldrn/popcorn-js/commit/ad03cb54d06f1b455766f3c...

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