Code reviews:

- refactor(layout): one multi-pane shell, and name the panel predicate honestly
- fix(layout): don't dock the sidebar on portrait tablets (#4024)

amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/WindowUtils.kt

@@ -45,9 +45,6 @@ private tailrec fun Context.getActivityWindow(): Window? =
         else -> null
     }
 
-@Composable
-fun getActivity(): Activity = LocalContext.current.getActivity()
-
 tailrec fun Context.getActivity(): ComponentActivity =
     when (this) {
         is ComponentActivity -> this

amethyst/src/main/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayout.kt

@@ -28,7 +28,6 @@ import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.windowsizeclass.ExperimentalMaterial3WindowSizeClassApi
 import androidx.compose.material3.windowsizeclass.WindowSizeClass
 import androidx.compose.material3.windowsizeclass.WindowWidthSizeClass
-import androidx.compose.material3.windowsizeclass.calculateWindowSizeClass
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.Immutable
 import androidx.compose.runtime.compositionLocalOf
@@ -38,7 +37,6 @@ import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.LocalConfiguration
 import androidx.compose.ui.unit.DpSize
 import androidx.compose.ui.unit.dp
-import com.vitorpamplona.amethyst.ui.components.getActivity
 
 /** How the app shell presents its top-level navigation for the current window size. */
 enum class NavigationStyle {
@@ -46,12 +44,13 @@ enum class NavigationStyle {
     BOTTOM_BAR,
 
     /**
-     * Medium windows (portrait tablets, unfolded foldables): a left navigation rail
-     * replaces the bottom bar; the drawer stays modal behind the rail's avatar button.
+     * Every non-Compact window that does not dock — portrait tablets and unfolded foldables at
+     * any width, plus short landscape windows: a left navigation rail replaces the bottom bar
+     * and the drawer stays modal behind the rail's avatar button.
      */
     NAV_RAIL,
 
-    /** Expanded windows (landscape tablets, desktop windows): the drawer docks permanently on the left. */
+    /** Wide, landscape, tall windows (landscape tablets, desktop): the drawer docks permanently on the left. */
     PERMANENT_DRAWER,
 }
 
@@ -62,7 +61,7 @@ enum class NavigationStyle {
 @Immutable
 data class ScreenLayoutSpec(
     val navigationStyle: NavigationStyle,
-    val showsNotificationPanel: Boolean,
+    val hasRoomForNotificationPanel: Boolean,
 ) {
     /**
      * True on the rail and permanent-drawer tiers. Large screens hide the bottom bar and pin
@@ -71,16 +70,18 @@ data class ScreenLayoutSpec(
     val isLargeScreen: Boolean get() = navigationStyle != NavigationStyle.BOTTOM_BAR
 
     companion object {
-        val Phone = ScreenLayoutSpec(NavigationStyle.BOTTOM_BAR, showsNotificationPanel = false)
+        val Phone = ScreenLayoutSpec(NavigationStyle.BOTTOM_BAR, hasRoomForNotificationPanel = false)
     }
 }
 
 val LocalScreenLayout = compositionLocalOf { ScreenLayoutSpec.Phone }
 
 /**
- * Minimum window width for the docked notification panel: the permanent drawer
- * ([PermanentDrawerWidth]) + a readable center pane + the panel ([NotificationPanelWidth])
- * only coexist comfortably from a landscape-tablet-sized window up.
+ * Minimum window width for the docked notification panel: a leading navigation pane, a
+ * readable center pane and the panel ([NotificationPanelWidth]) only coexist comfortably from
+ * a landscape-tablet-sized window up. Sized against the widest leading pane, the permanent
+ * drawer ([PermanentDrawerWidth]); the rail is narrower, so a railed window that clears this
+ * gets a roomier center pane rather than a tighter one.
  */
 private const val NOTIFICATION_PANEL_MIN_WINDOW_DP = 1200
 
@@ -114,8 +115,8 @@ private const val DOCK_MIN_WINDOW_HEIGHT_DP = 600
  * tall enough for the drawer's own content to be usable; everything else that is not Compact
  * falls through to the rail, which pairs with the existing swipe-in modal drawer.
  *
- * Takes plain dp rather than a [WindowWidthSizeClass] so a caller cannot pass a size class
- * that contradicts the size, and so the whole table is unit-testable without a composition.
+ * A square window counts as landscape and docks; `Configuration.ORIENTATION_LANDSCAPE`
+ * breaks that tie the other way, so the two disagree at exactly width == height.
  */
 @OptIn(ExperimentalMaterial3WindowSizeClassApi::class)
 internal fun decideNavigationStyle(
@@ -139,20 +140,10 @@ internal fun decideNavigationStyle(
 /**
  * Whether the window is wide enough to dock the notification feed beside the content.
  *
- * Deliberately not keyed on [NavigationStyle.PERMANENT_DRAWER]: a wide portrait window now
- * gets the rail, and gating on the dock would strip a panel it has today. Named
- * `decideNotificationPanel` rather than `showsNotificationPanel` so it does not shadow
- * [ScreenLayoutSpec.showsNotificationPanel] at the construction site.
- *
- * The [NavigationStyle.BOTTOM_BAR] term is not load-bearing — Compact is under 600dp, so a
- * bottom-bar window cannot reach 1200dp — but it makes that an invariant the code enforces.
+ * Deliberately not keyed on [NavigationStyle]: a wide portrait window now gets the rail, and
+ * gating on the dock would strip a panel it has today.
  */
-internal fun decideNotificationPanel(
-    style: NavigationStyle,
-    windowWidthDp: Int,
-): Boolean =
-    style != NavigationStyle.BOTTOM_BAR &&
-        windowWidthDp >= NOTIFICATION_PANEL_MIN_WINDOW_DP
+internal fun hasRoomForNotificationPanel(windowWidthDp: Int): Boolean = windowWidthDp >= NOTIFICATION_PANEL_MIN_WINDOW_DP
 
 /**
  * Centers a destination's content at [FeedContentMaxWidth]. The outer box paints the theme
@@ -178,23 +169,15 @@ fun CappedScreenContent(content: @Composable () -> Unit) {
     }
 }
 
-@OptIn(ExperimentalMaterial3WindowSizeClassApi::class)
 @Composable
 fun rememberScreenLayoutSpec(): ScreenLayoutSpec {
-    val widthSizeClass = calculateWindowSizeClass(getActivity()).widthSizeClass
-    val windowWidthDp = LocalConfiguration.current.screenWidthDp
-    return remember(widthSizeClass, windowWidthDp) {
-        val style =
-            when (widthSizeClass) {
-                WindowWidthSizeClass.Expanded -> NavigationStyle.PERMANENT_DRAWER
-                WindowWidthSizeClass.Medium -> NavigationStyle.NAV_RAIL
-                else -> NavigationStyle.BOTTOM_BAR
-            }
+    val configuration = LocalConfiguration.current
+    val windowWidthDp = configuration.screenWidthDp
+    val windowHeightDp = configuration.screenHeightDp
+    return remember(windowWidthDp, windowHeightDp) {
         ScreenLayoutSpec(
-            navigationStyle = style,
-            showsNotificationPanel =
-                style == NavigationStyle.PERMANENT_DRAWER &&
-                    windowWidthDp >= NOTIFICATION_PANEL_MIN_WINDOW_DP,
+            navigationStyle = decideNavigationStyle(windowWidthDp, windowHeightDp),
+            hasRoomForNotificationPanel = hasRoomForNotificationPanel(windowWidthDp),
         )
     }
 }

amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountSwitcherAndLeftDrawerLayout.kt

@@ -138,8 +138,10 @@ fun AccountSwitcherAndLeftDrawerLayout(
 }
 
 /**
- * Compact and Medium windows: the drawer slides in as a modal sheet. On Medium an
- * [AppNavigationRail] sits at the left edge in place of the phone bottom bar.
+ * Every window that does not dock the drawer: the drawer slides in as a modal sheet. On the
+ * rail tier an [AppNavigationRail] sits at the left edge in place of the phone bottom bar and
+ * the shell becomes multi-pane — a wide portrait window rails and is still wide enough for
+ * the notification panel.
  */
 @Composable
 private fun ModalDrawerShell(
@@ -183,11 +185,12 @@ private fun ModalDrawerShell(
         },
         content = {
             if (showRail) {
-                Row(Modifier.fillMaxSize()) {
-                    AppNavigationRail(nav, accountViewModel)
-                    VerticalDivider(thickness = DividerThickness)
-                    CenterPane(Modifier.weight(1f), content)
-                }
+                MultiPaneShell(
+                    accountViewModel = accountViewModel,
+                    nav = nav,
+                    leading = { AppNavigationRail(nav, accountViewModel) },
+                    content = content,
+                )
             } else {
                 content()
             }
@@ -196,8 +199,62 @@ private fun ModalDrawerShell(
 }
 
 /**
- * Expanded windows: the drawer is permanently docked on the left, the bottom bar disappears,
- * and — when the window is wide enough — the notification feed docks on the right.
+ * The wide-window arrangement both shells render: a [leading] navigation pane, the centre
+ * content, and the notification feed when the window has room. Shared because a wide portrait
+ * window now rails rather than docks and is still wide enough for the panel — the two shells
+ * differ only in which navigation pane leads.
+ */
+@Composable
+private fun MultiPaneShell(
+    accountViewModel: AccountViewModel,
+    nav: Nav,
+    leading: @Composable () -> Unit,
+    content: @Composable () -> Unit,
+) {
+    Row(Modifier.fillMaxSize()) {
+        leading()
+
+        VerticalDivider(thickness = DividerThickness)
+
+        CenterPane(Modifier.weight(1f), content)
+
+        NotificationSidePanelSlot(accountViewModel, nav)
+    }
+}
+
+/**
+ * The docked notification feed, when there is room for it. The panel duplicates the
+ * Notifications screen, so it steps aside while the user is there.
+ *
+ * The back-stack entry is collected here rather than in the shells so windows too narrow for
+ * the panel never observe it, and so navigation churn recomposes this slot instead of the
+ * whole shell. The trade is that a rail-tier window wide enough for the panel ends up with a
+ * second collector beside [ModalDrawerShell]'s own — one extra subscriber on a shared flow,
+ * against a shell restart per navigation on every window that cannot show the panel.
+ *
+ * [Route] matching goes through `remember` because `hasRoute` resolves a serializer
+ * reflectively on every call.
+ */
+@Composable
+private fun NotificationSidePanelSlot(
+    accountViewModel: AccountViewModel,
+    nav: Nav,
+) {
+    if (!LocalScreenLayout.current.hasRoomForNotificationPanel) return
+
+    val navBackStackEntry by nav.controller.currentBackStackEntryAsState()
+    val destination = navBackStackEntry?.destination
+    val onNotifications = remember(destination) { destination?.hasRoute<Route.Notification>() == true }
+
+    if (!onNotifications) {
+        VerticalDivider(thickness = DividerThickness)
+        NotificationSidePanel(accountViewModel, nav)
+    }
+}
+
+/**
+ * Wide, landscape, tall windows: the drawer is permanently docked on the left, the bottom bar
+ * disappears, and — when the window is wide enough — the notification feed docks on the right.
  */
 @Composable
 private fun PermanentDrawerShell(
@@ -206,24 +263,12 @@ private fun PermanentDrawerShell(
     openSheet: () -> Unit,
     content: @Composable () -> Unit,
 ) {
-    val navBackStackEntry by nav.controller.currentBackStackEntryAsState()
-    // The panel duplicates the Notifications screen, so it steps aside while the user is there.
-    val showPanel =
-        LocalScreenLayout.current.showsNotificationPanel &&
-            navBackStackEntry?.destination?.hasRoute<Route.Notification>() != true
-
-    Row(Modifier.fillMaxSize()) {
-        PermanentDrawerContent(nav, openSheet, accountViewModel)
-
-        VerticalDivider(thickness = DividerThickness)
-
-        CenterPane(Modifier.weight(1f), content)
-
-        if (showPanel) {
-            VerticalDivider(thickness = DividerThickness)
-            NotificationSidePanel(accountViewModel, nav)
-        }
-    }
+    MultiPaneShell(
+        accountViewModel = accountViewModel,
+        nav = nav,
+        leading = { PermanentDrawerContent(nav, openSheet, accountViewModel) },
+        content = content,
+    )
 }
 
 /**

amethyst/src/test/java/com/vitorpamplona/amethyst/ui/layouts/ScreenLayoutTest.kt

@@ -21,6 +21,8 @@
 package com.vitorpamplona.amethyst.ui.layouts
 
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
 import org.junit.Test
 
 /**
@@ -30,7 +32,7 @@ import org.junit.Test
  *
  * Sizes are window dp, width first. Every row of the spec's Behaviour table appears here.
  */
-class ScreenLayoutSpecTest {
+class ScreenLayoutTest {
     private fun assertStyle(
         expected: NavigationStyle,
         widthDp: Int,
@@ -86,19 +88,10 @@ class ScreenLayoutSpecTest {
     // ---- Notification panel ----
 
     @Test
-    fun panelHiddenJustBelowTheThreshold() = assertEquals(false, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1199))
+    fun panelHiddenJustBelowTheThreshold() = assertFalse(hasRoomForNotificationPanel(1199))
 
     @Test
-    fun panelShownExactlyAtTheThreshold() = assertEquals(true, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1200))
-
-    @Test
-    fun panelShownAboveTheThreshold() = assertEquals(true, decideNotificationPanel(NavigationStyle.PERMANENT_DRAWER, 1201))
-
-    @Test
-    fun panelSurvivesTheCollapseToTheRail() = assertEquals(true, decideNotificationPanel(NavigationStyle.NAV_RAIL, 1200))
-
-    @Test
-    fun panelNeverAppearsOnTheBottomBar() = assertEquals(false, decideNotificationPanel(NavigationStyle.BOTTOM_BAR, 1200))
+    fun panelShownExactlyAtTheThreshold() = assertTrue(hasRoomForNotificationPanel(1200))
 
     /**
      * A landscape-short window: wide enough for the panel, too short for the dock. Rail plus
@@ -106,8 +99,7 @@ class ScreenLayoutSpecTest {
      */
     @Test
     fun shortLandscapeYieldsRailPlusPanel() {
-        val style = decideNavigationStyle(1200, 599)
-        assertEquals(NavigationStyle.NAV_RAIL, style)
-        assertEquals(true, decideNotificationPanel(style, 1200))
+        assertStyle(NavigationStyle.NAV_RAIL, 1200, 599)
+        assertTrue(hasRoomForNotificationPanel(1200))
     }
 }