fix(reader): give the page back its top and bottom margins

app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt

@@ -13,9 +13,12 @@ import android.view.WindowManager
 import android.view.accessibility.AccessibilityManager
 import android.widget.FrameLayout
 import androidx.activity.compose.BackHandler
+import androidx.compose.foundation.background
 import androidx.compose.foundation.layout.Box
 import androidx.compose.foundation.layout.WindowInsets
+import androidx.compose.foundation.layout.displayCutout
 import androidx.compose.foundation.layout.fillMaxSize
+import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.layout.systemBars
 import androidx.compose.foundation.layout.windowInsetsPadding
 import androidx.compose.material3.CircularProgressIndicator
@@ -31,9 +34,12 @@ import androidx.compose.runtime.saveable.rememberSaveable
 import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.Color
 import androidx.compose.ui.layout.onSizeChanged
 import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.platform.LocalView
+import androidx.compose.ui.res.dimensionResource
+import androidx.compose.ui.unit.Dp
 import androidx.compose.ui.unit.IntSize
 import androidx.compose.ui.viewinterop.AndroidView
 import androidx.core.view.WindowCompat
@@ -49,6 +55,7 @@ import io.theficos.ereader.MainActivity
 import io.theficos.ereader.data.sync.SyncEnqueuer
 import io.theficos.ereader.reader.ReaderPreferences
 import io.theficos.ereader.reader.toEpubPreferences
+import io.theficos.quire.R
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.launch
 import org.readium.r2.navigator.epub.EpubNavigatorFactory
