Merge pull request #3739 from AnalyticalGraphicsInc/reentrant-entity

Allow EntityCollection events to be reentrant
CesiumGS · Mar 21, 2016 · 78db12e · 78db12e
2 parents 2d0df4d + 5b57d9d
commit 78db12e
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 4 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -19,6 +19,7 @@ Change Log
 * Fix issue where the `GroundPrimitive` volume was being clipped by the far plane. [#3706](https://github.com/AnalyticalGraphicsInc/cesium/issues/3706)
 * Fixed issue where `Camera.computeViewRectangle` was incorrect when crossing the international date line [#3717](https://github.com/AnalyticalGraphicsInc/cesium/issues/3717)
 * Added `Rectangle` result parameter to `Camera.computeViewRectangle`
+* Fixed a reentrancy bug in `EntityCollection.collectionChanged`. [#3739](https://github.com/AnalyticalGraphicsInc/cesium/pull/3739)
 * Fix bug when upsampling exaggerated terrain where the terrain heights were exaggerated at twice the value. [#3607](https://github.com/AnalyticalGraphicsInc/cesium/issues/3607)
 * Fixed a crash that would occur if you added and removed an `Entity` with a path without ever actually rendering it. [#3738](https://github.com/AnalyticalGraphicsInc/cesium/pull/3738)
 

diff --git a/Source/DataSources/EntityCollection.js b/Source/DataSources/EntityCollection.js
@@ -30,15 +30,29 @@ define([
     };
 
     function fireChangedEvent(collection) {
+        if (collection._firing) {
+            collection._refire = true;
+            return;
+        }
+
         if (collection._suspendCount === 0) {
             var added = collection._addedEntities;
             var removed = collection._removedEntities;
             var changed = collection._changedEntities;
             if (changed.length !== 0 || added.length !== 0 || removed.length !== 0) {
-                collection._collectionChanged.raiseEvent(collection, added.values, removed.values, changed.values);
-                added.removeAll();
-                removed.removeAll();
-                changed.removeAll();
+                collection._firing = true;
+                do {
+                    collection._refire = false;
+                    var addedArray = added.values.slice(0);
+                    var removedArray = removed.values.slice(0);
+                    var changedArray = changed.values.slice(0);
+
+                    added.removeAll();
+                    removed.removeAll();
+                    changed.removeAll();
+                    collection._collectionChanged.raiseEvent(collection, addedArray, removedArray, changedArray);
+                } while (collection._refire);
+                collection._firing = false;
             }
         }
     }
@@ -60,6 +74,8 @@ define([
         this._collectionChanged = new Event();
         this._id = createGuid();
         this._show = true;
+        this._firing = false;
+        this._refire  = false;
     }
 
     /**

diff --git a/Specs/DataSources/EntityCollectionSpec.js b/Specs/DataSources/EntityCollectionSpec.js
@@ -135,6 +135,53 @@ defineSuite([
         entityCollection.collectionChanged.removeEventListener(listener.onCollectionChanged, listener);
     });
 
+    it('raises expected events when reentrant', function() {
+        var entityCollection = new EntityCollection();
+
+        var entity = new Entity();
+        var entity2 = new Entity();
+        entityCollection.add(entity);
+        entityCollection.add(entity2);
+
+        var entityToDelete = new Entity();
+        entityCollection.add(entityToDelete);
+
+        var entityToAdd = new Entity();
+
+        var inCallback = false;
+        var listener = jasmine.createSpy('listener').and.callFake(function(collection, added, removed, changed) {
+            //When we set the name to `newName` below, this code will modify entity2's name, thus triggering
+            //another event firing that occurs after all current subscribers have been notified of the
+            //event we are inside of.
+
+            //By checking that inCallback is false, we are making sure the entity2.name assignment
+            //is delayed until after the first round of events is fired.
+            expect(inCallback).toBe(false);
+            inCallback = true;
+            if (entity2.name !== 'Bob') {
+                entity2.name = 'Bob';
+            }
+            if (entityCollection.contains(entityToDelete)) {
+                entityCollection.removeById(entityToDelete.id);
+            }
+            if (!entityCollection.contains(entityToAdd)) {
+                entityCollection.add(entityToAdd);
+            }
+            inCallback = false;
+        });
+        entityCollection.collectionChanged.addEventListener(listener);
+
+        entity.name = 'newName';
+        expect(listener.calls.count()).toBe(2);
+        expect(listener.calls.argsFor(0)).toEqual([entityCollection, [], [], [entity]]);
+        expect(listener.calls.argsFor(1)).toEqual([entityCollection, [entityToAdd], [entityToDelete], [entity2]]);
+
+        expect(entity.name).toEqual('newName');
+        expect(entity2.name).toEqual('Bob');
+        expect(entityCollection.contains(entityToDelete)).toEqual(false);
+        expect(entityCollection.contains(entityToAdd)).toEqual(true);
+    });
+
     it('suspended add/remove raises expected events', function() {
         var entity = new Entity();
         var entity2 = new Entity();