From a08777826baf794e312463c0526d7a0fd0efd695 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 09:00:48 +0200 Subject: [PATCH 01/10] Answer how a document is displayed over the document itself Two readers wrote in about the same screen from opposite ends: one wanted the document to follow the phone into night mode, the other wanted the margins off without going back to the landing screen for them. Both answers were somewhere else - one in the system settings, one behind a fold on another screen - so the document's own buttons carry them now. Three rows, each saying what tapping it does rather than what it is called: - **Night mode** puts the app in it through `AppCompatDelegate.setLocalNightMode` rather than the default mode, so reading at night no longer means turning the whole phone dark first. It recreates the activity the way a rotation does and survives it the same way: the loader is a ViewModel, the fragment saves the document, and the page view its own state. `NightModeSetting` remembers the choice - a local mode does not outlive the process - and stores no override at all once the answer agrees with the system again, or the app would sit in night mode through a morning the phone had long left. - **Darkening** is per kind of document rather than one answer for everything. A text document inverts into something that still reads; a scanned page or a photograph inverts into something nobody wrote. So `DocumentDarkening` defaults documents to dark and pdfs and images to light, and the button edits that for every document of the kind - it is never *this* pdf that inverts badly, it is pdfs. Google Docs forgets the same choice on reopen and Word remembers it for everything at once; per kind is the middle, and the row names the kind it will remember so the memory is not invisible. It applies live: darkening is a webview setting, not something the page was translated with. This reverses `aPdfIsDarkenedToo` from #596, which included every format deliberately, "until we have looked at enough of them to say which ones invert badly". Acrobat is the one that has looked: its night mode is opt-in per document. Presentations and drawings stay with documents, because that much is still a guess. The kind comes from the core's own table - `fileTypeByMimetype`, then the file type itself, never a prefix. `documentTypeByFileType` cannot answer it: application/pdf reports TEXT, exactly like an odt. - **The margins** are odrcore's `textDocumentMargin`, decided while translating, so the button renders the document again through the reload that edit mode already uses - the copy in the cache, which is not read or re-listed a second time. The landing switch stays: it writes the same preference, so the two can never disagree. Fullscreen moves up beside them, as the third row about what the page looks like rather than what can be done to it. `DarkModeTests` gains the switch darkening a day-mode app (the screen is measured, not the flag) and the per-kind answer surviving a reopen; `MainActivityTests` that the margin row renders the document again rather than only flipping a preference. All 76 pass on an API 36 emulator. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VNSUAUN1QX2QyFNw4rAmV2 --- CLAUDE.md | 20 +++ .../opendocument/droid/test/DarkModeTests.kt | 137 +++++++++++++++++- .../droid/test/MainActivityTests.kt | 38 +++++ .../droid/background/DocumentDarkening.kt | 66 +++++++++ .../droid/background/NightModeSetting.kt | 71 +++++++++ .../droid/background/PaginationSetting.kt | 7 +- .../droid/ui/activity/DocumentFragment.kt | 115 +++++++++++++-- .../droid/ui/activity/MainActivity.kt | 37 +++++ .../droid/ui/widget/DocumentActions.kt | 3 + .../opendocument/droid/ui/widget/PageView.kt | 3 +- .../main/res/drawable/ic_invert_colors.xml | 10 ++ app/src/main/res/drawable/ic_lightbulb.xml | 10 ++ app/src/main/res/drawable/ic_menu_book.xml | 20 +++ app/src/main/res/values/strings.xml | 10 ++ 14 files changed, 529 insertions(+), 18 deletions(-) create mode 100644 app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt create mode 100644 app/src/main/res/drawable/ic_invert_colors.xml create mode 100644 app/src/main/res/drawable/ic_lightbulb.xml create mode 100644 app/src/main/res/drawable/ic_menu_book.xml diff --git a/CLAUDE.md b/CLAUDE.md index dea03d87a36c..02ae3df34835 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -178,6 +178,26 @@ Both are needed: the first keeps `isRenderedByCore` off a `.bin`, the second sto bar appearing over a page that cannot draw. `LandingTests.aDocumentThatFailsToOpenComesBackToTheList` holds this. +### How the document is displayed is answered over the document, not in the settings + +Three of the buttons in `DocumentActions` are about what the page looks like rather than what can +be done to it, and each remembers what it was last told: + +- **Night mode** is the app's, through `AppCompatDelegate.setLocalNightMode` rather than the + default one, so a phone that stays light all day can still be read at night. `NightModeSetting` + stores no override at all once the choice agrees with the system again, or the app would sit in + night mode through a morning the phone had long left. +- **Darkening** is per kind of document, not per file: a text document inverts into something that + still reads, a pdf or an image does not, so `DocumentDarkening` defaults the first to dark and + the other two to light, and the button edits that for every document of the kind. It is the one + place a mime type decides anything about display - `Odr.fileTypeByMimetype`, not a prefix list. + Presentations are documents until someone has looked at enough of them to say otherwise. +- **The margins** are odrcore's `textDocumentMargin`, decided while translating, so the button + renders the document again through `DocumentLoader.reload` - the copy in the cache, not the file. + +Do not move these into a settings screen. `PaginationSetting` keeps its landing row because it +already had one and both write the same preference; the other two never get one. + ### Editability comes from the core, never from a mime type `Document.isEditable()`/`isSavable()` decides whether `DocumentFragment` offers the Edit diff --git a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt index d67d9dc83f48..9ee37dc71d1a 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt @@ -3,6 +3,9 @@ package app.opendocument.droid.test +import android.content.Context +import android.content.res.Configuration +import android.content.res.Resources import android.net.Uri import android.os.SystemClock import androidx.appcompat.app.AppCompatDelegate @@ -11,10 +14,15 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.LargeTest import androidx.test.platform.app.InstrumentationRegistry import androidx.test.rule.ActivityTestRule +import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry +import androidx.test.runner.lifecycle.Stage import androidx.webkit.WebSettingsCompat import androidx.webkit.WebViewFeature +import app.opendocument.droid.background.DocumentDarkening +import app.opendocument.droid.background.NightModeSetting import app.opendocument.droid.ui.activity.DocumentFragment import app.opendocument.droid.ui.activity.MainActivity +import app.opendocument.droid.ui.widget.DocumentActions import app.opendocument.droid.ui.widget.PageView import java.io.File import java.io.FileOutputStream @@ -29,10 +37,12 @@ import org.junit.Test import org.junit.runner.RunWith /** - * The document follows the app into night mode. + * The document follows the app into night mode, if it is a kind of document that reads better for + * it, and the switches over it are what say otherwise. * * A webview darkens a page algorithmically and only while the app theme reports itself dark, so * every test here puts the app in night mode first - in day mode nothing below would fail. + * [theSwitchDarkensADayModeApp] is the exception, and undoes it. */ @LargeTest @RunWith(AndroidJUnit4::class) @@ -49,6 +59,21 @@ class DarkModeTests { @After fun leaveNightMode() { setNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM) + + // both switches keep their answer on disk, where the next test would find it. asking for + // what the system says is what stores no override at all - see NightModeSetting.setNight + NightModeSetting.setNight(targetContext(), systemIsNight()) + + for (kind in DocumentDarkening.Kind.entries) { + DocumentDarkening.setAllowed(targetContext(), kind, kind.darkensByDefault) + } + + // switching night mode recreates the activity, leaving one behind that the rule does not + // know about - and it would still be up when the next test launches its own + val resumed = resumedMainActivity() + if (resumed != null && resumed !== mainActivityActivityTestRule.activity) { + onMainThread { resumed.finish() } + } } @Test @@ -56,10 +81,33 @@ class DarkModeTests { assertDarkened(openPageView("test.odt")) } - /** Every format, pdf included - see [PageView.setDarkeningAllowed]. */ + /** + * A pdf does not, which is the one kind the app is sure about: a scanned page inverts into + * something nobody wrote. See [DocumentDarkening]. + */ @Test - fun aPdfIsDarkenedToo() { - assertDarkened(openPageView("dummy.pdf")) + fun aPdfIsNotDarkened() { + val pageView = openPageView("dummy.pdf") + + Assert.assertFalse("the pdf was allowed to darken", pageView.isDarkeningAllowed) + assertNotDarkened(pageView) + } + + /** What the button says is kept for every document of that kind, not for the file it was on. */ + @Test + fun theDocumentSwitchIsRememberedForTheKind() { + // after openPageView, which is what launches it + val pageView = openPageView("dummy.pdf") + val activity = mainActivityActivityTestRule.activity + + Assert.assertFalse("the pdf started out darkened", pageView.isDarkeningAllowed) + + onMainThread { activity.onDocumentAction(DocumentActions.ACTION_DOCUMENT_DARKENING) } + + // no reload: darkening is a webview setting, not something the page was translated with + assertDarkened(pageView) + + assertDarkened(reopenPageView(activity, "dummy.pdf")) } /** What is drawn, not only the flag: a webview ignoring the setting passes the flag check. */ @@ -82,6 +130,46 @@ class DarkModeTests { Assert.assertTrue("the page stayed light - mean luminance $luminance", darkened) } + /** + * The switch over the document, which is the answer for a phone that stays in day mode all + * night - the only test here that starts in day mode, since that is what it switches out of. + */ + @Test + fun theSwitchDarkensADayModeApp() { + Assume.assumeTrue( + "this webview has no darkening api at all - nothing the app sets could reach it", + canDarken(), + ) + Assume.assumeFalse( + "the device itself is in night mode - there is no day mode to switch out of", + systemIsNight(), + ) + + // undoing what every other test here starts from: the switch is the only thing that + // should be putting this app in night mode + setNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM) + + openPageView("test.odt") + + onMainThread { + mainActivityActivityTestRule.activity.onDocumentAction( + DocumentActions.ACTION_NIGHT_MODE + ) + } + + // 60s and the last reading kept, for the reason theDrawnPageIsDark gives: this waits on + // the same screenshot, and an activity recreation happens before it + var luminance = WHITE + val darkened = waitFor(60000) { meanLuminance().also { luminance = it } < DARK_LUMINANCE } + + Assert.assertTrue("the page stayed light - mean luminance $luminance", darkened) + Assert.assertEquals( + "the switch was not remembered", + AppCompatDelegate.MODE_NIGHT_YES, + NightModeSetting.mode(targetContext()), + ) + } + /** Printing holds the page light, and only the last job still reading it gives it back. */ @Test fun printingGivesTheDarkeningBack() { @@ -189,6 +277,24 @@ class DarkModeTests { return checkNotNull(fragment.pageView) { "no page view" } } + /** The same file again in the activity already up: a page view that was told nothing yet. */ + private fun reopenPageView(activity: MainActivity, name: String): PageView { + val fragment = checkNotNull(waitForFragment(activity)) { "no document fragment" } + val before = fragment.lastDocument + + val uri = uriOf(extract(name)) + onMainThread { activity.loadUri(uri) } + + // not the uri, which is the one it already had: what says this load landed is a + // document that is not the one from the load before it + Assert.assertTrue( + "$name never loaded again", + waitFor(30000) { fragment.lastDocument != null && fragment.lastDocument !== before }, + ) + + return checkNotNull(fragment.pageView) { "no page view" } + } + private fun waitForFragment(activity: MainActivity): DocumentFragment? { var fragment: DocumentFragment? = null waitFor(30000) { @@ -220,6 +326,29 @@ class DarkModeTests { onMainThread { AppCompatDelegate.setDefaultNightMode(mode) } } + /** The device's own answer, which an activity carrying a local mode no longer gives. */ + private fun systemIsNight(): Boolean = + Resources.getSystem().configuration.uiMode and Configuration.UI_MODE_NIGHT_MASK == + Configuration.UI_MODE_NIGHT_YES + + private fun targetContext(): Context = + InstrumentationRegistry.getInstrumentation().targetContext + + /** Whatever is on screen, which after a night mode switch is not what the rule launched. */ + private fun resumedMainActivity(): MainActivity? { + val current = AtomicReference() + onMainThread { + for (candidate in + ActivityLifecycleMonitorRegistry.getInstance() + .getActivitiesInStage(Stage.RESUMED)) { + if (candidate is MainActivity) { + current.set(candidate) + } + } + } + return current.get() + } + private fun uriOf(file: File): Uri { val appCtx = InstrumentationRegistry.getInstrumentation().targetContext diff --git a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt index df28313d5322..2081d7c855dc 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt @@ -33,6 +33,7 @@ import androidx.test.rule.ActivityTestRule import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry import androidx.test.runner.lifecycle.Stage import app.opendocument.droid.R +import app.opendocument.droid.background.PaginationSetting import app.opendocument.droid.ui.EditActionModeCallback import app.opendocument.droid.ui.OpenFileIdling import app.opendocument.droid.ui.activity.DocumentFragment @@ -285,6 +286,40 @@ class MainActivityTests { Intents.intended(hasAction(Intent.ACTION_CREATE_DOCUMENT), times(1)) } + /** + * The margin switch renders the open document a second time. The margin is odrcore's, decided + * while translating, so flipping the setting alone would leave the page as it was until the + * document was closed and opened again. + */ + @Test + fun theMarginSwitchRendersTheDocumentAgain() { + val activity = mainActivityActivityTestRule.activity + val documentFragment = loadDocument(activity, requireTestFile("test.odt")) + + val before = documentFragment.lastDocument + Assert.assertNotNull(before) + + val margins = PaginationSetting.isEnabled(activity) + try { + InstrumentationRegistry.getInstrumentation().runOnMainSync { + activity.onDocumentAction(DocumentActions.ACTION_PAGE_MARGINS) + } + + Assert.assertEquals( + "the setting did not flip", + !margins, + PaginationSetting.isEnabled(activity), + ) + Assert.assertTrue( + "the document was never rendered again", + waitFor(RELOAD_TIMEOUT_MS) { documentFragment.lastDocument !== before }, + ) + } finally { + // it outlives the test otherwise: it is a preference, not activity state + PaginationSetting.setEnabled(activity, margins) + } + } + @Test fun testDocumentSurvivesRecreation() { val activity = mainActivityActivityTestRule.activity @@ -597,6 +632,9 @@ class MainActivityTests { private const val WINDOW_FOCUS_TIMEOUT_MS = 10000L + // a document already in the cache, translated a second time + private const val RELOAD_TIMEOUT_MS = 10000L + private const val DIALOG_TIMEOUT_MS = 10000L private val testFiles = mutableMapOf() diff --git a/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt new file mode 100644 index 000000000000..afd5da89a8dd --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt @@ -0,0 +1,66 @@ +package app.opendocument.droid.background + +import android.content.Context +import app.opendocument.core.FileCategory +import app.opendocument.core.FileType +import app.opendocument.core.Odr + +/** + * Whether a document follows the app into night mode, which is not one answer for everything the + * app opens: a text document inverts into something that still reads, a scanned page or a photo + * into something nobody wrote. + * + * So the answer is per [Kind], and the button over the document is what edits it - there is no + * switch for this on the landing screen. The kind is what is remembered rather than the file: it is + * never *this* pdf that inverts badly, it is pdfs. + */ +object DocumentDarkening { + + /** + * What the answer differs for, each named the way the button over the document says it. + * + * Presentations and drawings are [DOCUMENT] until someone has looked at enough of them to say + * otherwise - a designed page may well invert as badly as a pdf does, but that is a guess, and + * the two below are not. + */ + enum class Kind(val darkensByDefault: Boolean) { + + /** Text, spreadsheets, plain text, and everything else the core reflows into html. */ + DOCUMENT(true), + + /** + * Fixed pages, scans included, which is where the app's rendering is at its most literal. + */ + PDF(false), + + /** A photograph inverted is a photograph of nothing. */ + IMAGE(false), + } + + fun kindOf(mimeType: String?): Kind { + // not lowercased: the core's table is matched exactly and spells some entries with + // capitals ("macroEnabled"). canonicalMimeType has already been applied upstream + val fileType = mimeType?.let { Odr.fileTypeByMimetype(it) } ?: return Kind.DOCUMENT + + return when { + fileType == FileType.PORTABLE_DOCUMENT_FORMAT -> Kind.PDF + Odr.fileCategoryByFileType(fileType) == FileCategory.IMAGE -> Kind.IMAGE + else -> Kind.DOCUMENT + } + } + + /** Whether what [mimeType] names darkens, which is the default until the button says else. */ + fun isAllowed(context: Context, mimeType: String?): Boolean = + isAllowed(context, kindOf(mimeType)) + + fun isAllowed(context: Context, kind: Kind): Boolean = + AppPreferences.of(context).getBoolean(prefKey(kind), kind.darkensByDefault) + + fun setAllowed(context: Context, kind: Kind, allowed: Boolean) { + AppPreferences.of(context).edit().putBoolean(prefKey(kind), allowed).apply() + } + + private fun prefKey(kind: Kind) = PREF_PREFIX + kind.name.lowercase() + + private const val PREF_PREFIX = "darken_" +} diff --git a/app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt b/app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt new file mode 100644 index 000000000000..1bd8eac0d3be --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt @@ -0,0 +1,71 @@ +package app.opendocument.droid.background + +import android.content.Context +import android.content.res.Configuration +import android.content.res.Resources +import androidx.appcompat.app.AppCompatDelegate + +/** + * Whether the app is in night mode when the system says otherwise. + * + * The document follows the app rather than the system - a webview darkens a page algorithmically + * and only while the app theme reports itself dark - so this is also the switch for reading at + * night on a phone that stays light all day, and for keeping light a document that inverts badly. + * + * What it answers is handed to [AppCompatDelegate.setLocalNightMode], not to the default mode: + * `MainActivity` is the only screen there is, and a local mode leaves whatever asks + * `AppCompatDelegate` itself saying what it said before. + */ +object NightModeSetting { + + private const val PREF_NIGHT_MODE = "night_mode" + + /** + * The mode the activity's delegate is put in. + * + * [AppCompatDelegate.MODE_NIGHT_UNSPECIFIED] is no override at all, which is not the same as + * MODE_NIGHT_FOLLOW_SYSTEM: that one is an override too, and would talk over a default mode set + * anywhere else. + */ + fun mode(context: Context): Int = + AppPreferences.of(context).getInt(PREF_NIGHT_MODE, AppCompatDelegate.MODE_NIGHT_UNSPECIFIED) + + /** + * Remembers whether the app should be dark, and answers the mode that puts it there. + * + * Stored as no override whenever the answer wanted is the one that would be given anyway: an + * override agreeing with the system is one the user can never be rid of again, and the app + * would sit in night mode through a morning the phone had long left it for. + */ + fun setNight(context: Context, night: Boolean): Int { + val mode = + when { + night == isNightWithoutOverride() -> AppCompatDelegate.MODE_NIGHT_UNSPECIFIED + night -> AppCompatDelegate.MODE_NIGHT_YES + else -> AppCompatDelegate.MODE_NIGHT_NO + } + + AppPreferences.of(context).edit().putInt(PREF_NIGHT_MODE, mode).apply() + + return mode + } + + /** What [context] is showing right now, the override included - so ask an activity. */ + fun isNight(context: Context): Boolean = isNight(context.resources) + + /** + * What the app would show with the override taken away, which an activity carrying one can no + * longer say. `Resources.getSystem()` is the device configuration and nothing else, so a + * default mode set on top of it - the instrumented tests set one - is asked for separately. + */ + private fun isNightWithoutOverride(): Boolean = + when (AppCompatDelegate.getDefaultNightMode()) { + AppCompatDelegate.MODE_NIGHT_YES -> true + AppCompatDelegate.MODE_NIGHT_NO -> false + else -> isNight(Resources.getSystem()) + } + + private fun isNight(resources: Resources): Boolean = + resources.configuration.uiMode and Configuration.UI_MODE_NIGHT_MASK == + Configuration.UI_MODE_NIGHT_YES +} diff --git a/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt b/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt index e3293612368b..51719a6e6d06 100644 --- a/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt +++ b/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt @@ -11,9 +11,10 @@ import android.content.Context * is the user's answer now: the margins are what the document was written to look like, the full * width is what reads on a phone. * - * Only [CoreLoader] reads it, and only while translating, so a change reaches a document the next - * time it is opened. That is enough: the switch is on the landing screen, which is only reached by - * closing whatever was open, and reopening it translates again. + * Only [CoreLoader] reads it, and only while translating, so a change reaches a document by + * rendering it again. The landing screen's switch gets that for free - it is a document closed + * away, and opening one translates anyway - and the button over the open document asks + * `DocumentFragment.reloadForMargins` for it. */ object PaginationSetting { diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt index c67b19a32369..2c06db539970 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt @@ -24,11 +24,14 @@ import androidx.fragment.app.Fragment import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModelProvider import app.opendocument.droid.R +import app.opendocument.droid.background.DocumentDarkening import app.opendocument.droid.background.DocumentLoader import app.opendocument.droid.background.DocumentRequest import app.opendocument.droid.background.FileCache import app.opendocument.droid.background.IdentifiedFile import app.opendocument.droid.background.LoadedDocument +import app.opendocument.droid.background.NightModeSetting +import app.opendocument.droid.background.PaginationSetting import app.opendocument.droid.background.StreamUtil import app.opendocument.droid.background.SupportedDocumentTypes import app.opendocument.droid.background.UsageCounters @@ -159,10 +162,6 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { this.pageView = pageView pageView.setDocumentFragment(this) - - // every format, pdf included. the one place that is decided, and every new PageView - // passes through here - pageView.setDarkeningAllowed(true) } catch (t: Throwable) { // crashManager is not set yet: onViewCreated has not run @@ -222,6 +221,9 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { state.lastDocument?.let { lastDocument -> crashManager.log("restoring lastDocument") + // the page view is a new one, and knows nothing of what the old one was told + applyDarkening(lastDocument.file) + restoreTabs(lastDocument) prepareActions(lastDocument) } @@ -353,6 +355,49 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { reload(lastRequest, requireLastFile()) } + /** + * Tells the page whether it may follow the app into night mode - the one place that is decided, + * from what [DocumentDarkening] says about this kind of document. + */ + private fun applyDarkening(file: IdentifiedFile) { + pageView?.setDarkeningAllowed(DocumentDarkening.isAllowed(requireContext(), file.mimeType)) + } + + /** + * Flips that answer for every document of this kind, and shows it straight away: darkening is a + * webview setting, not something the page was translated with, so nothing is rendered again. + */ + fun toggleDarkening() { + val document = state.lastDocument ?: return + + val kind = DocumentDarkening.kindOf(document.file.mimeType) + DocumentDarkening.setAllowed( + requireContext(), + kind, + !DocumentDarkening.isAllowed(requireContext(), kind), + ) + + applyDarkening(document.file) + + // the row says what tapping it does, and what it does has just changed + prepareActions(document) + } + + /** + * The document again with the margins [PaginationSetting] now says, which is decided while + * translating - so there is no showing the change without rendering it a second time. + */ + fun reloadForMargins() { + if (!isAdded) { + return + } + + // not a new document, and not one the user went and opened either + freshOpenPending = false + + reload(requireLastRequest(), requireLastFile()) + } + /** The same document again, rendered differently - see [DocumentLoader.reload]. */ private fun reload(request: DocumentRequest, file: IdentifiedFile) { beforeLoad() @@ -441,9 +486,61 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { R.drawable.ic_edit, ) - // the order they unfold in, most wanted first + // what the display rows offer is the opposite of what is on screen, so each says what + // tapping it does rather than what it is called + val night = + DocumentActions.Action( + DocumentActions.ACTION_NIGHT_MODE, + if (NightModeSetting.isNight(requireContext())) R.string.menu_day_mode + else R.string.menu_night_mode, + R.drawable.ic_lightbulb, + ) + + // only while the app is dark: below that the webview darkens nothing whatever it is + // allowed, so the row would be a switch with nothing on the other end of it + val darkening = + if (!NightModeSetting.isNight(requireContext())) null + else { + val kind = DocumentDarkening.kindOf(document.file.mimeType) + val darkened = DocumentDarkening.isAllowed(requireContext(), kind) + + DocumentActions.Action( + DocumentActions.ACTION_DOCUMENT_DARKENING, + // it is remembered for the kind, not for the file, so it says which kind + when (kind) { + DocumentDarkening.Kind.PDF -> + if (darkened) R.string.menu_pdfs_light else R.string.menu_pdfs_dark + DocumentDarkening.Kind.IMAGE -> + if (darkened) R.string.menu_images_light else R.string.menu_images_dark + DocumentDarkening.Kind.DOCUMENT -> + if (darkened) R.string.menu_documents_light + else R.string.menu_documents_dark + }, + R.drawable.ic_invert_colors, + ) + } + + val margins = + DocumentActions.Action( + DocumentActions.ACTION_PAGE_MARGINS, + if (PaginationSetting.isEnabled(requireContext())) R.string.menu_fit_to_screen + else R.string.menu_page_borders, + R.drawable.ic_menu_book, + ) + + // the order they unfold in, most wanted first - and what a reader reaches for mid-document + // is how it is displayed, not what else can be done to it. the margins were only on the + // landing screen, which is a document closed away from anyone who wants them val unfolding = listOfNotNull( + night, + darkening, + margins, + DocumentActions.Action( + DocumentActions.ACTION_FULLSCREEN, + R.string.menu_fullscreen, + R.drawable.ic_fullscreen, + ), edit, DocumentActions.Action( DocumentActions.ACTION_TTS, @@ -470,11 +567,6 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { R.string.action_edit_save, R.drawable.ic_save, ), - DocumentActions.Action( - DocumentActions.ACTION_FULLSCREEN, - R.string.menu_fullscreen, - R.drawable.ic_fullscreen, - ), ) actions.setActions( @@ -555,6 +647,9 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { val activity = requireActivity() val file = document.file + // before the page is put in below, so it is drawn the way it is going to stay + applyDarkening(file) + analyticsManager.setCurrentScreen(activity, file.mimeType ?: UNKNOWN_FILE_TYPE) resetTabs() diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt b/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt index 63c6d3a16613..da5aa022d2c6 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt @@ -24,6 +24,8 @@ import androidx.lifecycle.ViewModelProvider import app.opendocument.droid.R import app.opendocument.droid.background.CatchAllSetting import app.opendocument.droid.background.DocumentLoader +import app.opendocument.droid.background.NightModeSetting +import app.opendocument.droid.background.PaginationSetting import app.opendocument.droid.background.PersistedUriPermissions import app.opendocument.droid.background.PrintingManager import app.opendocument.droid.background.SupportedDocumentTypes @@ -155,6 +157,11 @@ class MainActivity : AppCompatActivity() { } override fun onCreate(savedInstanceState: Bundle?) { + // before super, and remembered rather than left to the delegate: appcompat applies a mode + // the moment it is told, so setting it afterwards recreates the activity that has just + // been created, and it only keeps a local mode until the process goes + delegate.localNightMode = NightModeSetting.mode(this) + super.onCreate(savedInstanceState) setContentView(R.layout.main) @@ -533,6 +540,36 @@ class MainActivity : AppCompatActivity() { updateDocumentActionsVisible() } + DocumentActions.ACTION_NIGHT_MODE -> { + val night = !NightModeSetting.isNight(this) + + analyticsManager.report( + if (night) "menu_night_mode_enter" else "menu_night_mode_leave" + ) + + // recreates the activity, the way a rotation does - and survives it the same way: + // the loader is a ViewModel and the fragment saves the document and its page + delegate.localNightMode = NightModeSetting.setNight(this, night) + } + + DocumentActions.ACTION_DOCUMENT_DARKENING -> { + analyticsManager.report("menu_document_darkening") + + documentFragment?.toggleDarkening() + } + + DocumentActions.ACTION_PAGE_MARGINS -> { + val margins = !PaginationSetting.isEnabled(this) + + analyticsManager.report( + if (margins) "menu_page_margins_on" else "menu_page_margins_off" + ) + + PaginationSetting.setEnabled(this, margins) + + documentFragment?.reloadForMargins() + } + DocumentActions.ACTION_PRINT -> { analyticsManager.report("menu_print") diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/DocumentActions.kt b/app/src/main/java/app/opendocument/droid/ui/widget/DocumentActions.kt index 7540a682165f..517984162918 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/DocumentActions.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/DocumentActions.kt @@ -214,6 +214,9 @@ class DocumentActions(context: Context, attributeSet: AttributeSet?) : const val ACTION_OPEN_WITH: Int = 6 const val ACTION_SAVE: Int = 7 const val ACTION_FULLSCREEN: Int = 8 + const val ACTION_NIGHT_MODE: Int = 9 + const val ACTION_PAGE_MARGINS: Int = 10 + const val ACTION_DOCUMENT_DARKENING: Int = 11 private const val ANIMATION_MILLIS = 150L diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt index 3a459b9e77b5..00be6681398c 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt @@ -173,7 +173,8 @@ constructor(context: Context, attributeSet: AttributeSet?) : * app is in it: the webview darkens a page algorithmically, and at targetSdk 33 and up only * once the app theme reports itself as dark. * - * [DocumentFragment] decides which documents get it. + * [DocumentFragment] decides which documents get it, out of what `DocumentDarkening` says about + * the kind of document this is. */ fun setDarkeningAllowed(allowed: Boolean) { isDarkeningAllowed = allowed diff --git a/app/src/main/res/drawable/ic_invert_colors.xml b/app/src/main/res/drawable/ic_invert_colors.xml new file mode 100644 index 000000000000..8dacd7802254 --- /dev/null +++ b/app/src/main/res/drawable/ic_invert_colors.xml @@ -0,0 +1,10 @@ + + + diff --git a/app/src/main/res/drawable/ic_lightbulb.xml b/app/src/main/res/drawable/ic_lightbulb.xml new file mode 100644 index 000000000000..4686ca832ec1 --- /dev/null +++ b/app/src/main/res/drawable/ic_lightbulb.xml @@ -0,0 +1,10 @@ + + + diff --git a/app/src/main/res/drawable/ic_menu_book.xml b/app/src/main/res/drawable/ic_menu_book.xml new file mode 100644 index 000000000000..2788fde0d67b --- /dev/null +++ b/app/src/main/res/drawable/ic_menu_book.xml @@ -0,0 +1,20 @@ + + + + + + diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 8722823f7a90..bc1c7ffd8740 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -38,6 +38,16 @@ Go Pro Get Pro Fullscreen mode + Night mode + Day mode + Darken documents + Keep documents light + Darken PDFs + Keep PDFs light + Darken images + Keep images light + Fit to screen + Show page borders Print document Text-To-Speech Couldn\'t open selected app. From 88da46589615d6cc61d603f4e9e107bd123bedc1 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 11:53:47 +0200 Subject: [PATCH 02/10] Let the core say what darkens, and keep the reader's place Three answers the review asked for, and one the new engine can give. odrcore 6.8.0 renders a file dark itself, so `CoreLoader` translates every page with `HtmlColorScheme.SYSTEM`: a format that has a dark of its own now carries both, each behind the media query the webview answers, and the colours the file authored give way instead of being inverted. Nothing about the button changes - `PageView.setDarkeningAllowed` still picks between the two at display time, so darkening is still a webview setting and still renders nothing again. That makes `capabilitiesByFileType(...).colorScheme` the answer to what should darken, which `DocumentDarkening` had been guessing at. It defaults to that rather than to a list, and the guesses are gone: an image darkens now, because the core puts a dark ground under the photograph rather than into it, while a pdf and the media views have no dark of their own and are left as they were, offered but off. `Kind` is only what an override is remembered for. The margin button is offered where it does something. odrcore applies `textDocumentMargin` to a text document and nothing else, so on a pdf, an image or a spreadsheet the row translated the document again to show exactly the same page - and answered for the next text document opened while it was at it. Four types call themselves text and one of them is pdf, which is fixed pages laid out by a frontend the margin never reaches, so `PaginationSetting.affects` excludes it by name. And the margins reload keeps the reader where they were. It is the one thing that throws the page away and translates it again, and it came back at the top of the first tab; `DocumentFragment` now carries the tab and how far down it over to the document that arrives. A fraction rather than the offset, because the margins are exactly what changed the height it would have been measured against, and applied once the layout stops growing rather than when the load reports itself finished - a long document is still being laid out then. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML --- CLAUDE.md | 18 +++-- .../opendocument/droid/test/DarkModeTests.kt | 2 +- .../droid/test/MainActivityTests.kt | 29 ++++++++ .../droid/background/CoreLoader.kt | 8 +++ .../droid/background/DocumentDarkening.kt | 67 ++++++++++------- .../droid/background/PaginationSetting.kt | 25 +++++++ .../droid/ui/activity/DocumentFragment.kt | 48 ++++++++++--- .../opendocument/droid/ui/widget/PageView.kt | 71 +++++++++++++++++++ 8 files changed, 227 insertions(+), 41 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 02ae3df34835..53bc5964e7a4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -187,13 +187,21 @@ be done to it, and each remembers what it was last told: default one, so a phone that stays light all day can still be read at night. `NightModeSetting` stores no override at all once the choice agrees with the system again, or the app would sit in night mode through a morning the phone had long left. -- **Darkening** is per kind of document, not per file: a text document inverts into something that - still reads, a pdf or an image does not, so `DocumentDarkening` defaults the first to dark and - the other two to light, and the button edits that for every document of the kind. It is the one - place a mime type decides anything about display - `Odr.fileTypeByMimetype`, not a prefix list. - Presentations are documents until someone has looked at enough of them to say otherwise. +- **Darkening** defaults to what the core says and is overridden per kind of document, not per + file. `CoreLoader` translates every page with `HtmlColorScheme.SYSTEM`, so a format that has a + dark of its own carries both behind `prefers-color-scheme`, and + `capabilitiesByFileType(...).colorScheme` is whether it has one - which is what + `DocumentDarkening` defaults to rather than a list. A pdf and the media views have none, so + there all the webview can do is invert, which is offered but off. Do not put the guesses back: + it was a guess that presentations and images invert badly, and the core answers both. + `PageView.setDarkeningAllowed` picks between the two schemes at display time, which is why the + button changes nothing about the translation and renders nothing again. - **The margins** are odrcore's `textDocumentMargin`, decided while translating, so the button renders the document again through `DocumentLoader.reload` - the copy in the cache, not the file. + It reaches a *text* document only, which `PaginationSetting.affects` answers and the button is + gated on: everything else would be translated again to look exactly the same. A reload throws + the page away, so `DocumentFragment` carries the tab and how far down it the reader was over to + the document that comes back - as a fraction, the margins having changed the height. Do not move these into a settings screen. `PaginationSetting` keeps its landing row because it already had one and both write the same preference; the other two never get one. diff --git a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt index 9ee37dc71d1a..6989c20c6d54 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt @@ -65,7 +65,7 @@ class DarkModeTests { NightModeSetting.setNight(targetContext(), systemIsNight()) for (kind in DocumentDarkening.Kind.entries) { - DocumentDarkening.setAllowed(targetContext(), kind, kind.darkensByDefault) + DocumentDarkening.clear(targetContext(), kind) } // switching night mode recreates the activity, leaving one behind that the rule does not diff --git a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt index 2081d7c855dc..f876353b1bd0 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt @@ -320,6 +320,35 @@ class MainActivityTests { } } + /** + * The button is only there where tapping it changes something: odrcore lays a text document out + * with the margins or without them, and every other view the same either way. Launches nothing + * - it is the rule the button is gated on, and the core answers it. + */ + @Test + fun theMarginSwitchIsOnlyOfferedForTextDocuments() { + Assert.assertTrue( + "a text document is what the margins are for", + PaginationSetting.affects("application/vnd.oasis.opendocument.text"), + ) + + for (mimeType in + listOf( + "application/pdf", + "image/png", + "application/vnd.oasis.opendocument.spreadsheet", + "application/vnd.oasis.opendocument.presentation", + "text/plain", + )) { + Assert.assertFalse( + "$mimeType would have been rendered again to look exactly the same", + PaginationSetting.affects(mimeType), + ) + } + + Assert.assertFalse("nothing is not a text document", PaginationSetting.affects(null)) + } + @Test fun testDocumentSurvivesRecreation() { val activity = mainActivityActivityTestRule.activity diff --git a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt index b71b8db6f895..1b60761f4a75 100644 --- a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt @@ -8,6 +8,7 @@ import app.opendocument.core.DecodedFile import app.opendocument.core.Document import app.opendocument.core.DocumentType import app.opendocument.core.Html +import app.opendocument.core.HtmlColorScheme import app.opendocument.core.HtmlConfig import app.opendocument.core.HtmlView import app.opendocument.core.HttpServer @@ -149,6 +150,13 @@ class CoreLoader(private val context: Context) { htmlConfig.textDocumentMargin = paging htmlConfig.editable = editable + // both schemes, each behind the media query the reader's webview answers, rather than the + // one scheme it is being read in right now: what a page carries is decided here, while it + // is translated, and darkening is turned on and off over the open document without + // translating it again. PageView.setDarkeningAllowed is what picks between them, and a + // view whose format has no dark of its own is left to be inverted as before + htmlConfig.colorScheme = HtmlColorScheme.SYSTEM + val cacheDirectory = File(cachePath) cacheDirectory.deleteRecursively() cacheDirectory.mkdirs() diff --git a/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt index afd5da89a8dd..38b6f4542d7c 100644 --- a/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt +++ b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt @@ -7,59 +7,74 @@ import app.opendocument.core.Odr /** * Whether a document follows the app into night mode, which is not one answer for everything the - * app opens: a text document inverts into something that still reads, a scanned page or a photo - * into something nobody wrote. + * app opens: a text document reads dark, a scanned page inverted is something nobody wrote. * - * So the answer is per [Kind], and the button over the document is what edits it - there is no - * switch for this on the landing screen. The kind is what is remembered rather than the file: it is - * never *this* pdf that inverts badly, it is pdfs. + * The core answers it now - see [darkensByDefault] - and the button over the document is what + * overrides that. There is no switch for this on the landing screen. What is remembered is the + * [Kind] rather than the file: it is never *this* pdf that inverts badly, it is pdfs. */ object DocumentDarkening { /** - * What the answer differs for, each named the way the button over the document says it. - * - * Presentations and drawings are [DOCUMENT] until someone has looked at enough of them to say - * otherwise - a designed page may well invert as badly as a pdf does, but that is a guess, and - * the two below are not. + * What an override is remembered for, each named the way the button over the document says it. */ - enum class Kind(val darkensByDefault: Boolean) { + enum class Kind { /** Text, spreadsheets, plain text, and everything else the core reflows into html. */ - DOCUMENT(true), + DOCUMENT, /** * Fixed pages, scans included, which is where the app's rendering is at its most literal. */ - PDF(false), + PDF, - /** A photograph inverted is a photograph of nothing. */ - IMAGE(false), + /** A photograph, which the core darkens by putting a dark ground under rather than into. */ + IMAGE, } - fun kindOf(mimeType: String?): Kind { - // not lowercased: the core's table is matched exactly and spells some entries with - // capitals ("macroEnabled"). canonicalMimeType has already been applied upstream - val fileType = mimeType?.let { Odr.fileTypeByMimetype(it) } ?: return Kind.DOCUMENT + fun kindOf(mimeType: String?): Kind = kindOf(fileTypeOf(mimeType)) - return when { + private fun kindOf(fileType: FileType?): Kind = + when { + fileType == null -> Kind.DOCUMENT fileType == FileType.PORTABLE_DOCUMENT_FORMAT -> Kind.PDF Odr.fileCategoryByFileType(fileType) == FileCategory.IMAGE -> Kind.IMAGE else -> Kind.DOCUMENT } - } - /** Whether what [mimeType] names darkens, which is the default until the button says else. */ - fun isAllowed(context: Context, mimeType: String?): Boolean = - isAllowed(context, kindOf(mimeType)) + private fun fileTypeOf(mimeType: String?): FileType? = + // not lowercased: the core's table is matched exactly and spells some entries with + // capitals ("macroEnabled"). canonicalMimeType has already been applied upstream + mimeType?.let { Odr.fileTypeByMimetype(it) } + + /** + * Whether the core renders this type dark itself, which is what darkening defaults to. + * + * Where it does, the dark is the one the format was translated into - the colours the file + * authored giving way, a photograph keeping its own and taking a dark ground instead. Where it + * does not, a pdf and the media views, all that is left is the webview inverting what it was + * handed, so that is offered but not taken for granted. + */ + private fun darkensByDefault(fileType: FileType?): Boolean = + fileType != null && Odr.capabilitiesByFileType(fileType).colorScheme - fun isAllowed(context: Context, kind: Kind): Boolean = - AppPreferences.of(context).getBoolean(prefKey(kind), kind.darkensByDefault) + /** Whether what [mimeType] names darkens, the button's answer first and the core's after. */ + fun isAllowed(context: Context, mimeType: String?): Boolean { + val fileType = fileTypeOf(mimeType) + + return AppPreferences.of(context) + .getBoolean(prefKey(kindOf(fileType)), darkensByDefault(fileType)) + } fun setAllowed(context: Context, kind: Kind, allowed: Boolean) { AppPreferences.of(context).edit().putBoolean(prefKey(kind), allowed).apply() } + /** Forgets the override, which leaves the core answering for the kind again. */ + fun clear(context: Context, kind: Kind) { + AppPreferences.of(context).edit().remove(prefKey(kind)).apply() + } + private fun prefKey(kind: Kind) = PREF_PREFIX + kind.name.lowercase() private const val PREF_PREFIX = "darken_" diff --git a/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt b/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt index 51719a6e6d06..b0d48a38678c 100644 --- a/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt +++ b/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt @@ -1,6 +1,9 @@ package app.opendocument.droid.background import android.content.Context +import app.opendocument.core.DocumentType +import app.opendocument.core.FileType +import app.opendocument.core.Odr /** * Whether a document keeps the side margins of a printed page, or fills the screen. @@ -26,6 +29,28 @@ object PaginationSetting { fun isEnabled(context: Context): Boolean = AppPreferences.of(context).getBoolean(PREF_PAGINATION_ENABLED, DEFAULT_ENABLED) + /** + * Whether this reaches what [mimeType] names at all. + * + * odrcore lays a *text* document out with the margins or without them and nothing else: a + * presentation and a drawing are paged whatever it says, and a spreadsheet, a pdf, an image or + * a plain text file are never paged. Offering the button on one of those would render the + * document again to show nothing new, and quietly answer for the next text document opened. + */ + fun affects(mimeType: String?): Boolean { + // not lowercased, and for the same reason as DocumentDarkening.kindOf + val fileType = mimeType?.let { Odr.fileTypeByMimetype(it) } ?: return false + + // four types call themselves text: odt, docx, doc - and pdf, which is fixed pages the core + // lays out with a frontend of its own that the margin never reaches. So it is asked about + // the type rather than trusted here + if (fileType == FileType.PORTABLE_DOCUMENT_FORMAT) { + return false + } + + return Odr.documentTypeByFileType(fileType) == DocumentType.TEXT + } + fun setEnabled(context: Context, enabled: Boolean) { AppPreferences.of(context).edit().putBoolean(PREF_PAGINATION_ENABLED, enabled).apply() } diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt index 2c06db539970..bc3e2634c160 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt @@ -79,6 +79,15 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { private var freshOpenPending = false + /** + * Where the reader was before a reload that is not theirs, held from [reloadForMargins] until + * the document comes back. Null at every other load: opening a document belongs at its top. + */ + private var positionToRestore: ReadingPosition? = null + + /** A tab and how far down it, which survives the document being translated again. */ + private data class ReadingPosition(val tab: Int, val scrollFraction: Float) + private lateinit var tabLayout: TabLayout private lateinit var documentLoader: DocumentLoader @@ -374,7 +383,7 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { DocumentDarkening.setAllowed( requireContext(), kind, - !DocumentDarkening.isAllowed(requireContext(), kind), + !DocumentDarkening.isAllowed(requireContext(), document.file.mimeType), ) applyDarkening(document.file) @@ -392,6 +401,14 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { return } + // the page is thrown away and translated again, so where the reader had got to is taken + // along by hand - it is the same document, and they did not ask to be put back at the top + positionToRestore = + ReadingPosition( + maxOf(state.lastSelectedTab, 0), + pageView?.verticalScrollFraction ?: 0f, + ) + // not a new document, and not one the user went and opened either freshOpenPending = false @@ -502,7 +519,7 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { if (!NightModeSetting.isNight(requireContext())) null else { val kind = DocumentDarkening.kindOf(document.file.mimeType) - val darkened = DocumentDarkening.isAllowed(requireContext(), kind) + val darkened = DocumentDarkening.isAllowed(requireContext(), document.file.mimeType) DocumentActions.Action( DocumentActions.ACTION_DOCUMENT_DARKENING, @@ -520,13 +537,17 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { ) } + // odrcore applies them to a text document and nothing else, so anywhere else the row would + // render the document again to show nothing new - see PaginationSetting.affects val margins = - DocumentActions.Action( - DocumentActions.ACTION_PAGE_MARGINS, - if (PaginationSetting.isEnabled(requireContext())) R.string.menu_fit_to_screen - else R.string.menu_page_borders, - R.drawable.ic_menu_book, - ) + if (!PaginationSetting.affects(document.file.mimeType)) null + else + DocumentActions.Action( + DocumentActions.ACTION_PAGE_MARGINS, + if (PaginationSetting.isEnabled(requireContext())) R.string.menu_fit_to_screen + else R.string.menu_page_borders, + R.drawable.ic_menu_book, + ) // the order they unfold in, most wanted first - and what a reader reaches for mid-document // is how it is displayed, not what else can be done to it. the margins were only on the @@ -652,14 +673,23 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { analyticsManager.setCurrentScreen(activity, file.mimeType ?: UNKNOWN_FILE_TYPE) + // clears lastSelectedTab, so what reloadForMargins put aside is read after it resetTabs() + val restored = positionToRestore + positionToRestore = null + + // before the load below, which is the one it is waiting for. always, and not only when + // there is something to put back: a reload that failed on the way here would otherwise + // leave its fraction waiting for whatever document is opened next + pageView?.restoreScrollFraction(restored?.scrollFraction ?: 0f) + val titles = document.partTitles val pages = titles.size if (pages > 1) { addTabs(titles) - tabLayout.getTabAt(0)?.select() + tabLayout.getTabAt(restored?.tab?.coerceAtMost(pages - 1) ?: 0)?.select() } else if (pages == 1) { loadData(document.partUris[0].toString()) } diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt index 00be6681398c..cee319955cff 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt @@ -75,6 +75,8 @@ constructor(context: Context, attributeSet: AttributeSet?) : override fun onPageFinished(view: WebView, url: String) { super.onPageFinished(view, url) + restorePendingScroll(0) + buggyWebViewHandler.postDelayed( { if (!wasCommitCalled) { @@ -157,6 +159,70 @@ constructor(context: Context, attributeSet: AttributeSet?) : } } + /** + * Where the page sits, as a fraction of what there is to scroll. + * + * A fraction and not the offset itself: the one thing that reloads a document in place is a + * change to how it is laid out, and that changes the height the offset would be measured + * against. + */ + val verticalScrollFraction: Float + get() { + val scrollable = computeVerticalScrollRange() - computeVerticalScrollExtent() + + return if (scrollable <= 0) 0f + else (computeVerticalScrollOffset().toFloat() / scrollable).coerceIn(0f, 1f) + } + + private var scrollFractionToRestore: Float? = null + + /** The height the last attempt at restoring measured, to see whether it is still growing. */ + private var lastScrollableHeight = -1 + + private val scrollRestoreHandler = Handler(Looper.getMainLooper()) + + /** + * Puts the next page loaded back to [fraction] of its height. + * + * Not applied here: the page is still being laid out when the load reports itself finished, and + * until it has a height there is nothing to put anything back to. + */ + fun restoreScrollFraction(fraction: Float) { + scrollFractionToRestore = fraction.takeIf { it > 0f } + lastScrollableHeight = -1 + } + + /** + * Waits for a height that has stopped growing and scrolls to it - a long document goes on being + * laid out for a while, and measuring against the first height it reports lands near the top of + * where the reader was. Gives up after [SCROLL_RESTORE_ATTEMPTS], leaving the page where it is. + */ + private fun restorePendingScroll(attempt: Int) { + val fraction = scrollFractionToRestore ?: return + + val scrollable = computeVerticalScrollRange() - computeVerticalScrollExtent() + + if ( + (scrollable <= 0 || scrollable != lastScrollableHeight) && + attempt < SCROLL_RESTORE_ATTEMPTS + ) { + lastScrollableHeight = scrollable + + scrollRestoreHandler.postDelayed( + { restorePendingScroll(attempt + 1) }, + SCROLL_RESTORE_INTERVAL_MS, + ) + + return + } + + scrollFractionToRestore = null + + if (scrollable > 0) { + scrollTo(scrollX, (fraction * scrollable).toInt()) + } + } + /** What [setDarkeningAllowed] was last set to, whether or not printing has it suspended. */ var isDarkeningAllowed = false private set @@ -369,5 +435,10 @@ constructor(context: Context, attributeSet: AttributeSet?) : /** Where CoreLoader publishes a translated document. */ const val LOCAL_SERVER_URL_PREFIX = "http://localhost:" + + /** Two seconds of them, which a megabyte of text lays out well inside of. */ + const val SCROLL_RESTORE_ATTEMPTS = 20 + + const val SCROLL_RESTORE_INTERVAL_MS = 100L } } From 6533f8396a7ff76396a363079309f4280386d30f Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 12:17:36 +0200 Subject: [PATCH 03/10] Darken a file the core could not name, as before `darkensByDefault` asked the core about a file type and refused when there was none, which is a mime type the core does not know - and that file is being shown as text or as the html fallback, both of which have a dark of their own. It is also the kind `kindOf` gives it. The list this replaced said DOCUMENT and so darkened it; this now says the same. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML --- .../app/opendocument/droid/background/DocumentDarkening.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt index 38b6f4542d7c..261ad5ed69be 100644 --- a/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt +++ b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt @@ -56,7 +56,9 @@ object DocumentDarkening { * handed, so that is offered but not taken for granted. */ private fun darkensByDefault(fileType: FileType?): Boolean = - fileType != null && Odr.capabilitiesByFileType(fileType).colorScheme + // nothing named it, so it is being shown as text or as the html fallback, and both have a + // dark of their own - the same answer [Kind.DOCUMENT] is the kind for + fileType == null || Odr.capabilitiesByFileType(fileType).colorScheme /** Whether what [mimeType] names darkens, the button's answer first and the core's after. */ fun isAllowed(context: Context, mimeType: String?): Boolean { From fc21e49f2dc1944ed1d1f985c4fe53ddb9d83caf Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 14:02:55 +0200 Subject: [PATCH 04/10] Say what the webview was, when a page will not go dark These tests fail on one api level at a time, on emulators whose webview is far older than any developer machine's, and "the page stayed light - mean luminance 252" does not say which of the two darkening apis was in play, or whether the page view was allowed to darken at all. Both luminance assertions now say. Nothing about the app changes. `theSwitchDarkensADayModeApp` fails on api 29 and nowhere else, it fails the same before and after this branch's darkening changes, and the first guess at why - that the restored page was rendered before darkening reached it - was wrong: moving the call ahead of `restore` changed the reading not at all. So this asks the failing run instead of guessing again. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML --- .../opendocument/droid/test/DarkModeTests.kt | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt index 6989c20c6d54..327b93853fbe 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt @@ -17,6 +17,7 @@ import androidx.test.rule.ActivityTestRule import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry import androidx.test.runner.lifecycle.Stage import androidx.webkit.WebSettingsCompat +import androidx.webkit.WebViewCompat import androidx.webkit.WebViewFeature import app.opendocument.droid.background.DocumentDarkening import app.opendocument.droid.background.NightModeSetting @@ -127,7 +128,10 @@ class DarkModeTests { var luminance = WHITE val darkened = waitFor(60000) { meanLuminance().also { luminance = it } < DARK_LUMINANCE } - Assert.assertTrue("the page stayed light - mean luminance $luminance", darkened) + Assert.assertTrue( + "the page stayed light - mean luminance $luminance; ${darkeningDiagnosis()}", + darkened, + ) } /** @@ -162,7 +166,10 @@ class DarkModeTests { var luminance = WHITE val darkened = waitFor(60000) { meanLuminance().also { luminance = it } < DARK_LUMINANCE } - Assert.assertTrue("the page stayed light - mean luminance $luminance", darkened) + Assert.assertTrue( + "the page stayed light - mean luminance $luminance; ${darkeningDiagnosis()}", + darkened, + ) Assert.assertEquals( "the switch was not remembered", AppCompatDelegate.MODE_NIGHT_YES, @@ -223,6 +230,29 @@ class DarkModeTests { return darkening.get() } + /** + * What the webview is and what it was given, for a failure message: these tests fail on one api + * level at a time, on emulators whose webview is far older than any developer machine's, and + * "the page stayed light" alone does not say which of the two darkening apis was even in play. + */ + private fun darkeningDiagnosis(): String { + val algorithmic = WebViewFeature.isFeatureSupported(WebViewFeature.ALGORITHMIC_DARKENING) + val force = WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK) + + val webView = + try { + WebViewCompat.getCurrentWebViewPackage(targetContext())?.versionName ?: "none" + } catch (t: Throwable) { + "unknown (${t.javaClass.simpleName})" + } + + val pageView = resumedMainActivity()?.let { waitForFragment(it)?.pageView } + + return "webview $webView, algorithmicDarkening=$algorithmic, forceDark=$force, " + + "night=${NightModeSetting.isNight(targetContext())}, " + + "allowed=${pageView?.isDarkeningAllowed}, setting=${pageView?.let(::darkeningSetting)}" + } + private fun canDarken() = WebViewFeature.isFeatureSupported(WebViewFeature.ALGORITHMIC_DARKENING) || WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK) From 8f0d293908be18bb5895ed50cda79793c5fe1cdb Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 14:19:54 +0200 Subject: [PATCH 05/10] Invert on the old webview, rather than stand aside for a theme it cannot match The diagnosis the last commit added says what api 29 is: webview 74, no algorithmic darkening, force dark instead - and the app doing its part, the page view allowed to darken and force dark switched on, over a page that stayed light. Force dark defaults to standing aside for a page carrying a dark theme of its own, and since `HtmlColorScheme.SYSTEM` every page carries one. A webview this old answers prefers-color-scheme by the system alone and never matches it for a forced dark, so what it stands aside for is nothing at all. Asking for `DARK_STRATEGY_USER_AGENT_DARKENING_ONLY` says invert it anyway, which on that branch is the only dark there is; the newer branch is untouched and still gets the page's own dark, which is the better one. This is not the whole of api 29: main fails `theDrawnPageIsDark` there too, at the same luminance and without any of this branch's changes, so that webview was already not darkening reliably. What this fixes is the part this branch would otherwise have added to it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML --- .../app/opendocument/droid/ui/widget/PageView.kt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt index cee319955cff..83944556e9a1 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt @@ -278,6 +278,18 @@ constructor(context: Context, attributeSet: AttributeSet?) : if (WebViewFeature.isFeatureSupported(WebViewFeature.ALGORITHMIC_DARKENING)) { WebSettingsCompat.setAlgorithmicDarkeningAllowed(settings, darken) } else if (WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK)) { + // Invert, and do not stand aside for the page's own dark theme, which is what this api + // does by default. Every page carries one now - CoreLoader translates with + // HtmlColorScheme.SYSTEM - but a webview old enough to be on this branch answers + // prefers-color-scheme by the system alone and never matches it for a forced dark, so + // deferring to that theme is deferring to nothing and the page stays light + if (WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK_STRATEGY)) { + WebSettingsCompat.setForceDarkStrategy( + settings, + WebSettingsCompat.DARK_STRATEGY_USER_AGENT_DARKENING_ONLY, + ) + } + // ON rather than AUTO on the pre-webkit-1.6 api: AUTO is the platform's smart dark, // which an app declaring a dark theme is deliberately left out of, so it never fires // here. Asking the app whether it is in night mode is what AUTO cannot do for us From ea5ac3cdb29418d4ed90e6b538d4e8b8441a6ffa Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 14:57:11 +0200 Subject: [PATCH 06/10] Skip the pixel tests on a webview that cannot darken, and wait for the text Two levels were red for reasons of their own. Api 29 ships webview 74, which reports `FORCE_DARK` supported, takes the setting, hands it back, and draws the page exactly as light as before - force dark is only implemented from 76. So `canDarken` asks the version as well as the api, and the two tests that read pixels skip there rather than fail for a webview that was never going to darken. What the app does is still asserted on api 29, through `darkeningSetting`: allowed, and the setting it was given. A version that cannot be read counts as capable, since a skip taken by mistake is coverage quietly lost. `aMegabyteOfTextOpensAndIsSearchable` waited for the element count to stop changing and then searched, and found nothing. A megabyte is parsed in bursts, so two equal readings can both land inside one pause: the page had laid out, which is what that wait asserts, and was still filling. It now waits for the text as well - every line carries the needle once, so they are all in the page or it is not all there yet. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML --- .../opendocument/droid/test/DarkModeTests.kt | 34 +++++++++++++++++-- .../opendocument/droid/test/LargeTextTests.kt | 17 ++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt index 327b93853fbe..25cc41b85c6c 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt @@ -115,7 +115,8 @@ class DarkModeTests { @Test fun theDrawnPageIsDark() { Assume.assumeTrue( - "this webview has no darkening api at all - nothing the app sets could reach it", + "this webview cannot darken a page - nothing the app sets could reach it. " + + "${darkeningDiagnosis()}", canDarken(), ) @@ -141,7 +142,8 @@ class DarkModeTests { @Test fun theSwitchDarkensADayModeApp() { Assume.assumeTrue( - "this webview has no darkening api at all - nothing the app sets could reach it", + "this webview cannot darken a page - nothing the app sets could reach it. " + + "${darkeningDiagnosis()}", canDarken(), ) Assume.assumeFalse( @@ -253,9 +255,32 @@ class DarkModeTests { "allowed=${pageView?.isDarkeningAllowed}, setting=${pageView?.let(::darkeningSetting)}" } + /** + * Whether this webview can darken a page at all, which is not the same as its saying it can. + * + * The api 29 image ships webview 74, which reports `FORCE_DARK` supported, takes the setting + * and hands it straight back - and draws the page exactly as light as it was. Force dark is + * only implemented from 76. What the app does there is still asserted, through + * [darkeningSetting]; it is the screen that cannot be asked, so the two tests that read pixels + * skip instead of failing for a webview that was never going to darken. + * + * An unreadable version counts as capable: a missed skip is a failure to look at, a skip taken + * by mistake is coverage quietly lost. + */ private fun canDarken() = WebViewFeature.isFeatureSupported(WebViewFeature.ALGORITHMIC_DARKENING) || - WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK) + (WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK) && + webViewMajorVersion() >= FORCE_DARK_MIN_WEBVIEW) + + private fun webViewMajorVersion(): Int = + try { + WebViewCompat.getCurrentWebViewPackage(targetContext()) + ?.versionName + ?.substringBefore('.') + ?.toIntOrNull() ?: Int.MAX_VALUE + } catch (t: Throwable) { + Int.MAX_VALUE + } /** * What the middle of the screen draws, averaged - 0 is black and 255 white. @@ -399,6 +424,9 @@ class DarkModeTests { private companion object { /** Below this the page is dark rather than the white a document is authored on. */ + /** Force dark landed in this one; 74, which api 29 ships, takes the setting and lies. */ + private const val FORCE_DARK_MIN_WEBVIEW = 76 + private const val DARK_LUMINANCE = 128 private const val WHITE = 255 diff --git a/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt index 0b1aa915d61a..1986f920f20a 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt @@ -53,6 +53,15 @@ class LargeTextTests { val elements = awaitStableDom(pageView) Assert.assertTrue("the page never laid out", elements > 1) + // and then the text itself. A megabyte is parsed in bursts, so the element count can stop + // changing inside a pause rather than at the end - two equal readings both landing in one + // is a page still filling, which the search then ran against and found nothing in + var needles = -1 + Assert.assertTrue( + "the text never finished arriving - $NEEDLE is in the page $needles times, not $lines", + waitFor(TIMEOUT_MS) { needlesInDom(pageView).also { needles = it } == lines }, + ) + val matches = findAll(pageView, NEEDLE) val elapsed = SystemClock.elapsedRealtime() - start @@ -90,6 +99,14 @@ class LargeTextTests { return previous } + /** Every line carries the needle once, so the page is all there when they all are. */ + private fun needlesInDom(pageView: PageView): Int = + evaluateJavascript( + pageView, + "(document.body.textContent.match(/$NEEDLE/g) || []).length", + ) + ?.toIntOrNull() ?: -1 + private fun elementCount(pageView: PageView): Int = evaluateJavascript(pageView, "document.getElementsByTagName('*').length")?.toIntOrNull() ?: -1 From c3f39c5442945a4aa5441d33bc011412971b5cb8 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 15:38:57 +0200 Subject: [PATCH 07/10] Hold the reading position the margin switch used to lose Polled with a timeout and reported with what it actually read, the way the edit mode tests do it: the page has to be laid out again before there is anywhere to put anyone back to, so there is nothing to assert straight after the reload. A fraction on both sides. The margins are exactly what changes the height, so the same place in the text is a different offset once the page has been laid out again - hence a tolerance rather than an equality. It scrolls to the middle rather than the end, since the last screenful is one position however far past it the page is scrolled, and a test that stopped there would pass without anything being restored at all. `style-various-1.docx` because it is long enough to scroll, and a text document, which is the only kind the margin button is offered on. With the restore taken back out it fails with "the reader was put back at 0.0 of the page, not around 0.5", which is what it is for. `verticalScrollableHeight` is public now because the test cannot reach the protected webview call it was spelled out of, and it was written three times in here anyway. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML --- .../droid/test/MainActivityTests.kt | 94 +++++++++++++++++++ .../opendocument/droid/ui/widget/PageView.kt | 8 +- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt index f876353b1bd0..4a529d4b259c 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt @@ -48,7 +48,9 @@ import java.net.HttpURLConnection import java.net.URL import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger import java.util.concurrent.atomic.AtomicReference +import kotlin.math.abs import org.hamcrest.Matchers.equalTo import org.junit.After import org.junit.AfterClass @@ -349,6 +351,60 @@ class MainActivityTests { Assert.assertFalse("nothing is not a text document", PaginationSetting.affects(null)) } + /** + * The margins render the document again, and a reader who was halfway down it should still be + * halfway down it afterwards - the reload used to hand back the top of the page. + * + * A fraction on both sides, because the margins are exactly what changes the height: the same + * place in the text is a different offset once the page has been laid out again. Polled rather + * than asserted straight after the reload, the way the edit mode tests poll - the page has to + * be laid out again before there is anywhere to put anyone back to. + */ + @Test + fun theMarginSwitchKeepsTheReadingPosition() { + val activity = mainActivityActivityTestRule.activity + val documentFragment = loadDocument(activity, requireTestFile("style-various-1.docx")) + val pageView = checkNotNull(documentFragment.pageView) { "no page view" } + + Assert.assertTrue( + "the document never became long enough to scroll. dom=${describeDom(pageView)}", + waitFor(EDIT_MODE_TIMEOUT_MS) { scrollableHeight(pageView) > 0 }, + ) + + // a long way down, but not the very end: the last screenful is one position however far + // the page is scrolled past it, so it would pass without anything being restored at all + val before = scrollToFraction(pageView, READING_POSITION) + Assert.assertTrue( + "the page did not scroll - it read back at $before", + before > READING_POSITION / 2, + ) + + val document = documentFragment.lastDocument + val margins = PaginationSetting.isEnabled(activity) + try { + InstrumentationRegistry.getInstrumentation().runOnMainSync { + activity.onDocumentAction(DocumentActions.ACTION_PAGE_MARGINS) + } + + Assert.assertTrue( + "the document was never rendered again", + waitFor(RELOAD_TIMEOUT_MS) { documentFragment.lastDocument !== document }, + ) + + var restored = 0f + Assert.assertTrue( + "the reader was put back at ${restored} of the page, not around $before", + waitFor(EDIT_MODE_TIMEOUT_MS) { + restored = scrollFraction(pageView) + abs(restored - before) < POSITION_TOLERANCE + }, + ) + } finally { + // it outlives the test otherwise: it is a preference, not activity state + PaginationSetting.setEnabled(activity, margins) + } + } + @Test fun testDocumentSurvivesRecreation() { val activity = mainActivityActivityTestRule.activity @@ -557,6 +613,36 @@ class MainActivityTests { ) } + /** What there is to scroll, which is nothing at all until the page has laid out. */ + private fun scrollableHeight(pageView: PageView): Int { + val height = AtomicInteger(0) + InstrumentationRegistry.getInstrumentation().runOnMainSync { + height.set(pageView.verticalScrollableHeight) + } + return height.get() + } + + private fun scrollFraction(pageView: PageView): Float { + val fraction = AtomicReference(0f) + InstrumentationRegistry.getInstrumentation().runOnMainSync { + fraction.set(pageView.verticalScrollFraction) + } + return fraction.get() + } + + /** Scrolls there and answers where it actually landed. */ + private fun scrollToFraction(pageView: PageView, fraction: Float): Float { + InstrumentationRegistry.getInstrumentation().runOnMainSync { + pageView.scrollTo( + pageView.scrollX, + (pageView.verticalScrollableHeight * fraction).toInt(), + ) + } + InstrumentationRegistry.getInstrumentation().waitForIdleSync() + + return scrollFraction(pageView) + } + private fun describeDom(pageView: PageView): String = evaluateJavascript( pageView, @@ -664,6 +750,14 @@ class MainActivityTests { // a document already in the cache, translated a second time private const val RELOAD_TIMEOUT_MS = 10000L + /** Well down the document, and well clear of the last screenful - see the test. */ + private const val READING_POSITION = 0.5f + + /** + * The page is laid out again, so the same place in the text is near, not at, the offset. + */ + private const val POSITION_TOLERANCE = 0.15f + private const val DIALOG_TIMEOUT_MS = 10000L private val testFiles = mutableMapOf() diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt index 83944556e9a1..0000a585c450 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt @@ -168,12 +168,16 @@ constructor(context: Context, attributeSet: AttributeSet?) : */ val verticalScrollFraction: Float get() { - val scrollable = computeVerticalScrollRange() - computeVerticalScrollExtent() + val scrollable = verticalScrollableHeight return if (scrollable <= 0) 0f else (computeVerticalScrollOffset().toFloat() / scrollable).coerceIn(0f, 1f) } + /** How far the page can be scrolled: its height less the screenful already showing. */ + val verticalScrollableHeight: Int + get() = computeVerticalScrollRange() - computeVerticalScrollExtent() + private var scrollFractionToRestore: Float? = null /** The height the last attempt at restoring measured, to see whether it is still growing. */ @@ -200,7 +204,7 @@ constructor(context: Context, attributeSet: AttributeSet?) : private fun restorePendingScroll(attempt: Int) { val fraction = scrollFractionToRestore ?: return - val scrollable = computeVerticalScrollRange() - computeVerticalScrollExtent() + val scrollable = verticalScrollableHeight if ( (scrollable <= 0 || scrollable != lastScrollableHeight) && From 12020b389b01c5b6d87f077f94ca707be06fd80c Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 16:46:09 +0200 Subject: [PATCH 08/10] Say the same thing about the display buttons in fewer words Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011ou83nozsNLKjSPw7h29zH --- .../droid/background/CoreLoader.kt | 8 ++--- .../droid/background/DocumentDarkening.kt | 27 ++++------------- .../droid/background/NightModeSetting.kt | 29 ++++++++----------- .../droid/background/PaginationSetting.kt | 19 +++++------- .../droid/ui/activity/DocumentFragment.kt | 27 +++++++---------- .../droid/ui/activity/MainActivity.kt | 7 ++--- .../opendocument/droid/ui/widget/PageView.kt | 23 ++++++--------- 7 files changed, 52 insertions(+), 88 deletions(-) diff --git a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt index 1b60761f4a75..08f75922ccc0 100644 --- a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt @@ -150,11 +150,9 @@ class CoreLoader(private val context: Context) { htmlConfig.textDocumentMargin = paging htmlConfig.editable = editable - // both schemes, each behind the media query the reader's webview answers, rather than the - // one scheme it is being read in right now: what a page carries is decided here, while it - // is translated, and darkening is turned on and off over the open document without - // translating it again. PageView.setDarkeningAllowed is what picks between them, and a - // view whose format has no dark of its own is left to be inverted as before + // both schemes, each behind prefers-color-scheme, rather than the one it is being read in + // now: this is decided while translating, and darkening is turned on and off over the open + // document. PageView.setDarkeningAllowed picks between them htmlConfig.colorScheme = HtmlColorScheme.SYSTEM val cacheDirectory = File(cachePath) diff --git a/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt index 261ad5ed69be..de74fd8f1c63 100644 --- a/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt +++ b/app/src/main/java/app/opendocument/droid/background/DocumentDarkening.kt @@ -9,26 +9,15 @@ import app.opendocument.core.Odr * Whether a document follows the app into night mode, which is not one answer for everything the * app opens: a text document reads dark, a scanned page inverted is something nobody wrote. * - * The core answers it now - see [darkensByDefault] - and the button over the document is what - * overrides that. There is no switch for this on the landing screen. What is remembered is the - * [Kind] rather than the file: it is never *this* pdf that inverts badly, it is pdfs. + * The core answers it - see [darkensByDefault] - and the button over the document overrides that, + * for the [Kind] rather than the file: it is never *this* pdf that inverts badly, it is pdfs. */ object DocumentDarkening { - /** - * What an override is remembered for, each named the way the button over the document says it. - */ + /** What an override is remembered for, each named the way the button over the document says. */ enum class Kind { - - /** Text, spreadsheets, plain text, and everything else the core reflows into html. */ DOCUMENT, - - /** - * Fixed pages, scans included, which is where the app's rendering is at its most literal. - */ PDF, - - /** A photograph, which the core darkens by putting a dark ground under rather than into. */ IMAGE, } @@ -43,21 +32,17 @@ object DocumentDarkening { } private fun fileTypeOf(mimeType: String?): FileType? = - // not lowercased: the core's table is matched exactly and spells some entries with - // capitals ("macroEnabled"). canonicalMimeType has already been applied upstream + // not lowercased: the core's table is matched exactly, capitals included ("macroEnabled") mimeType?.let { Odr.fileTypeByMimetype(it) } /** * Whether the core renders this type dark itself, which is what darkening defaults to. * - * Where it does, the dark is the one the format was translated into - the colours the file - * authored giving way, a photograph keeping its own and taking a dark ground instead. Where it - * does not, a pdf and the media views, all that is left is the webview inverting what it was + * Where it does not - a pdf, the media views - all the webview can do is invert what it was * handed, so that is offered but not taken for granted. */ private fun darkensByDefault(fileType: FileType?): Boolean = - // nothing named it, so it is being shown as text or as the html fallback, and both have a - // dark of their own - the same answer [Kind.DOCUMENT] is the kind for + // nothing named it, so it is shown as text or as the html fallback, and both have a dark fileType == null || Odr.capabilitiesByFileType(fileType).colorScheme /** Whether what [mimeType] names darkens, the button's answer first and the core's after. */ diff --git a/app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt b/app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt index 1bd8eac0d3be..354c577a0225 100644 --- a/app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt +++ b/app/src/main/java/app/opendocument/droid/background/NightModeSetting.kt @@ -6,15 +6,12 @@ import android.content.res.Resources import androidx.appcompat.app.AppCompatDelegate /** - * Whether the app is in night mode when the system says otherwise. + * Whether the app is in night mode when the system says otherwise, which is also the switch for + * reading at night on a phone that stays light all day: a webview darkens a page only while the app + * theme reports itself dark. * - * The document follows the app rather than the system - a webview darkens a page algorithmically - * and only while the app theme reports itself dark - so this is also the switch for reading at - * night on a phone that stays light all day, and for keeping light a document that inverts badly. - * - * What it answers is handed to [AppCompatDelegate.setLocalNightMode], not to the default mode: - * `MainActivity` is the only screen there is, and a local mode leaves whatever asks - * `AppCompatDelegate` itself saying what it said before. + * Handed to [AppCompatDelegate.setLocalNightMode] rather than the default mode - `MainActivity` is + * the only screen there is, and a local mode leaves the default where anything else set it. */ object NightModeSetting { @@ -23,9 +20,8 @@ object NightModeSetting { /** * The mode the activity's delegate is put in. * - * [AppCompatDelegate.MODE_NIGHT_UNSPECIFIED] is no override at all, which is not the same as - * MODE_NIGHT_FOLLOW_SYSTEM: that one is an override too, and would talk over a default mode set - * anywhere else. + * [AppCompatDelegate.MODE_NIGHT_UNSPECIFIED] is no override at all, unlike + * MODE_NIGHT_FOLLOW_SYSTEM, which is one and would talk over a default mode set elsewhere. */ fun mode(context: Context): Int = AppPreferences.of(context).getInt(PREF_NIGHT_MODE, AppCompatDelegate.MODE_NIGHT_UNSPECIFIED) @@ -33,9 +29,8 @@ object NightModeSetting { /** * Remembers whether the app should be dark, and answers the mode that puts it there. * - * Stored as no override whenever the answer wanted is the one that would be given anyway: an - * override agreeing with the system is one the user can never be rid of again, and the app - * would sit in night mode through a morning the phone had long left it for. + * Stored as no override whenever it agrees with the system: one that does can never be got rid + * of again, and the app would sit in night mode through a morning the phone had long left. */ fun setNight(context: Context, night: Boolean): Int { val mode = @@ -54,9 +49,9 @@ object NightModeSetting { fun isNight(context: Context): Boolean = isNight(context.resources) /** - * What the app would show with the override taken away, which an activity carrying one can no - * longer say. `Resources.getSystem()` is the device configuration and nothing else, so a - * default mode set on top of it - the instrumented tests set one - is asked for separately. + * What the app would show without the override, which an activity carrying one cannot say. + * `Resources.getSystem()` is the device configuration alone, so a default mode set on top of it + * - the instrumented tests set one - is asked for separately. */ private fun isNightWithoutOverride(): Boolean = when (AppCompatDelegate.getDefaultNightMode()) { diff --git a/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt b/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt index b0d48a38678c..422b00d4c792 100644 --- a/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt +++ b/app/src/main/java/app/opendocument/droid/background/PaginationSetting.kt @@ -15,9 +15,8 @@ import app.opendocument.core.Odr * width is what reads on a phone. * * Only [CoreLoader] reads it, and only while translating, so a change reaches a document by - * rendering it again. The landing screen's switch gets that for free - it is a document closed - * away, and opening one translates anyway - and the button over the open document asks - * `DocumentFragment.reloadForMargins` for it. + * rendering it again: opening one from the landing screen does that anyway, and the button over an + * open document asks `DocumentFragment.reloadForMargins` for it. */ object PaginationSetting { @@ -32,18 +31,16 @@ object PaginationSetting { /** * Whether this reaches what [mimeType] names at all. * - * odrcore lays a *text* document out with the margins or without them and nothing else: a - * presentation and a drawing are paged whatever it says, and a spreadsheet, a pdf, an image or - * a plain text file are never paged. Offering the button on one of those would render the - * document again to show nothing new, and quietly answer for the next text document opened. + * odrcore lays a *text* document out with the margins or without them and nothing else, so + * anywhere else the button would render the document again to show nothing new - and quietly + * answer for the next text document opened. */ fun affects(mimeType: String?): Boolean { - // not lowercased, and for the same reason as DocumentDarkening.kindOf + // not lowercased, and for the same reason as DocumentDarkening.fileTypeOf val fileType = mimeType?.let { Odr.fileTypeByMimetype(it) } ?: return false - // four types call themselves text: odt, docx, doc - and pdf, which is fixed pages the core - // lays out with a frontend of its own that the margin never reaches. So it is asked about - // the type rather than trusted here + // pdf calls itself text too, but is fixed pages laid out by a frontend of its own that the + // margin never reaches if (fileType == FileType.PORTABLE_DOCUMENT_FORMAT) { return false } diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt index bc3e2634c160..94669c36ea4f 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt @@ -80,8 +80,8 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { private var freshOpenPending = false /** - * Where the reader was before a reload that is not theirs, held from [reloadForMargins] until - * the document comes back. Null at every other load: opening a document belongs at its top. + * Where the reader was, held from [reloadForMargins] until the document comes back. Null at + * every other load: opening a document belongs at its top. */ private var positionToRestore: ReadingPosition? = null @@ -364,17 +364,14 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { reload(lastRequest, requireLastFile()) } - /** - * Tells the page whether it may follow the app into night mode - the one place that is decided, - * from what [DocumentDarkening] says about this kind of document. - */ + /** Tells the page whether it may follow the app into night mode - see [DocumentDarkening]. */ private fun applyDarkening(file: IdentifiedFile) { pageView?.setDarkeningAllowed(DocumentDarkening.isAllowed(requireContext(), file.mimeType)) } /** * Flips that answer for every document of this kind, and shows it straight away: darkening is a - * webview setting, not something the page was translated with, so nothing is rendered again. + * webview setting, so nothing is rendered again. */ fun toggleDarkening() { val document = state.lastDocument ?: return @@ -394,15 +391,15 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { /** * The document again with the margins [PaginationSetting] now says, which is decided while - * translating - so there is no showing the change without rendering it a second time. + * translating - so it has to be rendered a second time to be seen. */ fun reloadForMargins() { if (!isAdded) { return } - // the page is thrown away and translated again, so where the reader had got to is taken - // along by hand - it is the same document, and they did not ask to be put back at the top + // the page is thrown away and translated again, so where the reader had got to is carried + // by hand - it is the same document, and they did not ask to be put back at the top positionToRestore = ReadingPosition( maxOf(state.lastSelectedTab, 0), @@ -514,7 +511,7 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { ) // only while the app is dark: below that the webview darkens nothing whatever it is - // allowed, so the row would be a switch with nothing on the other end of it + // allowed, so the row would be a switch with nothing on the other end val darkening = if (!NightModeSetting.isNight(requireContext())) null else { @@ -550,8 +547,7 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { ) // the order they unfold in, most wanted first - and what a reader reaches for mid-document - // is how it is displayed, not what else can be done to it. the margins were only on the - // landing screen, which is a document closed away from anyone who wants them + // is how it is displayed, not what else can be done to it val unfolding = listOfNotNull( night, @@ -679,9 +675,8 @@ class DocumentFragment : Fragment(), DocumentLoader.Listener { val restored = positionToRestore positionToRestore = null - // before the load below, which is the one it is waiting for. always, and not only when - // there is something to put back: a reload that failed on the way here would otherwise - // leave its fraction waiting for whatever document is opened next + // always, and not only when there is something to put back: a reload that failed on the + // way here would otherwise leave its fraction waiting for the next document opened pageView?.restoreScrollFraction(restored?.scrollFraction ?: 0f) val titles = document.partTitles diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt b/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt index da5aa022d2c6..35b5e5325ed5 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt @@ -157,9 +157,8 @@ class MainActivity : AppCompatActivity() { } override fun onCreate(savedInstanceState: Bundle?) { - // before super, and remembered rather than left to the delegate: appcompat applies a mode - // the moment it is told, so setting it afterwards recreates the activity that has just - // been created, and it only keeps a local mode until the process goes + // before super: appcompat applies a mode the moment it is told, so setting it afterwards + // recreates the activity that has just been created delegate.localNightMode = NightModeSetting.mode(this) super.onCreate(savedInstanceState) @@ -548,7 +547,7 @@ class MainActivity : AppCompatActivity() { ) // recreates the activity, the way a rotation does - and survives it the same way: - // the loader is a ViewModel and the fragment saves the document and its page + // the loader is a ViewModel, and the fragment saves the document delegate.localNightMode = NightModeSetting.setNight(this, night) } diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt index 0000a585c450..0fd31748ab14 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt @@ -162,9 +162,8 @@ constructor(context: Context, attributeSet: AttributeSet?) : /** * Where the page sits, as a fraction of what there is to scroll. * - * A fraction and not the offset itself: the one thing that reloads a document in place is a - * change to how it is laid out, and that changes the height the offset would be measured - * against. + * A fraction and not the offset: the one thing that reloads a document in place is a change to + * how it is laid out, which changes the height an offset would mean anything against. */ val verticalScrollFraction: Float get() { @@ -188,8 +187,7 @@ constructor(context: Context, attributeSet: AttributeSet?) : /** * Puts the next page loaded back to [fraction] of its height. * - * Not applied here: the page is still being laid out when the load reports itself finished, and - * until it has a height there is nothing to put anything back to. + * Not applied here: the page is still being laid out when the load reports itself finished. */ fun restoreScrollFraction(fraction: Float) { scrollFractionToRestore = fraction.takeIf { it > 0f } @@ -198,8 +196,8 @@ constructor(context: Context, attributeSet: AttributeSet?) : /** * Waits for a height that has stopped growing and scrolls to it - a long document goes on being - * laid out for a while, and measuring against the first height it reports lands near the top of - * where the reader was. Gives up after [SCROLL_RESTORE_ATTEMPTS], leaving the page where it is. + * laid out, and the first height it reports lands near the top of where the reader was. Gives + * up after [SCROLL_RESTORE_ATTEMPTS], leaving the page where it is. */ private fun restorePendingScroll(attempt: Int) { val fraction = scrollFractionToRestore ?: return @@ -243,8 +241,7 @@ constructor(context: Context, attributeSet: AttributeSet?) : * app is in it: the webview darkens a page algorithmically, and at targetSdk 33 and up only * once the app theme reports itself as dark. * - * [DocumentFragment] decides which documents get it, out of what `DocumentDarkening` says about - * the kind of document this is. + * [DocumentFragment] decides which documents get it, from `DocumentDarkening`. */ fun setDarkeningAllowed(allowed: Boolean) { isDarkeningAllowed = allowed @@ -282,11 +279,9 @@ constructor(context: Context, attributeSet: AttributeSet?) : if (WebViewFeature.isFeatureSupported(WebViewFeature.ALGORITHMIC_DARKENING)) { WebSettingsCompat.setAlgorithmicDarkeningAllowed(settings, darken) } else if (WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK)) { - // Invert, and do not stand aside for the page's own dark theme, which is what this api - // does by default. Every page carries one now - CoreLoader translates with - // HtmlColorScheme.SYSTEM - but a webview old enough to be on this branch answers - // prefers-color-scheme by the system alone and never matches it for a forced dark, so - // deferring to that theme is deferring to nothing and the page stays light + // invert rather than stand aside for the page's own dark theme, which is the default. + // Every page carries one now, but a webview old enough for this branch answers + // prefers-color-scheme by the system alone, so standing aside leaves the page light if (WebViewFeature.isFeatureSupported(WebViewFeature.FORCE_DARK_STRATEGY)) { WebSettingsCompat.setForceDarkStrategy( settings, From 4036b6b087057c038edf14fd3b14235b3d692cda Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 16:47:28 +0200 Subject: [PATCH 09/10] Trim the test comments and the note in CLAUDE.md to what they have to say Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011ou83nozsNLKjSPw7h29zH --- CLAUDE.md | 22 ++++----- .../opendocument/droid/test/DarkModeTests.kt | 46 ++++++++----------- .../opendocument/droid/test/LargeTextTests.kt | 4 +- .../droid/test/MainActivityTests.kt | 23 ++++------ 4 files changed, 40 insertions(+), 55 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 53bc5964e7a4..4c67254be4b3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -187,21 +187,17 @@ be done to it, and each remembers what it was last told: default one, so a phone that stays light all day can still be read at night. `NightModeSetting` stores no override at all once the choice agrees with the system again, or the app would sit in night mode through a morning the phone had long left. -- **Darkening** defaults to what the core says and is overridden per kind of document, not per - file. `CoreLoader` translates every page with `HtmlColorScheme.SYSTEM`, so a format that has a - dark of its own carries both behind `prefers-color-scheme`, and - `capabilitiesByFileType(...).colorScheme` is whether it has one - which is what - `DocumentDarkening` defaults to rather than a list. A pdf and the media views have none, so - there all the webview can do is invert, which is offered but off. Do not put the guesses back: - it was a guess that presentations and images invert badly, and the core answers both. - `PageView.setDarkeningAllowed` picks between the two schemes at display time, which is why the - button changes nothing about the translation and renders nothing again. +- **Darkening** defaults to `capabilitiesByFileType(...).colorScheme` - whether the format has a + dark of its own - and is overridden per kind of document, not per file. `CoreLoader` translates + every page with `HtmlColorScheme.SYSTEM` so both schemes ride behind `prefers-color-scheme`, and + `PageView.setDarkeningAllowed` picks between them at display time, which is why the button + renders nothing again. Do not put a list of formats back: it was a guess that presentations and + images invert badly, and the core answers both. - **The margins** are odrcore's `textDocumentMargin`, decided while translating, so the button renders the document again through `DocumentLoader.reload` - the copy in the cache, not the file. - It reaches a *text* document only, which `PaginationSetting.affects` answers and the button is - gated on: everything else would be translated again to look exactly the same. A reload throws - the page away, so `DocumentFragment` carries the tab and how far down it the reader was over to - the document that comes back - as a fraction, the margins having changed the height. + `PaginationSetting.affects` gates it on a *text* document: everything else would be translated + again to look the same. `DocumentFragment` carries the tab and how far down it the reader was + over to the document that comes back - as a fraction, the margins having changed the height. Do not move these into a settings screen. `PaginationSetting` keeps its landing row because it already had one and both write the same preference; the other two never get one. diff --git a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt index 25cc41b85c6c..cbbf557d7621 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/DarkModeTests.kt @@ -38,8 +38,8 @@ import org.junit.Test import org.junit.runner.RunWith /** - * The document follows the app into night mode, if it is a kind of document that reads better for - * it, and the switches over it are what say otherwise. + * The document follows the app into night mode where it reads better for it, and the switches over + * it are what say otherwise. * * A webview darkens a page algorithmically and only while the app theme reports itself dark, so * every test here puts the app in night mode first - in day mode nothing below would fail. @@ -61,16 +61,15 @@ class DarkModeTests { fun leaveNightMode() { setNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM) - // both switches keep their answer on disk, where the next test would find it. asking for - // what the system says is what stores no override at all - see NightModeSetting.setNight + // both switches keep their answer on disk, where the next test would find it NightModeSetting.setNight(targetContext(), systemIsNight()) for (kind in DocumentDarkening.Kind.entries) { DocumentDarkening.clear(targetContext(), kind) } - // switching night mode recreates the activity, leaving one behind that the rule does not - // know about - and it would still be up when the next test launches its own + // switching night mode recreates the activity, leaving one behind the rule knows nothing + // of - and it would still be up when the next test launches its own val resumed = resumedMainActivity() if (resumed != null && resumed !== mainActivityActivityTestRule.activity) { onMainThread { resumed.finish() } @@ -83,8 +82,7 @@ class DarkModeTests { } /** - * A pdf does not, which is the one kind the app is sure about: a scanned page inverts into - * something nobody wrote. See [DocumentDarkening]. + * A pdf does not: a scanned page inverts into something nobody wrote. See [DocumentDarkening]. */ @Test fun aPdfIsNotDarkened() { @@ -136,8 +134,8 @@ class DarkModeTests { } /** - * The switch over the document, which is the answer for a phone that stays in day mode all - * night - the only test here that starts in day mode, since that is what it switches out of. + * The switch over the document, for a phone that stays in day mode all night - the only test + * here that starts in day mode, since that is what it switches out of. */ @Test fun theSwitchDarkensADayModeApp() { @@ -151,8 +149,7 @@ class DarkModeTests { systemIsNight(), ) - // undoing what every other test here starts from: the switch is the only thing that - // should be putting this app in night mode + // the switch is the only thing that should be putting this app in night mode setNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM) openPageView("test.odt") @@ -163,8 +160,8 @@ class DarkModeTests { ) } - // 60s and the last reading kept, for the reason theDrawnPageIsDark gives: this waits on - // the same screenshot, and an activity recreation happens before it + // 60s and the last reading kept, for the reason theDrawnPageIsDark gives - and an + // activity recreation happens before it var luminance = WHITE val darkened = waitFor(60000) { meanLuminance().also { luminance = it } < DARK_LUMINANCE } @@ -233,9 +230,8 @@ class DarkModeTests { } /** - * What the webview is and what it was given, for a failure message: these tests fail on one api - * level at a time, on emulators whose webview is far older than any developer machine's, and - * "the page stayed light" alone does not say which of the two darkening apis was even in play. + * What the webview is and what it was given, for a failure message: these fail on one api level + * at a time, and "the page stayed light" does not say which darkening api was even in play. */ private fun darkeningDiagnosis(): String { val algorithmic = WebViewFeature.isFeatureSupported(WebViewFeature.ALGORITHMIC_DARKENING) @@ -258,14 +254,10 @@ class DarkModeTests { /** * Whether this webview can darken a page at all, which is not the same as its saying it can. * - * The api 29 image ships webview 74, which reports `FORCE_DARK` supported, takes the setting - * and hands it straight back - and draws the page exactly as light as it was. Force dark is - * only implemented from 76. What the app does there is still asserted, through - * [darkeningSetting]; it is the screen that cannot be asked, so the two tests that read pixels - * skip instead of failing for a webview that was never going to darken. - * - * An unreadable version counts as capable: a missed skip is a failure to look at, a skip taken - * by mistake is coverage quietly lost. + * The api 29 image ships webview 74, which reports `FORCE_DARK` supported, hands the setting + * straight back and draws the page as light as it was - force dark only arrived in 76. What the + * app does is still asserted through [darkeningSetting]; only the two tests that read pixels + * skip. An unreadable version counts as capable: a skip taken by mistake is coverage lost. */ private fun canDarken() = WebViewFeature.isFeatureSupported(WebViewFeature.ALGORITHMIC_DARKENING) || @@ -340,8 +332,8 @@ class DarkModeTests { val uri = uriOf(extract(name)) onMainThread { activity.loadUri(uri) } - // not the uri, which is the one it already had: what says this load landed is a - // document that is not the one from the load before it + // not the uri, which is the one it already had: what says this load landed is a document + // that is not the one from the load before Assert.assertTrue( "$name never loaded again", waitFor(30000) { fragment.lastDocument != null && fragment.lastDocument !== before }, diff --git a/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt index 1986f920f20a..fdcda7149c9d 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/LargeTextTests.kt @@ -54,8 +54,8 @@ class LargeTextTests { Assert.assertTrue("the page never laid out", elements > 1) // and then the text itself. A megabyte is parsed in bursts, so the element count can stop - // changing inside a pause rather than at the end - two equal readings both landing in one - // is a page still filling, which the search then ran against and found nothing in + // changing inside a pause rather than at the end, and the search would run against a page + // still filling var needles = -1 Assert.assertTrue( "the text never finished arriving - $NEEDLE is in the page $needles times, not $lines", diff --git a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt index 4a529d4b259c..69c4e8abeaf2 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt @@ -289,9 +289,8 @@ class MainActivityTests { } /** - * The margin switch renders the open document a second time. The margin is odrcore's, decided - * while translating, so flipping the setting alone would leave the page as it was until the - * document was closed and opened again. + * The margin switch renders the open document a second time: the margin is odrcore's, decided + * while translating, so flipping the setting alone would leave the page as it was. */ @Test fun theMarginSwitchRendersTheDocumentAgain() { @@ -324,8 +323,8 @@ class MainActivityTests { /** * The button is only there where tapping it changes something: odrcore lays a text document out - * with the margins or without them, and every other view the same either way. Launches nothing - * - it is the rule the button is gated on, and the core answers it. + * with the margins or without them, and every other view the same either way. Launches + * nothing - it is the rule the button is gated on, and the core answers it. */ @Test fun theMarginSwitchIsOnlyOfferedForTextDocuments() { @@ -352,13 +351,11 @@ class MainActivityTests { } /** - * The margins render the document again, and a reader who was halfway down it should still be - * halfway down it afterwards - the reload used to hand back the top of the page. + * The margins render the document again, and a reader halfway down it should still be halfway + * down it afterwards - the reload used to hand back the top of the page. * - * A fraction on both sides, because the margins are exactly what changes the height: the same - * place in the text is a different offset once the page has been laid out again. Polled rather - * than asserted straight after the reload, the way the edit mode tests poll - the page has to - * be laid out again before there is anywhere to put anyone back to. + * A fraction on both sides, the margins being exactly what changes the height. Polled rather + * than asserted straight after the reload, the way the edit mode tests poll. */ @Test fun theMarginSwitchKeepsTheReadingPosition() { @@ -371,8 +368,8 @@ class MainActivityTests { waitFor(EDIT_MODE_TIMEOUT_MS) { scrollableHeight(pageView) > 0 }, ) - // a long way down, but not the very end: the last screenful is one position however far - // the page is scrolled past it, so it would pass without anything being restored at all + // a long way down, but not the end: the last screenful is one position however far the page + // is scrolled past it, so it would pass with nothing restored at all val before = scrollToFraction(pageView, READING_POSITION) Assert.assertTrue( "the page did not scroll - it read back at $before", From c7d82e60cf6c72c46ab83a662fd8d0481d49438b Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Tue, 18 Aug 2026 16:55:10 +0200 Subject: [PATCH 10/10] Do not let a page that was navigated away from fail the one that replaced it (#602) * Do not let a page that was navigated away from fail the one that replaced it `testDOCXEditMode` and `testODTEditMode` have been failing an api level at a time all week, always with "webview did not answer in 10000ms document=no result" - and `no result` is the tell: `lastDocument` is only null once something called `unload`, so the document had not merely failed to become editable, it had been given up on. Logcat from a failing run says what did it. A page that never commits leaves a reload scheduled 2.5s out - the workaround for a webview reporting progress 100 over a blank page - and nothing cancelled it when the next page was asked for. The test before had deliberately loaded a page that 404s, so its retry fired inside the next test, went back to a server that had gone with the document that owned it, and got a 404. `onReceivedHttpError` reported that against whatever document was on screen by then, which was the docx being opened for edit mode. So `loadUrl` cancels a retry the page before it left waiting, the way `destroy` already does when the whole view is replaced. And `failPage` answers only for the page it was last asked to load: a request made for a document already closed can still be answered long after, and the document on screen is not the one that failed. Nothing is loosened. `LandingTests.aDocumentThatFailsToOpenComesBackToTheList` holds the case that matters - a document whose page really is a 404 - and the whole instrumented suite, 80 tests, passes locally. Co-Authored-By: Claude Opus 5 (1M context) * Check the retry against the page still wanted, not only the flag Cancelling on `loadUrl` clears the retries queued until then, and not the one queued after: page A can finish - and schedule its retry - once B has already been asked for. `wasCommitCalled` is about whichever page is being waited on, so if B has not committed within the 2.5s, A's retry reads B's flag, believes it is about itself, and loads A back over B. Which is the navigation this set out to stop, arrived at from the other side. So the retry asks whether the page it names is still the page wanted. Co-Authored-By: Claude Opus 5 (1M context) --- .../opendocument/droid/ui/widget/PageView.kt | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt index 0fd31748ab14..bc2a1ec80cf7 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt @@ -53,6 +53,9 @@ constructor(context: Context, attributeSet: AttributeSet?) : private var wasCommitCalled = false + /** What [loadUrl] was last given: the only page whose failure is this document's. */ + private var loadedUrl: String? = null + private var isBridgeAttached = false init { @@ -79,7 +82,12 @@ constructor(context: Context, attributeSet: AttributeSet?) : buggyWebViewHandler.postDelayed( { - if (!wasCommitCalled) { + // [url] and not whatever is loaded now: this callback can arrive after + // another page was asked for, which cancels the retries queued until + // then but not the one queued here. wasCommitCalled is about the page + // being waited on, so on its own it would answer for that other page + // and put this one back over it + if (!wasCommitCalled && url == loadedUrl) { crashManager.log(RuntimeException("commit was not called")) loadUrl(url) @@ -311,6 +319,14 @@ constructor(context: Context, attributeSet: AttributeSet?) : // the third party viewers an ONLINE result loads here. takes effect on the next load if (!url.startsWith(JAVASCRIPT_SCHEME)) { attachBridge(isOwnContent(url)) + + // a page that never committed left a retry waiting in onPageFinished. Now that another + // page has been asked for, that retry would load the old one back over it - and the + // document it belonged to has taken its server with it, so what it would find there is + // a 404 this page is then given up on for + buggyWebViewHandler.removeCallbacksAndMessages(null) + + loadedUrl = url } super.loadUrl(url) @@ -334,6 +350,13 @@ constructor(context: Context, attributeSet: AttributeSet?) : return } + // and only the page being shown. A request made for a document already closed can still be + // answered here, long after the page moved on, and the document on screen is not the one + // that failed + if (loadedUrl != null && url.toString() != loadedUrl) { + return + } + documentFragment.onPageFailed() }