search

frankyghost / projekktor

The Free Web Video Player
194 stars 75 forks source link

Fixing issue around removing listeners #24

Closed jason-brodie-codebaby closed 10 years ago

jason-brodie-codebaby commented 10 years ago

Previous fix was erroneous as all listeners were removed, not the correctly scoped ones. This fix properly maintains the reference to the listener callback, so that when the cue point is removed, it removes correctly. Fixes issue #20 by correcting pull #21

rwlodkowski commented 10 years ago

I'm afraid that it's not the essence of that issue. The listeners are not removed because of bad loop definition. Check out my new cuepoints system (if you're working with cuepoints now I think you'll like it) and the solution for that problem: https://github.com/fixedmachine/projekktor/commit/7ebceda379eb2828c1838e0b6c42751f846b287f

If you want to play with it: http://projekktor.wlodkowski.net/demo_cuepoints_enhanced.php

I'll be grateful for your feedback.

jason-brodie-codebaby commented 10 years ago

I really like the thought you've put into this. While my implementation of the library doesn't ever show cue point, I do highly rely on the add/remove of the cue points. So for my implementation, the best additions were your fixes. The 'once' cue point idea is really nice, as well as the other additions you added, though unfortunately my implementation does not warrant such features.

Jason Brodie, Software Engineer CodeBaby(C) | 719.387.8358 x 132 | codebaby.comhttp://codebaby.com/

From: Rados³aw W³odkowski notifications@github.com<mailto:notifications@github.com> Reply-To: frankyghost/projekktor reply@reply.github.com<mailto:reply@reply.github.com> Date: Tuesday, February 4, 2014 8:14 PM To: frankyghost/projekktor projekktor@noreply.github.com<mailto:projekktor@noreply.github.com> Cc: j jason.brodie@codebaby.com<mailto:jason.brodie@codebaby.com> Subject: Re: [projekktor] Fixing issue around removing listeners (#24)

I'm afraid that it's not the essence of that issue. The listeners are not removed because of bad loop definition. Check out my new cuepoints system (if you're working with cuepoints now I think you'll like it) and the solution for that problem: fixedmachine@7ebcedahttps://github.com/fixedmachine/projekktor/commit/7ebceda379eb2828c1838e0b6c42751f846b287f

If you want to play with it: http://projekktor.wlodkowski.net/demo_cuepoints_enhanced.php

I'll be grateful for your feedback.

Reply to this email directly or view it on GitHubhttps://github.com/frankyghost/projekktor/pull/24#issuecomment-34134033.

rwlodkowski commented 10 years ago

Thanks for feedback. I'm happy that you see that features useful. I hope that my enhanced cuepoints implementation will be merged to the new version of projekktor.

jason-brodie-codebaby commented 10 years ago

One thing I really do appreciate is the cleanliness of removing the cuepoint listeners. I like how you've name spaced them, as I think that is much more reliable then removal by callback comparison. I'm currently using your fork as the primary in my project so I'll let you know if I run into any cue point issues with your implementation.

Jason Brodie, Software Engineer CodeBaby(C) | 719.387.8358 x 132 | codebaby.comhttp://codebaby.com/

From: Rados³aw W³odkowski notifications@github.com<mailto:notifications@github.com> Reply-To: frankyghost/projekktor reply@reply.github.com<mailto:reply@reply.github.com> Date: Friday, February 7, 2014 3:08 AM To: frankyghost/projekktor projekktor@noreply.github.com<mailto:projekktor@noreply.github.com> Cc: j jason.brodie@codebaby.com<mailto:jason.brodie@codebaby.com> Subject: Re: [projekktor] Fixing issue around removing listeners (#24)

Thanks for feedback. I'm happy that you see that features useful. I hope that my enhanced cuepoints implementation will be merged to the new version of projekktor.

Reply to this email directly or view it on GitHubhttps://github.com/frankyghost/projekktor/pull/24#issuecomment-34423284.

rwlodkowski commented 10 years ago

fixed with #29