Dissolve the loader chain

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,16 +172,16 @@ 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.
 
 ### 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.

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<String?>(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 <T> roundTrip(value: T, creator: Parcelable.Creator<T>): T {
+        val parcel = Parcel.obtain()
+
+        return try {
+            (value as Parcelable).writeToParcel(parcel, 0)
+            parcel.setDataPosition(0)
+
+            creator.createFromParcel(parcel)
+        } finally {
+            parcel.recycle()
+        }
+    }
+}

app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt → 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() {

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

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.

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