WIP fix for parents detaching while we're transitioning

Needs tests before merging

conductor/src/main/java/com/bluelinelabs/conductor/Controller.java

@@ -90,6 +90,7 @@ public abstract class Controller {
     private final ArrayList<RouterRequiringFunc> onRouterSetListeners = new ArrayList<>();
     private WeakReference<View> destroyedView;
     private boolean isPerformingExitTransition;
+    private boolean willBeDetachedAfterTransition;
     private boolean isContextAvailable;
 
     @NonNull
@@ -814,7 +815,7 @@ public abstract class Controller {
     }
 
     final void prepareForHostDetach() {
-        needsAttach = needsAttach || attached;
+        needsAttach = needsAttach || (attached && !willBeDetachedAfterTransition && view.getParent() != null);
 
         for (ControllerHostedRouter router : childRouters) {
             router.prepareForHostDetach();
@@ -1325,6 +1326,7 @@ public abstract class Controller {
     final void changeStarted(@NonNull ControllerChangeHandler changeHandler, @NonNull ControllerChangeType changeType) {
         if (!changeType.isEnter) {
             isPerformingExitTransition = true;
+            willBeDetachedAfterTransition = changeHandler.removesFromViewOnPush();
             for (ControllerHostedRouter router : childRouters) {
                 router.setDetachFrozen(true);
             }
@@ -1341,6 +1343,7 @@ public abstract class Controller {
     final void changeEnded(@NonNull ControllerChangeHandler changeHandler, @NonNull ControllerChangeType changeType) {
         if (!changeType.isEnter) {
             isPerformingExitTransition = false;
+            willBeDetachedAfterTransition = false;
             for (ControllerHostedRouter router : childRouters) {
                 router.setDetachFrozen(false);
             }
@@ -1364,20 +1367,21 @@ public abstract class Controller {
         changeHandler.onEnd();
     }
 
-    final void setDetachFrozen(boolean frozen) {
-        if (isDetachFrozen != frozen) {
-            isDetachFrozen = frozen;
+    final void onDetachFreezeUpdated(boolean frozen) {
+        if (isDetachFrozen == frozen) {
+            return;
+        }
 
-            for (ControllerHostedRouter router : childRouters) {
-                router.setDetachFrozen(frozen);
-            }
+        isDetachFrozen = frozen;
+        for (ControllerHostedRouter router : childRouters) {
+            router.onParentDetachFreezeUpdated(frozen);
+        }
 
-            if (!frozen && view != null && viewWasDetached) {
-                View aView = view;
-                detach(view, false, false);
-                if (view == null && aView.getParent() == router.container) {
-                    router.container.removeView(aView); // need to remove the view when this controller is a child controller
-                }
+        if (!frozen && view != null && viewWasDetached) {
+            View aView = view;
+            detach(view, false, false);
+            if (view == null && aView.getParent() == router.container) {
+                router.container.removeView(aView); // need to remove the view when this controller is a child controller
             }
         }
     }

conductor/src/main/java/com/bluelinelabs/conductor/ControllerHostedRouter.java

@@ -29,6 +29,7 @@ class ControllerHostedRouter extends Router {
 
     @IdRes private int hostId;
     private String tag;
+    private boolean isParentDetachFrozen;
     private boolean isDetachFrozen;
     private boolean boundToContainer;
 
@@ -89,13 +90,24 @@ class ControllerHostedRouter extends Router {
         container = null;
     }
 
+    final void onParentDetachFreezeUpdated(boolean frozen) {
+        isParentDetachFrozen = frozen;
+        for (RouterTransaction transaction : backstack) {
+            reportDetachFrozen(transaction.controller());
+        }
+    }
+
     final void setDetachFrozen(boolean frozen) {
         isDetachFrozen = frozen;
         for (RouterTransaction transaction : backstack) {
-            transaction.controller().setDetachFrozen(frozen);
+            reportDetachFrozen(transaction.controller());
         }
     }
 
+    private void reportDetachFrozen(@NonNull Controller controller) {
+        controller.onDetachFreezeUpdated(isDetachFrozen || isParentDetachFrozen);
+    }
+
     @Override
     void destroy(boolean popViews) {
         setDetachFrozen(false);
@@ -104,17 +116,17 @@ class ControllerHostedRouter extends Router {
 
     @Override
     protected void pushToBackstack(@NonNull RouterTransaction entry) {
-        if (isDetachFrozen) {
-            entry.controller().setDetachFrozen(true);
+        if (isDetachFrozen || isParentDetachFrozen) {
+            reportDetachFrozen(entry.controller());
         }
         super.pushToBackstack(entry);
     }
 
     @Override
     public void setBackstack(@NonNull List<RouterTransaction> newBackstack, @Nullable ControllerChangeHandler changeHandler) {
-        if (isDetachFrozen) {
+        if (isDetachFrozen || isParentDetachFrozen) {
             for (RouterTransaction transaction : newBackstack) {
-                transaction.controller().setDetachFrozen(true);
+                reportDetachFrozen(transaction.controller());
             }
         }
         super.setBackstack(newBackstack, changeHandler);

conductor/src/test/java/com/bluelinelabs/conductor/ControllerLifecycleCallbacksTests.kt

@@ -572,6 +572,45 @@ class ControllerLifecycleCallbacksTests {
     Assert.assertTrue(child2.isAttached)
   }
 
+  @Test
+  fun testChildTransactionDuringParentTransaction() {
+    val parent = TestController()
+    parent.retainViewMode = RetainViewMode.RETAIN_DETACH
+    activityController.get().router.pushController(parent.asTransaction())
+
+    val child = TestController()
+    val childRouter = parent.getChildRouter(parent.getView()!!.findViewById(TestController.VIEW_ID))
+    childRouter.setRoot(child.asTransaction())
+
+    val childMockChangeHandler = MockChangeHandler.defaultHandler()
+    val childDelayHandler = childMockChangeHandler.delayTransaction()
+
+    val child2 = TestController()
+    childRouter.pushController(child2.asTransaction(
+      pushChangeHandler = childMockChangeHandler,
+    ))
+
+    shadowOf(getMainLooper()).idle()
+
+    val parentMockChangeHandler = MockChangeHandler.defaultHandler()
+    val parentDelayHandler = parentMockChangeHandler.delayTransaction()
+
+    activityController.get().router.pushController(
+      TestController().asTransaction(
+        pushChangeHandler = parentMockChangeHandler,
+      )
+    )
+
+    childDelayHandler.onDelayEnded()
+    parentDelayHandler.onDelayEnded()
+
+    activityController.get().router.popCurrentController()
+    shadowOf(getMainLooper()).idle()
+
+    Assert.assertFalse(child.isAttached)
+    Assert.assertTrue(child2.isAttached)
+  }
+
   private fun getPushHandler(
     expectedCallState: CallState,
     controller: TestController

conductor/src/test/java/com/bluelinelabs/conductor/util/MockChangeHandler.java

@@ -1,123 +0,0 @@
-package com.bluelinelabs.conductor.util;
-
-import android.os.Bundle;
-import androidx.annotation.NonNull;
-import androidx.annotation.Nullable;
-import android.view.View;
-import android.view.ViewGroup;
-
-import com.bluelinelabs.conductor.ControllerChangeHandler;
-
-public class MockChangeHandler extends ControllerChangeHandler {
-
-    private static final String KEY_REMOVES_FROM_VIEW_ON_PUSH = "MockChangeHandler.removesFromViewOnPush";
-    private static final String KEY_TAG = "MockChangeHandler.tag";
-
-    public static class ChangeHandlerListener {
-        public void willStartChange() { }
-        public void didAttachOrDetach() { }
-        public void didEndChange() { }
-    }
-
-    private final ChangeHandlerListener listener;
-    private boolean removesFromViewOnPush;
-
-    public View from;
-    public View to;
-    public String tag;
-
-    public static MockChangeHandler defaultHandler() {
-        return new MockChangeHandler(true, null, null);
-    }
-
-    public static MockChangeHandler noRemoveViewOnPushHandler() {
-        return new MockChangeHandler(false, null, null);
-    }
-
-    public static MockChangeHandler noRemoveViewOnPushHandler(String tag) {
-        return new MockChangeHandler(false, tag, null);
-    }
-
-    public static MockChangeHandler listeningChangeHandler(@NonNull ChangeHandlerListener listener) {
-        return new MockChangeHandler(true, null, listener);
-    }
-
-    public static MockChangeHandler taggedHandler(String tag, boolean removeViewOnPush) {
-        return new MockChangeHandler(removeViewOnPush, tag, null);
-    }
-
-    public MockChangeHandler() {
-        listener = null;
-    }
-
-    private MockChangeHandler(boolean removesFromViewOnPush, String tag, ChangeHandlerListener listener) {
-        this.removesFromViewOnPush = removesFromViewOnPush;
-        this.tag = tag;
-
-        if (listener == null) {
-            this.listener = new ChangeHandlerListener() { };
-        } else {
-            this.listener = listener;
-        }
-    }
-
-    @Override
-    public void performChange(@NonNull ViewGroup container, @Nullable View from, @Nullable View to, boolean isPush, @NonNull ControllerChangeCompletedListener changeListener) {
-        this.from = from;
-        this.to = to;
-
-        listener.willStartChange();
-
-        if (isPush) {
-            if (to != null) {
-                container.addView(to);
-                listener.didAttachOrDetach();
-            }
-
-            if (removesFromViewOnPush && from != null) {
-                container.removeView(from);
-            }
-        } else {
-            container.removeView(from);
-            listener.didAttachOrDetach();
-
-            if (to != null) {
-                container.addView(to);
-            }
-
-        }
-
-        changeListener.onChangeCompleted();
-        listener.didEndChange();
-    }
-
-    @Override
-    public boolean removesFromViewOnPush() {
-        return removesFromViewOnPush;
-    }
-
-    @Override
-    public void saveToBundle(@NonNull Bundle bundle) {
-        super.saveToBundle(bundle);
-        bundle.putBoolean(KEY_REMOVES_FROM_VIEW_ON_PUSH, removesFromViewOnPush);
-        bundle.putString(KEY_TAG, tag);
-    }
-
-    @Override
-    public void restoreFromBundle(@NonNull Bundle bundle) {
-        super.restoreFromBundle(bundle);
-        removesFromViewOnPush = bundle.getBoolean(KEY_REMOVES_FROM_VIEW_ON_PUSH);
-        tag = bundle.getString(KEY_TAG);
-    }
-
-    @NonNull
-    @Override
-    public ControllerChangeHandler copy() {
-        return new MockChangeHandler(removesFromViewOnPush, tag, listener);
-    }
-
-    @Override
-    public boolean isReusable() {
-        return true;
-    }
-}

conductor/src/test/java/com/bluelinelabs/conductor/util/MockChangeHandler.kt

@@ -0,0 +1,132 @@
+package com.bluelinelabs.conductor.util
+
+import android.os.Bundle
+import android.view.View
+import android.view.ViewGroup
+import com.bluelinelabs.conductor.ControllerChangeHandler
+
+class MockChangeHandler @JvmOverloads constructor(
+  private var removesFromViewOnPush: Boolean = false,
+  var tag: String? = null,
+  private val listener: ChangeHandlerListener? = null,
+) : ControllerChangeHandler() {
+  @JvmField
+  var from: View? = null
+  @JvmField
+  var to: View? = null
+  private var delayHandler: DelayHandler? = null
+
+  fun delayTransaction(): DelayHandler {
+    return object : DelayHandler() {
+      override fun onDelayEnded() {
+        val changeData = changeData ?: throw IllegalStateException("Attempting to end transaction delay before ready.")
+        performChange(changeData)
+      }
+    }.also { delayHandler = it }
+  }
+
+  override fun performChange(
+    container: ViewGroup,
+    from: View?,
+    to: View?,
+    isPush: Boolean,
+    changeListener: ControllerChangeCompletedListener
+  ) {
+    if (delayHandler != null) {
+      delayHandler!!.changeData = ChangeData(container, from, to, isPush, changeListener)
+    } else {
+      performChange(ChangeData(container, from, to, isPush, changeListener))
+    }
+  }
+
+  override fun removesFromViewOnPush(): Boolean {
+    return removesFromViewOnPush
+  }
+
+  override fun saveToBundle(bundle: Bundle) {
+    super.saveToBundle(bundle)
+    bundle.putBoolean(KEY_REMOVES_FROM_VIEW_ON_PUSH, removesFromViewOnPush)
+    bundle.putString(KEY_TAG, tag)
+  }
+
+  override fun restoreFromBundle(bundle: Bundle) {
+    super.restoreFromBundle(bundle)
+    removesFromViewOnPush = bundle.getBoolean(KEY_REMOVES_FROM_VIEW_ON_PUSH)
+    tag = bundle.getString(KEY_TAG)
+  }
+
+  override fun copy(): ControllerChangeHandler {
+    return MockChangeHandler(removesFromViewOnPush, tag, listener)
+  }
+
+  override fun isReusable() = true
+
+  private fun performChange(changeData: ChangeData) {
+    from = changeData.from
+    to = changeData.to
+
+    listener?.willStartChange()
+
+    if (changeData.isPush) {
+      if (to != null) {
+        changeData.container.addView(to)
+        listener?.didAttachOrDetach()
+      }
+      if (removesFromViewOnPush && from != null) {
+        changeData.container.removeView(from)
+      }
+    } else {
+      changeData.container.removeView(from)
+      listener?.didAttachOrDetach()
+      if (to != null) {
+        changeData.container.addView(to)
+      }
+    }
+
+    changeData.changeListener.onChangeCompleted()
+    listener?.didEndChange()
+  }
+
+  companion object {
+    private const val KEY_REMOVES_FROM_VIEW_ON_PUSH = "MockChangeHandler.removesFromViewOnPush"
+    private const val KEY_TAG = "MockChangeHandler.tag"
+    fun defaultHandler(): MockChangeHandler {
+      return MockChangeHandler(true, null, null)
+    }
+
+    fun noRemoveViewOnPushHandler(): MockChangeHandler {
+      return MockChangeHandler(false, null, null)
+    }
+
+    fun noRemoveViewOnPushHandler(tag: String?): MockChangeHandler {
+      return MockChangeHandler(false, tag, null)
+    }
+
+    fun listeningChangeHandler(listener: ChangeHandlerListener): MockChangeHandler {
+      return MockChangeHandler(true, null, listener)
+    }
+
+    fun taggedHandler(tag: String?, removeViewOnPush: Boolean): MockChangeHandler {
+      return MockChangeHandler(removeViewOnPush, tag, null)
+    }
+  }
+
+  open class ChangeHandlerListener {
+    open fun willStartChange() {}
+    open fun didAttachOrDetach() {}
+    open fun didEndChange() {}
+  }
+
+  abstract class DelayHandler {
+    var changeData: ChangeData? = null
+    abstract fun onDelayEnded()
+  }
+
+  data class ChangeData(
+    val container: ViewGroup,
+    val from: View?,
+    val to: View?,
+    val isPush: Boolean,
+    val changeListener: ControllerChangeCompletedListener,
+  )
+}