Fixes issues in Router.pushController, Router.popController & Router.setBackstack where pushChangeHandler().removesFromViewOnPush() was not respected

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

@@ -135,7 +135,22 @@ public boolean popController(@NonNull Controller controller) {
 
         if (poppingTopController) {
             trackDestroyingController(backstack.pop());
-            performControllerChange(backstack.peek(), topTransaction, false);
+            // Ensure all entries on the backstack, which should be visible, becomes so
+            List<RouterTransaction> newVisibleTransactions = getVisibleTransactions(backstack.iterator(), false);
+            if (newVisibleTransactions.isEmpty()) {
+                // Nothing else is visible, so just pop
+                performControllerChange(backstack.peek(), topTransaction, false);
+            } else {
+                // Loop backwards to ensure the last change made is the new top Controller
+                for (int i = newVisibleTransactions.size() - 1; i >= 0; i--) {
+                    RouterTransaction newVisibleTransaction = newVisibleTransactions.get(i);
+                    if (i == 0) {
+                        performControllerChange(newVisibleTransaction, topTransaction, false, topTransaction.popChangeHandler());
+                    } else {
+                        performControllerChange(newVisibleTransaction, null, false, topTransaction.popChangeHandler());
+                    }
+                }
+            }
         } else {
             RouterTransaction removedTransaction = null;
             RouterTransaction nextTransaction = null;
@@ -179,8 +194,12 @@ public boolean popController(@NonNull Controller controller) {
     public void pushController(@NonNull RouterTransaction transaction) {
         ThreadUtils.ensureMainThread();
 
+        List<RouterTransaction> oldVisibleTransactions = getVisibleTransactions(backstack.iterator(), false);
         RouterTransaction from = backstack.peek();
         pushToBackstack(transaction);
+
+        ensureRemovesFromViewOnPushRemovalIsEnforced(transaction, from, oldVisibleTransactions, null);
+
         performControllerChange(transaction, from, true);
     }
 
@@ -201,16 +220,7 @@ public void replaceTopController(@NonNull RouterTransaction transaction) {
         }
 
         final ControllerChangeHandler handler = transaction.pushChangeHandler();
-        if (topTransaction != null) {
-            //noinspection ConstantConditions
-            final boolean oldHandlerRemovedViews = topTransaction.pushChangeHandler() == null || topTransaction.pushChangeHandler().removesFromViewOnPush();
-            final boolean newHandlerRemovesViews = handler == null || handler.removesFromViewOnPush();
-            if (!oldHandlerRemovedViews && newHandlerRemovesViews) {
-                for (RouterTransaction visibleTransaction : getVisibleTransactions(backstack.iterator(), true)) {
-                    performControllerChange(null, visibleTransaction, true, handler);
-                }
-            }
-        }
+        ensureRemovesFromViewOnPushRemovalIsEnforced(transaction, topTransaction, getVisibleTransactions(backstack.iterator(), true), handler);
 
         pushToBackstack(transaction);
 
@@ -931,6 +941,33 @@ private void removeAllExceptVisibleAndUnowned() {
         }
     }
 
+    /**
+     * Goes through all {@param oldVisibleTransactions} to ensure {@link ControllerChangeHandler#removesFromViewOnPush()} == false
+     * is only respected if the preceding {@link Controller} is also visible. Detaches all {@link Controller}s who are no longer supposed to be attached.
+     *
+     * @param newTopTransaction      The transaction intended to be the new top
+     * @param oldTopTransaction      The transaction which was previously the top, if any
+     * @param oldVisibleTransactions All transactions which were previously deemed visible as per {@link Router#getVisibleTransactions(Iterator, boolean)}
+     * @param changeHandler          Optional {@link ControllerChangeHandler} to override transition when removing any {@link Controller}s deemed to require removal
+     */
+    private void ensureRemovesFromViewOnPushRemovalIsEnforced(
+            @NonNull RouterTransaction newTopTransaction,
+            @Nullable RouterTransaction oldTopTransaction,
+            @NonNull List<RouterTransaction> oldVisibleTransactions,
+            @Nullable ControllerChangeHandler changeHandler) {
+        if (oldTopTransaction != null) {
+            final ControllerChangeHandler newChangeHandler = newTopTransaction.pushChangeHandler() != null ? newTopTransaction.pushChangeHandler() : changeHandler;
+            //noinspection ConstantConditions
+            final boolean oldHandlerRemovedViews = oldTopTransaction.pushChangeHandler() == null || oldTopTransaction.pushChangeHandler().removesFromViewOnPush();
+            final boolean newHandlerRemovesViews = newChangeHandler == null || newChangeHandler.removesFromViewOnPush();
+            if (!oldHandlerRemovedViews && newHandlerRemovesViews) {
+                for (RouterTransaction visibleTransaction : oldVisibleTransactions) {
+                    performControllerChange(null, visibleTransaction, false);
+                }
+            }
+        }
+    }
+
     // Swap around transaction indices to ensure they don't get thrown out of order by the
     // developer rearranging the backstack at runtime.
     private void ensureOrderedTransactionIndices(List<RouterTransaction> backstack) {
@@ -981,7 +1018,9 @@ private List<RouterTransaction> getVisibleTransactions(@NonNull Iterator<RouterT
                 transactions.add(transaction);
             }
 
-            visible = transaction.pushChangeHandler() != null && !transaction.pushChangeHandler().removesFromViewOnPush();
+            // We only care about removesFromViewOnPush if the current transaction is visible
+            //noinspection ConstantConditions
+            visible = visible && transaction.pushChangeHandler() != null && !transaction.pushChangeHandler().removesFromViewOnPush();
 
             if (onlyTop && !visible) {
                 break;

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

@@ -251,6 +251,103 @@ class RouterTests {
     Assert.assertTrue(topTransaction.controller.isAttached)
   }
 
+  @Test
+  fun testPushThenPopWithSetBackstackWithNoRemoveViewOnPush() {
+    val oldRootTransaction = TestController().asTransaction()
+    val oldTopTransaction = TestController().asTransaction(
+      pushChangeHandler = MockChangeHandler.noRemoveViewOnPushHandler()
+    )
+    router.setRoot(oldRootTransaction)
+    router.pushController(oldTopTransaction)
+    Assert.assertEquals(2, router.backstackSize.toLong())
+    Assert.assertTrue(oldRootTransaction.controller.isAttached)
+    Assert.assertTrue(oldTopTransaction.controller.isAttached)
+
+    val newTopTransaction = TestController().asTransaction()
+    router.pushController(newTopTransaction)
+
+    Assert.assertEquals(3, router.backstackSize.toLong())
+    Assert.assertFalse(oldRootTransaction.controller.isAttached)
+    Assert.assertFalse(oldTopTransaction.controller.isAttached)
+    Assert.assertTrue(newTopTransaction.controller.isAttached)
+
+    val backstack2 = listOf(oldRootTransaction, oldTopTransaction)
+    router.setBackstack(backstack2, null)
+
+    Assert.assertEquals(2, router.backstackSize.toLong())
+    val fetchedBackstack = router.getBackstack()
+    Assert.assertEquals(oldRootTransaction, fetchedBackstack[0])
+    Assert.assertEquals(oldTopTransaction, fetchedBackstack[1])
+    Assert.assertTrue(oldRootTransaction.controller.isAttached)
+    Assert.assertTrue(oldTopTransaction.controller.isAttached)
+    Assert.assertFalse(newTopTransaction.controller.isAttached)
+  }
+
+  @Test
+  fun testPopUsingSetBackstackWithNoRemoveViewOnPush() {
+    val oldRootTransaction = TestController().asTransaction()
+    val oldTopTransaction = TestController().asTransaction(
+      pushChangeHandler = MockChangeHandler.noRemoveViewOnPushHandler()
+    )
+    router.setRoot(oldRootTransaction)
+    router.pushController(oldTopTransaction)
+    Assert.assertEquals(2, router.backstackSize.toLong())
+    Assert.assertTrue(oldRootTransaction.controller.isAttached)
+    Assert.assertTrue(oldTopTransaction.controller.isAttached)
+
+    val newTopTransaction = TestController().asTransaction()
+    val backstack = listOf(oldRootTransaction, oldTopTransaction, newTopTransaction)
+    router.setBackstack(backstack, null)
+
+    Assert.assertEquals(3, router.backstackSize.toLong())
+    Assert.assertFalse(oldRootTransaction.controller.isAttached)
+    Assert.assertFalse(oldTopTransaction.controller.isAttached)
+    Assert.assertTrue(newTopTransaction.controller.isAttached)
+
+    val backstack2 = listOf(oldRootTransaction, oldTopTransaction)
+    router.setBackstack(backstack2, null)
+
+    Assert.assertEquals(2, router.backstackSize.toLong())
+    val fetchedBackstack = router.getBackstack()
+    Assert.assertEquals(oldRootTransaction, fetchedBackstack[0])
+    Assert.assertEquals(oldTopTransaction, fetchedBackstack[1])
+    Assert.assertTrue(oldRootTransaction.controller.isAttached)
+    Assert.assertTrue(oldTopTransaction.controller.isAttached)
+    Assert.assertFalse(newTopTransaction.controller.isAttached)
+  }
+
+  @Test
+  fun testPopWithNoRemoveViewOnPush() {
+    val oldRootTransaction = TestController().asTransaction()
+    val oldTopTransaction = TestController().asTransaction(
+      pushChangeHandler = MockChangeHandler.noRemoveViewOnPushHandler()
+    )
+    router.setRoot(oldRootTransaction)
+    router.pushController(oldTopTransaction)
+    Assert.assertEquals(2, router.backstackSize.toLong())
+    Assert.assertTrue(oldRootTransaction.controller.isAttached)
+    Assert.assertTrue(oldTopTransaction.controller.isAttached)
+
+    val newTopTransaction = TestController().asTransaction()
+    val backstack = listOf(oldRootTransaction, oldTopTransaction, newTopTransaction)
+    router.setBackstack(backstack, null)
+
+    Assert.assertEquals(3, router.backstackSize.toLong())
+    Assert.assertFalse(oldRootTransaction.controller.isAttached)
+    Assert.assertFalse(oldTopTransaction.controller.isAttached)
+    Assert.assertTrue(newTopTransaction.controller.isAttached)
+
+    router.popCurrentController()
+
+    Assert.assertEquals(2, router.backstackSize.toLong())
+    val fetchedBackstack = router.getBackstack()
+    Assert.assertEquals(oldRootTransaction, fetchedBackstack[0])
+    Assert.assertEquals(oldTopTransaction, fetchedBackstack[1])
+    Assert.assertTrue(oldRootTransaction.controller.isAttached)
+    Assert.assertTrue(oldTopTransaction.controller.isAttached)
+    Assert.assertFalse(newTopTransaction.controller.isAttached)
+  }
+
   @Test
   fun testPopToRoot() {
     val rootTransaction = TestController().asTransaction()

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

@@ -105,7 +105,7 @@ class ViewLeakTests {
 
     router.pushController(TestController().asTransaction())
     shadowOf(Looper.getMainLooper()).idle()
-    Assert.assertNotNull(view.parent)
+    Assert.assertNull(view.parent)
 
     router.popToRoot()
     shadowOf(Looper.getMainLooper()).idle()

demo/src/main/java/com/bluelinelabs/conductor/demo/controllers/DialogController.kt

@@ -4,6 +4,8 @@ import android.os.Bundle
 import android.text.method.LinkMovementMethod
 import android.view.View
 import androidx.core.os.bundleOf
+import com.bluelinelabs.conductor.RouterTransaction
+import com.bluelinelabs.conductor.changehandler.HorizontalChangeHandler
 import com.bluelinelabs.conductor.demo.R
 import com.bluelinelabs.conductor.demo.controllers.base.BaseController
 import com.bluelinelabs.conductor.demo.databinding.ControllerDialogBinding
@@ -23,7 +25,21 @@ class DialogController(args: Bundle) : BaseController(R.layout.controller_dialog
     binding.title.text = args.getCharSequence(KEY_TITLE)
     binding.description.text = args.getCharSequence(KEY_DESCRIPTION)
     binding.description.movementMethod = LinkMovementMethod.getInstance()
-
+    binding.next.setOnClickListener {
+      val backstack = router.backstack.plus(
+        RouterTransaction.with(TextController("Came here via setBackstack - Now test back button!"))
+          .pushChangeHandler(HorizontalChangeHandler())
+          .popChangeHandler(HorizontalChangeHandler())
+      )
+      router.setBackstack(backstack, backstack.last().pushChangeHandler())
+    }
+    binding.nextPush.setOnClickListener {
+      router.pushController(
+        RouterTransaction.with(TextController("Came here via pushController - Now test back button!"))
+          .pushChangeHandler(HorizontalChangeHandler())
+          .popChangeHandler(HorizontalChangeHandler())
+      )
+    }
     binding.dismiss.setOnClickListener { router.popController(this) }
     binding.dialogBackground.setOnClickListener { router.popController(this) }
   }

demo/src/main/res/layout/controller_dialog.xml

@@ -46,6 +46,36 @@
             android:layout_marginLeft="24dp"
             />
 
+        <Button
+            android:id="@+id/next"
+            style="@style/Widget.AppCompat.Button.Borderless.Colored"
+            android:layout_width="wrap_content"
+            android:layout_height="36dp"
+            android:layout_gravity="end|bottom"
+            android:layout_marginBottom="8dp"
+            android:layout_marginEnd="8dp"
+            android:layout_marginLeft="8dp"
+            android:layout_marginRight="8dp"
+            android:layout_marginTop="8dp"
+            android:minWidth="64dp"
+            android:text="@string/next"
+            />
+
+        <Button
+            android:id="@+id/nextPush"
+            style="@style/Widget.AppCompat.Button.Borderless.Colored"
+            android:layout_width="wrap_content"
+            android:layout_height="36dp"
+            android:layout_gravity="end|bottom"
+            android:layout_marginBottom="8dp"
+            android:layout_marginEnd="8dp"
+            android:layout_marginLeft="8dp"
+            android:layout_marginRight="8dp"
+            android:layout_marginTop="8dp"
+            android:minWidth="64dp"
+            android:text="@string/next_controller"
+            />
+
         <Button
             android:id="@+id/dismiss"
             style="@style/Widget.AppCompat.Button.Borderless.Colored"