Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically destroy removed instances #150

Merged
merged 1 commit into from
Nov 24, 2014
Merged

Conversation

sterlinghirsh
Copy link
Contributor

With multiple instances of jplayer, if one got removed from the dom without being destroyed, the others would cause a javascript error on play. This was caused by keeping a list of instances ($.jPlayer.prototype.instances) that would get out of sync with the page. With this patch, jPlayer clears the list of any nonexistent instances before iterating over the list.

The error was triggered from pauseOthers() and $.jPlayer.pause().

With multiple instances of jplayer, if one got removed from the dom without being destroyed, the others would cause a javascript error on play. This was caused by keeping a list of instances ($.jPlayer.prototype.instances) that would get out of sync with the page. With this patch, jPlayer clears the list of any nonexistent instances before iterating over the list.

The error was triggered from pauseOthers() and $.jPlayer.pause().
@thepag
Copy link
Contributor

thepag commented Nov 20, 2014

This is generic notification that all users issuing pull requests must sign our CLA before we can consider merging with the jPlayer project.

@thepag
Copy link
Contributor

thepag commented Nov 24, 2014

Reviewing... That call from $.jPlayer.pause() to the destroyRemoved() on the $.jPlayer.prototype would run in the wrong context. The this would be wrong.

Still considering with a fix for that this... I had thought that you'd be using an event handler on remove to execute destroy() or something along those lines.

@thepag
Copy link
Contributor

thepag commented Nov 24, 2014

Scratch that... The this.instances is a static object on the prototype. So from an instantiation of jPlayer, you can use this.instances, while you can use $.jPlayer.prototype.instances from anywhere and they point at the same object.

@thepag
Copy link
Contributor

thepag commented Nov 24, 2014

Going to merge this. i expect some conflict, since pauseOthers was overhauled. I will simply move that line to the tellOthers method.

@thepag
Copy link
Contributor

thepag commented Nov 24, 2014

Something is going wrong with our git repo and the merge is happening with the wrong file.

I am halting all merges until I can figure out what the problem is, otherwise all I am doing is copying and pasting in your code and contributor blame would be in the built dist files, instead of the source files.

@Afterster
Copy link
Contributor

Just a suggestion, but if you're going this way you should combine the multiple step commits into one before pushing, to avoid too much noise on git history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants