2.4.0: GlobalServices.Factory changes (fixes #231, fixes #232)

Zhuinden · Jul 8, 2020 · a4ac80c · a4ac80c
1 parent 5101835
commit a4ac80c
Show file tree

Hide file tree

Showing 6 changed files with 148 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,13 +1,22 @@
 # Change log
 
--Simple Stack X.X.X (XXXX-XX-XX)
+-Simple Stack 2.4.0 (2020-07-08)
 --------------------------------
+- SIGNATURE CHANGE: `GlobalServices.Factory` now receives `Backstack` parameter in `create()`. (#231)
 
-- UPDATE: Create and release` simple-stack-extensions:2.0.0` for default scoping and default fragment behaviors.
+I'm aware this is technically "breaking", but the effect should be minor, and hopefully shouldn't cause problems.`
+
+The `Backstack` cannot be added as a `service` directly, but it can be added as an `alias`.
+
+- FIX: `GlobalServices.Factory`'s `create()` method was non-null, but `@Nonnull` was missing.
+
+- MINOR FIX: Adding the `Backstack` from `serviceBinder.getBackstack()` with `addService()` would cause a loop in `toBundle()`. Now it explicitly throws `IllegalArgumentException` instead sooner (not reported before).
 
 - DEPRECATED: `backstack.removeAllStateChangeCompletionListeners()`. This was added "for convenience", but in reality it is not a good/safe API, and it should not exist.
 
-- ADD: `GlobalServices.SCOPE_TAG` to make it possible without relying on internals to see the scope tag of global services.
+- UPDATE: Create and release` simple-stack-extensions:2.0.0` for default scoping and default fragment behaviors.
+
+- ADD: `GlobalServices.SCOPE_TAG` to make it possible to see the scope tag of global services without relying on internals.
 
 -Simple Stack 2.3.2 (2020-04-11)
 --------------------------------

diff --git a/README.md b/README.md
@@ -49,15 +49,15 @@ 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.3.2")
+implementation("com.github.Zhuinden:simple-stack:2.4.0")
 implementation("com.github.Zhuinden:simple-stack-extensions:2.0.0")
 ```
 
 or
 
 ``` groovy
 // build.gradle
-implementation 'com.github.Zhuinden:simple-stack:2.3.2'
+implementation 'com.github.Zhuinden:simple-stack:2.4.0'
 implementation 'com.github.Zhuinden:simple-stack-extensions:2.0.0'
 ```
 

diff --git a/simple-stack/src/main/java/com/zhuinden/simplestack/GlobalServices.java b/simple-stack/src/main/java/com/zhuinden/simplestack/GlobalServices.java
@@ -137,8 +137,10 @@ public interface Factory {
         /**
          * Invoked when the global scope is created.
          *
+         * @param backstack the backstack
          * @return the global services
          */
-        GlobalServices create();
+        @Nonnull
+        GlobalServices create(@Nonnull Backstack backstack);
     }
 }
diff --git a/simple-stack/src/main/java/com/zhuinden/simplestack/ScopeManager.java b/simple-stack/src/main/java/com/zhuinden/simplestack/ScopeManager.java
@@ -331,7 +331,13 @@ void setGlobalServices(GlobalServices.Factory globalServiceFactory) {
     private void buildGlobalScope() {
         if(!scopes.containsKey(GLOBAL_SCOPE_TAG)) {
             if(globalServiceFactory != null) {
-                globalServices = globalServiceFactory.create();
+                globalServices = globalServiceFactory.create(backstack);
+
+                for(Map.Entry<String, Object> entry: globalServices.services()) {
+                    if(entry.getValue() == backstack) {
+                        throw new IllegalArgumentException("The root backstack should not be added as a service, as it would cause a circular save-state loop. Adding it as an alias would work, but should typically not be necessary because of `serviceBinder.getBackstack()`.");
+                    }
+                }
             }
 
             ScopeNode scope = globalServices.getScope();
@@ -353,6 +359,12 @@ private void buildScope(Object key, String scopeTag, boolean isExplicitParent, b
             if(!isDummyScope) {
                 scopedServices.bindServices(new ServiceBinder(this, key, scopeTag, scope));
 
+                for(Map.Entry<String, Object> entry: scope.services()) {
+                    if(entry.getValue() == backstack) {
+                        throw new IllegalArgumentException("The root backstack should not be added as a service, as it would cause a circular save-state loop. Adding it as an alias would work, but should typically not be necessary because of `serviceBinder.getBackstack()`.");
+                    }
+                }
+
                 restoreAndNotifyServices(scopeTag, scope);
             }
         }

diff --git a/simple-stack/src/test/java/com/zhuinden/simplestack/ScopingGlobalScopeTest.java b/simple-stack/src/test/java/com/zhuinden/simplestack/ScopingGlobalScopeTest.java
@@ -24,6 +24,7 @@
 import com.zhuinden.simplestack.helpers.TestKeyWithScope;
 import com.zhuinden.statebundle.StateBundle;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import java.util.ArrayList;
@@ -733,8 +734,9 @@ public void globalServicesFactoryWorks() {
 
         Backstack backstack = new Backstack();
         backstack.setGlobalServices(new GlobalServices.Factory() {
+            @Nonnull
             @Override
-            public GlobalServices create() {
+            public GlobalServices create(@Nonnull Backstack backstack) {
                 return GlobalServices.builder()
                         .addService("service", service)
                         .build();
@@ -764,8 +766,9 @@ public void globalServicesFactoryOverridesGlobalServices() {
                 .build());
 
         backstack.setGlobalServices(new GlobalServices.Factory() {
+            @Nonnull
             @Override
-            public GlobalServices create() {
+            public GlobalServices create(@Nonnull Backstack backstack) {
                 return GlobalServices.builder()
                         .addService("service2", service2)
                         .build();
@@ -791,8 +794,9 @@ public void globalServicesFactoryRunsAgainAfterFinalization() {
 
         final AtomicReference<Object> serviceRef = new AtomicReference<>();
         backstack.setGlobalServices(new GlobalServices.Factory() {
+            @Nonnull
             @Override
-            public GlobalServices create() {
+            public GlobalServices create(@Nonnull Backstack backstack) {
                 Object service = new Object();
                 serviceRef.set(service);
                 return GlobalServices.builder()
@@ -824,4 +828,55 @@ public void handleStateChange(@Nonnull StateChange stateChange, @Nonnull Callbac
 
         assertThat(service1).isNotSameAs(service2);
     }
+
+
+    @Test
+    public void globalServicesFactoryFailsIfBackstackIsAddedAsService() {
+        Backstack backstack = new Backstack();
+
+        backstack.setGlobalServices(new GlobalServices.Factory() {
+            @Nonnull
+            @Override
+            public GlobalServices create(@Nonnull Backstack backstack) {
+                return GlobalServices.builder().addService("backstack", backstack).build();
+            }
+        });
+
+        backstack.setup(History.of(new TestKey("hello!")));
+        try {
+            backstack.setStateChanger(new StateChanger() {
+                @Override
+                public void handleStateChange(@Nonnull StateChange stateChange, @Nonnull Callback completionCallback) {
+                    completionCallback.stateChangeComplete();
+                }
+            });
+            StateBundle bundle = backstack.toBundle();
+            Assert.fail("This would fail on `toBundle()`");
+        } catch(IllegalArgumentException e) {
+            // OK!
+        }
+    }
+
+    @Test
+    public void globalServicesFactorySucceedsIfBackstackIsAddedAsAlias() {
+        Backstack backstack = new Backstack();
+
+        backstack.setGlobalServices(new GlobalServices.Factory() {
+            @Nonnull
+            @Override
+            public GlobalServices create(@Nonnull Backstack backstack) {
+                return GlobalServices.builder().addAlias("backstack", backstack).build();
+            }
+        });
+
+        backstack.setup(History.of(new TestKey("hello!")));
+        backstack.setStateChanger(new StateChanger() {
+                @Override
+                public void handleStateChange(@Nonnull StateChange stateChange, @Nonnull Callback completionCallback) {
+                    completionCallback.stateChangeComplete();
+            }
+            });
+
+        assertThat(backstack.lookupService("backstack")).isSameAs(backstack);
+    }
 }
diff --git a/simple-stack/src/test/java/com/zhuinden/simplestack/ScopingTest.java b/simple-stack/src/test/java/com/zhuinden/simplestack/ScopingTest.java
@@ -24,8 +24,7 @@
 import com.zhuinden.simplestack.helpers.TestKeyWithOnlyParentServices;
 import com.zhuinden.statebundle.StateBundle;
 
-import junit.framework.Assert;
-
+import org.junit.Assert;
 import org.junit.Test;
 import org.mockito.Mockito;
 
@@ -225,6 +224,65 @@ public String getScopeTag() {
         }
     }
 
+    @Test
+    public void serviceBinderThrowsIfRootBackstackIsAService() {
+        final String serviceTag = "backstack";
+
+        final Backstack backstack = new Backstack();
+
+        TestKeyWithScope testKeyWithScope = new TestKeyWithScope("blah") {
+            @Override
+            public void bindServices(ServiceBinder serviceBinder) {
+                assertThat(serviceBinder.getScopeTag()).isEqualTo(getScopeTag());
+                serviceBinder.addService(serviceTag, backstack);
+            }
+
+            @Nonnull
+            @Override
+            public String getScopeTag() {
+                return "beep";
+            }
+        };
+
+
+        backstack.setScopedServices(new ServiceProvider());
+        backstack.setup(History.of(testKeyWithScope));
+        try {
+            backstack.setStateChanger(stateChanger);
+            Assert.fail("This would cause a save-state loop in toBundle()");
+        } catch(IllegalArgumentException e) {
+            // OK!
+        }
+    }
+
+    @Test
+    public void serviceBinderSucceedsIfRootBackstackIsAnAlias() {
+        final String serviceTag = "backstack";
+
+        final Backstack backstack = new Backstack();
+
+        TestKeyWithScope testKeyWithScope = new TestKeyWithScope("blah") {
+            @Override
+            public void bindServices(ServiceBinder serviceBinder) {
+                assertThat(serviceBinder.getScopeTag()).isEqualTo(getScopeTag());
+                serviceBinder.addAlias(serviceTag, backstack);
+            }
+
+            @Nonnull
+            @Override
+            public String getScopeTag() {
+                return "beep";
+            }
+        };
+
+
+        backstack.setScopedServices(new ServiceProvider());
+        backstack.setup(History.of(testKeyWithScope));
+        backstack.setStateChanger(stateChanger);
+
+        assertThat(backstack.lookupService(serviceTag)).isSameAs(backstack);
+    }
+
     @Test
     public void serviceBinderAddThrowsForNullService() {
         final String serviceTag = "serviceTag";