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"> - -