Update NavigatorImpl.pop() to invoke onBackPressedDispatcher (#264)

The current back solution seems to work when tapping the system back
button or triggering the back gesture. However, manual calls to
Navigator.pop (triggered by an on screen back button) when viewing the
root backstack record will crash the app. This happens purposely as we
don't want to allow an empty backstack.

To resolve this, NavigatorImpl has been updated to invoke a
onBackPressedDispatcher whenever pop() is called while on the root
backstack record.

Co-authored-by: Zac Sweers <pandanomic@gmail.com>

circuit/src/androidMain/kotlin/com/slack/circuit/Navigator.android.kt

@@ -15,6 +15,7 @@
  */
 package com.slack.circuit
 
+import androidx.activity.compose.LocalOnBackPressedDispatcherOwner
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.remember
 import com.slack.circuit.backstack.SaveableBackStack
@@ -27,14 +28,28 @@ import com.slack.circuit.backstack.isEmpty
  * @see NavigableCircuitContent
  *
  * @param backstack The backing [SaveableBackStack] to navigate.
+ * @param onRootPop Invoked when the backstack is at root (size 1) and the user presses the back
+ * button. Defaults to delegating to the [LocalOnBackPressedDispatcherOwner].
  */
 @Composable
 public fun rememberCircuitNavigator(
   backstack: SaveableBackStack,
-): Navigator = remember { NavigatorImpl(backstack) }
+  onRootPop: () -> Unit = backDispatcherRootPop()
+): Navigator {
+  return remember { NavigatorImpl(backstack, onRootPop) }
+}
+
+@Composable
+private fun backDispatcherRootPop(): () -> Unit {
+  val onBackPressedDispatcher =
+    LocalOnBackPressedDispatcherOwner.current?.onBackPressedDispatcher
+      ?: error("No OnBackPressedDispatcherOwner found, unable to handle root navigation pops.")
+  return onBackPressedDispatcher::onBackPressed
+}
 
 internal class NavigatorImpl(
   private val backstack: SaveableBackStack,
+  private val onRootPop: () -> Unit
 ) : Navigator {
 
   init {
@@ -44,7 +59,11 @@ internal class NavigatorImpl(
   override fun goTo(screen: Screen) = backstack.push(screen)
 
   override fun pop(): Screen? {
-    check(!backstack.isAtRoot) { "Cannot pop the root screen." }
+    if (backstack.isAtRoot) {
+      onRootPop()
+      return null
+    }
+
     return backstack.pop()?.screen
   }
 
@@ -55,11 +74,18 @@ internal class NavigatorImpl(
     other as NavigatorImpl
 
     if (backstack != other.backstack) return false
+    if (onRootPop != other.onRootPop) return false
 
     return true
   }
 
-  override fun hashCode() = backstack.hashCode()
+  override fun hashCode(): Int {
+    var result = backstack.hashCode()
+    result = 31 * result + onRootPop.hashCode()
+    return result
+  }
 
-  override fun toString() = "NavigatorImpl(backstack=$backstack)"
+  override fun toString(): String {
+    return "NavigatorImpl(backstack=$backstack, onRootPop=$onRootPop)"
+  }
 }

circuit/src/androidTest/kotlin/com/slack/circuit/NavigatorTest.kt

@@ -35,11 +35,10 @@ private object TestScreen : Screen {
 
 @RunWith(RobolectricTestRunner::class)
 class NavigatorTest {
-
   @Test
   fun errorWhenBackstackIsEmpty() {
     val backstack = SaveableBackStack()
-    val t = assertFailsWith<IllegalStateException> { NavigatorImpl(backstack = backstack) }
+    val t = assertFailsWith<IllegalStateException> { NavigatorImpl(backstack) {} }
     assertThat(t).hasMessageThat().contains("Backstack size must not be empty.")
   }
 
@@ -48,11 +47,19 @@ class NavigatorTest {
     val backstack = SaveableBackStack()
     backstack.push(TestScreen)
     backstack.push(TestScreen)
-    val navigator = NavigatorImpl(backstack = backstack)
+
+    var onRootPop = 0
+    val navigator = NavigatorImpl(backstack) { onRootPop++ }
+
     assertThat(backstack).hasSize(2)
+    assertThat(onRootPop).isEqualTo(0)
+
+    navigator.pop()
+    assertThat(backstack).hasSize(1)
+    assertThat(onRootPop).isEqualTo(0)
+
     navigator.pop()
     assertThat(backstack).hasSize(1)
-    val t = assertFailsWith<IllegalStateException> { navigator.pop() }
-    assertThat(t).hasMessageThat().contains("Cannot pop the root screen.")
+    assertThat(onRootPop).isEqualTo(1)
   }
 }

samples/star/src/main/kotlin/com/slack/circuit/star/common/NavigableTopBarScreen.kt

@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2022 Slack Technologies, LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.slack.circuit.star.common
+
+import androidx.activity.compose.LocalOnBackPressedDispatcherOwner
+import androidx.compose.foundation.Image
+import androidx.compose.material.icons.Icons
+import androidx.compose.material.icons.filled.Close
+import androidx.compose.material3.IconButton
+import androidx.compose.runtime.Composable
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.vector.rememberVectorPainter
+
+@Composable
+fun BackPressNavIcon(
+  modifier: Modifier = Modifier,
+  iconButtonContent: @Composable () -> Unit = { ClosedIconImage() },
+) {
+  val onBackPressedDispatcher =
+    LocalOnBackPressedDispatcherOwner.current?.onBackPressedDispatcher
+      ?: error("No local LocalOnBackPressedDispatcherOwner found.")
+  IconButton(modifier = modifier, onClick = onBackPressedDispatcher::onBackPressed) {
+    iconButtonContent()
+  }
+}
+
+@Composable
+private fun ClosedIconImage(modifier: Modifier = Modifier) {
+  Image(
+    modifier = modifier,
+    painter = rememberVectorPainter(image = Icons.Filled.Close),
+    contentDescription = "Close",
+  )
+}

samples/star/src/main/kotlin/com/slack/circuit/star/petdetail/PetDetail.kt

@@ -34,13 +34,17 @@ import androidx.compose.foundation.lazy.LazyColumn
 import androidx.compose.foundation.lazy.LazyListScope
 import androidx.compose.foundation.shape.CornerSize
 import androidx.compose.material3.Button
+import androidx.compose.material3.CenterAlignedTopAppBar
 import androidx.compose.material3.CircularProgressIndicator
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.Scaffold
 import androidx.compose.material3.Surface
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.LocalConfiguration
@@ -49,7 +53,6 @@ import androidx.compose.ui.platform.testTag
 import androidx.compose.ui.res.stringResource
 import androidx.compose.ui.text.capitalize
 import androidx.compose.ui.text.intl.LocaleList
-import androidx.compose.ui.text.style.TextAlign
 import androidx.compose.ui.unit.dp
 import com.google.accompanist.flowlayout.FlowCrossAxisAlignment
 import com.google.accompanist.flowlayout.FlowMainAxisAlignment
@@ -62,6 +65,7 @@ import com.slack.circuit.Screen
 import com.slack.circuit.codegen.annotations.CircuitInject
 import com.slack.circuit.retained.produceRetainedState
 import com.slack.circuit.star.R
+import com.slack.circuit.star.common.BackPressNavIcon
 import com.slack.circuit.star.data.Animal
 import com.slack.circuit.star.di.AppScope
 import com.slack.circuit.star.petdetail.PetDetailTestConstants.ANIMAL_CONTAINER_TAG
@@ -91,7 +95,9 @@ data class PetDetailScreen(val petId: Long, val photoUrlMemoryCacheKey: String?)
   }
 }
 
-internal fun Animal.toPetDetailState(photoUrlMemoryCacheKey: String?): PetDetailScreen.State {
+internal fun Animal.toPetDetailState(
+  photoUrlMemoryCacheKey: String?,
+): PetDetailScreen.State {
   return PetDetailScreen.State.Success(
     url = url,
     photoUrls = photos.map { it.large },
@@ -107,7 +113,7 @@ internal fun Animal.toPetDetailState(photoUrlMemoryCacheKey: String?): PetDetail
         gender,
         size,
         status
-      )
+      ),
   )
 }
 
