Fixed removing multiple listeners within event callbacks #5827 #5833
Conversation
|
@speigg, thanks for the pull request! Maintainers, we have a signed CLA from @speigg, so you can review this at any time. I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to I am a bot who helps you make Cesium awesome! Thanks again. |
speigg
changed the title
|
Thanks for the PR @speigg ! If you add |
CHANGES.md
Outdated
| @@ -1,5 +1,8 @@ | |||
| Change Log | |||
| ========== | |||
|
|
|||
| * Fixed removing multiple event listeners within event callbacks. [#5827](https://github.com/AnalyticalGraphicsInc/cesium/issues/5827) | |||
There was a problem hiding this comment.
Add this bullet under the next release, under ### 1.38 - 2017-10-02
| @@ -120,6 +120,10 @@ define([ | |||
| return false; | |||
| }; | |||
|
|
|||
| function compareNumber(a,b) { | |||
There was a problem hiding this comment.
Since this is so short, it can probably be inline.
toRemove.sort(function (a, b) {
...
});
There was a problem hiding this comment.
Wouldn't that add an unnecessary callback allocation every time an event is raised (thus creating garbage and requiring the GC to be invoked more often)?
Specs/Core/EventSpec.js
Outdated
| var remove; | ||
| var listener = function() { | ||
| if (index % 2 === 0) { | ||
| remove(); |
There was a problem hiding this comment.
This test is a little hard to follow. Can you try structuring it a little more linearly like the other tests?
|
@ggetz I've rewritten the test case to make it simpler, as you suggested |
Fixed a bug that occurs when removing multiple event listeners within an event callback (see #5827). Added a test case to EventSpec.