Answer how a document is displayed over the document itself - #600
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0b80ef249
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VNSUAUN1QX2QyFNw4rAmV2
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
andiwand
force-pushed
the
feat/viewer-view-toggles
branch
from
August 18, 2026 09:53
b0b80ef to
88da465
Compare
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
andiwand
force-pushed
the
feat/viewer-view-toggles
branch
from
August 18, 2026 12:03
5ea496b to
fc21e49
Compare
…not 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
…e 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kd9KchUHN41DQ3DUreK8ML
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ou83nozsNLKjSPw7h29zH
… say Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ou83nozsNLKjSPw7h29zH
…aced 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com>
andiwand
added a commit
that referenced
this pull request
Aug 18, 2026
* Write 4.16.0's store copy The three display buttons from #600 - night mode, darkening and the margins - went in without a changelog entry, so the copy would have been written from a release that did not mention them. They are in `Unreleased` now, one line each for what a reader sees rather than for where the answer moved from. The English is written under the 400 character budget at 395, which leaves the fifteen translations room inside play's 500: German is the longest at 475 and French 460. Four paragraphs, the same shape as 4.15.0 - what a pdf now looks like, night mode over the document, text that opens at once, and .docx spacing. Everything else in the section is either invisible from the outside or too small to spend a paragraph on. `scripts/store-copy.py v4.16.0` translated it, one agent per language and a second reader of that language after; the English was left alone as the copy that was reviewed. `store-listing.py --version v4.16.0` confirms all fifteen. Co-Authored-By: Claude Opus 5 (1M context) * Cut the heading to 4.16.0 The release run reads the section under `## 4.16.0` twice - once before building, so missing copy costs seconds rather than a version, and again in the record job, where it becomes the body of the drafted github release. Unreleased is not a version, so the heading has to name one before v4.16.0 is dispatched. Nothing under it changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 leaving the document to find the switch. Both answers lived somewhere else - the system settings, and a fold on the landing 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 is
AppCompatDelegate.setLocalNightModeon the activity 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.NightModeSettingremembers it, 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 defaults to what the core says -
capabilitiesByFileType(...).colorScheme, whether the format carries a dark of its own - and the button overrides that for every document of the kind: it is never this pdf that inverts badly, it is pdfs. It applies live, darkening being a webview setting rather than something the page was translated with. That replaces the deliberate "every format" of #596 with the core's answer instead of a guess about which ones invert badly.The margins are odrcore's
textDocumentMargin, decided while translating, so the button renders the document again - the copy in the cache, with the reader put back where they were. The landing switch stays and writes the same preference, so the two can never disagree; being the only way in was what made it far away.Fullscreen moves up beside these two, as the third row about what the page looks like rather than what can be done to it.
DarkModeTestsgains the switch darkening a day-mode app - the screen is measured, not the flag - and the per-kind answer surviving a reopen.MainActivityTestsgains the margin row rendering the document again and holding the reading position. All 76 pass on an API 36 emulator. The new strings are English only until the next translation run.🤖 Generated with Claude Code