2.6.5: reverse-iterate completion listeners to fix #263

Author: Zhuinden

CHANGELOG.md

@@ -1,13 +1,24 @@
 # Change log
 
+-Simple Stack 2.6.5 (2022-11-11)
+--------------------------------
+
+- FIX: `Backstack.CompletionListener` added to `Backstack` that unregistered themselves during dispatching notifications
+  would cause either a ConcurrentModificationException or invalid results, this is now fixed and no longer the case (
+  #263, thanks @angusholder)
+
+- MINOR CHANGE: When `Backstack.CompletionListener`'s are being notified, the state changer is temporarily removed (
+  similarly to dispatching `ScopedServices.Activated` events), so that navigation actions invoked on `Backstack` are
+  deferred until all `Backstack.CompletionListener`s are notified.
+
 -Simple Stack 2.6.4 (2022-04-21)
 --------------------------------
 
-- FIX: Attempt at fixing a crash related to `LinkedHashMap.retainAll()` specifically on Android 6 and Android 6.1 devices (#256).
+- FIX: Attempt at fixing a crash related to `LinkedHashMap.retainAll()` specifically on Android 6 and Android 6.1
+  devices (#256).
 
 - 2.6.3 had an issue with `maven-publish` and transitive dependencies were missing, and is therefore skipped.
 
-
 -Simple Stack 2.6.2 (2021-06-07)
 --------------------------------
 

README.md

@@ -69,7 +69,7 @@ and then, add the dependency to your module's `build.gradle.kts` (or `build.grad
 
 ``` kotlin
 // build.gradle.kts
-implementation("com.github.Zhuinden:simple-stack:2.6.4")
+implementation("com.github.Zhuinden:simple-stack:2.6.5")
 
 implementation("com.github.Zhuinden.simple-stack-extensions:core-ktx:2.2.4")
 implementation("com.github.Zhuinden.simple-stack-extensions:fragments:2.2.4")
@@ -83,7 +83,7 @@ or
 
 ``` groovy
 // build.gradle
-implementation 'com.github.Zhuinden:simple-stack:2.6.4'
+implementation 'com.github.Zhuinden:simple-stack:2.6.5'
 
 implementation 'com.github.Zhuinden.simple-stack-extensions:core-ktx:2.2.4'
 implementation 'com.github.Zhuinden.simple-stack-extensions:fragments:2.2.4'

simple-stack/build.gradle.kts

@@ -74,7 +74,7 @@ afterEvaluate {
             register("mavenJava", MavenPublication::class) {
                 groupId = "com.github.Zhuinden"
                 artifactId = "simple-stack"
-                version = "2.6.4"
+                version = "2.6.5"
 
                 from(components["release"])
                 artifact(sourcesJar.get())

simple-stack/src/main/java/com/zhuinden/simplestack/NavigationCore.java

@@ -705,9 +705,17 @@ public void removeCompletionListeners() {
     }
 
     private void notifyCompletionListeners(StateChange stateChange) {
-        for(Backstack.CompletionListener completionListener : completionListeners) {
+        final StateChanger currentStateChanger = stateChanger;
+        if(currentStateChanger != null) {
+            stateChanger = null;
+        }
+        for(int i = completionListeners.size() - 1; i >= 0; i--) {
+            Backstack.CompletionListener completionListener = completionListeners.get(i);
             completionListener.stateChangeCompleted(stateChange);
         }
+        if(stateChanger == null && currentStateChanger != null) {
+            this.stateChanger = currentStateChanger; // do not use `setStateChanger(REATTACH)` here, it would try to start state changes twice in succession
+        }
     }
 
     // force execute

simple-stack/src/test/java/com/zhuinden/simplestack/BackstackCoreTest.java

@@ -26,6 +26,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
 import javax.annotation.Nonnull;
@@ -273,6 +274,67 @@ public void handleStateChange(@Nonnull StateChange _stateChange, @Nonnull Callba
         Mockito.verify(completionListener, Mockito.only()).stateChangeCompleted(stateChange);
     }
 
+    @Test
+    public void completionListenerCanRemoveItself() {
+        TestKey initial = new TestKey("hello");
+        final TestKey second = new TestKey("world");
+        final Backstack backstack = new Backstack();
+        backstack.setup(History.of(initial));
+
+        final AtomicInteger listener1Called = new AtomicInteger(0);
+        final AtomicInteger listener2Called = new AtomicInteger(0);
+        final AtomicInteger listener3Called = new AtomicInteger(0);
+
+        Backstack.CompletionListener completionListener1 = new Backstack.CompletionListener() {
+            @Override
+            public void stateChangeCompleted(@Nonnull StateChange stateChange) {
+                listener1Called.addAndGet(1);
+            }
+        };
+        Backstack.CompletionListener completionListener2 = new Backstack.CompletionListener() {
+            @Override
+            public void stateChangeCompleted(@Nonnull StateChange stateChange) {
+                listener2Called.addAndGet(1);
+                backstack.removeCompletionListener(this);
+            }
+        };
+        Backstack.CompletionListener completionListener3 = new Backstack.CompletionListener() {
+            @Override
+            public void stateChangeCompleted(@Nonnull StateChange stateChange) {
+                listener3Called.addAndGet(1);
+            }
+        };
+        StateChanger stateChanger = new StateChanger() {
+            @Override
+            public void handleStateChange(@Nonnull StateChange _stateChange, @Nonnull Callback completionCallback) {
+                stateChange = _stateChange;
+                callback = completionCallback;
+            }
+        };
+        backstack.addCompletionListener(completionListener1);
+        backstack.addCompletionListener(completionListener2);
+        backstack.addCompletionListener(completionListener2); // intentional duplicate line to reproduce #263
+        backstack.addCompletionListener(completionListener3);
+        backstack.setStateChanger(stateChanger);
+
+        callback.stateChangeComplete();
+
+        assertThat(backstack.isStateChangePending()).isFalse();
+
+        assertThat(listener1Called.get()).isEqualTo(1);
+        assertThat(listener2Called.get()).isEqualTo(2);
+        assertThat(listener3Called.get()).isEqualTo(1);
+
+        backstack.goTo(second);
+
+        callback.stateChangeComplete();
+
+        assertThat(listener1Called.get()).isEqualTo(2);
+        assertThat(listener2Called.get()).isEqualTo(2);
+        assertThat(listener3Called.get()).isEqualTo(2);
+    }
+
+
     @Test
     public void removedCompletionListenerShouldNotBeCalled() {
         TestKey initial = new TestKey("hello");