#259 ✓ checked-in
boaz (at bocoup)

removePlugin is only on the popcorn instance, but removes plugins from the entire Popcorn scope

Reported by boaz (at bocoup) | January 11th, 2011 @ 06:59 PM | in 0.4

Currently, Popcorn.removPlugin() is undefined, and Popcorn( "#video" ).removePlugin( 'pluginname' ) removes a plugin from all of popcorn.

This is undesirable. removPlugin should work like this:

Popcorn.removePlugin( 'pluginname' ) should be a static method that removes a plugin from Popcorn.

Popcorn( "#video" ).removePlugin( 'pluginname' ) should remove plugins from that instance, and leave other instances alone.

Comments and changes to this ticket

  • Scott Downe

    Scott Downe January 20th, 2011 @ 03:27 PM

    • State changed from “new” to “assigned”

    Right, I agree.

    Plugins are being registered on Popcorn, so should be removed as such.

    Popcorn( "#video" ).removePlugin( 'pluginname' ) should not even exist, as plugins don't exist on an instance of popcorn.

  • Scott Downe

    Scott Downe January 26th, 2011 @ 02:39 PM

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

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

    OK, if you want to remove all plugins from popcorn, use:

    Popcorn.removePlugin()
    

    From an instance:

    Popcorn().removePlugin()
    

    A significant change was done for this, that being: tracks are no longer removed when the plugin is removed, but instead removed when a track that has a plugin that no longer exists is executed, it is removed then instead.

    The advantages of this is it saves going through potentially large loops and removing potentially large amounts of data, that may never be used again anyway. Also, removing all tracks from all instances of popcorn could potentially be pretty nasty.

    A little philosophy comes from this: "Why remove something that will never be?"

  • annasob

    annasob January 27th, 2011 @ 05:39 PM

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

    Rick i am told u are still reviewing this. Take ur time but im just making it known.

  • Scott Downe

    Scott Downe January 31st, 2011 @ 11:53 AM

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

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

    The current state ^^

    There is still the problem of a plugin being available to an instance after removal. All tracks will be removed, but the plugin still exists.

    I also think the way removeTrackEvent removes by id can be improved upon, as currently I am looping twice. Once to find the id of anything with name === to track type, and again to find it and remove it based on the id. What we should probably do is allow removeTrackEvent to remove by plugin name AND id... while not changing the API of removeTrackEvent.

    I also think maybe there should be a way to access all instances of Popcron from Popcorn, that way I can remove all tracks from all of popcorn in one function, and not rely on the update function to remove anything that does not exist.

  • Rick

    Rick January 31st, 2011 @ 04:28 PM

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

    From what I can tell, Good solution.

    Good to go.

  • Scott Downe

    Scott Downe February 4th, 2011 @ 03:35 PM

    Can someone do a super review on this?

  • annasob

    annasob February 7th, 2011 @ 03:59 PM

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

    Code looks good but a lot of tests are failing in FF 3.6

  • Scott Downe

    Scott Downe February 8th, 2011 @ 08:37 AM

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

    Not going to make it into 0.3.

    Also, exec test is failing on 3.6 in a clean 0.3.

    I will be back into it for the later half of the week.

  • Scott Downe

    Scott Downe February 14th, 2011 @ 03:01 PM

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

    Alright, tests are passing in 4.0 and 3.6.

    This was a combination of inaccurate testing and bugs in core.

    exec test is still failing in FF 3.6, but it is failing in 0.4 under FF 3.6 as well, so not this tickets problem.

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

    I merged with 0.4, and included the fixes introduced in #319 as the removePlugin function was moved to another location.

    With the number of changes here, and the time gap and differences of 0.3 now being 0.4, I think this should be peer-reviewed again.

  • Rick

    Rick February 14th, 2011 @ 04:07 PM

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

    I see a lot of awesome here. Nice work

  • annasob

    annasob February 15th, 2011 @ 05:03 PM

    • State changed from “super-review-requested” to “staged”

    Code looks good. Core tests run on Chrome, FF, and Minefield

    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 “7” 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

Referenced by

Pages