diff --git a/CLAUDE.md b/CLAUDE.md index e1246f0b178d..eb457a8b167f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,7 +52,7 @@ what it replaced, and the second round of tapping is always the expensive one. **Document Processing Pipeline:** - `CoreLoader` - Primary document processor using the native C++ ODR core library -- `RawLoader` - Plain text and other raw file processor +- `RawLoader` - The three files odrcore does not render for us: csv, svg and xml - `OnlineLoader` - Remote document fetcher - `MetadataLoader` - Document metadata extractor @@ -159,9 +159,17 @@ the `.pro` suffix) differ on purpose - do not "fix" the mismatch: ### Supported file types come from odrcore, and a test keeps the manifest in step `SupportedDocumentTypes` used to be a hand written table of mime *prefixes*. Since odrcore -6.1 it is derived: `Odr.allFileTypes()` filtered by `Odr.capabilitiesByFileType(...) -.translateHtml` and `Odr.fileCategoryByFileType(...) == DOCUMENT` is what `CoreLoader` -renders, and `Odr.mimetypesByFileType` / `Odr.fileExtensionsByFileType` expand each of those +6.1 it is derived, and it answers two separate questions: + +- **what `CoreLoader` renders** (`CORE_FILE_TYPES`): `Odr.allFileTypes()` filtered by + `Odr.capabilitiesByFileType(...).translateHtml`, minus csv. Since 6.2 that covers text, + images, zip and cfb, fonts, audio and video - which is why `RawLoader` lost its viewers. +- **what the app claims** (`CLAIMED_FILE_TYPES`): the same filter narrowed to + `Odr.fileCategoryByFileType(...) == DOCUMENT`, plus text, csv and zip. The manifest mirrors + this. Keep it narrow - the app plays an mp3 handed to it but does not want it in the share + sheet. + +`Odr.mimetypesByFileType` / `Odr.fileExtensionsByFileType` expand each of those into every spelling the core accepts - the templates, the macro-enabled variants, the `application/x-vnd.oasis...` family and the `-flat-xml` ones. Do not put a list of mime prefixes back; the whole point is that the app cannot claim a format the core does not have, @@ -170,9 +178,8 @@ or miss one it does. A prefix match is also what made the app claim `.xlsb` and then fail to open it - the core gives it a file type of its own with an empty capability row now. -Two declarations are left. The app's own choice of which of the core's formats go to -`CoreLoader` and which to `RawLoader` - text, csv, zip and `image/` are named in -`SupportedDocumentTypes`, because that is an app decision the core knows nothing about. And +Two declarations are left. The app's own choice of what to *claim*, named in +`SupportedDocumentTypes` as `CLAIMED_FILE_TYPES` because the core knows nothing about it. And the `STRICT_CATCH` `activity-alias` in `AndroidManifest.xml`, which cannot be collapsed into the first because XML cannot read any of this. Its three intent-filters are *generated* from the same table (an intent-filter matches a mime type exactly, so all 40 spellings and 41 @@ -182,18 +189,39 @@ that `SupportedDocumentTypes` and the package manager give the same answer, so a upstream and forgotten in the manifest fails CI rather than shipping. The tables live in `libodr_jni`, so anything that reads them needs a device. That is why -`CoreLoaderTest`, `SupportedDocumentTypesTest` and `OnlineLoaderTest` are instrumented tests -and not JVM ones - none of them opens a file, they just cannot ask the table from a plain -JVM. What the core decides *after* the file is in the cache is unchanged: `MetadataLoader` -runs libmagic over the copy, and `CoreLoader.isDocumentEditable` asks the opened document. +`CoreLoaderTest`, `SupportedDocumentTypesTest`, `RawLoaderTest` and `OnlineLoaderTest` are +instrumented tests and not JVM ones - none of them opens a file, they just cannot ask the +table from a plain JVM. What the core decides *after* the file is in the cache is unchanged: +`MetadataLoader` runs libmagic over the copy, and `CoreLoader.isDocumentEditable` asks the +opened document. One consequence of claiming every spelling: `MetadataLoader` puts whatever mime type it ended up with through `SupportedDocumentTypes.canonicalMimeType`, so the loaders behind it see one -per format. `CoreLoader` matches the whole set and does not care, but `RawLoader` routes by -mime type *prefix* - a provider volunteering `application/x-zip-compressed` or -`application/csv` would be offered the app and then told the file is unsupported. -`SupportedFormatsTest.everythingTheAppClaimsIsLoadedBySomebody` is what holds that: every mime -type the app claims has to reach a loader that takes it. +per format. Both loaders match whole sets rather than prefixes now, but a provider +volunteering `application/x-zip-compressed` or `application/csv` still has to reach one of +them. `SupportedFormatsTest.everythingTheAppClaimsIsLoadedBySomebody` is what holds that: +every mime type the app claims has to reach a loader that takes it. + +One catch when reading the table directly, as `isDocument` does: the core matches mime types +*exactly* and spells some with capitals (`macroEnabled`), so do not `lowercase()` first. The +lookups against our own sets are the other way round - `mimeTypesOf` lowercases what it stores. + +### `RawLoader` is asked before `CoreLoader`, not after it + +It used to be the fallback at the end of the chain, reached only when the core threw. Once +odrcore learned to render text, images, zip and media the core stopped throwing, and the +PhotoSwipe, Plyr, JSZip and csv-to-html-table viewers in `assets/` became unreachable without +anyone noticing. The first four are gone; the csv one was worth keeping. + +So `LoaderService.onSuccess` asks `rawLoader.isSupported` *first*. +`SupportedDocumentTypes.isRenderedByRaw` is the whole list: csv (the only case where the +order matters, since `CORE_FILE_TYPES` excludes it), plus svg and xml, which have no odrcore +file type at all. A `RawLoader` failure falls through to the core; everything the core cannot +open goes to the upload offer rather than being renamed and handed to the WebView on spec. + +Routing by name is guarded - see `nameSays`. The core identifies by content, so a `report.csv` +holding an odt is an odt, and only a file it did not recognize (or called plain text) may be +routed by its extension. ### Editability comes from the core, never from a mime type diff --git a/app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt b/app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt index bd827cd6507a..f97f0b2ce153 100644 --- a/app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/background/CoreLoaderTest.kt @@ -88,13 +88,33 @@ class CoreLoaderTest { assertTrue(isSupported("application/x-pdf")) } + /** What odrcore 6.2 renders beyond documents, each of which used to need a viewer in assets. */ + @Test + fun whatTheCoreRendersBesidesDocumentsIsSupported() { + assertTrue(isSupported("text/plain")) + assertTrue(isSupported("image/png")) + // the image types 6.2 added + assertTrue(isSupported("image/webp")) + assertTrue(isSupported("image/heic")) + assertTrue(isSupported("image/avif")) + assertTrue(isSupported("application/zip")) + // rendered when handed one, never claimed in the share sheet + assertTrue(isSupported("audio/mpeg")) + assertTrue(isSupported("video/mp4")) + } + + /** The one format left to RawLoader although the core would take it - its table viewer. */ @Test fun whatRawLoaderShowsIsNotClaimed() { - // the core does render these, but RawLoader is what gives them their player or viewer - assertFalse(isSupported("text/plain")) assertFalse(isSupported("text/csv")) - assertFalse(isSupported("image/png")) - assertFalse(isSupported("application/zip")) + assertFalse(isSupported("application/csv")) + } + + /** No file type in the core at all, so nothing here could name them either. */ + @Test + fun whatTheCoreHasNoFileTypeForIsNotClaimed() { + assertFalse(isSupported("image/svg+xml")) + assertFalse(isSupported("application/xml")) } @Test diff --git a/app/src/androidTest/java/app/opendocument/droid/background/OnlineLoaderTest.kt b/app/src/androidTest/java/app/opendocument/droid/background/OnlineLoaderTest.kt index c10c96e4243c..695c1aefa266 100644 --- a/app/src/androidTest/java/app/opendocument/droid/background/OnlineLoaderTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/background/OnlineLoaderTest.kt @@ -8,9 +8,9 @@ import org.junit.Test import org.junit.runner.RunWith /** - * Instrumented rather than a JVM test since odrcore 6.1: [OnlineLoader.isConvertible] falls through - * to [CoreLoader.isSupported], which reads the core's format table out of `libodr_jni` now instead - * of matching mime prefixes in kotlin. Nothing here uploads anything or touches the network. + * Instrumented since odrcore 6.1: [OnlineLoader.isConvertible] falls through to + * [SupportedDocumentTypes.isDocument], which reads the core's table in `libodr_jni`. Nothing here + * uploads anything or touches the network. */ @SmallTest @RunWith(AndroidJUnit4::class) @@ -20,7 +20,7 @@ class OnlineLoaderTest { @Before fun setUp() { - onlineLoader = OnlineLoader(null, CoreLoader(null)) + onlineLoader = OnlineLoader(null) } private fun options(fileType: String): FileLoader.Options { @@ -85,15 +85,20 @@ class OnlineLoaderTest { "application/vnd.openxmlformats-officedocument.presentationml.presentation" ) ) - // delegated to CoreLoader + // whatever else the core files as a document Assert.assertTrue(isConvertible("application/vnd.oasis.opendocument.text")) + // including what it cannot open - the converter runs libreoffice, not odrcore + Assert.assertTrue(isConvertible("application/vnd.ms-excel.sheet.binary.macroEnabled.12")) } + /** The categories a converter cannot help with. */ @Test fun handsEverythingElseToAThirdPartyViewer() { Assert.assertFalse(isConvertible("text/plain")) Assert.assertFalse(isConvertible("image/png")) Assert.assertFalse(isConvertible("application/zip")) + Assert.assertFalse(isConvertible("audio/mpeg")) + Assert.assertFalse(isConvertible("video/mp4")) Assert.assertFalse(isConvertible("application/vnd.apple.pages")) } } diff --git a/app/src/androidTest/java/app/opendocument/droid/background/RawLoaderTest.kt b/app/src/androidTest/java/app/opendocument/droid/background/RawLoaderTest.kt new file mode 100644 index 000000000000..098e655dc72c --- /dev/null +++ b/app/src/androidTest/java/app/opendocument/droid/background/RawLoaderTest.kt @@ -0,0 +1,118 @@ +package app.opendocument.droid.background + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.filters.SmallTest +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.BeforeClass +import org.junit.Test +import org.junit.runner.RunWith + +/** + * The three things [RawLoader] still has a viewer for, and the many it handed back to the core. + * + * Instrumented because the csv spellings come from odrcore's table in `libodr_jni`. + */ +@SmallTest +@RunWith(AndroidJUnit4::class) +class RawLoaderTest { + + private lateinit var rawLoader: RawLoader + + @Before + fun setUp() { + // no context: isSupported() is pure, and constructing a loader has no side effects + rawLoader = RawLoader(null) + } + + private fun isSupported(fileType: String?, filename: String? = null): Boolean { + val options = FileLoader.Options() + options.fileType = fileType + options.filename = filename + + return rawLoader.isSupported(options) + } + + /** + * How svg and xml actually arrive: the core identifies by content and an svg *is* text, so + * `Odr.mimetype` says `text/plain` and only the name knows better. + */ + @Test + fun whatTheCoreMisreadsAsTextIsRoutedByItsName() { + assertTrue(isSupported("text/plain", "drawing.svg")) + assertTrue(isSupported("text/plain", "feed.xml")) + assertTrue(isSupported("text/plain", "rows.csv")) + // and a text file is still a text file + assertFalse(isSupported("text/plain", "readme.txt")) + assertFalse(isSupported("text/plain", "notes")) + } + + /** + * The other half: the bytes win whenever the core recognized them. An odt called `report.csv` + * would otherwise reach the table viewer, succeed, and leave no fallback. + */ + @Test + fun aMisnamedDocumentIsStillADocument() { + assertFalse(isSupported("application/vnd.oasis.opendocument.text", "report.csv")) + assertFalse(isSupported("application/pdf", "report.csv")) + assertFalse(isSupported("application/zip", "archive.svg")) + assertFalse(isSupported("image/png", "scan.xml")) + // but nothing detected at all still leaves the name as the only thing to go on + assertTrue(isSupported("application/octet-stream", "rows.csv")) + assertTrue(isSupported(null, "drawing.svg")) + } + + /** The core renders a csv line by line; `text-prefix.html` builds a table out of one. */ + @Test + fun csvIsSupportedInEverySpelling() { + assertTrue(isSupported("text/csv")) + assertTrue(isSupported("application/csv")) + assertTrue(isSupported("text/comma-separated-values")) + } + + /** Neither has a file type in odrcore, so nothing else would show them at all. */ + @Test + fun whatTheCoreHasNoFileTypeForIsSupported() { + assertTrue(isSupported("image/svg+xml")) + assertTrue(isSupported("application/xml")) + assertTrue(isSupported("text/xml")) + } + + /** + * Each had a viewer here until odrcore 6.2 learned to render it. Keeps them from coming back. + */ + @Test + fun whatTheCoreRendersIsLeftToIt() { + assertFalse(isSupported("text/plain")) + assertFalse(isSupported("image/png")) + assertFalse(isSupported("image/webp")) + assertFalse(isSupported("application/zip")) + assertFalse(isSupported("audio/mpeg")) + assertFalse(isSupported("video/mp4")) + assertFalse(isSupported("application/json")) + } + + /** No rename-and-hope for these - the upload offer beats a blank page. */ + @Test + fun whatNobodyCanShowIsNotSupported() { + assertFalse(isSupported("application/pdf")) + assertFalse(isSupported("application/vnd.oasis.opendocument.text")) + assertFalse(isSupported("application/octet-stream")) + assertFalse(isSupported("text/vcard")) + assertFalse(isSupported("text/rtf")) + assertFalse(isSupported(null)) + } + + companion object { + + // @JvmStatic because junit requires @BeforeClass to be static + @JvmStatic + @BeforeClass + fun prepare() { + // the csv mime types come from a table in libodr_jni + CoreLoader.initializeCore(InstrumentationRegistry.getInstrumentation().targetContext) + } + } +} 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 d7c95825d275..ab825db9cf50 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt @@ -96,15 +96,13 @@ class SupportedFormatsTest { * Everything the app offers itself for reaches a loader that takes it. * * Claiming a mime type nobody loads is the "cannot open" the user gets on a file they picked us - * for, and it is what reading the core's whole table risks: [CoreLoader] matches every spelling - * of a format, but [RawLoader] goes by mime type prefix and so only ever sees the canonical one - * that [MetadataLoader] resolves to - `application/csv` and `multipart/x-zip` are claimed and - * would otherwise be dropped between the two. + * for. [CoreLoader] takes almost all of it now; the exception is csv, which [RawLoader] keeps + * for its table viewer and which [LoaderService] therefore routes before the core. */ @Test fun everythingTheAppClaimsIsLoadedBySomebody() { val coreLoader = CoreLoader(null) - val rawLoader = RawLoader(null, coreLoader) + val rawLoader = RawLoader(null) for (mimeType in SupportedDocumentTypes.MIME_TYPES) { val options = FileLoader.Options() diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 1d808574a11a..9ccf13c7b7bc 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -236,11 +236,14 @@ tools:ignore="AppLinkUrlError"> @@ -302,10 +305,10 @@ --> diff --git a/app/src/main/assets/audio.html b/app/src/main/assets/audio.html deleted file mode 100644 index afc70f1182e1..000000000000 --- a/app/src/main/assets/audio.html +++ /dev/null @@ -1,43 +0,0 @@ - - - - - - - - - - - - - - - - - - \ No newline at end of file diff --git a/app/src/main/assets/video.html b/app/src/main/assets/video.html deleted file mode 100644 index 5feb85e40fdf..000000000000 --- a/app/src/main/assets/video.html +++ /dev/null @@ -1,43 +0,0 @@ - - - - - - - - - - - - - -