From 828b5fe0dd92d580b3838fb2aa912634cd534e07 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 10 Aug 2026 23:44:15 +0200 Subject: [PATCH] Dissolve the loader chain With one loader left there is no chain to drive, so the machinery built for four goes and the work it was carrying moves to components named after it. DocumentLoader replaces LoaderService, the FileLoader base and LoaderServiceQueue. It is a ViewModel scoped to MainActivity: it survives a configuration change and is there before anything asks it for a document, so the queue that held work until the binder arrived has nothing left to hold. Under it, FileCache stores the working copy, FileIdentifier names and types it, CoreLoader renders it and DocumentSaver writes it back - the last two lifted out of MetadataLoader and LoaderService unchanged. The mutable Options bag was the baton one loader passed to the next, so it splits: DocumentRequest is what the user asked for, IdentifiedFile the cached copy it turned out to be, LoadedDocument the two plus the parts to show. fileExists goes with it - holding an IdentifiedFile is the answer - along with LoaderType and the already dead Options.limit. A bundle written by the shipped version can name FileLoader$Result, and Bundle reads its whole map at the first access, so the restore in DocumentFragment is guarded rather than each retired name being mapped. Losing the reopened document beats throwing on launch, and DocumentParcelTest covers the round trip that nothing else did: the recreation test restores from the view model, which never parcels. An empty file now reports back with no IdentifiedFile, so "Open With" offers the uri the user picked rather than a zero byte copy of it. Nothing else changes for the user: a render sweep over 60 documents matches the previous build on every one, signal, text and all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018y4srZKc2SQa7JRYcDbTcc --- CLAUDE.md | 52 +-- .../droid/background/DocumentParcelTest.kt | 156 +++++++++ ...oreLoaderTest.kt => RenderedByCoreTest.kt} | 19 +- .../app/opendocument/droid/test/CoreTest.kt | 21 +- .../opendocument/droid/test/LandingTests.kt | 4 +- .../droid/test/MainActivityTests.kt | 12 +- .../droid/test/SupportedFormatsTest.kt | 19 +- app/src/main/AndroidManifest.xml | 5 - .../droid/background/CoreLoader.kt | 91 ++--- .../droid/background/DocumentLoader.kt | 244 +++++++++++++ .../droid/background/DocumentRequest.kt | 51 +++ .../droid/background/DocumentSaver.kt | 142 ++++++++ .../{AndroidFileCache.kt => FileCache.kt} | 31 +- .../droid/background/FileIdentifier.kt | 151 ++++++++ .../droid/background/FileLoader.kt | 255 -------------- .../droid/background/IdentifiedFile.kt | 50 +++ .../droid/background/LoadedDocument.kt | 64 ++++ .../droid/background/LoaderService.kt | 328 ------------------ .../droid/background/LoaderServiceQueue.kt | 53 --- .../droid/background/MetadataLoader.kt | 193 ----------- .../droid/background/MimeTypeResolver.kt | 2 +- .../background/PersistedUriPermissions.kt | 2 +- .../droid/background/RecentDocumentsUtil.kt | 6 +- .../background/SupportedDocumentTypes.kt | 2 +- .../droid/ui/activity/DocumentFragment.kt | 297 +++++++++------- .../droid/ui/activity/MainActivity.kt | 49 +-- .../opendocument/droid/ui/widget/PageView.kt | 6 +- .../droid/background/LoaderTypeTest.kt | 28 -- tools/render-sweep/README.md | 2 +- tools/render-sweep/render-sweep.sh | 2 +- 30 files changed, 1165 insertions(+), 1172 deletions(-) create mode 100644 app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt rename app/src/androidTest/java/app/opendocument/droid/background/{CoreLoaderTest.kt => RenderedByCoreTest.kt} (91%) create mode 100644 app/src/main/java/app/opendocument/droid/background/DocumentLoader.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/DocumentRequest.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/DocumentSaver.kt rename app/src/main/java/app/opendocument/droid/background/{AndroidFileCache.kt => FileCache.kt} (74%) create mode 100644 app/src/main/java/app/opendocument/droid/background/FileIdentifier.kt delete mode 100644 app/src/main/java/app/opendocument/droid/background/FileLoader.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/IdentifiedFile.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/LoadedDocument.kt delete mode 100644 app/src/main/java/app/opendocument/droid/background/LoaderService.kt delete mode 100644 app/src/main/java/app/opendocument/droid/background/LoaderServiceQueue.kt delete mode 100644 app/src/main/java/app/opendocument/droid/background/MetadataLoader.kt delete mode 100644 app/src/test/java/app/opendocument/droid/background/LoaderTypeTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index 7739a6f09925..dea03d87a36c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,15 +14,21 @@ Guidance for Claude Code (claude.ai/code) working in this repository. ## Architecture -A `FileLoader` loads on `LoaderService`'s background thread and reports through -`FileLoaderListener`; `LoaderServiceQueue` holds requests until the service is bound. -`MetadataLoader` caches and identifies the file and `CoreLoader` renders it, publishing the -html on a local server. Those two are the whole chain - there is no fallback after the core, -and what it cannot open is reported as an unsupported format. - -`MainActivity` owns the service binding and the action modes (find, tts, edit), and swaps -between `LandingFragment` (recent documents and settings) and `DocumentFragment`, which -shows the result in `PageView` - a WebView - with `DocumentActions` over it. +`DocumentLoader` opens a document on its own background thread and reports back on the main +one, straight through: `FileCache` stores the bytes, `FileIdentifier` names and types the +copy, `CoreLoader` renders it and publishes the html on a local server, and `DocumentSaver` +writes it back. There is nothing after the core - what it cannot open is reported as an +unsupported format. + +It is a `ViewModel` scoped to `MainActivity`, so it survives a configuration change and is +there before anything asks it for a document. A `DocumentRequest` is what the user asked for, +an `IdentifiedFile` the cached copy it turned out to be, and a `LoadedDocument` the two plus +the parts to show. Do not add a loader base class or a loader-type enum: there is one loader, +and a second one is a format odrcore should learn instead. + +`MainActivity` owns the loader and the action modes (find, tts, edit), and swaps between +`LandingFragment` (recent documents and settings) and `DocumentFragment`, which shows the +result in `PageView` - a WebView - with `DocumentActions` over it. There is **no options menu**: `menu_main.xml` is gone and the action bar is hidden. An action on the open document is a `DocumentActions` button; anything else - the ad removal, @@ -64,7 +70,7 @@ code says otherwise. Do not add a `BuildConfig.FLAVOR` comparison back - it was resource bool `DISABLE_TRACKING` was both mistakes at once: there is no tracking to disable, `AnalyticsManager` and `CrashManager` write to logcat and nowhere else. -Those two take no switch at all, which is why `LoaderService` just constructs them. Ads +Those two take no switch at all, which is why `DocumentLoader` just constructs them. Ads and billing are what `MainActivity.initializeManagers` gates, on `Features.withAds` *and* `PlayServices` - the device half of the answer, and the reason that method can run twice, once more after google's own dialog comes back. @@ -142,13 +148,13 @@ fail to open it. XML cannot read any of that, so the `STRICT_CATCH` alias' three intent-filters are *generated* from the same table - a filter matches a mime type exactly, so all 49 spellings and 41 extensions are written out. `SupportedFormatsTest` asserts that -`SupportedDocumentTypes` and the package manager agree, and that every claimed mime type -reaches `CoreLoader`, so a format added upstream and forgotten fails CI. +`SupportedDocumentTypes` and the package manager agree, and that every claimed mime type is +one `isRenderedByCore` takes, so a format added upstream and forgotten fails CI. -The tables live in `libodr_jni`, which is why `CoreLoaderTest` and +The tables live in `libodr_jni`, which is why `RenderedByCoreTest` and `SupportedDocumentTypesTest` are instrumented though neither opens a file. After caching it -is `Odr.mimetype` that decides, canonicalized through `canonicalMimeType` so the loaders see -one spelling per format. +is `Odr.mimetype` that decides, canonicalized through `canonicalMimeType` so one spelling per +format reaches the core. Reading the core's table directly, as `isDocument` does, must not `lowercase()` first: it matches exactly and spells some types with capitals (`macroEnabled`). Our own sets are the @@ -166,8 +172,8 @@ Text is the core's fallback for bytes nothing else claims, and it does not refus it cannot name a charset for - it answers `text/plain` and throws only once a page is rendered, on the server thread, long after `CoreLoader` reported success. -So `MetadataLoader` drops a `text/plain` whose file has no charset (`hasKnownCharset`) and -lets the fallbacks below it decide, and `CoreLoader.host()` refuses the same file up front. +So `FileIdentifier` drops a `text/plain` whose file has no charset (`hasKnownCharset`) and +lets the guesses below it decide, and `CoreLoader.host()` refuses the same file up front. Both are needed: the first keeps `isRenderedByCore` off a `.bin`, the second stops a success bar appearing over a page that cannot draw. `LandingTests.aDocumentThatFailsToOpenComesBackToTheList` holds this. @@ -175,7 +181,7 @@ bar appearing over a page that cannot draw. ### Editability comes from the core, never from a mime type `Document.isEditable()`/`isSavable()` decides whether `DocumentFragment` offers the Edit -button, carried on `FileLoader.Result.isEditable`. `CoreLoader.host()` only holds a document +button, carried on `LoadedDocument.isEditable`. `CoreLoader.host()` only holds a document open when the core says yes, so having one *is* the answer. Do not reintroduce a list of editable formats in the UI. @@ -201,8 +207,8 @@ The only java is `com/commonsware/android/print`, vendored so it can be diffed a upstream. It calls nothing of ours, so no java-to-kotlin call exists and `@JvmStatic`, `@JvmField`, `@JvmOverloads` and `@Throws` are not needed for interop. -What remains is for runtimes that reflect over the bytecode: `@JvmField` on `FileLoader`'s -`CREATOR`s (parcelable needs a static field) and `@JvmStatic` on `@BeforeClass` / -`@AfterClass` in the instrumented tests. `ProgressDialogFragment` needed `@JvmOverloads` too -while it took an argument - a fragment the framework re-creates has to have a no-arg -constructor. +What remains is for runtimes that reflect over the bytecode: `@JvmField` on the `CREATOR`s of +`DocumentRequest`, `IdentifiedFile` and `LoadedDocument` (parcelable needs a static field), +and `@JvmStatic` on `@BeforeClass` / `@AfterClass` in the instrumented tests. +`ProgressDialogFragment` needed `@JvmOverloads` too while it took an argument - a fragment +the framework re-creates has to have a no-arg constructor. diff --git a/app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt b/app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt new file mode 100644 index 000000000000..bca398ada55b --- /dev/null +++ b/app/src/androidTest/java/app/opendocument/droid/background/DocumentParcelTest.kt @@ -0,0 +1,156 @@ +package app.opendocument.droid.background + +import android.net.Uri +import android.os.Parcel +import android.os.Parcelable +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.filters.SmallTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith + +/** + * The saved instance state of an open document, written and read back. + * + * `DocumentFragment` parcels these into the bundle that survives process death, where a field + * written and read in a different order is silently the neighbouring one. Nothing else covers the + * round trip - the recreation test restores from the view model, which does not parcel. + * + * Instrumented because [Parcel] is the framework's. + */ +@SmallTest +@RunWith(AndroidJUnit4::class) +class DocumentParcelTest { + + @Test + fun aRequestComesBackAsItself() { + val request = + DocumentRequest(Uri.parse("content://provider/document/1"), persistentUri = true) + .apply { + editable = true + password = "passwort" + } + + val restored = roundTrip(request, DocumentRequest.CREATOR) + + assertEquals(request.uri, restored.uri) + assertTrue(restored.persistentUri) + assertTrue(restored.editable) + assertEquals("passwort", restored.password) + } + + @Test + fun aRequestWithoutAPasswordComesBackWithoutOne() { + val restored = + roundTrip( + DocumentRequest(Uri.parse("content://provider/document/2"), persistentUri = false), + DocumentRequest.CREATOR, + ) + + assertNull(restored.password) + assertEquals(false, restored.persistentUri) + assertEquals(false, restored.editable) + } + + @Test + fun anIdentifiedFileComesBackAsItself() { + val file = + IdentifiedFile( + Uri.parse("content://at.tomtasche.reader.pro.provider/cache.1/cached-file.tmp"), + "report.odt", + "application/vnd.oasis.opendocument.text", + "odt", + ) + + val restored = roundTrip(file, IdentifiedFile.CREATOR) + + assertEquals(file.cacheUri, restored.cacheUri) + assertEquals("report.odt", restored.filename) + assertEquals("application/vnd.oasis.opendocument.text", restored.mimeType) + assertEquals("odt", restored.extension) + } + + /** What nothing could name: the mime type and the extension are both allowed to be missing. */ + @Test + fun anUnnamedFileComesBackUnnamed() { + val restored = + roundTrip( + IdentifiedFile(Uri.parse("content://provider/cache.1/x.tmp"), "x", null, null), + IdentifiedFile.CREATOR, + ) + + assertNull(restored.mimeType) + assertNull(restored.extension) + assertEquals("x", restored.filename) + } + + /** A spreadsheet, which is the only shape with more than one part and named tabs. */ + @Test + fun aDocumentComesBackWithEveryPart() { + val document = + LoadedDocument( + DocumentRequest(Uri.parse("content://provider/document/3"), persistentUri = true), + IdentifiedFile( + Uri.parse("content://provider/cache.1/cached-file.tmp"), + "budget.ods", + "application/vnd.oasis.opendocument.spreadsheet", + "ods", + ), + listOf("hey", "ho", "Sheet3"), + listOf( + Uri.parse("http://localhost:29665/file/odr/0.html"), + Uri.parse("http://localhost:29665/file/odr/1.html"), + Uri.parse("http://localhost:29665/file/odr/2.html"), + ), + isEditable = true, + ) + + val restored = roundTrip(document, LoadedDocument.CREATOR) + + assertEquals(document.request.uri, restored.request.uri) + assertEquals("budget.ods", restored.file.filename) + assertEquals(listOf("hey", "ho", "Sheet3"), restored.partTitles) + assertEquals(document.partUris, restored.partUris) + assertTrue(restored.isEditable) + } + + /** Everything but a spreadsheet: one part, and the core does not name it. */ + @Test + fun aSinglePartDocumentKeepsItsNullTitle() { + val restored = + roundTrip( + LoadedDocument( + DocumentRequest(Uri.parse("content://provider/document/4"), false), + IdentifiedFile( + Uri.parse("content://provider/cache.1/cached-file.tmp"), + "letter.odt", + "application/vnd.oasis.opendocument.text", + "odt", + ), + listOf(null), + listOf(Uri.parse("http://localhost:29665/file/odr/document.html")), + isEditable = false, + ), + LoadedDocument.CREATOR, + ) + + assertEquals(1, restored.partTitles.size) + assertNull(restored.partTitles[0]) + assertEquals(false, restored.isEditable) + } + + private fun roundTrip(value: T, creator: Parcelable.Creator): T { + val parcel = Parcel.obtain() + + return try { + (value as Parcelable).writeToParcel(parcel, 0) + parcel.setDataPosition(0) + + creator.createFromParcel(parcel) + } finally { + parcel.recycle() + } + } +} diff --git a/app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt b/app/src/androidTest/java/app/opendocument/droid/background/RenderedByCoreTest.kt similarity index 91% rename from app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt rename to app/src/androidTest/java/app/opendocument/droid/background/RenderedByCoreTest.kt index c144305f2be5..6cbfde9857f3 100644 --- a/app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/background/RenderedByCoreTest.kt @@ -4,7 +4,6 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.SmallTest import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue -import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -17,22 +16,10 @@ import org.junit.runner.RunWith */ @SmallTest @RunWith(AndroidJUnit4::class) -class CoreLoaderTest { +class RenderedByCoreTest { - private lateinit var coreLoader: CoreLoader - - @Before - fun setUp() { - // no context: isSupported() is pure, and constructing a loader has no side effects - coreLoader = CoreLoader(null) - } - - private fun isSupported(fileType: String?): Boolean { - val options = FileLoader.Options() - options.fileType = fileType - - return coreLoader.isSupported(options) - } + private fun isSupported(fileType: String?): Boolean = + SupportedDocumentTypes.isRenderedByCore(fileType) @Test fun opendocumentIsSupported() { diff --git a/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt b/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt index eb6bbaf76176..4704663ffb55 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt @@ -1,14 +1,10 @@ package app.opendocument.droid.test -import android.os.Handler -import android.os.Looper import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.LargeTest import androidx.test.platform.app.InstrumentationRegistry import app.opendocument.core.OdrException import app.opendocument.droid.background.CoreLoader -import app.opendocument.droid.background.FileLoader -import app.opendocument.droid.nonfree.AnalyticsManager import app.opendocument.droid.nonfree.CrashManager import java.io.File import java.io.FileOutputStream @@ -241,22 +237,11 @@ class CoreTest { fun startServer() { val appCtx = InstrumentationRegistry.getInstrumentation().targetContext - // nothing here goes through loadAsync, so both handlers can be the main looper and - // the listener is never called back - val handler = Handler(Looper.getMainLooper()) + // every test here calls host() straight, so all initialize has to do is start the + // core and its server val loader = CoreLoader(appCtx) sharedLoader = loader - loader.initialize( - object : FileLoader.FileLoaderListener { - override fun onSuccess(result: FileLoader.Result) {} - - override fun onError(result: FileLoader.Result, error: Throwable) {} - }, - handler, - handler, - AnalyticsManager(), - CrashManager(), - ) + loader.initialize(CrashManager()) } @JvmStatic diff --git a/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt index 274b15bb66af..bc396cf7fd52 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt @@ -375,11 +375,11 @@ class LandingTests { } /** - * A recent document that is not a document at all: bytes no loader can make anything of, so the + * A recent document that is not a document at all: bytes the core can make nothing of, so the * load fails the way a truncated download or a renamed file does. * * The extension is part of the fixture. `Odr.mimetype` cannot identify these bytes, so - * `MetadataLoader` falls back to what the provider makes of the filename - and that type is + * `FileIdentifier` falls back to what the provider makes of the filename - and that type is * what decides which failure the user gets. `.bin` gives `application/octet-stream`, which the * core does not claim, so the file is reported as an unsupported format. Name it `.odt` and the * core claims the format and fails on the bytes, which is the broken-file dialog instead. 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 70673d468de2..f415ba854256 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/MainActivityTests.kt @@ -224,7 +224,7 @@ class MainActivityTests { fun testDocumentSurvivesRecreation() { val activity = mainActivityActivityTestRule.activity val documentFragment = loadDocument(activity, requireTestFile("test.odt")) - val before = documentFragment.lastResult + val before = documentFragment.lastDocument Assert.assertNotNull(before) // not rotation, which MainActivity handles itself: recreate() is what a locale or font @@ -242,8 +242,8 @@ class MainActivityTests { ) Assert.assertEquals( "document was reloaded instead of restored", - before!!.options.originalUri, - afterRecreation.lastResult?.options?.originalUri, + before!!.request.uri, + afterRecreation.lastDocument?.request?.uri, ) } @@ -405,8 +405,8 @@ class MainActivityTests { * lose. */ private fun describeLoadedDocument(fragment: DocumentFragment): String { - val result = fragment.lastResult ?: return "no result" - val url = result.partUris.firstOrNull() ?: return "no part uri" + val document = fragment.lastDocument ?: return "no result" + val url = document.partUris.firstOrNull() ?: return "no part uri" return try { val connection = URL(url.toString()).openConnection() as HttpURLConnection @@ -417,7 +417,7 @@ class MainActivityTests { val html = connection.inputStream.bufferedReader().use { it.readText() } "$url http=${connection.responseCode} length=${html.length}" + " contenteditable=${html.contains("contenteditable")}" + - " translatable=${result.options.translatable}" + " editable=${document.request.editable}" } finally { connection.disconnect() } diff --git a/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt b/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt index 1058857a2b9b..a72061c8d43c 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt @@ -8,8 +8,6 @@ import androidx.test.filters.SmallTest import androidx.test.platform.app.InstrumentationRegistry import app.opendocument.core.Odr import app.opendocument.droid.background.CatchAllSetting -import app.opendocument.droid.background.CoreLoader -import app.opendocument.droid.background.FileLoader import app.opendocument.droid.background.SupportedDocumentTypes import org.junit.Assert import org.junit.BeforeClass @@ -84,22 +82,19 @@ class SupportedFormatsTest { } /** - * Everything the app offers itself for reaches [CoreLoader]. Claiming a mime type nothing loads - * is the "cannot open" the user gets on a file they picked us for, so `CLAIMED_FILE_TYPES` and - * `CORE_FILE_TYPES` must not come apart. + * Everything the app offers itself for is something the core renders. Claiming a mime type + * nothing loads is the "cannot open" the user gets on a file they picked us for, so + * `CLAIMED_FILE_TYPES` and `CORE_FILE_TYPES` must not come apart. */ @Test fun everythingTheAppClaimsIsLoadedByTheCore() { - val coreLoader = CoreLoader(null) - for (mimeType in SupportedDocumentTypes.MIME_TYPES) { - val options = FileLoader.Options() - options.fileType = SupportedDocumentTypes.canonicalMimeType(mimeType) + val canonical = SupportedDocumentTypes.canonicalMimeType(mimeType) Assert.assertTrue( - "$mimeType is claimed by the app, but the core loader does not take it" + - " (as ${options.fileType})", - coreLoader.isSupported(options), + "$mimeType is claimed by the app, but the core does not render it" + + " (as $canonical)", + SupportedDocumentTypes.isRenderedByCore(canonical), ) } } diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 51479c8e1d76..f14728621ba5 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -50,11 +50,6 @@ android:networkSecurityConfig="@xml/network_security_config" tools:replace="android:label"> - -