From e5c23438c86cc3f586f4b4039e0bc80dc5f84c5d Mon Sep 17 00:00:00 2001 From: vito Date: Mon, 24 Aug 2026 14:43:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(reader):=20=F0=9F=90=9B=20give=20the=20page?= =?UTF-8?q?=20back=20its=20top=20and=20bottom=20margins=20(#97)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The text has been running the full height of the screen since the last release, and on a phone with a camera in the display it runs straight under it — the reporter has been tilting their Pixel 8 Pro on every page to read the first couple of sentences. Readium applies the page's vertical margins and the display-cutout clearance in the same pass, and that pass only ever ran from the window-insets listener it registers when `shouldApplyInsetsPadding` is on. Turning that flag off to stop it re-paginating the chapter under the reader (#95) therefore took the margins with it, not just the late resize. Nothing else was applying either, so the page went edge to edge: on an emulator with a punch-hole the WebView measured [0,0][1080,2424], where the same build before #95 settles at [0,241][1080,2319] — 136px of camera plus 105px of margin at the top, 105px at the bottom. Quire already owns the reader's insets, so it now owns the page's margins too. ReaderPageArea holds the text clear of `WindowInsets.displayCutout` — Android's own per-device, per-orientation answer to what is covering the screen, so a punch-hole, a notch or nothing at all all come through the same value with no device list to keep current — and adds Readium's own 40dp (20dp landscape) on top of it. Asking for the cutout unconditionally is safe because `windowInsetsPadding` subtracts what an ancestor already consumed: with full-screen reading off the subtree is padded by the system bars, which in portrait cover the cutout, and the page comes away with nothing left to clear rather than inset twice. Two things fall out of putting the margins here rather than in Readium's view tree. The size that is left is measured by the same `onSizeChanged` the re-anchor machinery keys on, so insets arriving a frame late are just another viewport change and the reader keeps its place across them — which is what #95 could not do. And the strip the padding exposes is Quire's to paint, so ReaderTheme now carries the colour ReadiumCSS paints the page with and the margin reads as part of the page instead of a band around it; ReaderThemeTest pins those against Readium's own values so an upgrade that repaints a theme fails there rather than on screen. Verified on an emulator with a punch-hole cutout, against the book from #95. Portrait [0,241][1080,2319] and landscape [136,53][2424,1027] — the camera moves to the left edge on rotation and the page moves with it. Three background-and-resume cycles and three rotate-and-return trips leave the portrait screenshot byte-identical, so #95 stays fixed. With full-screen reading off the page sits at [0,242][1080,2256]: status bar plus margin, the cutout adding nothing. Dark theme samples black in the margin as well as the page. Closes #97 --- .../ereader/ui/reader/ReaderScreen.kt | 94 ++++++++++++--- app/src/main/res/values-land/dimens.xml | 5 + app/src/main/res/values/dimens.xml | 10 ++ .../ereader/ui/reader/ReaderPageAreaTest.kt | 113 ++++++++++++++++++ .../ereader/reader/ReaderPreferences.kt | 20 +++- .../ereader/reader/ReaderThemeTest.kt | 15 +++ 6 files changed, 236 insertions(+), 21 deletions(-) create mode 100644 app/src/main/res/values-land/dimens.xml create mode 100644 app/src/main/res/values/dimens.xml create mode 100644 app/src/testDebug/java/io/theficos/ereader/ui/reader/ReaderPageAreaTest.kt diff --git a/app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt b/app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt index 560aeef..0660726 100644 --- a/app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt +++ b/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,7 +307,6 @@ private fun ReaderContent( onNext: () -> Unit, onToggleChrome: () -> Unit, onPageLoaded: () -> Unit, - onViewportChanged: (IntSize) -> Unit, ) { val activity = LocalContext.current as FragmentActivity val containerId = rememberSaveable { View.generateViewId() } @@ -257,11 +314,8 @@ private fun ReaderContent( var fragment by remember { mutableStateOf(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, ), ) diff --git a/app/src/main/res/values-land/dimens.xml b/app/src/main/res/values-land/dimens.xml new file mode 100644 index 0000000..b67d082 --- /dev/null +++ b/app/src/main/res/values-land/dimens.xml @@ -0,0 +1,5 @@ + + + + 20dp + diff --git a/app/src/main/res/values/dimens.xml b/app/src/main/res/values/dimens.xml new file mode 100644 index 0000000..eee7cec --- /dev/null +++ b/app/src/main/res/values/dimens.xml @@ -0,0 +1,10 @@ + + + + 40dp + diff --git a/app/src/testDebug/java/io/theficos/ereader/ui/reader/ReaderPageAreaTest.kt b/app/src/testDebug/java/io/theficos/ereader/ui/reader/ReaderPageAreaTest.kt new file mode 100644 index 0000000..9a89381 --- /dev/null +++ b/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 + } +} diff --git a/reader/src/main/java/io/theficos/ereader/reader/ReaderPreferences.kt b/reader/src/main/java/io/theficos/ereader/reader/ReaderPreferences.kt index 2175fe7..fc35125 100644 --- a/reader/src/main/java/io/theficos/ereader/reader/ReaderPreferences.kt +++ b/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 diff --git a/reader/src/test/java/io/theficos/ereader/reader/ReaderThemeTest.kt b/reader/src/test/java/io/theficos/ereader/reader/ReaderThemeTest.kt index 81efbf6..850bee5 100644 --- a/reader/src/test/java/io/theficos/ereader/reader/ReaderThemeTest.kt +++ b/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())