@@ -119,13 +125,17 @@ constructor(
 ) : Presenter<PetDetailScreen.State> {
   @Composable
   override fun present(): PetDetailScreen.State {
+    var title by remember { mutableStateOf<String?>(null) }
     val state by
       produceRetainedState<PetDetailScreen.State>(PetDetailScreen.State.Loading) {
         val animal = petRepository.getAnimal(screen.petId)
         value =
           when (animal) {
             null -> PetDetailScreen.State.UnknownAnimal
-            else -> animal.toPetDetailState(screen.photoUrlMemoryCacheKey)
+            else -> {
+              title = animal.name
+              animal.toPetDetailState(screen.photoUrlMemoryCacheKey)
+            }
           }
       }
 
@@ -152,14 +162,31 @@ internal fun PetDetail(state: PetDetailScreen.State) {
   systemUiController.setStatusBarColor(MaterialTheme.colorScheme.background)
   systemUiController.setNavigationBarColor(MaterialTheme.colorScheme.background)
   val isLandscape = LocalConfiguration.current.orientation == Configuration.ORIENTATION_LANDSCAPE
-  Scaffold(modifier = Modifier.systemBarsPadding()) { padding ->
+  Scaffold(
+    modifier = Modifier.systemBarsPadding(),
+    topBar = {
+      CenterAlignedTopAppBar(
+        title = {
+          when (state) {
+            is PetDetailScreen.State.Success -> {
+              Text(state.name)
+            }
+            else -> {
+              // Do nothing
+            }
+          }
+        },
+        navigationIcon = { BackPressNavIcon() }
+      )
+    }
+  ) { padding ->
     when (state) {
-      PetDetailScreen.State.Loading -> {
+      is PetDetailScreen.State.Loading -> {
         Box(modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
           CircularProgressIndicator(modifier = Modifier.testTag(PROGRESS_TAG))
         }
       }
-      PetDetailScreen.State.UnknownAnimal -> {
+      is PetDetailScreen.State.UnknownAnimal -> {
         Box(modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
           Text(
             modifier = Modifier.testTag(UNKNOWN_ANIMAL_TAG),
@@ -206,14 +233,6 @@ internal fun PetDetail(state: PetDetailScreen.State) {
 }
 
 private fun LazyListScope.petDetailDescriptions(state: PetDetailScreen.State.Success) {
-  item(state.name) {
-    Text(
-      modifier = Modifier.fillMaxWidth(),
-      textAlign = TextAlign.Center,
-      text = state.name,
-      style = MaterialTheme.typography.displayLarge
-    )
-  }
   item(state.tags) {
     FlowRow(
       modifier = Modifier.fillMaxWidth(),

samples/star/src/test/kotlin/com/slack/circuit/star/petdetail/PetDetailPresenterTest.kt

@@ -46,7 +46,7 @@ class PetDetailPresenterTest {
     val presenter = PetDetailPresenter(screen, repository)
 
     presenter.test {
-      assertThat(PetDetailScreen.State.Loading).isEqualTo(awaitItem())
+      assertThat(awaitItem()).isEqualTo(PetDetailScreen.State.Loading)
 
       val success = animal.toPetDetailState(animal.photos.first().small)
       assertThat(awaitItem()).isEqualTo(success)

samples/star/src/test/kotlin/com/slack/circuit/star/petdetail/PetDetailUiTest.kt

@@ -43,6 +43,21 @@ import org.robolectric.RobolectricTestRunner
 class PetDetailUiTest {
   @get:Rule val composeTestRule = createComposeRule()
 
+  // TODO this seems like not the greatest test pattern, maybe something we can offer better
+  //  solutions for via semantics.
+  private var carouselScreen: PetPhotoCarouselScreen? = null
+  private val circuitConfig =
+    CircuitConfig.Builder()
+      .setOnUnavailableContent { screen ->
+        when (screen) {
+          is PetPhotoCarouselScreen -> {
+            PetPhotoCarousel(PetPhotoCarouselScreen.State(screen))
+            carouselScreen = screen
+          }
+        }
+      }
+      .build()
+
   @Before
   fun setup() {
     val fakeImageLoader =
@@ -55,7 +70,9 @@ class PetDetailUiTest {
   @Test
   fun petDetail_show_progress_indicator_for_loading_state() {
     composeTestRule.run {
-      setContent { PetDetail(PetDetailScreen.State.Loading) }
+      setContent {
+        CircuitCompositionLocals(circuitConfig) { PetDetail(PetDetailScreen.State.Loading) }
+      }
 
       onNodeWithTag(PROGRESS_TAG).assertIsDisplayed()
       onNodeWithTag(UNKNOWN_ANIMAL_TAG).assertDoesNotExist()
@@ -66,7 +83,9 @@ class PetDetailUiTest {
   @Test
   fun petDetail_show_message_for_unknown_animal_state() {
     composeTestRule.run {
-      setContent { PetDetail(PetDetailScreen.State.UnknownAnimal) }
+      setContent {
+        CircuitCompositionLocals(circuitConfig) { PetDetail(PetDetailScreen.State.UnknownAnimal) }
+      }
 
       onNodeWithTag(PROGRESS_TAG).assertDoesNotExist()
       onNodeWithTag(ANIMAL_CONTAINER_TAG).assertDoesNotExist()
@@ -93,15 +112,6 @@ class PetDetailUiTest {
         tags = listOf("dog", "terrier", "male"),
       )
 
-    var carouselScreen: PetPhotoCarouselScreen? = null
-    val circuitConfig =
-      CircuitConfig.Builder()
-        .setOnUnavailableContent { screen ->
-          carouselScreen = screen as PetPhotoCarouselScreen
-          PetPhotoCarousel(PetPhotoCarouselScreen.State(screen))
-        }
-        .build()
-
     val expectedScreen =
       PetPhotoCarouselScreen(
         name = success.name,