Fix Conductor pending changes lock, related to #287 discussion (#570)

bluelinelabs · Mar 28, 2020 · 6df6ce8 · 6df6ce8
1 parent 7ddc115
commit 6df6ce8
Show file tree

Hide file tree

Showing 4 changed files with 114 additions and 2 deletions.
diff --git a/conductor/src/main/java/com/bluelinelabs/conductor/Router.java b/conductor/src/main/java/com/bluelinelabs/conductor/Router.java
@@ -631,10 +631,13 @@ public void onActivityDestroyed(@NonNull Activity activity) {
     }
 
     public void prepareForHostDetach() {
+        pendingControllerChanges.clear(); // rely on backstack based restoration in rebindIfNeeded
+
         for (RouterTransaction transaction : backstack) {
             if (ControllerChangeHandler.completeHandlerImmediately(transaction.controller.getInstanceId())) {
                 transaction.controller.setNeedsAttach(true);
             }
+
             transaction.controller.prepareForHostDetach();
         }
     }
@@ -820,11 +823,17 @@ private void performControllerChange(@Nullable final Controller to, @Nullable fi
         if (pendingControllerChanges.size() > 0) {
             // If we already have changes queued up (awaiting full container attach), queue this one up as well so they don't happen
             // out of order.
+            if (to != null) {
+                to.setNeedsAttach(true);
+            }
             pendingControllerChanges.add(transaction);
         } else if (from != null && (changeHandler == null || changeHandler.removesFromViewOnPush()) && !containerFullyAttached) {
             // If the change handler will remove the from view, we have to make sure the container is fully attached first so we avoid NPEs
             // within ViewGroup (details on issue #287). Post this to the container to ensure the attach is complete before we try to remove
             // anything.
+            if (to != null) {
+                to.setNeedsAttach(true);
+            }
             pendingControllerChanges.add(transaction);
             container.post(new Runnable() {
                 @Override

diff --git a/conductor/src/test/java/com/bluelinelabs/conductor/ReattachCaseTests.java b/conductor/src/test/java/com/bluelinelabs/conductor/ReattachCaseTests.java
@@ -3,7 +3,9 @@
 import android.os.Bundle;
 import android.view.ViewGroup;
 
+import com.bluelinelabs.conductor.internal.LifecycleHandler;
 import com.bluelinelabs.conductor.util.ActivityProxy;
+import com.bluelinelabs.conductor.util.AttachFakingFrameLayout;
 import com.bluelinelabs.conductor.util.MockChangeHandler;
 import com.bluelinelabs.conductor.util.TestController;
 import com.bluelinelabs.conductor.util.ViewUtils;
@@ -287,6 +289,67 @@ public void testPopMiddleControllerAttaches() {
         assertTrue(controller3.isAttached());
     }
 
+    @Test
+    public void testPendingChanges() {
+        Controller controller1 = new TestController();
+        Controller controller2 = new TestController();
+
+        ActivityProxy activityProxy = new ActivityProxy().create(null);
+        AttachFakingFrameLayout container = new AttachFakingFrameLayout(activityProxy.getActivity());
+        container.setNeedDelayPost(true); // to simulate calling posts after resume
+
+        activityProxy.setView(container);
+
+        Router router = Conductor.attachRouter(activityProxy.getActivity(), container, null);
+        router.setRoot(RouterTransaction.with(controller1));
+        router.pushController(RouterTransaction.with(controller2));
+
+        activityProxy.start().resume();
+        container.setNeedDelayPost(false);
+
+        assertTrue(controller2.isAttached());
+    }
+
+    @Test
+    public void testPendingChangesAfterRotation() {
+        Controller controller1 = new TestController();
+        Controller controller2 = new TestController();
+
+        // first activity
+        ActivityProxy activityProxy = new ActivityProxy().create(null);
+        AttachFakingFrameLayout container1 = new AttachFakingFrameLayout(activityProxy.getActivity());
+
+        container1.setNeedDelayPost(true); // delay forever as view will be removed
+        activityProxy.setView(container1);
+
+        // first attachRouter: Conductor.attachRouter(activityProxy.getActivity(), container1, null)
+        LifecycleHandler lifecycleHandler = LifecycleHandler.install(activityProxy.getActivity());
+        Router router = lifecycleHandler.getRouter(container1, null);
+        router.setRoot(RouterTransaction.with(controller1));
+
+        // setup controllers
+        router.pushController(RouterTransaction.with(controller2));
+
+        // simulate setRequestedOrientation in activity onCreate
+        activityProxy.start().resume();
+        Bundle savedState = new Bundle();
+        activityProxy.saveInstanceState(savedState).pause().stop(true);
+
+        // recreate activity and view
+        activityProxy = new ActivityProxy().create(savedState);
+        AttachFakingFrameLayout container2 = new AttachFakingFrameLayout(activityProxy.getActivity());
+        activityProxy.setView(container2);
+
+        // second attach router with the same lifecycleHandler (do manually as Roboelectric recreates retained fragments)
+        // Conductor.attachRouter(activityProxy.getActivity(), container2, savedState);
+        router = lifecycleHandler.getRouter(container2, savedState);
+        router.rebindIfNeeded();
+
+        activityProxy.start().resume();
+
+        assertTrue(controller2.isAttached());
+    }
+
     private void sleepWakeDevice() {
         activityProxy.saveInstanceState(new Bundle()).pause();
         activityProxy.resume();

diff --git a/conductor/src/test/java/com/bluelinelabs/conductor/util/ActivityProxy.java b/conductor/src/test/java/com/bluelinelabs/conductor/util/ActivityProxy.java
@@ -7,18 +7,23 @@
 import org.robolectric.android.controller.ActivityController;
 
 public class ActivityProxy {
+    private final @IdRes int containerId = 4;
 
     private ActivityController<TestActivity> activityController;
     private AttachFakingFrameLayout view;
 
     public ActivityProxy() {
         activityController = Robolectric.buildActivity(TestActivity.class);
 
-        @IdRes int containerId = 4;
         view = new AttachFakingFrameLayout(activityController.get());
         view.setId(containerId);
     }
 
+    public void setView(AttachFakingFrameLayout view) {
+        this.view = view;
+        view.setId(containerId);
+    }
+
     public ActivityProxy create(Bundle savedInstanceState) {
         activityController.create(savedInstanceState);
         return this;

diff --git a/conductor/src/test/java/com/bluelinelabs/conductor/util/AttachFakingFrameLayout.java b/conductor/src/test/java/com/bluelinelabs/conductor/util/AttachFakingFrameLayout.java
@@ -10,6 +10,8 @@
 import android.widget.FrameLayout;
 
 import java.io.FileDescriptor;
+import java.util.ArrayList;
+import java.util.List;
 
 public class AttachFakingFrameLayout extends FrameLayout {
 
@@ -62,6 +64,9 @@ public boolean unlinkToDeath(DeathRecipient recipient, int flags) {
 
     private boolean reportAttached;
 
+    private boolean needDelayPost;
+    private List<Runnable> delayedPosts = new ArrayList<>();
+
     public AttachFakingFrameLayout(Context context) {
         super(context);
     }
@@ -110,4 +115,34 @@ public void onViewRemoved(View child) {
         super.onViewRemoved(child);
     }
 
-}
+    @Override
+    public boolean post(Runnable action) {
+        if (needDelayPost) {
+            delayedPosts.add(action);
+        } else {
+            return super.post(action);
+        }
+
+        return true;
+    }
+
+    public void runDelayedPosts() {
+        for (Runnable runnable : delayedPosts) {
+            runnable.run();
+        }
+
+        clearDelayedPosts();
+    }
+
+    public void clearDelayedPosts() {
+        delayedPosts.clear();
+    }
+
+    public void setNeedDelayPost(boolean needDelayPost) {
+        this.needDelayPost = needDelayPost;
+
+        if (!this.needDelayPost && delayedPosts.size() > 0) {
+            runDelayedPosts();
+        }
+    }
+}