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())