Tech change handler spam #1859
Tech change handler spam #1859
Conversation
benjipott
changed the title
| @@ -81,6 +75,15 @@ vjs.MediaTechController.prototype.initControlsListeners = function(){ | |||
| }); | |||
| }; | |||
|
|
|||
| vjs.MediaTechController.prototype.removeControlsListeners = function () { | |||
There was a problem hiding this comment.
There's already a removeControlsListeners. Should this be combined with that?
There was a problem hiding this comment.
I think there's a problem with this. It will remove the removeControlsListeners and activateControls listeners when controlsdisabled is triggered, but what if controlsenabled is triggered again? I think this will break that use case.
e.g.
- enable controls
- disable controls
- enable controls again
|
Thanks for the help! This looks like a good update to me. Any interested in writing a test for this? It doesn't appear we have any tests for this yet, but it would go in the media tech tests file. |
|
Have you verified that this fixes the issue, and can you describe how this is fixing the problem? |
| @@ -239,7 +256,8 @@ vjs.MediaTechController.prototype.stopTrackingCurrentTime = function(){ | |||
| this.player().trigger('timeupdate'); | |||
| }; | |||
|
|
|||
| vjs.MediaTechController.prototype.dispose = function() { | |||
| vjs.MediaTechController.prototype.dispose = function(){ | |||
| this.removeControlsListeners(); | |||
There was a problem hiding this comment.
This actuallyl shouldn't be needed. When you use this.off(player, 'controlsenabled', this.activateControls); it should automatically remove the activateControls listener from the player when this tech is disposed. It's a feature of the off method.
|
Just wanted to chip in and say I've experienced several of the problems this pull is supposed to solve, and I think the root problem lies deeper. I've tried the best I could to explain my findings in short in a new issue at #1896, hopefully you can make sense of my description there. The problems this pull solves are related to these event handlers (MediaTechController.initControlsListeners) but the real problem is that these handlers, among others, should have been automatically deregistred by the Component.on() logic in response to the dispose event. |
|
I'm going to close this in lieu of @vasklund's investigation in #1896. We shouldn't pull in things to fix a symptom that we should solve upstream. Thanks for the PR, though @benjipott! |
Disables spam error handlers when changing tech
Fixes #882
#1485#1505 #1484