@@ -169,21 +176,32 @@ fun ReaderScreen(viewModel: ReaderViewModel, onClose: () -> Unit) {
                             else Modifier.windowInsetsPadding(WindowInsets.systemBars),
                         ),
                 ) {
-                    ReaderContent(
-                        publication = s.publication,
-                        initialLocator = s.initialLocator,
-                        preferences = preferences,
-                        onLocator = viewModel::publishLocator,
-                        onNavigatorReady = viewModel::bindNavigator,
-                        onPrev = viewModel::pageBackward,
-                        onNext = viewModel::pageForward,
-                        onToggleChrome = viewModel::toggleChrome,
-                        onPageLoaded = viewModel::reanchorViewport,
+                    ReaderPageArea(
+                        // Immersive draws the page into the display cutout, so the text has to be
+                        // held clear of the camera itself. With immersive off the subtree above
+                        // has already been padded by the system bars, which in portrait cover the
+                        // cutout — windowInsetsPadding subtracts what an ancestor consumed, so
+                        // this resolves to nothing there rather than insetting the page twice.
+                        insets = WindowInsets.displayCutout,
+                        verticalMargin = dimensionResource(R.dimen.reader_vertical_margin),
+                        background = Color(preferences.theme.pageBackground),
                         onViewportChanged = { size ->
                             viewport = size
                             viewModel.onViewportChanged(size.width, size.height)
                         },
-                    )
+                    ) {
+                        ReaderContent(
+                            publication = s.publication,
+                            initialLocator = s.initialLocator,
+                            preferences = preferences,
+                            onLocator = viewModel::publishLocator,
+                            onNavigatorReady = viewModel::bindNavigator,
+                            onPrev = viewModel::pageBackward,
+                            onNext = viewModel::pageForward,
+                            onToggleChrome = viewModel::toggleChrome,
+                            onPageLoaded = viewModel::reanchorViewport,
+                        )
+                    }
 
                     ReaderTopBar(
                         visible = chromeVisible,
@@ -238,6 +256,46 @@ fun ReaderScreen(viewModel: ReaderViewModel, onClose: () -> Unit) {
     }
 }
 
+/**
+ * The page: the area a Readium column is laid out in, and the strip of margin around it.
+ *
+ * Quire owns the reader's insets (see `shouldApplyInsetsPadding` below), which means it owns the
+ * page's margins too. Turning Readium's inset padding off to stop it re-paginating the chapter
+ * under the reader (issue #95) also took away the only path that ever applied any vertical
+ * padding, so the text ran the full height of the screen and, on a phone with a punch-hole
+ * camera, straight under it (issue #97).
+ *
+ * [insets] is what the page must be held clear of; [verticalMargin] is the breathing room it
+ * gets on top of that. [background] paints the whole area, margin included, because the WebView
+ * only paints the part it occupies and a strip in some other colour reads as a band around the
+ * page rather than as part of it.
+ *
+ * [onViewportChanged] reports the size of what is left after all of that — the height that
+ * decides how much text fits in a column, so it, not the window and not the insets, is the
+ * authoritative viewport. Every change is re-anchored across by the view model, whatever caused
+ * it (see `ReaderViewModel.onViewportChanged`), which is what lets the page keep its margins
+ * without also losing the reader's place when the insets arrive a frame late.
+ */
+@Composable
+internal fun ReaderPageArea(
+    insets: WindowInsets,
+    verticalMargin: Dp,
+    background: Color,
+    onViewportChanged: (IntSize) -> Unit,
+    content: @Composable () -> Unit,
+) {
+    Box(
+        modifier = Modifier
+            .fillMaxSize()
+            .background(background)
+            .windowInsetsPadding(insets)
+            .padding(vertical = verticalMargin)
+            .onSizeChanged(onViewportChanged),
+    ) {
+        content()
+    }
+}
+
 @Composable
 private fun ReaderContent(
     publication: Publication,
@@ -249,19 +307,15 @@ private fun ReaderContent(
     onNext: () -> Unit,
     onToggleChrome: () -> Unit,
     onPageLoaded: () -> Unit,
-    onViewportChanged: (IntSize) -> Unit,
 ) {
     val activity = LocalContext.current as FragmentActivity
     val containerId = rememberSaveable { View.generateViewId() }
     val tag = "reader-${publication.metadata.identifier ?: containerId}"
     var fragment by remember { mutableStateOf<EpubNavigatorFragment?>(null) }
 
     AndroidView(
-        // This is the node whose height decides how much text fits in a Readium column, so
-        // it — not the window, and not the insets — is the authoritative viewport.
-        modifier = Modifier
-            .fillMaxSize()
-            .onSizeChanged(onViewportChanged),
+        // Fills the page area, which is what ReaderPageArea measures as the viewport.
+        modifier = Modifier.fillMaxSize(),
         factory = { ctx ->
             ReaderTapDispatcher(ctx).apply {
                 layoutParams = ViewGroup.LayoutParams(
@@ -322,6 +376,12 @@ private fun ReaderContent(
                 //
                 // With full-screen reading off it was simply double-inset: the whole reader
                 // subtree is already padded by WindowInsets.systemBars in ReaderScreen.
+                //
+                // That listener was also, in practice, the only thing that ever applied the
+                // page's vertical margins — Readium bundles them into the same padding pass —
+                // so switching it off left the text running edge to edge (issue #97).
+                // ReaderPageArea applies both now; don't turn this back on or the page is
+                // padded twice, late, and by someone this file can't see.
                 shouldApplyInsetsPadding = false,
             ),
         )

app/src/main/res/values-land/dimens.xml

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources>
+    <!-- Landscape has far less height to spend on margins, so the page keeps half of it. -->
+    <dimen name="reader_vertical_margin">20dp</dimen>
+</resources>

app/src/main/res/values/dimens.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources>
+    <!--
+      Breathing room above and below the text of a page. Readium used to add this itself, but
+      only ever through the window-insets listener that ReaderScreen turns off (issue #95), so
+      turning that off took the margins with it and left the text running edge to edge (#97).
+      Quire applies it now; the value is Readium's own, so the page reads as it always did.
+    -->
+    <dimen name="reader_vertical_margin">40dp</dimen>
+</resources>

app/src/testDebug/java/io/theficos/ereader/ui/reader/ReaderPageAreaTest.kt

@@ -0,0 +1,113 @@
+package io.theficos.ereader.ui.reader
+
+import androidx.compose.foundation.layout.Box
+import androidx.compose.foundation.layout.WindowInsets
+import androidx.compose.foundation.layout.fillMaxSize
+import androidx.compose.foundation.layout.windowInsetsPadding
+import androidx.compose.ui.Modifier
+import androidx.compose.ui.graphics.Color
+import androidx.compose.ui.layout.onSizeChanged
+import androidx.compose.ui.platform.LocalDensity
+import androidx.compose.ui.test.junit4.createComposeRule
+import androidx.compose.ui.unit.IntSize
+import androidx.compose.ui.unit.dp
+import com.google.common.truth.Truth.assertThat
+import org.junit.Rule
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+
+/**
+ * The reader's page has to be held clear of whatever the window draws under — a punch-hole camera
+ * above all — and to keep a margin of its own above and below the text.
+ *
+ * Issue #97: turning off Readium's inset padding to stop it re-paginating the chapter under the
+ * reader (#95) also took away the only thing that ever applied either, so the text ran the full
+ * height of the screen and, on a phone with a centred camera, straight under it. Quire applies
+ * both now and reports what is left as the viewport — the height a Readium column is laid out in,
+ * which is what the re-anchor machinery keys on.
+ *
+ * The insets are passed in rather than read off the window: a Robolectric window has none, and
+ * what matters here is the arithmetic on the way to the page, not where a cutout comes from.
+ */
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [33], application = android.app.Application::class)
+class ReaderPageAreaTest {
+
+    @get:Rule
+    val composeRule = createComposeRule()
+
+    @Test fun `the page clears the cutout and keeps its own margin`() {
+        val page = measurePage(insets = WindowInsets(0, CUTOUT_TOP_PX, 0, 0))
+
+        assertThat(page.viewport.height)
+            .isEqualTo(page.available.height - CUTOUT_TOP_PX - 2 * page.marginPx)
+        // A top cutout costs height, never width.
+        assertThat(page.viewport.width).isEqualTo(page.available.width)
+    }
+
+    @Test fun `with nothing to clear the page still keeps its margin`() {
+        // A phone with no cutout at all: the text must not sit against the top and bottom edges
+        // just because there is no inset pushing it away from them.
+        val page = measurePage(insets = WindowInsets(0, 0, 0, 0))
+
+        assertThat(page.viewport.height).isEqualTo(page.available.height - 2 * page.marginPx)
+    }
+
+    @Test fun `an inset an ancestor already applied is not applied twice`() {
+        // With full-screen reading off, ReaderScreen pads the whole reader subtree by the system
+        // bars, which in portrait cover the cutout. The page asks for the cutout regardless, and
+        // has to come away with nothing left to clear rather than insetting the text a second time.
+        val page = measurePage(
+            insets = WindowInsets(0, CUTOUT_TOP_PX, 0, 0),
+            consumedByAncestor = WindowInsets(0, STATUS_BAR_PX, 0, 0),
+        )
+
+        assertThat(page.viewport.height)
+            .isEqualTo(page.available.height - STATUS_BAR_PX - 2 * page.marginPx)
+    }
+
+    /** What one render measured: the room it started with, and the viewport it reported. */
+    private class Page(val available: IntSize, val viewport: IntSize, val marginPx: Int)
+
+    /**
+     * Renders a page area filling the whole test root. [consumedByAncestor] stands in for padding
+     * an enclosing composable applied before the page got its turn.
+     */
+    private fun measurePage(
+        insets: WindowInsets,
+        consumedByAncestor: WindowInsets? = null,
+    ): Page {
+        var available = IntSize.Zero
+        var viewport = IntSize.Zero
+        var marginPx = 0
+        composeRule.setContent {
+            marginPx = with(LocalDensity.current) { MARGIN.roundToPx() }
+            Box(
+                Modifier
+                    .fillMaxSize()
+                    .onSizeChanged { available = it }
+                    .then(consumedByAncestor?.let { Modifier.windowInsetsPadding(it) } ?: Modifier)
+            ) {
+                ReaderPageArea(
+                    insets = insets,
+                    verticalMargin = MARGIN,
+                    background = Color.White,
+                    onViewportChanged = { viewport = it },
+                ) {
+                    Box(Modifier.fillMaxSize())
+                }
+            }
+        }
+        composeRule.waitForIdle()
+        assertThat(available.height).isGreaterThan(0)
+        return Page(available, viewport, marginPx)
+    }
+
+    private companion object {
+        val MARGIN = 40.dp
+        const val CUTOUT_TOP_PX = 136
+        const val STATUS_BAR_PX = 142
+    }
+}

reader/src/main/java/io/theficos/ereader/reader/ReaderPreferences.kt

@@ -5,6 +5,9 @@ import org.readium.r2.navigator.preferences.Color as ReadiumColor
 import org.readium.r2.navigator.preferences.FontFamily as ReadiumFontFamily
 import org.readium.r2.navigator.preferences.Theme
 
+/** Calibre's "sepia dark" background, and the page colour that goes with it. */
+private const val DARK_SEPIA_BACKGROUND = 0xFF39322B.toInt()
+
 /**
  * A reader colour scheme.
  *
@@ -14,17 +17,25 @@ import org.readium.r2.navigator.preferences.Theme
  * unconditionally (unlike line height or paragraph indent, which are gated behind
  * `readium-advanced-on`), so they hold whether or not publisher styles are on.
  *
+ * [pageBackground] is the colour the page ends up being painted, whoever decides it — Readium's
+ * own appearance for the three native schemes, the override above for one that sets it. Quire
+ * pads the page area itself (issue #97) and so has to paint the strip that padding exposes;
+ * anything but the page's own colour reads as a band around the page instead of part of it.
+ * A native scheme's value has to be kept in step with Readium by hand — ReaderThemeTest asserts
+ * the pair still agree, so an upgrade that repaints a theme fails there rather than on screen.
+ *
  * [isDark] drives the system bars, not the page: it must be true whenever the background is dark,
  * which is not the same question as "is the Readium theme DARK".
  */
 enum class ReaderTheme(
     val isDark: Boolean,
+    val pageBackground: Int,
     internal val textColor: Int? = null,
     internal val backgroundColor: Int? = null,
 ) {
-    LIGHT(isDark = false),
-    DARK(isDark = true),
-    SEPIA(isDark = false),
+    LIGHT(isDark = false, pageBackground = 0xFFFFFFFF.toInt()),
+    DARK(isDark = true, pageBackground = 0xFF000000.toInt()),
+    SEPIA(isDark = false, pageBackground = 0xFFFAF4E8.toInt()),
 
     /**
      * Warm text on a warm-dark background, for reading in the dark without the glare of SEPIA or
@@ -33,8 +44,9 @@ enum class ReaderTheme(
      */
     DARK_SEPIA(
         isDark = true,
+        pageBackground = DARK_SEPIA_BACKGROUND,
         textColor = 0xFFF6F3E9.toInt(),
-        backgroundColor = 0xFF39322B.toInt(),
+        backgroundColor = DARK_SEPIA_BACKGROUND,
     ),
     ;
     // Deliberately no Readium type in this enum: Readium's Theme initialises itself through

reader/src/test/java/io/theficos/ereader/reader/ReaderThemeTest.kt

@@ -40,6 +40,21 @@ class ReaderThemeTest {
         }
     }
 
+    @Test fun `every theme knows the colour its page is painted`() {
+        // Quire paints the reader's margins itself (issue #97), in this colour, so it has to be
+        // the one the page ends up being. For the three native schemes that is Readium's own
+        // value, copied by hand — this is where a Readium upgrade that repaints a theme surfaces,
+        // instead of as a mismatched band above and below the text.
+        assertThat(ReaderTheme.LIGHT.pageBackground).isEqualTo(Theme.LIGHT.backgroundColor)
+        assertThat(ReaderTheme.DARK.pageBackground).isEqualTo(Theme.DARK.backgroundColor)
+        assertThat(ReaderTheme.SEPIA.pageBackground).isEqualTo(Theme.SEPIA.backgroundColor)
+        // DARK_SEPIA overrides the background rather than inheriting one, so its own override is
+        // what the page is painted.
+        assertThat(ReaderTheme.DARK_SEPIA.pageBackground).isEqualTo(
+            ReaderPreferences(theme = ReaderTheme.DARK_SEPIA).toEpubPreferences().backgroundColor?.int
+        )
+    }
+
     @Test fun `dark sepia emits Calibre's sepia-dark colours`() {
         val prefs = ReaderPreferences(theme = ReaderTheme.DARK_SEPIA).toEpubPreferences()
         assertThat(prefs.textColor?.int).isEqualTo(0xFFF6F3E9.toInt())