diff --git a/CLAUDE.md b/CLAUDE.md index 897ac0ac0f12..0a04d4a319f7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -145,6 +145,52 @@ 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, 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. + +That walk has a blind spot, and a second test covers it. The table matches by *prefix*, so +it also claims the ooxml templates, slideshows and macro-enabled variants - spellings +odrcore never produces, because `mimetypeByFileType` names one canonical mime per format. An +intent-filter, on the other hand, matches a mime type exactly, so every one of those has to +be spelled out in the manifest. `theOoxmlVariantsReachUsToo` lists them and asserts both +sides say yes. + +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 item 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`. + ### Language Kotlin; support is built into AGP 9, no kotlin plugin is applied. The only java left is 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..04b42fda0877 --- /dev/null +++ b/app/src/androidTest/java/app/opendocument/droid/test/SupportedFormatsTest.kt @@ -0,0 +1,258 @@ +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 named after the core's own table has to be one odrcore still recognises. */ + @Test + fun theExtensionFallbackNamesFormatsTheCoreKnows() { + for (extension in SupportedDocumentTypes.CORE_EXTENSIONS) { + Assert.assertNotEquals( + "odrcore does not know the extension .$extension", + FileType.UNKNOWN, + Odr.fileTypeByFileExtension(extension), + ) + } + } + + /** + * The other half of the fallback, and why it is kept apart: odrcore's extension table names one + * suffix per format, so it answers UNKNOWN for the ooxml templates, macro enabled documents and + * slideshows even though it renders all of them - a `.dotx` is the same wordprocessing package + * as a `.docx`, and libmagic identifies it by content once the file is in the cache. + * + * Kept as an assertion rather than a comment so that a future core which does learn them turns + * this red instead of leaving the split in place for no reason. + */ + @Test + fun theOoxmlVariantExtensionsAreOnesTheCoreDoesNotName() { + for (extension in SupportedDocumentTypes.OOXML_VARIANT_EXTENSIONS) { + Assert.assertEquals( + "odrcore now knows .$extension - move it to CORE_EXTENSIONS", + 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"), + ) + } + } + + /** + * A file manager that sends `ACTION_VIEW` with no mime type at all, which is the case the + * `pathPattern` filters exist for - and the one an `application/octet-stream` check cannot + * stand in for, because that mime type is claimed outright and so matches whatever the patterns + * happen to say. + * + * This is what pins the pattern list against [SupportedDocumentTypes.EXTENSIONS]: leaving an + * extension out of the manifest was invisible until now, and `.ott` had been missing from it + * for as long as the table has claimed it. + */ + @Test + fun aFileWithNoMimeTypeReachesUsByItsExtension() { + for (extension in SupportedDocumentTypes.EXTENSIONS) { + Assert.assertTrue( + "the manifest has no pathPattern for .$extension", + resolvesToUs(null, "document.$extension"), + ) + } + } + + /** And the same route stays shut for what the app does not claim. */ + @Test + fun aFileWithNoMimeTypeAndAnUnrelatedExtensionReachesNobody() { + for (extension in listOf("vcf", "mp3", "mp4", "bin", "apk")) { + Assert.assertFalse( + "the manifest offers for .$extension", + resolvesToUs(null, "document.$extension"), + ) + } + } + + /** + * The spellings the mime *prefixes* carry along - the ooxml templates, slideshows and macro + * enabled variants - which [theManifestClaimsExactlyWhatTheAppClaims] cannot reach: odrcore + * names one canonical mime type per format, so its [FileType] walk never produces any of these. + * + * They still have to hold, because an intent-filter matches a mime type exactly. Without them + * spelled out in the manifest a .dotx opened from the folder browser while the same file from + * another app did not offer OpenDocument at all. + */ + @Test + fun theOoxmlVariantsReachUsToo() { + val variants = + listOf( + "application/vnd.openxmlformats-officedocument.wordprocessingml.template", + "application/vnd.openxmlformats-officedocument.spreadsheetml.template", + "application/vnd.openxmlformats-officedocument.presentationml.template", + "application/vnd.openxmlformats-officedocument.presentationml.slideshow", + "application/vnd.ms-word.document.macroEnabled.12", + "application/vnd.ms-word.template.macroEnabled.12", + "application/vnd.ms-excel.sheet.macroEnabled.12", + "application/vnd.ms-excel.template.macroEnabled.12", + "application/vnd.ms-powerpoint.presentation.macroEnabled.12", + "application/vnd.ms-powerpoint.template.macroEnabled.12", + "application/vnd.ms-powerpoint.slideshow.macroEnabled.12", + ) + + for (mimeType in variants) { + Assert.assertTrue( + "the app does not offer for $mimeType", + SupportedDocumentTypes.isSupported(mimeType, null), + ) + Assert.assertTrue( + "the manifest does not offer for $mimeType", + resolvesToUs(mimeType, "document"), + ) + } + } + + /** 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. + * + * A null [mimeType] is the no-type intent a file manager sends, which only the `pathPattern` + * filters can answer; `setData` rather than `setDataAndType`, because passing a null type to + * the latter clears the data along with it. + */ + private fun resolvesToUs(mimeType: String?, filename: String): Boolean { + val context = InstrumentationRegistry.getInstrumentation().targetContext + + val uri = Uri.parse("content://app.opendocument.test/$filename") + + val intent = + Intent(Intent.ACTION_VIEW).apply { + addCategory(Intent.CATEGORY_DEFAULT) + if (mimeType == null) setData(uri) else setDataAndType(uri, 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 f85dfdccc660..3a560a3dcf0d 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -234,7 +234,7 @@ android:label="@string/app_title" android:targetActivity="app.opendocument.droid.ui.activity.MainActivity" tools:ignore="AppLinkUrlError"> - + @@ -253,7 +253,28 @@ + + + + + + + + + + + + + + + @@ -297,6 +318,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -311,6 +360,27 @@ + + + + + + + + + + + + + + + + + + + + + @@ -325,6 +395,83 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -385,6 +532,34 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -399,6 +574,27 @@ + + + + + + + + + + + + + + + + + + + + + @@ -413,6 +609,83 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/app/src/main/java/app/opendocument/droid/background/CatchAllSetting.kt b/app/src/main/java/app/opendocument/droid/background/CatchAllSetting.kt new file mode 100644 index 000000000000..d366d02b443f --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/CatchAllSetting.kt @@ -0,0 +1,80 @@ +package app.opendocument.droid.background + +import android.content.ComponentName +import android.content.Context +import android.content.SharedPreferences +import android.content.pm.PackageManager + +/** + * Whether the app offers itself for every file type, or only for the document types it actually + * supports. + * + * Backed by the two activity aliases in the manifest: exactly one of them is enabled at a time, and + * which one decides how broad the intent filter the system sees is. + */ +object CatchAllSetting { + + private const val PREF_CATCH_ALL_ENABLED = "catch_all_enabled" + + // these keep the historical at.tomtasche.reader names on purpose: the component name is + // what the OS persists when a user picks "always open .odt with this app", and what + // setComponentEnabledSetting stores the toggle against. renaming them would drop those + // defaults for every existing install, so the manifest declares the aliases under the old + // names too. + private const val CATCH_ALL_COMPONENT = "at.tomtasche.reader.ui.activity.MainActivity.CATCH_ALL" + private const val STRICT_CATCH_COMPONENT = + "at.tomtasche.reader.ui.activity.MainActivity.STRICT_CATCH" + + /** + * Puts the aliases back in the state the stored setting asks for. + * + * Has to run on every launch, not just when the landing screen is shown: users upgrading from a + * version that shipped different alias defaults are only corrected here, and the app is + * regularly started straight into a document by an external intent. + */ + fun applyOnLaunch(context: Context): Boolean { + val enabled = isEnabled(context) + + apply(context, enabled) + + return enabled + } + + fun isEnabled(context: Context): Boolean = + // catch-all is only active if the user explicitly opted in. new installs and existing + // users who never touched the switch default to STRICT_CATCH, so we no longer volunteer + // to open unrelated file types like contacts (issue #477). + preferences(context).getBoolean(PREF_CATCH_ALL_ENABLED, false) + + fun setEnabled(context: Context, enabled: Boolean) { + preferences(context).edit().putBoolean(PREF_CATCH_ALL_ENABLED, enabled).apply() + + apply(context, enabled) + } + + private fun apply(context: Context, enabled: Boolean) { + toggleComponent(context, CATCH_ALL_COMPONENT, enabled) + toggleComponent(context, STRICT_CATCH_COMPONENT, !enabled) + } + + private fun toggleComponent(context: Context, className: String, enabled: Boolean) { + val newState = + if (enabled) PackageManager.COMPONENT_ENABLED_STATE_ENABLED + else PackageManager.COMPONENT_ENABLED_STATE_DISABLED + + context.packageManager.setComponentEnabledSetting( + ComponentName(context, className), + newState, + PackageManager.DONT_KILL_APP, + ) + } + + /** + * The preferences android.preference.PreferenceManager used to hand out. That class is + * deprecated and its androidx replacement lives in a whole preference-ui library we do not + * otherwise need, so the default file is opened by name instead - keeping the existing + * catch-all setting of users who upgrade. + */ + private fun preferences(context: Context): SharedPreferences = + context.getSharedPreferences(context.packageName + "_preferences", Context.MODE_PRIVATE) +} 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..7498c5429d0c 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,18 +42,31 @@ 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) { - - private var server: HttpServer? = null - private var httpThread: Thread? = null +class CoreLoader(context: Context?) : FileLoader(context, LoaderType.CORE) { private var document: Document? = null private var lastInputPath: String? = null private var lastCachePath: String? = null - // the port the server actually got, which is not always the preferred one. see bind() - private var serverPort = PREFERRED_SERVER_PORT + /** + * 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 shared server and the port it actually got, which is not always the preferred one. + // see bind() and startSharedServer() + private val server: HttpServer? + get() = sharedServer + + private val serverPort: Int + get() = sharedServerPort override fun initialize( listener: FileLoaderListener, @@ -68,86 +80,22 @@ class CoreLoader(context: Context?, private val doOoxml: Boolean) : // right after new CoreLoader() anyway. initializeCore(context) - val server = HttpServer() - this.server = server - - serverPort = bind(server, crashManager) - - httpThread = - Thread { - try { - server.listen() - } catch (e: Throwable) { - crashManager.log(e) - } - } - .also { it.start() } + startSharedServer(crashManager) super.initialize(listener, mainHandler, backgroundHandler, analyticsManager, crashManager) } /** - * Binds [server], preferring [PREFERRED_SERVER_PORT], and returns the port it got. + * What the core renders itself, out of [SupportedDocumentTypes] so the folder browser and this + * cannot disagree about it. * - * The preferred port is occupied more often than it looks. Both flavors hardcode it, so lite - * and pro collide whenever they run on one device - the instrumented tests do exactly that, one - * flavor after the other - and a port does not come free the moment its owner goes away: the - * sockets the webview opened linger in TIME_WAIT for a minute. That second half is what - * [HttpServer.Options.reuseAddress] covers, on by default since the core started setting - * SO_REUSEADDR rather than cpp-httplib's SO_REUSEPORT; a live server of the other flavor still - * holds the port for real, hence the fallback to any free one. - * - * A failed bind used to be invisible: the old `listen(host, port)` returned void either way, so - * an occupied port left the app with no server and nothing to show for it - every document then - * failed with ERR_CONNECTION_REFUSED behind chrome's own error page. [HttpServer.bind] throws - * instead, and reports back the port that a request has to be addressed to. + * 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. */ - private fun bind(server: HttpServer, crashManager: CrashManager): Int { - // a preference of ANY_PORT is a request for an arbitrary free port, which is what the - // fallback below asks for anyway - there is nothing to try first - if (PREFERRED_SERVER_PORT != ANY_PORT) { - try { - return server.bind(SERVER_BIND_ADDRESS, PREFERRED_SERVER_PORT) - } catch (e: Throwable) { - crashManager.log( - IOException( - "core server cannot bind $SERVER_BIND_ADDRESS:$PREFERRED_SERVER_PORT", - e, - ) - ) - } - } - - try { - return server.bind(SERVER_BIND_ADDRESS, ANY_PORT) - } catch (e: Throwable) { - crashManager.log(IOException("core server cannot bind any port", e)) - } - - // nothing is bound at this point, so [listen] will fail as well and this port only decides - // which url the documents that follow are refused at. It must not be ANY_PORT, which would - // address every one of them to :0. - return if (PREFERRED_SERVER_PORT != ANY_PORT) PREFERRED_SERVER_PORT - 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") - )) - } + override fun isSupported(options: Options): Boolean = + SupportedDocumentTypes.isRenderedByCore(options.fileType) override fun loadSync(options: Options) { val result = Result(type, options) @@ -191,6 +139,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 +182,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() + } } } @@ -299,24 +257,14 @@ class CoreLoader(context: Context?, private val doOoxml: Boolean) : return outputFile } + /** + * Drops what this loader published and leaves the server itself running - see + * [startSharedServer] for why it is never stopped. + */ override fun close() { super.close() - val server = this.server - if (server != null) { - server.stop() - httpThread?.let { - try { - it.join(1000) - } catch (e: InterruptedException) { - crashManager.log(e) - } - } - httpThread = null - - server.close() - this.server = null - } + server?.clear() closeDocument() } @@ -332,6 +280,109 @@ class CoreLoader(context: Context?, private val doOoxml: Boolean) : companion object { private const val TAG = "CoreLoader" + /** + * The one http server of the process, started on the first [initialize] and never stopped. + * + * Never stopped on purpose. Both of the ways odrcore lets a server go are unsafe while the + * thread that called [HttpServer.listen] is still inside it, and that thread only comes + * back some time after `stop()` asks it to: + * + * - `HttpServer.close()` destroys the native server there and then, which is a use after + * free that surfaces as a SIGSEGV in `httplib::Server::listen_internal`. Dropping the + * reference instead is no better - odrcore's `NativeResource` frees the handle from a + * reference queue too. + * - `stop()` on its own closes the listening socket while the accept loop is still using + * it, and closes it a second time on the way out. In between, the fd number is handed to + * whatever opens a file next; on android fdsan catches that and aborts the process + * ("attempted to close file descriptor N, actually owned by ZipArchive"), and on a + * platform without fdsan it would quietly close someone else's file. + * + * Both are teardown only, so the loader does not tear down: [close] calls + * [HttpServer.clear] to drop the documents it published and leaves the socket listening. + * The process exit reclaims it, which is what was really doing the work before - nothing + * here ever outlived its process. Reported upstream as + * opendocument-app/OpenDocument.core#631. + * + * A second consequence is that the port is bound once rather than once per service + * lifetime, so [bind]'s fallback stops being reached by our own teardown. + */ + private var sharedServer: HttpServer? = null + + private var sharedServerPort = PREFERRED_SERVER_PORT + + /** + * Binds [server], preferring [PREFERRED_SERVER_PORT], and returns the port it got. + * + * The preferred port is occupied more often than it looks. Both flavors hardcode it, so + * lite and pro collide whenever they run on one device - the instrumented tests do exactly + * that, one flavor after the other - and a port does not come free the moment its owner + * goes away: the sockets the webview opened linger in TIME_WAIT for a minute. That second + * half is what [HttpServer.Options.reuseAddress] covers, on by default since the core + * started setting SO_REUSEADDR rather than cpp-httplib's SO_REUSEPORT; a live server of the + * other flavor still holds the port for real, hence the fallback to any free one. + * + * A failed bind used to be invisible: the old `listen(host, port)` returned void either + * way, so an occupied port left the app with no server and nothing to show for it - every + * document then failed with ERR_CONNECTION_REFUSED behind chrome's own error page. + * [HttpServer.bind] throws instead, and reports back the port that a request has to be + * addressed to. + */ + private fun bind(server: HttpServer, crashManager: CrashManager): Int { + // a preference of ANY_PORT is a request for an arbitrary free port, which is what the + // fallback below asks for anyway - there is nothing to try first + if (PREFERRED_SERVER_PORT != ANY_PORT) { + try { + return server.bind(SERVER_BIND_ADDRESS, PREFERRED_SERVER_PORT) + } catch (e: Throwable) { + crashManager.log( + IOException( + "core server cannot bind $SERVER_BIND_ADDRESS:$PREFERRED_SERVER_PORT", + e, + ) + ) + } + } + + try { + return server.bind(SERVER_BIND_ADDRESS, ANY_PORT) + } catch (e: Throwable) { + crashManager.log(IOException("core server cannot bind any port", e)) + } + + // nothing is bound at this point, so [listen] will fail as well and this port only + // decides which url the documents that follow are refused at. It must not be + // ANY_PORT, which would address every one of them to :0. + return if (PREFERRED_SERVER_PORT != ANY_PORT) PREFERRED_SERVER_PORT + else FALLBACK_SERVER_PORT + } + + /** Starts [sharedServer] if this is the first loader to ask. */ + @Synchronized + private fun startSharedServer(crashManager: CrashManager) { + if (sharedServer != null) { + return + } + + val server = HttpServer() + + sharedServer = server + sharedServerPort = bind(server, crashManager) + + Thread { + try { + server.listen() + } catch (e: Throwable) { + crashManager.log(e) + } + } + .also { + // daemon so it cannot keep the process alive: it is never asked to stop + it.isDaemon = true + it.name = "odr-http-server" + it.start() + } + } + /** The loopback address the server binds to. */ private const val SERVER_BIND_ADDRESS = "127.0.0.1" 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 7ff1c6ac46b1..b1cbdfe72aee 100644 --- a/app/src/main/java/app/opendocument/droid/background/OnlineLoader.kt +++ b/app/src/main/java/app/opendocument/droid/background/OnlineLoader.kt @@ -142,8 +142,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 new file mode 100644 index 000000000000..1c40505a2a0e --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/SupportedDocumentTypes.kt @@ -0,0 +1,153 @@ +package app.opendocument.droid.background + +/** + * The one list of what the app claims to open, for both of the places that have to guess. + * + * 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. + * + * 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. + * + * 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 { + + /** + * 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", + "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", + ) + + /** + * 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 odrcore's own `fileTypeByFileExtension` names, one suffix per format. + * + * Public so `SupportedFormatsTest` can hold each of them against that table - the assertion + * that this list stays a subset of what the core recognises. + */ + val CORE_EXTENSIONS: Set = + setOf( + "odt", + "ods", + "odp", + "odg", + "ott", + "ots", + "otp", + "otg", + "doc", + "docx", + "xls", + "xlsx", + "ppt", + "pptx", + "pdf", + "txt", + "csv", + "zip", + ) + + /** + * The ooxml templates, macro enabled documents and slideshows, which odrcore's extension table + * leaves out although it renders every one of them - they are the same packages as the `x` + * suffixed ones, and libmagic identifies them by content once the file is in the cache. + * + * Kept apart from [CORE_EXTENSIONS] so that a test can say which of the two invariants applies + * to which half, rather than quietly dropping the check for all of them. + */ + val OOXML_VARIANT_EXTENSIONS: Set = + setOf( + "docm", + "dotx", + "dotm", + "xlsm", + "xltx", + "xltm", + "pptm", + "potx", + "potm", + "ppsx", + "ppsm", + ) + + /** + * The extensions to fall back on, spelling out the same set once more because providers + * regularly volunteer nothing better than `application/octet-stream` - and because a file + * manager that sends `ACTION_VIEW` with no mime type at all leaves the name as the only thing + * anyone can go on. That second case is what the `pathPattern` filters in AndroidManifest.xml + * cover, and they have to list exactly this set. + * + * The ooxml variants are here for the same reason [CORE_MIME_PREFIXES] matches by prefix: a + * `.dotx` is the same wordprocessing package as a `.docx`. Note that odrcore's own + * `fileTypeByFileExtension` does *not* know them - its extension table names one suffix per + * format - which is why [SupportedFormatsTest] holds only [CORE_EXTENSIONS] against it. + * + * Public so `SupportedFormatsTest` can walk it: it holds every one of these against the + * manifest. + */ + val EXTENSIONS: Set = CORE_EXTENSIONS + OOXML_VARIANT_EXTENSIONS + + /** 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 { + // 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 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 5fe7aa31c5bc..a5478df9c63e 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 @@ -254,7 +254,7 @@ class DocumentFragment : Fragment(), LoaderService.LoaderListener, MenuProvider menu.findItem(R.id.menu_print).isVisible = true // the other menu items are dynamically enabled based on the loaded document - state.lastResult?.let { prepareMenu(it.loaderType, it.options.fileType) } + state.lastResult?.let { prepareMenu(it) } } override fun onMenuItemSelected(menuItem: MenuItem): Boolean { @@ -400,31 +400,15 @@ class DocumentFragment : Fragment(), LoaderService.LoaderListener, MenuProvider menu.findItem(R.id.menu_tts).isVisible = enabled } - private fun prepareMenu(loaderType: FileLoader.LoaderType, fileType: String?) { - var isEditEnabled = false - var isDarkModeSupported = true + private fun prepareMenu(result: FileLoader.Result) { + // 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 + toggleDocumentMenu(true, result.isEditable) - 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 - } - } - - toggleDocumentMenu(true, isEditEnabled) - pageView?.toggleDarkMode(isDarkModeSupported) + // pdf is the one thing worth leaving alone: inverting a page of scanned paper helps nobody + val fileType = result.options.fileType + pageView?.toggleDarkMode(fileType?.startsWith("application/pdf") != true) } private fun requestInAppRating(activity: Activity) { 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 6e11765a9714..c453e9117a93 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 @@ -3,11 +3,8 @@ package app.opendocument.droid.ui.activity import android.app.Activity import android.content.ActivityNotFoundException import android.content.ComponentName -import android.content.Context import android.content.Intent import android.content.ServiceConnection -import android.content.SharedPreferences -import android.content.pm.PackageManager import android.content.pm.ResolveInfo import android.content.res.Configuration import android.net.Uri @@ -33,6 +30,7 @@ import androidx.core.view.WindowInsetsCompat import androidx.core.view.WindowInsetsControllerCompat import androidx.fragment.app.DialogFragment import app.opendocument.droid.R +import app.opendocument.droid.background.CatchAllSetting import app.opendocument.droid.background.LoaderService import app.opendocument.droid.background.LoaderServiceQueue import app.opendocument.droid.background.PersistedUriPermissions @@ -282,34 +280,14 @@ class MainActivity : AppCompatActivity(), MenuProvider { } private fun initializeCatchAllSwitch() { - // these keep the historical at.tomtasche.reader names on purpose: the component - // name is what the OS persists when a user picks "always open .odt with this - // app", and what setComponentEnabledSetting below stores the toggle against. - // renaming them would drop those defaults for every existing install, so the - // manifest declares the aliases under the old names too. - val catchAllComponent = - ComponentName(this, "at.tomtasche.reader.ui.activity.MainActivity.CATCH_ALL") - val strictCatchComponent = - ComponentName(this, "at.tomtasche.reader.ui.activity.MainActivity.STRICT_CATCH") - - val preferences = getDefaultSharedPreferences() - - // catch-all is only active if the user explicitly opted in. new installs and - // existing users who never touched the switch default to STRICT_CATCH, so we - // no longer volunteer to open unrelated file types like contacts (issue #477). - val isCatchAllEnabled = preferences.getBoolean(PREF_CATCH_ALL_ENABLED, false) - - // retoggle components for users upgrading to latest version of app - toggleComponent(catchAllComponent, isCatchAllEnabled) - toggleComponent(strictCatchComponent, !isCatchAllEnabled) + // retoggles the aliases for users upgrading from a version that shipped different + // defaults, and answers with what the stored setting says + val isCatchAllEnabled = CatchAllSetting.applyOnLaunch(this) val catchAllSwitch = findViewById(R.id.landing_catch_all) catchAllSwitch.setOnCheckedChangeListener { _, isChecked -> - preferences.edit().putBoolean(PREF_CATCH_ALL_ENABLED, isChecked).apply() - - toggleComponent(catchAllComponent, isChecked) - toggleComponent(strictCatchComponent, !isChecked) + CatchAllSetting.setEnabled(this, isChecked) } catchAllSwitch.isChecked = isCatchAllEnabled @@ -319,22 +297,6 @@ class MainActivity : AppCompatActivity(), MenuProvider { ) } - /** - * The preferences android.preference.PreferenceManager used to hand out. That class is - * deprecated and its androidx replacement lives in a whole preference-ui library we do not - * otherwise need, so the default file is opened by name instead - keeping the existing - * catch-all setting of users who upgrade. - */ - private fun getDefaultSharedPreferences(): SharedPreferences = - getSharedPreferences(packageName + "_preferences", Context.MODE_PRIVATE) - - private fun toggleComponent(component: ComponentName, enabled: Boolean) { - val newState = - if (enabled) PackageManager.COMPONENT_ENABLED_STATE_ENABLED - else PackageManager.COMPONENT_ENABLED_STATE_DISABLED - packageManager.setComponentEnabledSetting(component, newState, PackageManager.DONT_KILL_APP) - } - override fun onSaveInstanceState(outState: Bundle) { super.onSaveInstanceState(outState) @@ -787,7 +749,6 @@ class MainActivity : AppCompatActivity(), MenuProvider { const val SAVED_KEY_OPENED_EXTERNALLY = "OPENED_EXTERNALLY" const val GOOGLE_REQUEST_CODE = 1993 const val DOCUMENT_FRAGMENT_TAG = "document_fragment" - const val PREF_CATCH_ALL_ENABLED = "catch_all_enabled" // taken from: https://stackoverflow.com/a/36829889/198996 private fun isTesting(): Boolean = 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 new file mode 100644 index 000000000000..224856f7480f --- /dev/null +++ b/app/src/test/java/app/opendocument/droid/background/SupportedDocumentTypesTest.kt @@ -0,0 +1,87 @@ +package app.opendocument.droid.background + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class SupportedDocumentTypesTest { + + @Test + fun opendocumentTypesAreSupported() { + assertTrue(supported("application/vnd.oasis.opendocument.text", "report.odt")) + assertTrue(supported("application/vnd.oasis.opendocument.spreadsheet", "budget.ods")) + assertTrue(supported("application/vnd.oasis.opendocument.presentation", "deck.odp")) + assertTrue(supported("application/vnd.oasis.opendocument.graphics", "drawing.odg")) + assertTrue(supported("application/vnd.oasis.opendocument.text-template", "letter.ott")) + } + + @Test + fun officeAndPdfTypesAreSupported() { + assertTrue( + supported( + "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + "notes.docx", + ) + ) + assertTrue(supported("application/msword", "old.doc")) + assertTrue(supported("application/pdf", "manual.pdf")) + assertTrue(supported("text/plain", "readme.txt")) + assertTrue(supported("text/csv", "rows.csv")) + assertTrue(supported("image/png", "scan.png")) + } + + @Test + fun presentationsAndTheLegacyBinaryFormatsAreSupported() { + assertTrue( + supported( + "application/vnd.openxmlformats-officedocument.presentationml.presentation", + "deck.pptx", + ) + ) + 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 + fun aKnownExtensionWinsOverAnUnhelpfulMimeType() { + // this is the common case: providers fall back to octet-stream for anything they do not + // have a mapping for, which includes most opendocument files + assertTrue(supported("application/octet-stream", "report.odt")) + assertTrue(supported("application/octet-stream", "budget.ods")) + } + + @Test + fun aKnownMimeTypeWinsOverAMissingExtension() { + assertTrue(supported("application/pdf", "scan-without-extension")) + } + + @Test + fun unrelatedTypesAreNotSupported() { + // the reason catch-all defaults to off - see issue #477 + assertFalse(supported("text/vcard", "contact.vcf")) + assertFalse(supported("text/x-vcard", "contact.vcf")) + assertFalse(supported("audio/mpeg", "song.mp3")) + assertFalse(supported("video/mp4", "clip.mp4")) + assertFalse(supported("application/octet-stream", "firmware.bin")) + } + + @Test + fun nothingKnownAtAllIsNotSupported() { + assertFalse(supported(null, null)) + assertFalse(supported(null, "mystery")) + assertFalse(supported("application/octet-stream", null)) + } + + @Test + fun matchingIsCaseInsensitive() { + assertTrue(supported("APPLICATION/PDF", "MANUAL.PDF")) + assertTrue(supported(null, "Report.ODT")) + } + + private fun supported(mimeType: String?, filename: String?) = + SupportedDocumentTypes.isSupported(mimeType, filename) +}