Merge pull request #37 from facebook/bugfix

Convert ReentrantCallback to CopyOnWriteArraySet
facebookarchive · Aug 1, 2014 · a569472 · a569472
2 parents 2b64287 + fb1b3bc
commit a569472
Show file tree

Hide file tree

Showing 5 changed files with 91 additions and 103 deletions.
diff --git a/rebound-core/src/main/java/com/facebook/rebound/BaseSpringSystem.java b/rebound-core/src/main/java/com/facebook/rebound/BaseSpringSystem.java
@@ -31,7 +31,7 @@ public class BaseSpringSystem {
   private final Set<Spring> mActiveSprings = new CopyOnWriteArraySet<Spring>();
   private final SpringClock mClock;
   private final SpringLooper mSpringLooper;
-  private final ReentrantCallback<SpringSystemListener> mListeners = new ReentrantCallback<SpringSystemListener>();
+  private final CopyOnWriteArraySet<SpringSystemListener> mListeners = new CopyOnWriteArraySet<SpringSystemListener>();
   private long mLastTimeMillis = -1;
   private boolean mIdle = true;
 
@@ -198,14 +198,14 @@ public void addListener(SpringSystemListener newListener) {
     if (newListener == null) {
       throw new IllegalArgumentException("newListener is required");
     }
-    mListeners.addListener(newListener);
+    mListeners.add(newListener);
   }
 
   public void removeListener(SpringSystemListener listenerToRemove) {
     if (listenerToRemove == null) {
       throw new IllegalArgumentException("listenerToRemove is required");
     }
-    mListeners.removeListener(listenerToRemove);
+    mListeners.remove(listenerToRemove);
   }
 
   public void removeAllListeners() {

diff --git a/rebound-core/src/main/java/com/facebook/rebound/ReentrantCallback.java b/rebound-core/src/main/java/com/facebook/rebound/ReentrantCallback.java
diff --git a/rebound-core/src/main/java/com/facebook/rebound/Spring.java b/rebound-core/src/main/java/com/facebook/rebound/Spring.java
@@ -10,6 +10,8 @@
 
 package com.facebook.rebound;
 
+import java.util.concurrent.CopyOnWriteArraySet;
+
 /**
  * Classical spring implementing Hooke's law with configurable friction and tension.
  */
@@ -43,7 +45,7 @@ private static class PhysicsState {
   // thresholds for determining when the spring is at rest
   private double mRestSpeedThreshold = 0.005;
   private double mDisplacementFromRestThreshold = 0.005;
-  private ReentrantCallback<SpringListener> mListeners = new ReentrantCallback<SpringListener>();
+  private CopyOnWriteArraySet<SpringListener> mListeners = new CopyOnWriteArraySet<SpringListener>();
   private double mTimeAccumulator = 0;
 
   private final BaseSpringSystem mSpringSystem;
@@ -474,7 +476,7 @@ public Spring addListener(SpringListener newListener) {
     if (newListener == null) {
       throw new IllegalArgumentException("newListener is required");
     }
-    mListeners.addListener(newListener);
+    mListeners.add(newListener);
     return this;
   }
 
@@ -487,7 +489,7 @@ public Spring removeListener(SpringListener listenerToRemove) {
     if (listenerToRemove == null) {
       throw new IllegalArgumentException("listenerToRemove is required");
     }
-    mListeners.removeListener(listenerToRemove);
+    mListeners.remove(listenerToRemove);
     return this;
   }
 

diff --git a/rebound-core/src/test/java/com/facebook/rebound/SpringSystemTest.java b/rebound-core/src/test/java/com/facebook/rebound/SpringSystemTest.java
@@ -180,6 +180,45 @@ public void testActivatingAndDeactivatingSpring() {
     assertTrue(mSpringSystemSpy.getIsIdle());
   }
 
+  @Test
+  public void testCanAddListenersWhileIterating() {
+    when(mMockSpring.systemShouldAdvance()).thenReturn(true, false, false);
+    mSpringSystemSpy.addListener(new SimpleSpringSystemListener() {
+      @Override
+      public void onAfterIntegrate(BaseSpringSystem springSystem) {
+        springSystem.addListener(new SimpleSpringSystemListener());
+      }
+    });
+    mSpringSystemSpy.addListener(new SimpleSpringSystemListener());
+    mSpringSystemSpy.loop();
+  }
 
+  @Test
+  public void testCanRemoveListenersWhileIterating() {
+    when(mMockSpring.systemShouldAdvance()).thenReturn(true, false, false);
+    final SimpleSpringSystemListener nextListener = new SimpleSpringSystemListener();
+    mSpringSystemSpy
+        .addListener(new SimpleSpringSystemListener() {
+          @Override
+          public void onAfterIntegrate(BaseSpringSystem springSystem) {
+            springSystem.removeListener(nextListener);
+          }
+        });
+    mSpringSystemSpy
+        .addListener(nextListener);
+    mSpringSystemSpy.loop();
+  }
+
+  private class SimpleSpringSystemListener implements SpringSystemListener {
+    @Override
+    public void onBeforeIntegrate(BaseSpringSystem springSystem) {
+
+    }
+
+    @Override
+    public void onAfterIntegrate(BaseSpringSystem springSystem) {
+
+    }
+  }
 }
 
diff --git a/rebound-core/src/test/java/com/facebook/rebound/SpringTest.java b/rebound-core/src/test/java/com/facebook/rebound/SpringTest.java
@@ -172,7 +172,7 @@ public void testSpringListeners() {
    * push a Spring out of rest right after having gone to rest.
    */
   @Test
-  public void testSpringNoActivateAfterRest() {
+  public void testSpringDoesNotActivateAfterRest() {
     SpringListener listener = spy(new SimpleSpringListener());
     mSpring.addListener(listener);
     mSpring
@@ -204,7 +204,7 @@ public void testSpringNoActivateAfterRest() {
   }
 
   @Test
-  public void testSpringNoActivateAfterSetAtRestCall() {
+  public void testSpringDoesNotActivateAfterSetAtRestCall() {
     SpringListener listener = spy(new SimpleSpringListener());
     mSpring.addListener(listener);
     mSpring
@@ -295,7 +295,7 @@ public void testSpringListenersWithOvershootClamping() {
   }
 
   @Test
-  public void testSpringBehaviorWhenUpdatingEndStateButAndNotAtRest() {
+  public void testSpringBehaviorWhenUpdatingEndStateWhileNotAtRest() {
     SpringListener listener = spy(new SimpleSpringListener());
     mSpring
         .setCurrentValue(1)
@@ -323,4 +323,45 @@ public void testSpringBehaviorWhenUpdatingEndStateButAndNotAtRest() {
     inOrder.verify(listener, never()).onSpringUpdate(mSpring);
   }
 
+  @Test
+  public void testCanAddListenersWhileIterating() {
+    Spring spring = createTestSpring()
+      .addListener(new SimpleSpringListener() {
+      @Override
+      public void onSpringUpdate(Spring spring) {
+        spring.addListener(new SimpleSpringListener());
+      }
+    });
+    iterateUntilRest(spring);
+  }
+
+  @Test
+  public void testCanRemoveListenersWhileIterating() {
+    final SpringListener nextListener = new SimpleSpringListener();
+    Spring spring = createTestSpring()
+        .addListener(new SimpleSpringListener() {
+          @Override
+          public void onSpringUpdate(Spring spring) {
+            spring.removeListener(nextListener);
+          }
+        })
+        .addListener(nextListener);
+    iterateUntilRest(spring);
+  }
+
+  private Spring createTestSpring () {
+    return new Spring(mSpringSystem)
+        .setSpringConfig(new SpringConfig(TENSION, FRICTION))
+        .setEndValue(END_VALUE);
+  }
+
+  private void iterateUntilRest(Spring spring) {
+    int i = 0;
+    int simulatedMsPerFrame = 16;
+    while (!spring.isAtRest() ) {
+      spring.advance(i / 1000.0, simulatedMsPerFrame /1000.0);
+      i += simulatedMsPerFrame;
+    }
+  }
+
 }