fix(mobile): stop the landscape video rotation loop

DESIGN.md

@@ -306,13 +306,16 @@ America, where mono would fit fewer characters on the line.
 
 Five destinations: **Files, History, Transfers, Downloads, Account**, in that
 order. `NavigationSuiteScaffold`'s layout and suite with an explicit layout type:
-a navigation bar below 600dp width and the collapsed wide navigation rail from
-600dp, following the window. When five destinations don't
-fit, in short windows or at large font scales, a scrollable drawer takes over.
+a navigation bar below 600dp width or 480dp height, and the collapsed wide
+navigation rail otherwise, following the window. A phone in landscape is compact
+in height, so it keeps the bar, as Material's navigation suite does. When five
+destinations don't fit, in short windows or at large font scales, a scrollable
+drawer takes over. Changing layout never rebuilds the content under it.
 The selected item uses the stock indicator with a filled Phosphor
 glyph in `on-secondary-container`; unselected items use the regular glyph.
 
-From 840dp, Files is list-detail: the folder's bar and list on the left (520dp,
+From 840dp wide and 480dp tall, Files is list-detail, so a phone in landscape
+keeps one pane: the folder's bar and list on the left (520dp,
 or half the space beside the rail when that is less), the selected file's detail on the right
 with its preview, primary actions and facts (size, added, length, subtitles,
 folder). Here a tap on a file selects it into

mobile/src/androidTest/kotlin/io/putdotio/android/MobileKeyboardNavigationProofTest.kt

