diff --git a/CLAUDE.md b/CLAUDE.md index 95859fd9d9f2..79bc80de16f8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -145,17 +145,43 @@ the `.pro` suffix) differ on purpose - do not "fix" the mismatch: `AndroidFileCache`, the SharedPreferences name in `MainActivity`) follows `applicationId` and must stay that way, or existing users lose their saved prefs. -### Supported file types are declared twice - -`SupportedDocumentTypes` (mime prefixes plus an extension fallback) and the `STRICT_CATCH` -`activity-alias` in `AndroidManifest.xml` describe the same set from two directions - the manifest -is what the system offers the app for, `SupportedDocumentTypes` is what the folder browser on the -landing screen offers itself for. **Nothing keeps them in step, so change both.** - -The duplication is deliberate: `MetadataLoader` decides what is really supported by running -libmagic over the file *after* copying it into the cache, and `CoreLoader.isSupported()` / -`RawLoader.isSupported()` both take a filled-in `FileLoader.Options`. A folder listing only has a -name and whatever mime type the provider volunteered, so it cannot ask them and has to guess. +### Supported file types are declared twice, and a test keeps them in step + +`SupportedDocumentTypes` is the single kotlin table: mime prefixes for what the core renders, mime +prefixes for what `RawLoader` shows, plus an extension fallback for the `application/octet-stream` +a provider volunteers when it knows nothing better. `CoreLoader.isSupported()` defers to it - it +used to keep a second copy of the same prefixes. + +The `STRICT_CATCH` `activity-alias` in `AndroidManifest.xml` is the second declaration and cannot be +collapsed into the first, because XML cannot read a kotlin list. It is covered instead: +`SupportedFormatsTest` (instrumented) walks odrcore's own `FileType` values, asks +`Odr.mimetypeByFileType` for each, and asserts that `SupportedDocumentTypes` and the package +manager give the same answer - so a format added on one side and forgotten on the other fails CI +rather than shipping. + +The set follows odrcore: whatever it keeps reference html output for in `test/data/reference-output` +is what the app claims - odf, ooxml, the legacy binary doc/ppt/xls and pdf. Text, csv and images are +the core's too but belong to `RawLoader`, which only gets its turn when `CoreLoader.isSupported()` +says no. + +Why the app has a table at all when odrcore has `Odr.fileTypeByMimetype` / +`Odr.fileTypeByFileExtension`: both are native calls into `libodr_jni`, and both know exactly one +canonical mime type per format - no templates, no macro-enabled variants, no +`application/x-vnd.oasis...`, which are the spellings content providers actually hand out. The +guessing stage needs to be broader than the core's own lookup. What the core *does* decide is +everything after the file is in the cache: `MetadataLoader` runs libmagic over the copy, and +`CoreLoader.isDocumentEditable` asks the opened document. + +### Editability comes from the core, never from a mime type + +`Document.isEditable()`/`isSavable()` is what decides whether `DocumentFragment` puts the Edit +button up, carried across on `FileLoader.Result.isEditable`. `CoreLoader.host()` only holds a +document open when the core says yes, so "we have a document" *is* the answer. + +Do not reintroduce a list of editable formats in the UI. The core already says no to the legacy +binary doc/ppt/xls, to ooxml spreadsheets and presentations, and to every spreadsheet including odf +(its own TODO, the same gap as issue #442) - all of which the app used to spell out by mime prefix +and got wrong the moment a new format started going through `CoreLoader`. ### Storage access diff --git a/app/src/androidTest/assets/11KB.doc b/app/src/androidTest/assets/11KB.doc new file mode 100644 index 000000000000..3940e5d26a65 Binary files /dev/null and b/app/src/androidTest/assets/11KB.doc differ diff --git a/app/src/androidTest/assets/file_example_XLS_10.xls b/app/src/androidTest/assets/file_example_XLS_10.xls new file mode 100644 index 000000000000..1464ee150a46 Binary files /dev/null and b/app/src/androidTest/assets/file_example_XLS_10.xls differ diff --git a/app/src/androidTest/assets/style-various-1.ppt b/app/src/androidTest/assets/style-various-1.ppt new file mode 100644 index 000000000000..330883837974 Binary files /dev/null and b/app/src/androidTest/assets/style-various-1.ppt differ diff --git a/app/src/androidTest/assets/style-various-1.pptx b/app/src/androidTest/assets/style-various-1.pptx new file mode 100644 index 000000000000..527847468a6f Binary files /dev/null and b/app/src/androidTest/assets/style-various-1.pptx differ 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 f09fcf29e138..f829aa4553a6 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/CoreTest.kt @@ -31,6 +31,10 @@ class CoreTest { private var passwordTestFile: File? = null private var spreadsheetTestFile: File? = null private var docxTestFile: File? = null + private var pptxTestFile: File? = null + private var docTestFile: File? = null + private var pptTestFile: File? = null + private var xlsTestFile: File? = null private val coreLoader: CoreLoader get() = checkNotNull(sharedLoader) { "the core loader was not started" } @@ -58,6 +62,22 @@ class CoreTest { assetManager.open("style-various-1.docx"), File(appCtx.cacheDir, "style-various-1.docx"), ) + pptxTestFile = + extract( + assetManager.open("style-various-1.pptx"), + File(appCtx.cacheDir, "style-various-1.pptx"), + ) + docTestFile = extract(assetManager.open("11KB.doc"), File(appCtx.cacheDir, "11KB.doc")) + pptTestFile = + extract( + assetManager.open("style-various-1.ppt"), + File(appCtx.cacheDir, "style-various-1.ppt"), + ) + xlsTestFile = + extract( + assetManager.open("file_example_XLS_10.xls"), + File(appCtx.cacheDir, "file_example_XLS_10.xls"), + ) } @After @@ -66,6 +86,10 @@ class CoreTest { passwordTestFile?.delete() spreadsheetTestFile?.delete() docxTestFile?.delete() + pptxTestFile?.delete() + docTestFile?.delete() + pptTestFile?.delete() + xlsTestFile?.delete() } @Test @@ -109,6 +133,90 @@ class CoreTest { Assert.assertTrue("the edited document should have been saved", result.isFile) } + /** + * The formats that used to be sent off for conversion instead: presentations, and the legacy + * binary microsoft ones. The core renders all four, so the app hands them to it. + */ + @Test + fun testPptx() { + val views = + coreLoader.host( + prefix = "pptx-test", + inputPath = requireFile(pptxTestFile).absolutePath, + cachePath = File(cacheDir(), "pptx_cache").path, + ) + Assert.assertFalse("hosting the PPTX file should produce a view", views.isEmpty()) + } + + @Test + fun testDoc() { + val views = + coreLoader.host( + prefix = "doc-test", + inputPath = requireFile(docTestFile).absolutePath, + cachePath = File(cacheDir(), "doc_cache").path, + ) + Assert.assertFalse("hosting the DOC file should produce a view", views.isEmpty()) + } + + @Test + fun testPpt() { + val views = + coreLoader.host( + prefix = "ppt-test", + inputPath = requireFile(pptTestFile).absolutePath, + cachePath = File(cacheDir(), "ppt_cache").path, + ) + Assert.assertFalse("hosting the PPT file should produce a view", views.isEmpty()) + } + + @Test + fun testXls() { + val views = + coreLoader.host( + prefix = "xls-test", + inputPath = requireFile(xlsTestFile).absolutePath, + cachePath = File(cacheDir(), "xls_cache").path, + ) + Assert.assertFalse("hosting the XLS file should produce a view", views.isEmpty()) + } + + /** + * Which of the formats the core renders it can also write back again - the answer + * `DocumentFragment` puts the Edit button up by. Offering it for a document the core will not + * save only gets as far as a failed save, which is what happened to the legacy binary formats + * when they started going through the core. + */ + @Test + fun testEditableFormats() { + assertEditable("odt-editable", requireFile(testFile), true) + assertEditable("docx-editable", requireFile(docxTestFile), true) + + // the core declares these read only: the three legacy binary formats, ooxml presentations + // and every spreadsheet - the last being issue #442, which the core has its own TODO for + assertEditable("doc-editable", requireFile(docTestFile), false) + assertEditable("ppt-editable", requireFile(pptTestFile), false) + assertEditable("xls-editable", requireFile(xlsTestFile), false) + assertEditable("pptx-editable", requireFile(pptxTestFile), false) + assertEditable("ods-editable", requireFile(spreadsheetTestFile), false) + } + + private fun assertEditable(prefix: String, file: File, expected: Boolean) { + coreLoader.host( + prefix = prefix, + inputPath = file.absolutePath, + cachePath = File(cacheDir(), prefix).path, + editable = true, + keepDocument = true, + ) + + Assert.assertEquals( + "the core should report ${file.name} as ${if (expected) "editable" else "read only"}", + expected, + coreLoader.isDocumentEditable, + ) + } + @Test fun testPasswordProtectedDocumentWithoutPassword() { Assert.assertThrows(OdrException.FileEncrypted::class.java) { @@ -176,7 +284,7 @@ class CoreTest { // set of statics. 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()) - val loader = CoreLoader(appCtx, true) + val loader = CoreLoader(appCtx) sharedLoader = loader loader.initialize( object : FileLoader.FileLoaderListener { diff --git a/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt b/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt new file mode 100644 index 000000000000..3541394ccc3f --- /dev/null +++ b/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt @@ -0,0 +1,163 @@ +package app.opendocument.droid.test + +import android.content.Intent +import android.content.pm.PackageManager +import android.net.Uri +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.filters.SmallTest +import androidx.test.platform.app.InstrumentationRegistry +import app.opendocument.core.FileType +import app.opendocument.core.Odr +import app.opendocument.droid.background.CatchAllSetting +import app.opendocument.droid.background.CoreLoader +import app.opendocument.droid.background.SupportedDocumentTypes +import org.junit.Assert +import org.junit.BeforeClass +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Keeps the two remaining declarations of what the app opens in step with each other, so that + * changing one and forgetting the other is a failing test rather than a bug report. + * + * [SupportedDocumentTypes] is one of them. The other is the STRICT_CATCH `activity-alias` in + * AndroidManifest.xml, which is XML and cannot read a kotlin list - but it can be *asked*: an + * intent-filter is only worth anything if the package manager resolves a real intent through it, + * which is what [resolvesToUs] does here. There used to be a third copy in `CoreLoader`; that one + * is gone, it defers to [SupportedDocumentTypes] now. + * + * The mime side needs no list of its own at all: [FileType] is the core's complete set of formats + * and [Odr.mimetypeByFileType] names each one, so the test walks what odrcore knows and demands + * that the app and the manifest give the same answer for every entry. + */ +@SmallTest +@RunWith(AndroidJUnit4::class) +class SupportedFormatsTest { + + /** + * For every format odrcore can name a mime type for, what the app offers itself for and what + * the manifest offers it for have to be the same answer. + * + * This is the assertion that catches a format being added to the loader without being added to + * the manifest, which is the failure the user sees as "the app is not in the share sheet". + */ + @Test + fun theManifestClaimsExactlyWhatTheAppClaims() { + var checked = 0 + + for (fileType in FileType.values()) { + val mimeType = mimeTypeOrNull(fileType) ?: continue + checked++ + + val claimedByApp = SupportedDocumentTypes.isSupported(mimeType, null) + val claimedByManifest = resolvesToUs(mimeType, "document") + + Assert.assertEquals( + "$fileType ($mimeType): the app says $claimedByApp, the manifest says" + + " $claimedByManifest - AndroidManifest.xml and SupportedDocumentTypes have" + + " drifted apart", + claimedByApp, + claimedByManifest, + ) + } + + // a guard on the guard: if the core ever stops naming mime types the loop above would + // silently assert nothing at all + Assert.assertTrue("no file type was checked", checked > 10) + } + + /** Every extension the folder browser offers for has to be one odrcore actually recognises. */ + @Test + fun theExtensionFallbackNamesFormatsTheCoreKnows() { + for (extension in SupportedDocumentTypes.EXTENSIONS) { + Assert.assertNotEquals( + "odrcore does not know the extension .$extension", + FileType.UNKNOWN, + Odr.fileTypeByFileExtension(extension), + ) + } + } + + /** + * The case the extension fallback exists for: a provider that volunteers nothing better than + * `application/octet-stream`. The app goes by the name, and the manifest has to as well - it + * claims `application/octet-stream` outright, so this is really a check that the claim stays. + */ + @Test + fun anOctetStreamWithAKnownExtensionReachesUs() { + for (extension in SupportedDocumentTypes.EXTENSIONS) { + Assert.assertTrue( + "the app does not offer for document.$extension", + SupportedDocumentTypes.isSupported( + "application/octet-stream", + "document.$extension", + ), + ) + Assert.assertTrue( + "the manifest does not offer for document.$extension", + resolvesToUs("application/octet-stream", "document.$extension"), + ) + } + } + + /** What the app deliberately stays out of - the reason the catch-all filter defaults to off. */ + @Test + fun unrelatedTypesReachNeither() { + for (mimeType in listOf("text/vcard", "text/calendar", "audio/mpeg", "video/mp4")) { + Assert.assertFalse( + "the app should not offer for $mimeType", + SupportedDocumentTypes.isSupported(mimeType, null), + ) + Assert.assertFalse( + "the manifest should not offer for $mimeType", + resolvesToUs(mimeType, "document"), + ) + } + } + + /** + * Whether an `ACTION_VIEW` for this mime type and filename lands on our own activity - the only + * thing an intent-filter is actually good for, and the one question a hand written XML list can + * still be asked. + */ + private fun resolvesToUs(mimeType: String, filename: String): Boolean { + val context = InstrumentationRegistry.getInstrumentation().targetContext + + val intent = + Intent(Intent.ACTION_VIEW).apply { + addCategory(Intent.CATEGORY_DEFAULT) + setDataAndType(Uri.parse("content://app.opendocument.test/$filename"), mimeType) + } + + return context.packageManager + .queryIntentActivities(intent, PackageManager.MATCH_DEFAULT_ONLY) + .any { it.activityInfo?.packageName == context.packageName } + } + + /** Null for the types odrcore has no mime type for - word perfect, rtf, cfb and the like. */ + private fun mimeTypeOrNull(fileType: FileType): String? = + try { + Odr.mimetypeByFileType(fileType) + } catch (e: RuntimeException) { + null + } + + companion object { + + // @JvmStatic because junit requires @BeforeClass to be static + @JvmStatic + @BeforeClass + fun prepare() { + val context = InstrumentationRegistry.getInstrumentation().targetContext + + // Odr's tables live in libodr_jni; nothing here opens a file, so the data paths that + // initializeCore also sets up are along for the ride rather than needed + CoreLoader.initializeCore(context) + + // STRICT_CATCH is what this test is about, and the component states survive a test run + // - a previous one that flipped the switch would otherwise leave CATCH_ALL answering + // for everything. False is the shipped default, so nothing has to be put back. + CatchAllSetting.setEnabled(context, false) + } + } +} diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 24cedbd452fd..a732a11621ae 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -240,7 +240,7 @@ android:label="@string/app_title" android:targetActivity="app.opendocument.droid.ui.activity.MainActivity" tools:ignore="AppLinkUrlError"> - + @@ -259,7 +259,10 @@ + + + @@ -331,6 +334,27 @@ + + + + + + + + + + + + + + + + + + + + + @@ -419,6 +443,27 @@ + + + + + + + + + + + + + + + + + + + + + diff --git a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt index 887f2c1bb97a..f062007df1d3 100644 --- a/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/CoreLoader.kt @@ -9,7 +9,6 @@ import app.opendocument.core.DecodePreference import app.opendocument.core.DecodedFile import app.opendocument.core.Document import app.opendocument.core.DocumentType -import app.opendocument.core.FileType import app.opendocument.core.GlobalParams import app.opendocument.core.Html import app.opendocument.core.HtmlConfig @@ -43,8 +42,7 @@ import java.io.IOException */ // the context is nullable like FileLoader's own field, which close() clears and the unit // tests never set - isSupported() is pure and does not need one -class CoreLoader(context: Context?, private val doOoxml: Boolean) : - FileLoader(context, LoaderType.CORE) { +class CoreLoader(context: Context?) : FileLoader(context, LoaderType.CORE) { private var server: HttpServer? = null private var httpThread: Thread? = null @@ -53,6 +51,18 @@ class CoreLoader(context: Context?, private val doOoxml: Boolean) : private var lastInputPath: String? = null private var lastCachePath: String? = null + /** + * Whether the document [host] last opened is one [edit] can do something with. + * + * This is the core's own answer, not a list of formats kept here: [host] only holds a document + * open when it reports itself editable and savable, so having one is the answer. The core says + * no to the legacy binary formats, to ooxml spreadsheets and presentations and to opendocument + * spreadsheets - the last of those being the same gap as issue #442, which the app used to + * check for by mime type on its own. + */ + val isDocumentEditable: Boolean + get() = document != null + // the port the server actually got, which is not always the preferred one. see bind() private var serverPort = PREFERRED_SERVER_PORT @@ -131,23 +141,17 @@ class CoreLoader(context: Context?, private val doOoxml: Boolean) : else FALLBACK_SERVER_PORT } - override fun isSupported(options: Options): Boolean { - val fileType = options.fileType ?: return false - return fileType.startsWith("application/vnd.oasis.opendocument") || - fileType.startsWith("application/x-vnd.oasis.opendocument") || - fileType.startsWith("application/vnd.oasis.opendocument.text-master") || - fileType.startsWith("application/msword") || - (doOoxml && - (fileType.startsWith( - "application/vnd.openxmlformats-officedocument.wordprocessingml.document" - ) || - fileType.startsWith( - "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" - ) - // TODO: enable pptx too - // fileType.startsWith("application/vnd.openxmlformats-officedocument.presentationml.presentation") - )) - } + /** + * What the core renders itself, out of [SupportedDocumentTypes] so the folder browser and this + * cannot disagree about it. + * + * Text, csv and images are left out although the core takes those too - [RawLoader] is what + * gives them their player or their viewer, and this answer is what lets it have its turn. The + * rest of it decides whether a failed load is worth reporting and which viewer [OnlineLoader] + * falls back to. + */ + override fun isSupported(options: Options): Boolean = + SupportedDocumentTypes.isRenderedByCore(options.fileType) override fun loadSync(options: Options) { val result = Result(type, options) @@ -191,6 +195,8 @@ class CoreLoader(context: Context?, private val doOoxml: Boolean) : keepDocument = true, ) + result.isEditable = isDocumentEditable + for (view in views) { result.partTitles.add(view.name) result.partUris.add(Uri.parse(view.url)) @@ -232,10 +238,18 @@ class CoreLoader(context: Context?, private val doOoxml: Boolean) : if (keepDocument) { closeDocument() - // .doc-files are not real documents in core - if (file.isDocumentFile && file.fileType() != FileType.LEGACY_WORD_DOCUMENT) { + if (file.isDocumentFile) { // TODO this will cause a second load - document = file.asDocumentFile().document() + val document = file.asDocumentFile().document() + + // keep only what [edit] can do something with, and let the core be the one to say + // so - see [isDocumentEditable]. Holding a read only document open buys a second + // parse and nothing else. + if (document.isEditable && document.isSavable) { + this.document = document + } else { + document.close() + } } } diff --git a/app/src/main/java/app/opendocument/droid/background/FileLoader.kt b/app/src/main/java/app/opendocument/droid/background/FileLoader.kt index c9eaf94b6e6f..8785706eefd8 100644 --- a/app/src/main/java/app/opendocument/droid/background/FileLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/FileLoader.kt @@ -200,6 +200,13 @@ abstract class FileLoader(context: Context?, protected val type: LoaderType) { val partTitles: MutableList = LinkedList() val partUris: MutableList = LinkedList() + /** + * Whether the loaded document can be edited and saved again, as the core reports it - see + * [CoreLoader.isDocumentEditable]. False for every other loader, which have nothing to + * edit. + */ + var isEditable: Boolean = false + override fun describeContents(): Int = 0 override fun writeToParcel(parcel: Parcel, flags: Int) { @@ -207,6 +214,7 @@ abstract class FileLoader(context: Context?, protected val type: LoaderType) { parcel.writeParcelable(options, 0) parcel.writeList(partTitles) parcel.writeList(partUris) + ParcelUtil.writeBoolean(parcel, isEditable) } companion object { @@ -225,6 +233,7 @@ abstract class FileLoader(context: Context?, protected val type: LoaderType) { return Result(loaderType, options).also { parcel.readList(it.partTitles, classLoader) parcel.readList(it.partUris, classLoader) + it.isEditable = ParcelUtil.readBoolean(parcel) } } diff --git a/app/src/main/java/app/opendocument/droid/background/LoaderService.kt b/app/src/main/java/app/opendocument/droid/background/LoaderService.kt index f04a8ef8de31..cb2088a971a7 100644 --- a/app/src/main/java/app/opendocument/droid/background/LoaderService.kt +++ b/app/src/main/java/app/opendocument/droid/background/LoaderService.kt @@ -60,7 +60,7 @@ class LoaderService : Service(), FileLoader.FileLoaderListener { crashManager, ) - coreLoader = CoreLoader(this, true) + coreLoader = CoreLoader(this) coreLoader.initialize(this, mainHandler, backgroundHandler, analyticsManager, crashManager) rawLoader = RawLoader(this, coreLoader) diff --git a/app/src/main/java/app/opendocument/droid/background/OnlineLoader.kt b/app/src/main/java/app/opendocument/droid/background/OnlineLoader.kt index 3e534f1a6797..6863e2308b56 100644 --- a/app/src/main/java/app/opendocument/droid/background/OnlineLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/OnlineLoader.kt @@ -133,8 +133,12 @@ class OnlineLoader(context: Context?, private val coreLoader: CoreLoader) : } private fun buildViewerUri(options: Options, downloadUrl: String): Uri { - if (coreLoader.isSupported(options)) { - // ODF does not seem to be supported by google docs viewer + // ODF does not seem to be supported by google docs viewer, but pdf is the one thing the + // microsoft viewer will not take - and the core opens both, so what it supports no longer + // tells the two apart on its own + val isPdf = options.fileType?.startsWith("application/pdf") == true + + if (coreLoader.isSupported(options) && !isPdf) { return Uri.parse(MICROSOFT_VIEWER_URL + downloadUrl) } diff --git a/app/src/main/java/app/opendocument/droid/background/SupportedDocumentTypes.kt b/app/src/main/java/app/opendocument/droid/background/SupportedDocumentTypes.kt index 20ae54b4b992..bb07ebc63301 100644 --- a/app/src/main/java/app/opendocument/droid/background/SupportedDocumentTypes.kt +++ b/app/src/main/java/app/opendocument/droid/background/SupportedDocumentTypes.kt @@ -1,39 +1,72 @@ package app.opendocument.droid.background /** - * Whether a document is worth offering in the folder browser. + * The one list of what the app claims to open, for both of the places that have to guess. * - * Deliberately free of Android dependencies so it can be covered by plain JVM unit tests. + * There used to be two copies of this - one here for the folder browser, one in [CoreLoader] for + * the loader - which is one copy too many for a set that only ever changes as a whole. + * [isRenderedByCore] is what [CoreLoader.isSupported] answers with, [isSupported] adds what + * [RawLoader] shows and the extension fallback on top, and the STRICT_CATCH `activity-alias` in + * AndroidManifest.xml is the third and last declaration, which XML keeps out of reach. + * `SupportedFormatsTest` walks this table and asks the package manager whether the manifest still + * agrees, so the two cannot drift silently any more. * - * This is a guess, not the real answer. [MetadataLoader] decides what is supported by running - * libmagic over the file once it has been copied into the cache, and neither [CoreLoader] nor - * [RawLoader] can answer before that - so a folder listing, which only has a name and whatever mime - * type the provider volunteered, has to go on appearances. + * Deliberately free of Android dependencies, and of the core's own tables - odrcore does know + * [app.opendocument.core.Odr.fileTypeByMimetype] and `fileTypeByFileExtension`, but both are native + * calls into libodr_jni, and both only know one canonical mime type per format: no templates, no + * macro enabled variants, no `application/x-vnd.oasis...`, which are exactly the spellings a + * content provider hands out. What the core does decide is everything *after* the file is in the + * cache - [MetadataLoader] runs its libmagic over it, and [CoreLoader.isDocumentEditable] asks the + * opened document itself. * - * Mirrors the STRICT_CATCH intent filter in AndroidManifest.xml. **Keep the two in step**: that - * filter is what the system offers us for, this is what we offer ourselves for. + * So this is a guess, not the real answer: a folder listing only has a name and whatever mime type + * the provider volunteered, and it has to go on appearances. */ object SupportedDocumentTypes { - /** Mime types and prefixes taken from the STRICT_CATCH filter. */ - private val MIME_PREFIXES = + /** + * What the core renders itself: opendocument, ooxml, the three legacy binary microsoft formats + * and pdf. odrcore keeps reference html output for every one of them, doc, ppt and xls + * included. + * + * Matching by prefix carries the variants along: the ooxml prefix covers the templates, and the + * macro enabled ones are spelled `vnd.ms-word.document.macroEnabled.12` and so on, which is why + * the legacy powerpoint and excel types appear here without their subtypes and why + * `vnd.ms-word` is listed next to `msword`. + */ + private val CORE_MIME_PREFIXES = listOf( + // odf, including the master documents and the x- spelling some providers use "application/vnd.oasis.opendocument", - // the two ooxml types the filter spells out, rather than the family they belong to: - // pptx shares that prefix and CoreLoader does not open it yet, so a folder would - // offer a presentation and then drop it into the upload flow - "application/vnd.openxmlformats-officedocument.wordprocessingml.document", - "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + "application/x-vnd.oasis.opendocument", + // ooxml: word, excel and powerpoint, their templates and their macro variants + "application/vnd.openxmlformats-officedocument", + "application/vnd.ms-word", + // legacy binary microsoft: doc, xls and ppt "application/msword", + "application/vnd.ms-excel", + "application/vnd.ms-powerpoint", + // not a document, but the core renders it just the same "application/pdf", - "application/zip", - "text/plain", - "text/csv", - "image/", ) - /** The extensions the STRICT_CATCH pathPatterns spell out. */ - private val EXTENSIONS = + /** + * What [RawLoader] shows instead - text, csv, images and zip - which the core would take too + * but which get their own player or viewer here. + * + * Narrower than [RawLoader.isSupported], which also takes audio and video: those are worth + * playing once someone hands us one, but not worth offering the app for. + */ + private val RAW_MIME_PREFIXES = listOf("text/plain", "text/csv", "image/", "application/zip") + + /** + * The extensions to fall back on, spelling out the same set once more because providers + * regularly volunteer nothing better than `application/octet-stream`. + * + * Public so `SupportedFormatsTest` can walk it: it holds every one of these against the core's + * own extension table and against the manifest. + */ + val EXTENSIONS: Set = setOf( "odt", "ods", @@ -45,27 +78,37 @@ object SupportedDocumentTypes { "otg", "doc", "docx", + "xls", "xlsx", + "ppt", + "pptx", "pdf", "txt", "csv", "zip", ) + /** Whether [CoreLoader] is expected to render this - see [CORE_MIME_PREFIXES]. */ + fun isRenderedByCore(mimeType: String?): Boolean = matches(mimeType, CORE_MIME_PREFIXES) + + /** Whether the folder browser should offer this at all. */ fun isSupported(mimeType: String?, filename: String?): Boolean { - // providers regularly hand out application/octet-stream for anything they do not know, - // so a recognised extension has to be able to win on its own + // a recognised extension has to be able to win on its own, or every octet-stream is lost val extension = MimeTypeResolver.parseExtension(filename)?.lowercase() if (extension != null && extension in EXTENSIONS) { return true } + return isRenderedByCore(mimeType) || matches(mimeType, RAW_MIME_PREFIXES) + } + + private fun matches(mimeType: String?, prefixes: List): Boolean { if (mimeType == null) { return false } val normalized = mimeType.lowercase() - return MIME_PREFIXES.any { normalized.startsWith(it) } + return prefixes.any { normalized.startsWith(it) } } } diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt index ed774bbe0b88..bac352217085 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/DocumentFragment.kt @@ -381,33 +381,11 @@ class DocumentFragment : Fragment(), LoaderService.LoaderListener { * that finished loading is not one of those things. */ private fun prepareActions(result: FileLoader.Result) { - val loaderType = result.loaderType - val fileType = result.options.fileType - - var isEditEnabled = false - var isDarkModeSupported = true - - if (loaderType == FileLoader.LoaderType.CORE) { - isEditEnabled = true - - // Edit is currently broken for ODS spreadsheets - // See: https://github.com/opendocument-app/OpenDocument.droid/issues/442 - if ( - fileType != null && - fileType.startsWith("application/vnd.oasis.opendocument.spreadsheet") - ) { - isEditEnabled = false - } - - // Edit is not supported for PDF documents - if (fileType != null && fileType.startsWith("application/pdf")) { - isEditEnabled = false - isDarkModeSupported = false - } - } - + // whether editing is on offer is the core's answer, not a list of formats kept here: it + // knows which of the documents it renders it can also write back, which is why neither the + // legacy binary formats nor the spreadsheets of issue #442 need naming val unfolding = ArrayList() - if (isEditEnabled) { + if (result.isEditable) { unfolding.add( DocumentActions.Action( DocumentActions.ACTION_EDIT, @@ -468,7 +446,7 @@ class DocumentFragment : Fragment(), LoaderService.LoaderListener { unfolding, ) - pageView?.toggleDarkMode(isDarkModeSupported) + pageView?.disableDarkening() } /** Takes the buttons away while the document has the screen to itself. */ diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt b/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt index d30765784db4..74ab4ea391e6 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/MainActivity.kt @@ -504,7 +504,7 @@ class MainActivity : AppCompatActivity() { analyticsManager.report("menu_print") documentFragment?.pageView?.let { pageView -> - pageView.toggleDarkMode(false) + pageView.disableDarkening() printingManager.print(this, pageView) } diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt index d39cd49ebbd4..1453e9bc0502 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/PageView.kt @@ -160,11 +160,13 @@ constructor(context: Context, attributeSet: AttributeSet?) : * moved to Material3.DayNight - at targetSdk 33 and up the webview only darkens when the app * theme reports itself as dark, so under the old AppCompat.Light theme none of this ever fired. * - * [isDarkModeSupported] is kept as the hook a user facing "dark documents" setting would use; - * DocumentFragment already passes false for pdfs. + * There is no parameter to this any more. It used to take an `isDarkModeSupported` that nothing + * in here ever read - both callers reached the same three lines - while [DocumentFragment] went + * to the trouble of working out that a pdf is not worth inverting. A user facing "dark + * documents" setting can grow the argument back when there is something on the other end of it. */ @Suppress("DEPRECATION") // setForceDarkAllowed and setForceDark are the pre-webkit-1.6 api - fun toggleDarkMode(isDarkModeSupported: Boolean) { + fun disableDarkening() { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { isForceDarkAllowed = false } diff --git a/app/src/test/java/app/opendocument/droid/background/CoreLoaderTest.kt b/app/src/test/java/app/opendocument/droid/background/CoreLoaderTest.kt new file mode 100644 index 000000000000..2b1ef296cdae --- /dev/null +++ b/app/src/test/java/app/opendocument/droid/background/CoreLoaderTest.kt @@ -0,0 +1,92 @@ +package app.opendocument.droid.background + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +/** + * What the core is expected to render. odrcore v6 keeps reference html output for every format + * asserted here, so these are the types a load is expected to succeed for. + */ +class CoreLoaderTest { + + 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) + } + + @Test + fun opendocumentIsSupported() { + assertTrue(isSupported("application/vnd.oasis.opendocument.text")) + assertTrue(isSupported("application/vnd.oasis.opendocument.spreadsheet")) + assertTrue(isSupported("application/vnd.oasis.opendocument.presentation")) + assertTrue(isSupported("application/vnd.oasis.opendocument.graphics")) + assertTrue(isSupported("application/vnd.oasis.opendocument.text-master")) + // the spelling some providers use + assertTrue(isSupported("application/x-vnd.oasis.opendocument.text")) + } + + @Test + fun ooxmlIsSupported() { + assertTrue( + isSupported("application/vnd.openxmlformats-officedocument.wordprocessingml.document") + ) + assertTrue(isSupported("application/vnd.openxmlformats-officedocument.spreadsheetml.sheet")) + assertTrue( + isSupported("application/vnd.openxmlformats-officedocument.presentationml.presentation") + ) + assertTrue( + isSupported("application/vnd.openxmlformats-officedocument.wordprocessingml.template") + ) + } + + @Test + fun theMacroEnabledOoxmlVariantsAreSupported() { + // spelled outside the openxmlformats family, which is why the legacy types below are + // matched by their prefix and vnd.ms-word is listed at all + assertTrue(isSupported("application/vnd.ms-word.document.macroEnabled.12")) + assertTrue(isSupported("application/vnd.ms-excel.sheet.macroEnabled.12")) + assertTrue(isSupported("application/vnd.ms-powerpoint.presentation.macroEnabled.12")) + } + + @Test + fun theLegacyBinaryFormatsAreSupported() { + assertTrue(isSupported("application/msword")) + assertTrue(isSupported("application/vnd.ms-excel")) + assertTrue(isSupported("application/vnd.ms-powerpoint")) + } + + @Test + fun pdfIsSupported() { + assertTrue(isSupported("application/pdf")) + } + + @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")) + } + + @Test + fun whatTheCoreCannotOpenIsNotClaimed() { + assertFalse(isSupported("application/vnd.wordperfect")) + assertFalse(isSupported("text/rtf")) + assertFalse(isSupported("application/vnd.apple.pages")) + assertFalse(isSupported("application/octet-stream")) + assertFalse(isSupported(null)) + } +} diff --git a/app/src/test/java/app/opendocument/droid/background/OnlineLoaderTest.kt b/app/src/test/java/app/opendocument/droid/background/OnlineLoaderTest.kt index 20ce05bb644d..230764ee872f 100644 --- a/app/src/test/java/app/opendocument/droid/background/OnlineLoaderTest.kt +++ b/app/src/test/java/app/opendocument/droid/background/OnlineLoaderTest.kt @@ -10,7 +10,7 @@ class OnlineLoaderTest { @Before fun setUp() { - onlineLoader = OnlineLoader(null, CoreLoader(null, true)) + onlineLoader = OnlineLoader(null, CoreLoader(null)) } private fun options(fileType: String): FileLoader.Options { @@ -84,6 +84,6 @@ class OnlineLoaderTest { Assert.assertFalse(isConvertible("text/plain")) Assert.assertFalse(isConvertible("image/png")) Assert.assertFalse(isConvertible("application/zip")) - Assert.assertFalse(isConvertible("application/vnd.ms-excel.sheet.macroEnabled.12")) + Assert.assertFalse(isConvertible("application/vnd.apple.pages")) } } diff --git a/app/src/test/java/app/opendocument/droid/background/RawLoaderTest.kt b/app/src/test/java/app/opendocument/droid/background/RawLoaderTest.kt index c682c7a8ad86..e49e0244174b 100644 --- a/app/src/test/java/app/opendocument/droid/background/RawLoaderTest.kt +++ b/app/src/test/java/app/opendocument/droid/background/RawLoaderTest.kt @@ -10,7 +10,7 @@ class RawLoaderTest { @Before fun setUp() { - rawLoader = RawLoader(null, CoreLoader(null, true)) + rawLoader = RawLoader(null, CoreLoader(null)) } private fun isSupported(fileType: String): Boolean { diff --git a/app/src/test/java/app/opendocument/droid/background/SupportedDocumentTypesTest.kt b/app/src/test/java/app/opendocument/droid/background/SupportedDocumentTypesTest.kt index 5d089641280e..224856f7480f 100644 --- a/app/src/test/java/app/opendocument/droid/background/SupportedDocumentTypesTest.kt +++ b/app/src/test/java/app/opendocument/droid/background/SupportedDocumentTypesTest.kt @@ -31,16 +31,19 @@ class SupportedDocumentTypesTest { } @Test - fun presentationsTheCoreCannotOpenAreNotSupported() { - // pptx sits in the same ooxml family as docx and xlsx, but neither the STRICT_CATCH filter - // nor CoreLoader takes it - listing one would promise an open that ends in the upload flow - assertFalse( + fun presentationsAndTheLegacyBinaryFormatsAreSupported() { + assertTrue( supported( "application/vnd.openxmlformats-officedocument.presentationml.presentation", "deck.pptx", ) ) - assertFalse(supported("application/vnd.ms-powerpoint", "deck.ppt")) + assertTrue(supported("application/vnd.ms-powerpoint", "deck.ppt")) + assertTrue(supported("application/vnd.ms-excel", "budget.xls")) + // the octet-stream a provider volunteers for them, caught by the extension instead + assertTrue(supported("application/octet-stream", "deck.pptx")) + assertTrue(supported("application/octet-stream", "deck.ppt")) + assertTrue(supported("application/octet-stream", "budget.xls")) } @Test