From 20c2e05e0caa4b4b9bdb2dfa9cde58a06880cfe5 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 2 Aug 2026 23:11:21 +0200 Subject: [PATCH 1/3] Let the core render the raw files it has learned to render odrcore 6.2 turns text, images, zip and cfb, fonts, audio and video into html itself, so the viewers RawLoader kept in assets/ - PhotoSwipe, Plyr, JSZip - had nothing left to do. They were in fact already unreachable: LoaderService routes METADATA -> CORE unconditionally and only falls back to RawLoader when the core *throws*, which it stopped doing for those formats. Deleting them takes 916KB of vendored javascript with them. What is left in RawLoader has a reason to be: - csv, because the core files it as text and renders it line by line where text-prefix.html builds a real table. This one is a restoration: on main a .csv already showed as plain text, the table viewer having quietly died with the 6.2 upgrade. It is excluded from CORE_FILE_TYPES and asked for first, since the core would otherwise succeed and RawLoader would never get a turn. - svg, which odrcore has no file type for at all. - xml and whatever else reaches the generic branch. SupportedDocumentTypes now answers two questions instead of one. What the app *claims* (CLAIMED_FILE_TYPES) is unchanged, so AndroidManifest.xml does not move and the app still stays out of the share sheet for an mp3; what CoreLoader *renders* (CORE_FILE_TYPES) is now every translateHtml format whatever its category, so an mp3 handed to us still plays. Two bugs fixed on the way: - OnlineLoader used CoreLoader.isSupported to mean "is an office document", which stops being true once the core loader claims images and media - a png would have been sent to the libreoffice converter and the microsoft viewer. It asks SupportedDocumentTypes.isDocument now, which also makes .xlsb convertible: the converter is not odrcore and can manage one. - svg never reached RawLoader at all. MetadataLoader asks Odr.mimetype first and the core content-detects an svg as text/plain, so the image/svg+xml a provider volunteered never reached routing - and MimeTypeResolver.resolve then rewrites the extension to "txt", losing the name too. Routing on the original filename fixes it, and an svg renders as an image for the first time rather than as a line-numbered text dump. The "open it in another app" hint follows the core's file category now rather than "the raw loader ran", which said the same thing only while everything that was not a document went to RawLoader. RawLoaderTest moves to androidTest: the csv spellings come from the core's table, which lives in libodr_jni. Verified on an emulator against a build of main over the same corpus: png, wav, txt, zip and odt route identically, and only csv, svg and xml change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019PhqfvUDxKNH7b9ueC5iuE --- CLAUDE.md | 66 +- .../droid/background/CoreLoaderTest.kt | 31 +- .../droid/background/OnlineLoaderTest.kt | 17 +- .../droid/background/RawLoaderTest.kt | 108 + .../droid/test/SupportedFormatsTest.kt | 8 +- app/src/main/AndroidManifest.xml | 21 +- app/src/main/assets/audio.html | 43 - app/src/main/assets/video.html | 43 - app/src/main/assets/zip-prefix.html | 2229 ----------------- app/src/main/assets/zip-suffix.html | 220 -- .../droid/background/CoreLoader.kt | 28 +- .../droid/background/LoaderService.kt | 34 +- .../droid/background/OnlineLoader.kt | 15 +- .../droid/background/RawLoader.kt | 168 +- .../background/SupportedDocumentTypes.kt | 149 +- .../droid/ui/activity/DocumentFragment.kt | 7 +- .../droid/background/RawLoaderTest.kt | 49 - 17 files changed, 417 insertions(+), 2819 deletions(-) create mode 100644 app/src/androidTest/java/app/opendocument/droid/background/RawLoaderTest.kt delete mode 100644 app/src/main/assets/audio.html delete mode 100644 app/src/main/assets/video.html delete mode 100644 app/src/main/assets/zip-prefix.html delete mode 100644 app/src/main/assets/zip-suffix.html delete mode 100644 app/src/test/java/app/opendocument/droid/background/RawLoaderTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index e1246f0b178d..4c26c8e205f1 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,18 @@ 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 is far more than + documents - text, images, zip and cfb, fonts, audio and video all get a page of their own, + which is why `RawLoader` no longer needs a viewer for any of them. +- **what the app claims** (`CLAIMED_FILE_TYPES`): the same filter narrowed to + `Odr.fileCategoryByFileType(...) == DOCUMENT`, plus text, csv and zip. This is what the + manifest mirrors. Keep it narrow - the app renders an mp3 handed to it but has no business + in the share sheet for one. + +`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 +179,10 @@ 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* - the core's document +formats plus text, csv, zip and `image/` are named in `SupportedDocumentTypes` as +`CLAIMED_FILE_TYPES`, because that is an app decision the core knows nothing about, and it is +deliberately much narrower than what `CoreLoader` will render. 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 +192,42 @@ 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 of its own with capitals (`macroEnabled`), so do not `lowercase()` +before handing it one. The lookups against our own sets are the other way round, because +`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: `LoaderService` sent everything to +`CoreLoader` and only reached for `RawLoader` when that threw. That stopped working the +moment odrcore learned to render text, images, zip and media - the core stopped throwing, so +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* and only falls through to +the core. `SupportedDocumentTypes.isRenderedByRaw` is the whole list and it is three items: + +- **csv**, which the core renders line-numbered like any other text where `text-prefix.html` + builds a real table. The only case where the order actually matters. +- **svg** and **xml**, which have no odrcore file type at all. + +`CORE_FILE_TYPES` excludes csv for the same reason, so the two answers cannot both be yes. If +`RawLoader` fails, the core gets its turn as a fallback; everything the core cannot open goes +to the upload offer rather than being renamed and handed to the WebView on spec. ### 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..f36c0dbb3b2f 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,36 @@ class CoreLoaderTest { assertTrue(isSupported("application/x-pdf")) } + /** + * Everything odrcore 6.2 renders beyond the document formats, which the app used to give a + * viewer of its own in `assets/` and no longer needs to. + */ + @Test + fun whatTheCoreRendersBesidesDocumentsIsSupported() { + assertTrue(isSupported("text/plain")) + assertTrue(isSupported("image/png")) + // the image types 6.2 added, which the old five-format list did not reach + assertTrue(isSupported("image/webp")) + assertTrue(isSupported("image/heic")) + assertTrue(isSupported("image/avif")) + assertTrue(isSupported("application/zip")) + // never claimed in the share sheet, but played when handed one - see SupportedDocumentTypes + 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..38845c8333d2 100644 --- a/app/src/androidTest/java/app/opendocument/droid/background/OnlineLoaderTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/background/OnlineLoaderTest.kt @@ -9,8 +9,9 @@ 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. + * to [SupportedDocumentTypes.isDocument], 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. */ @SmallTest @RunWith(AndroidJUnit4::class) @@ -20,7 +21,7 @@ class OnlineLoaderTest { @Before fun setUp() { - onlineLoader = OnlineLoader(null, CoreLoader(null)) + onlineLoader = OnlineLoader(null) } private fun options(fileType: String): FileLoader.Options { @@ -85,15 +86,23 @@ 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 itself - the converter runs libreoffice, not odrcore + Assert.assertTrue(isConvertible("application/vnd.ms-excel.sheet.binary.macroEnabled.12")) } + /** + * The categories a converter cannot help with. This used to ask [CoreLoader.isSupported], which + * meant "is a document" until the core loader took over images, archives and media. + */ @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..ef0f938cf59c --- /dev/null +++ b/app/src/androidTest/java/app/opendocument/droid/background/RawLoaderTest.kt @@ -0,0 +1,108 @@ +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 rather than a JVM test because the csv spellings come from odrcore's format table, + * which lives in `libodr_jni` - the same reason [CoreLoaderTest] is one. + */ +@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) + } + + /** + * The case svg and xml actually arrive as. [MetadataLoader] asks the core to identify the file + * before anything else, and an svg *is* text - so `Odr.mimetype` says `text/plain` and the name + * is all that is left to tell it apart. This is what the render sweep sees on a real device. + */ + @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 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")) + } + + /** + * All of these had a viewer here until odrcore 6.2 learned to render them: PhotoSwipe for the + * images, Plyr for the media, JSZip for the archives. They were already unreachable by + * then - [LoaderService] tried the core first - so this keeps them from being routed back by + * accident. + */ + @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 @@ - - - - - - - - - - - - - -