@@ -70,7 +70,8 @@ class MobileKeyboardNavigationProofTest {
     fun keyboardKeepsNavigationAndTheFocusedSearchEditor() {
         val orientation = InstrumentationRegistry.getArguments().getString("putio.accessibility.orientation")
         require(orientation == "portrait" || orientation == "landscape")
-        val navigationTag = if (orientation == "portrait") MOBILE_NAV_BAR_TAG else MOBILE_NAV_RAIL_TAG
+        // A phone in landscape is compact in height, so it keeps the bar.
+        val navigationTag = MOBILE_NAV_BAR_TAG
         var query by mutableStateOf("")
         val mounted = mutableStateOf(true)
         val playerFactory = ShellProofPlayerFactory(null)

mobile/src/main/kotlin/io/putdotio/android/MobileAdaptiveNavigation.kt

@@ -22,6 +22,10 @@ import androidx.compose.material3.TextButton
 import androidx.compose.material3.rememberDrawerState
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.movableContentOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.rememberUpdatedState
 import androidx.compose.runtime.rememberCoroutineScope
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.LocalDensity
@@ -40,6 +44,9 @@ internal const val MOBILE_NAV_DRAWER_TAG = "mobile-navigation-drawer"
 
 internal enum class MobileNavigationLayout { Bar, Rail, Modal }
 
+/** Material's compact window height, a phone in landscape: below 480dp. */
+internal fun isCompactHeight(height: Dp): Boolean = height < 480.dp
+
 @Composable
 internal fun mobileNavigationLayout(width: Dp, height: Dp): MobileNavigationLayout {
     val density = LocalDensity.current
@@ -58,8 +65,8 @@ internal fun mobileNavigationLayout(width: Dp, height: Dp): MobileNavigationLayo
     val tallest = with(density) { labels.maxOf { it.height }.toDp() }
     // Reserve the stock components' icon, label spacing and padding. A bar item grows to an equal
     // share of the bar and its label may fill it; a label on the 96dp rail takes its width less the
-    // item padding.
-    return if (width >= 600.dp) {
+    // item padding. A compact-height window, such as a phone in landscape, keeps the phone's bar.
+    return if (width >= 600.dp && !isCompactHeight(height)) {
         val railHeight = (24.dp + 12.dp + tallest + 24.dp) * MobileDestination.entries.size + 16.dp
         if (widest <= 80.dp && availableHeight >= railHeight) {
             MobileNavigationLayout.Rail
@@ -85,8 +92,11 @@ internal fun MobileNavigationContainer(
     enabled: Boolean = true,
     content: @Composable ((() -> Unit)?) -> Unit,
 ) {
+    // Moving between layouts must keep the content, the open player included, rather than rebuild it.
+    val currentContent by rememberUpdatedState(content)
+    val slot = remember { movableContentOf<(() -> Unit)?> { currentContent(it) } }
     if (layout != MobileNavigationLayout.Modal) {
-        content(null)
+        slot(null)
         return
     }
     val drawer = rememberDrawerState(DrawerValue.Closed)
@@ -129,7 +139,7 @@ internal fun MobileNavigationContainer(
             }
         },
     ) {
-        content { scope.launch(start = CoroutineStart.UNDISPATCHED) { drawer.open() } }
+        slot { scope.launch(start = CoroutineStart.UNDISPATCHED) { drawer.open() } }
     }
     // Register with the overlay so NavHost callbacks added after route changes remain underneath it.
     if (visible) BackHandler(onBack = close)

mobile/src/main/kotlin/io/putdotio/android/MobileShell.kt

@@ -77,7 +77,7 @@ import io.putdotio.android.files.FilesItemId
 import io.putdotio.android.files.FilesOpenOrigin
 import io.putdotio.android.files.FilesRepository
 import io.putdotio.android.files.FilesSubtitleLanguages
-import io.putdotio.android.files.isFilesListDetailWidth
+import io.putdotio.android.files.isFilesListDetail
 import io.putdotio.android.files.showsFilesListDetail
 import io.putdotio.android.files.MobileContinueWatching
 import io.putdotio.android.files.MobileFilesActionsController
@@ -353,9 +353,10 @@ internal fun MobileShell(
             BoxWithConstraints(modifier = Modifier.fillMaxSize()) {
                 val navigationLayout = mobileNavigationLayout(maxWidth, maxHeight)
                 // Files keeps its panes while Search covers it, so nothing under Search is rebuilt.
-                val filesListDetail = isFilesListDetailWidth(maxWidth)
+                val filesListDetail = isFilesListDetail(maxWidth, maxHeight)
                 val topBarInFilesPane = showsFilesListDetail(
                     width = maxWidth,
+                    height = maxHeight,
                     onFiles = selectedDestination == MobileDestination.Files,
                     searchOpen = searchOpen,
                     isPlayback = isPlayback,
@@ -901,8 +902,8 @@ private fun MobileNavigationAlerts(
 
 /**
  * The navigation suite, top bar and now-playing bar around one content slot; hiding them never
- * moves it. Below 600dp the suite is a navigation bar, from 600dp a rail, and in the drawer layout
- * none, since the modal drawer outside this slot takes its place.
+ * moves it. Below 600dp wide or 480dp tall the suite is a navigation bar, otherwise a rail, and in
+ * the drawer layout none, since the modal drawer outside this slot takes its place.
  */
 @Composable
 private fun MobileChrome(

mobile/src/main/kotlin/io/putdotio/android/files/MobileFilesListDetail.kt

@@ -20,15 +20,25 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.testTag
 import androidx.compose.ui.unit.Dp
 import androidx.compose.ui.unit.dp
+import io.putdotio.android.isCompactHeight
 
 internal const val MOBILE_FILES_LIST_PANE_TAG = "mobile-files-list-pane"
 
-/** Files shows list and detail side by side in a window 840dp or wider. */
-internal fun isFilesListDetailWidth(width: Dp): Boolean = width >= FILES_LIST_DETAIL_MIN_WIDTH
+/**
+ * Files shows list and detail side by side in a window 840dp or wider that is not compact in height:
+ * a phone in landscape keeps one pane.
+ */
+internal fun isFilesListDetail(width: Dp, height: Dp): Boolean =
+    width >= FILES_LIST_DETAIL_MIN_WIDTH && !isCompactHeight(height)
 
 /** Whether Files' bar sits over its list pane: the panes are on screen, not under Search or the player. */
-internal fun showsFilesListDetail(width: Dp, onFiles: Boolean, searchOpen: Boolean, isPlayback: Boolean): Boolean =
-    isFilesListDetailWidth(width) && onFiles && !searchOpen && !isPlayback
+internal fun showsFilesListDetail(
+    width: Dp,
+    height: Dp,
+    onFiles: Boolean,
+    searchOpen: Boolean,
+    isPlayback: Boolean,
+): Boolean = isFilesListDetail(width, height) && onFiles && !searchOpen && !isPlayback
 
 private val FILES_LIST_DETAIL_MIN_WIDTH: Dp = 840.dp
 

mobile/src/test/kotlin/io/putdotio/android/MobileAdaptiveNavigationTest.kt

@@ -6,6 +6,10 @@ import androidx.activity.OnBackPressedDispatcherOwner
 import androidx.activity.compose.LocalOnBackPressedDispatcherOwner
 import androidx.compose.foundation.layout.WindowInsets
 import androidx.compose.foundation.layout.ime
+import androidx.compose.runtime.DisposableEffect
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.setValue
 import androidx.compose.ui.platform.LocalDensity
 import androidx.compose.ui.platform.LocalView
 import androidx.compose.ui.semantics.SemanticsActions
@@ -43,6 +47,7 @@ import io.putdotio.android.files.FilesFolder
 import io.putdotio.android.files.FilesItem
 import io.putdotio.android.files.FilesItemId
 import io.putdotio.android.files.FilesPage
+import io.putdotio.android.files.MOBILE_FILES_LIST_PANE_TAG
 import io.putdotio.android.files.MOBILE_FILES_LIST_TAG
 import io.putdotio.android.playback.ActiveAudio
 import io.putdotio.android.playback.MobilePlayerFactory
@@ -201,6 +206,17 @@ class MobileOrdinaryNavigationTest : NavigationShellFixture(1f) {
         assertNavigationLabels(MOBILE_NAV_RAIL_TAG)
     }
 
+    @Test
+    @Config(qualifiers = "en-rUS-w914dp-h411dp-land")
+    fun phoneLandscapeKeepsItsBarAndOneFilesPane() {
+        mount()
+        compose.onNodeWithTag(MOBILE_NAV_MENU_TAG).assertDoesNotExist()
+        compose.onNodeWithTag(MOBILE_NAV_RAIL_TAG).assertDoesNotExist()
+        compose.onNodeWithTag(MOBILE_NAV_BAR_TAG).assertIsDisplayed()
+        compose.onNodeWithTag(MOBILE_FILES_LIST_PANE_TAG).assertDoesNotExist()
+        compose.onNodeWithTag(MOBILE_FILES_LIST_TAG).assertIsDisplayed()
+    }
+
     @Test
     fun openingAndClosingTheKeyboardPreservesPhoneNavigationAndTheFocusedTransferDraft() {
         assertKeyboardKeepsTransferDraft(MOBILE_NAV_BAR_TAG)
@@ -240,6 +256,37 @@ class MobileOrdinaryNavigationTest : NavigationShellFixture(1f) {
     }
 }
 
+@RunWith(AndroidJUnit4::class)
+@Config(sdk = [35])
+class MobileNavigationContainerTest {
+    @get:Rule val compose = createComposeRule()
+
+    @Test
+    fun changingLayoutKeepsTheContentItHolds() {
+        var layout by mutableStateOf(MobileNavigationLayout.Bar)
+        var created = 0
+        var disposed = 0
+        compose.setContent {
+            PutioTheme {
+                MobileNavigationContainer(layout, MobileDestination.Files, onDestination = {}) {
+                    DisposableEffect(Unit) {
+                        created++
+                        onDispose { disposed++ }
+                    }
+                }
+            }
+        }
+        for (next in listOf(MobileNavigationLayout.Modal, MobileNavigationLayout.Rail, MobileNavigationLayout.Bar)) {
+            compose.runOnIdle { layout = next }
+            compose.waitForIdle()
+        }
+        compose.runOnIdle {
+            assertEquals("A layout change must not rebuild the content, an open player included", 1, created)
+            assertEquals(0, disposed)
+        }
+    }
+}
+
 abstract class NavigationShellFixture(fontScale: Float) {
     protected val compose = createComposeRule()
     protected lateinit var backOwner: OnBackPressedDispatcherOwner

mobile/src/test/kotlin/io/putdotio/android/MobileShellTest.kt

@@ -40,6 +40,7 @@ import androidx.compose.ui.test.performScrollTo
 import androidx.compose.ui.test.performScrollToNode
 import androidx.compose.ui.test.performTouchInput
 import androidx.compose.ui.unit.Dp
+import androidx.compose.ui.unit.DpSize
 import androidx.compose.ui.unit.dp
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.LifecycleOwner
@@ -1576,15 +1577,55 @@ class MobileShellPlaybackTest {
         compose.onNodeWithTag(MOBILE_NAV_BAR_TAG).assertIsDisplayed()
     }
 
+    @Test
+    fun anOpenVideoKeepsItsPlayerAcrossNavigationLayouts() {
+        var created = 0
+        val factory = object : MobilePlayerFactory by NoAudioSessionFactory {
+            override fun create(context: Context, mediaType: PlaybackMediaType): Media3Player {
+                created += 1
+                return NoAudioSessionFactory.create(context, mediaType)
+            }
+        }
+        var size by mutableStateOf(DpSize(411.dp, 914.dp))
+        compose.setContent {
+            PutioTheme {
+                Box(modifier = Modifier.requiredSize(size)) {
+                    MobileShell(
+                        filesState = videoFilesState(),
+                        accountSettingsState = readyAccountSettingsState(),
+                        appConfigState = readyAndroidAppConfigState(),
+                        account = Account,
+                        playbackRepository = EndingPlaybackRepository,
+                        playbackPlayerFactory = factory,
+                        sessionId = Session,
+                        onFilesEvent = { true },
+                        onAccountSettingsEvent = {},
+                        onPlaybackAuthenticationRequired = {},
+                        onSignOut = {},
+                    )
+                }
+            }
+        }
+        compose.onNodeWithText("episode.mkv").performClick()
+        compose.runOnIdle { assertEquals(1, created) }
+        // Bar, then the drawer in a window past the 840dp list-detail width, then the rail beside
+        // list-detail, then a phone in landscape: the player behind each must be the same one.
+        for (next in listOf(DpSize(914.dp, 260.dp), DpSize(1280.dp, 800.dp), DpSize(914.dp, 411.dp))) {
+            compose.runOnIdle { size = next }
+            compose.waitForIdle()
+        }
+        compose.runOnIdle { assertEquals("A layout change must not rebuild the open player", 1, created) }
+    }
+
     @Test
     fun phoneDestinationStateSurvivesPlayback() =
         assertDestinationStateSurvivesPlayback(width = 360.dp, navigationTag = MOBILE_NAV_BAR_TAG)
 
     @Test
     fun tabletDestinationStateSurvivesPlayback() =
-        assertDestinationStateSurvivesPlayback(width = 700.dp, navigationTag = MOBILE_NAV_RAIL_TAG)
+        assertDestinationStateSurvivesPlayback(width = 700.dp, navigationTag = MOBILE_NAV_RAIL_TAG, height = 480.dp)
 
-    private fun assertDestinationStateSurvivesPlayback(width: Dp, navigationTag: String) {
+    private fun assertDestinationStateSurvivesPlayback(width: Dp, navigationTag: String, height: Dp = 460.dp) {
         val pending = MutableStateFlow(false)
         val requests = NowPlayingRequests(pending) { pending.value = false }
         val factory = object : MobilePlayerFactory by NoAudioSessionFactory {
@@ -1593,7 +1634,7 @@ class MobileShellPlaybackTest {
         }
         compose.setContent {
             PutioTheme {
-                Box(modifier = Modifier.requiredSize(width = width, height = 460.dp)) {
+                Box(modifier = Modifier.requiredSize(width = width, height = height)) {
                     MobileShell(
                         nowPlayingRequests = requests,
                         playbackPlayerFactory = factory,