From 509ca7621d55feedb013f7ad2baff2f143ba25f7 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 26 Jul 2026 22:12:02 +0200 Subject: [PATCH] Browse the folders you have granted access to "See the files on my phone" has no permission behind it any more. READ_EXTERNAL_STORAGE has not reached documents since scoped storage, READ_MEDIA_* covers only images, video and audio, and Play restricts MANAGE_EXTERNAL_STORAGE to file managers and backup apps - a document viewer asking for it gets the listing rejected. The manifest still declares nothing but INTERNET, and it stays that way. ACTION_OPEN_DOCUMENT_TREE does the same job with the user's consent instead: they pick a folder once, we persist a read only grant, and the landing screen lists and navigates it from then on. Read only - no write flag is ever requested. Listing goes through DocumentsContract rather than androidx's DocumentFile, which issues a fresh query per child the moment a name or type is read. One cursor covers a whole folder. SupportedDocumentTypes decides what is worth showing. It has to guess: MetadataLoader only knows what is supported after libmagic has run over the cached file, and a folder listing has nothing but a name and whatever mime type the provider volunteered - which is regularly application/octet-stream, hence the extension fallback. It mirrors the STRICT_CATCH intent filter and nothing keeps the two in step, so both sides now say so, as does CLAUDE.md. prune() learns that a document below a granted tree is covered by that tree's grant, which it can tell from the uri alone - so nothing has to record where a document was opened from. The tree grant is taken through PersistedUriPermissions rather than the resolver directly, so that it joins the in flight set the recent list's grants already use: addFolderTree writes the tree down on the view model's executor, and a prune in that window would find a grant nothing points at yet and hand it straight back. Same race, same fix, one folder instead of one document. That decision is now isReferenced(), which takes the strings rather than a context and so can be covered by a plain jvm test, with the release half lifted out beside it to keep prune() down to the loop it is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TiSZ4FeptMUsFjAuCewzQZ --- CLAUDE.md | 27 +++ .../opendocument/droid/test/LandingTests.kt | 32 +++ .../droid/background/DocumentTreeBrowser.kt | 125 ++++++++++ .../droid/background/FolderTreesUtil.kt | 125 ++++++++++ .../background/PersistedUriPermissions.kt | 78 ++++--- .../background/SupportedDocumentTypes.kt | 67 ++++++ .../droid/ui/activity/LandingFragment.kt | 216 +++++++++++++++++- .../droid/ui/activity/LandingViewModel.kt | 89 +++++++- .../droid/ui/widget/LandingAdapter.kt | 43 +++- .../droid/ui/widget/LandingItem.kt | 14 ++ app/src/main/res/drawable/ic_arrow_upward.xml | 10 + app/src/main/res/layout/fragment_landing.xml | 8 + app/src/main/res/values/strings.xml | 7 + .../background/PersistedUriPermissionsTest.kt | 30 ++- .../background/SupportedDocumentTypesTest.kt | 71 ++++++ 15 files changed, 891 insertions(+), 51 deletions(-) create mode 100644 app/src/main/java/app/opendocument/droid/background/DocumentTreeBrowser.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/FolderTreesUtil.kt create mode 100644 app/src/main/java/app/opendocument/droid/background/SupportedDocumentTypes.kt create mode 100644 app/src/main/res/drawable/ic_arrow_upward.xml create mode 100644 app/src/test/java/app/opendocument/droid/background/SupportedDocumentTypesTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index 897ac0ac0f12..95859fd9d9f2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -145,6 +145,33 @@ 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. + +### Storage access + +The app declares **no storage permission at all** - only `INTERNET` - and it has to stay that way. +`READ_EXTERNAL_STORAGE` has not reached documents since scoped storage, `READ_MEDIA_*` only covers +images/video/audio, and Play restricts `MANAGE_EXTERNAL_STORAGE` to file managers and backup apps, +so a document viewer asking for it gets the listing rejected. + +Everything therefore goes through SAF: `ACTION_OPEN_DOCUMENT` for a single file and +`ACTION_OPEN_DOCUMENT_TREE` (read only, never `FLAG_GRANT_WRITE_URI_PERMISSION`) for the folders +the landing screen browses. Those grants have to be persisted to survive a restart - +`PersistedUriPermissions` takes them and reclaims them by reconciling against the recent list and +the granted trees, rather than releasing on close. Do not add a release next to a +`documentFragment.loadUri()`: that call only queues the load onto `LoaderService`, so the stream is +opened long after it returns. + ### Language Kotlin; support is built into AGP 9, no kotlin plugin is applied. The only java left is diff --git a/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt b/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt index 2de747335a94..64167ba9f6b7 100644 --- a/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt +++ b/app/src/androidTest/java/app/opendocument/droid/test/LandingTests.kt @@ -29,6 +29,7 @@ import app.opendocument.droid.ui.activity.MainActivity import java.io.File import java.io.FileOutputStream import java.io.InputStream +import org.hamcrest.Matchers.allOf import org.junit.After import org.junit.Assert import org.junit.Before @@ -120,6 +121,32 @@ class LandingTests { ) } + @Test + fun addingAFolderAsksTheSystemForATree() { + Intents.intending(hasAction(Intent.ACTION_OPEN_DOCUMENT_TREE)) + .respondWith(Instrumentation.ActivityResult(Activity.RESULT_CANCELED, null)) + + launch() + + onView(withId(R.id.landing_empty_add_folder)).perform(click()) + + Intents.intended(hasAction(Intent.ACTION_OPEN_DOCUMENT_TREE)) + } + + @Test + fun theFoldersSectionOffersToAddOne() { + seedRecentDocument() + + launch() + + onView(withText(R.string.landing_section_folders)).check(matches(isDisplayed())) + + // the empty state carries a button with the same label, so match the one on screen - + // the empty state is gone once there is a recent document to show + onView(allOf(withText(R.string.landing_add_folder), isDisplayed())) + .check(matches(isDisplayed())) + } + @Test fun theCatchAllSettingIsOffered() { seedRecentDocument() @@ -155,8 +182,13 @@ class LandingTests { RecentDocumentsUtil.addRecentDocument(context(), TEST_DOCUMENT, uriOf(requireTestFile())) } + /** + * Both stores, not just the recent documents: a folder granted on this device - by a previous + * run, or by hand - would keep the empty state from ever being shown. + */ private fun clearLandingState() { context().deleteFile("recent_documents.json") + context().deleteFile("folder_trees.json") } private fun waitForDocumentFragment(activity: MainActivity): Boolean { diff --git a/app/src/main/java/app/opendocument/droid/background/DocumentTreeBrowser.kt b/app/src/main/java/app/opendocument/droid/background/DocumentTreeBrowser.kt new file mode 100644 index 000000000000..48b45d430cfc --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/DocumentTreeBrowser.kt @@ -0,0 +1,125 @@ +package app.opendocument.droid.background + +import android.content.Context +import android.net.Uri +import android.provider.DocumentsContract + +/** + * Lists what is inside a directory tree the user granted access to. + * + * Uses [DocumentsContract] rather than androidx's DocumentFile: DocumentFile.listFiles() issues a + * fresh query per child as soon as the name or type is asked for, which is one binder round trip + * per file. One cursor over the whole folder is what this needs instead. + */ +object DocumentTreeBrowser { + + class Child( + val documentId: String, + val displayName: String, + val mimeType: String, + val lastModified: Long, + ) { + val isDirectory: Boolean + get() = mimeType == DocumentsContract.Document.MIME_TYPE_DIR + } + + /** + * The supported documents and the sub directories of [parentDocumentId], directories first and + * then by name. + * + * Does binder work, so it must not run on the main thread. + */ + fun listChildren(context: Context, treeUri: Uri, parentDocumentId: String): List { + val childrenUri = + DocumentsContract.buildChildDocumentsUriUsingTree(treeUri, parentDocumentId) + + val children = ArrayList() + + try { + context.contentResolver + .query( + childrenUri, + arrayOf( + DocumentsContract.Document.COLUMN_DOCUMENT_ID, + DocumentsContract.Document.COLUMN_DISPLAY_NAME, + DocumentsContract.Document.COLUMN_MIME_TYPE, + DocumentsContract.Document.COLUMN_LAST_MODIFIED, + ), + null, + null, + null, + ) + .use { cursor -> + if (cursor == null) { + return emptyList() + } + + while (cursor.moveToNext()) { + val documentId = cursor.getString(0) ?: continue + val displayName = cursor.getString(1) ?: continue + val mimeType = cursor.getString(2) ?: "" + + // dotfiles are not something anyone browses for a document + if (displayName.startsWith(".")) { + continue + } + + val isDirectory = mimeType == DocumentsContract.Document.MIME_TYPE_DIR + if ( + !isDirectory && + !SupportedDocumentTypes.isSupported(mimeType, displayName) + ) { + continue + } + + children.add(Child(documentId, displayName, mimeType, cursor.getLong(3))) + } + } + } catch (e: Exception) { + // a revoked grant, an unmounted sd card or a provider that died - nothing to list + return emptyList() + } + + children.sortWith( + compareBy { !it.isDirectory } + .thenBy(String.CASE_INSENSITIVE_ORDER) { + it.displayName + } + ) + + return children + } + + /** The document id of the folder the user picked, i.e. the root of the tree. */ + fun rootDocumentId(treeUri: Uri): String = DocumentsContract.getTreeDocumentId(treeUri) + + fun documentUri(treeUri: Uri, documentId: String): Uri = + DocumentsContract.buildDocumentUriUsingTree(treeUri, documentId) + + /** The name the provider gives the folder itself, falling back to its last path segment. */ + fun displayNameOf(context: Context, treeUri: Uri): String { + val documentUri = documentUri(treeUri, rootDocumentId(treeUri)) + + try { + context.contentResolver + .query( + documentUri, + arrayOf(DocumentsContract.Document.COLUMN_DISPLAY_NAME), + null, + null, + null, + ) + .use { cursor -> + if (cursor != null && cursor.moveToFirst()) { + cursor.getString(0)?.let { + return it + } + } + } + } catch (e: Exception) { + // fall through to the uri + } + + return treeUri.lastPathSegment ?: treeUri.toString() + } +} diff --git a/app/src/main/java/app/opendocument/droid/background/FolderTreesUtil.kt b/app/src/main/java/app/opendocument/droid/background/FolderTreesUtil.kt new file mode 100644 index 000000000000..2deecb2e745a --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/FolderTreesUtil.kt @@ -0,0 +1,125 @@ +package app.opendocument.droid.background + +import android.content.Context +import android.net.Uri +import java.io.BufferedReader +import java.io.InputStreamReader +import java.io.OutputStreamWriter +import java.nio.charset.StandardCharsets +import org.json.JSONArray +import org.json.JSONObject + +/** + * The folders the user granted access to, in the order they added them. + * + * A json file rather than shared preferences because the order matters and every entry carries a + * cached display name, which a StringSet could not express. Same shape as [RecentDocumentsUtil]. + */ +object FolderTreesUtil { + + /** + * How many folders to keep. Each holds a persisted uri permission, and those are capped per + * package - together with [RecentDocumentList.MAX_ENTRIES] this stays far below the limit. + */ + const val MAX_TREES: Int = 8 + + private const val FILENAME = "folder_trees.json" + + private const val KEY_URI = "uri" + private const val KEY_DISPLAY_NAME = "displayName" + private const val KEY_ADDED_AT = "addedAt" + + class FolderTree(val uri: Uri, val displayName: String, val addedAt: Long) + + @Synchronized fun getFolderTrees(context: Context): List = read(context) + + /** + * Adds [uri], or refreshes the name of a folder that is already there. + * + * @return the folders that fell out, so their grants can be released. + */ + @Synchronized + fun addFolderTree(context: Context, uri: Uri, displayName: String): List { + val value = uri.toString() + + val remaining = read(context).filter { it.uri.toString() != value } + val trees = ArrayList(remaining.size + 1) + trees.addAll(remaining) + trees.add(FolderTree(uri, displayName, System.currentTimeMillis())) + + if (trees.size <= MAX_TREES) { + write(context, trees) + + return emptyList() + } + + // oldest first, so the ones that drop off the front are the least recently added + val evicted = trees.subList(0, trees.size - MAX_TREES).toList() + write(context, trees.subList(trees.size - MAX_TREES, trees.size).toList()) + + return evicted + } + + @Synchronized + fun removeFolderTree(context: Context, uri: Uri) { + val value = uri.toString() + + val current = read(context) + val remaining = current.filter { it.uri.toString() != value } + + if (remaining.size != current.size) { + write(context, remaining) + } + } + + private fun read(context: Context): List { + val jsonArray = + try { + context.openFileInput(FILENAME).use { input -> + BufferedReader(InputStreamReader(input, StandardCharsets.UTF_8)).use { reader -> + JSONArray(reader.readText()) + } + } + } catch (e: Exception) { + return emptyList() + } + + val trees = ArrayList(jsonArray.length()) + for (i in 0 until jsonArray.length()) { + val tree = jsonArray.optJSONObject(i) ?: continue + + val uri = tree.optString(KEY_URI, "") + if (uri.isEmpty()) { + continue + } + + trees.add( + FolderTree( + Uri.parse(uri), + tree.optString(KEY_DISPLAY_NAME, uri), + tree.optLong(KEY_ADDED_AT, 0), + ) + ) + } + + return trees + } + + private fun write(context: Context, trees: List) { + val jsonArray = JSONArray() + for (tree in trees) { + val json = JSONObject() + json.put(KEY_URI, tree.uri.toString()) + json.put(KEY_DISPLAY_NAME, tree.displayName) + json.put(KEY_ADDED_AT, tree.addedAt) + + jsonArray.put(json) + } + + context.openFileOutput(FILENAME, Context.MODE_PRIVATE).use { output -> + OutputStreamWriter(output, StandardCharsets.UTF_8).use { writer -> + writer.write(jsonArray.toString()) + } + } + } +} diff --git a/app/src/main/java/app/opendocument/droid/background/PersistedUriPermissions.kt b/app/src/main/java/app/opendocument/droid/background/PersistedUriPermissions.kt index 13561deaffbc..c353908be7f1 100644 --- a/app/src/main/java/app/opendocument/droid/background/PersistedUriPermissions.kt +++ b/app/src/main/java/app/opendocument/droid/background/PersistedUriPermissions.kt @@ -2,6 +2,7 @@ package app.opendocument.droid.background import android.content.Context import android.content.Intent +import android.content.UriPermission import android.net.Uri /** @@ -17,16 +18,17 @@ object PersistedUriPermissions { private const val READ_FLAG = Intent.FLAG_GRANT_READ_URI_PERMISSION /** - * The grants taken during this process that the recent list does not name yet. + * The grants taken during this process that nothing on disk names yet. * * Taking a grant and writing down what needs it are not one step: a document only reaches the - * list once [MetadataLoader] has read it, which is long after [takeRead] ran - `loadUri` merely - * queues the load. A [prune] in that window would enumerate the fresh grant, find nothing - * referring to it and hand it straight back, leaving the entry that is about to be written - * unreadable on the next launch - the very failure releasing on close used to cause. + * recent list once [MetadataLoader] has read it, which is long after [takeRead] ran - `loadUri` + * merely queues the load - and a folder only reaches the tree list once the landing view + * model's executor gets to it. A [prune] in either window would enumerate the fresh grant, find + * nothing referring to it and hand it straight back, leaving the entry that is about to be + * written unreadable on the next launch - the very failure releasing on close used to cause. * - * Empty on a fresh process, so a grant whose document never made it onto the list is reclaimed - * on the next launch rather than leaking for good. + * Empty on a fresh process, so a grant whose document never made it onto a list is reclaimed on + * the next launch rather than leaking for good. */ private val pendingGrants = HashSet() @@ -83,9 +85,9 @@ object PersistedUriPermissions { * Does file and binder work, so it must not run on the main thread. */ fun prune(context: Context) { - // the three reads are in this order on purpose. a grant taken while this runs is either - // missing from the snapshot, or still pending when the pending set is read - which happens - // after the list that would have settled it, so it cannot fall through both + // the reads are in this order on purpose. a grant taken while this runs is either missing + // from the snapshot, or still pending when the pending set is read - which happens after + // the lists that would have settled it, so it cannot fall through both val held = context.contentResolver.persistedUriPermissions val keep = HashSet() @@ -93,32 +95,52 @@ object PersistedUriPermissions { keep.add(entry.uri) } + val trees = FolderTreesUtil.getFolderTrees(context).map { it.uri.toString() } + keep.addAll(trees) + keep.addAll(settlePending(keep)) for (permission in held) { - val uri = permission.uri.toString() - if (uri in keep) { - continue + if (!isReferenced(permission.uri.toString(), keep, trees)) { + release(context, permission) } + } + } - var flags = 0 - if (permission.isReadPermission) { - flags = flags or READ_FLAG - } - if (permission.isWritePermission) { - flags = flags or Intent.FLAG_GRANT_WRITE_URI_PERMISSION - } + /** + * Whether [uri] is still spoken for, either by name or by sitting below a granted tree. + * + * [prune]'s whole decision, and free of android imports, so a plain jvm test can cover it. + */ + internal fun isReferenced(uri: String, keep: Set, trees: List): Boolean { + if (uri in keep) { + return true + } - try { - // releasing with flags we do not hold throws, hence taking them off the grant - context.contentResolver.releasePersistableUriPermission(permission.uri, flags) - } catch (e: Exception) { - // nothing to do about it, and it will be retried on the next launch - } + // a document below a granted tree is reachable through that grant, and its uri spells + // the tree out: content://authority/tree//document/. so coverage + // can be recognised from the string, without recording where a document came from. + return trees.any { uri.startsWith("$it/") } + } + + private fun release(context: Context, permission: UriPermission) { + var flags = 0 + if (permission.isReadPermission) { + flags = flags or READ_FLAG + } + if (permission.isWritePermission) { + flags = flags or Intent.FLAG_GRANT_WRITE_URI_PERMISSION + } + + try { + // releasing with flags we do not hold throws, hence taking them off the grant + context.contentResolver.releasePersistableUriPermission(permission.uri, flags) + } catch (e: Exception) { + // nothing to do about it, and it will be retried on the next launch } } - /** Remembers a grant until the recent list names it. */ + /** Remembers a grant until one of the stored lists names it. */ @Synchronized internal fun markPending(uri: String) { pendingGrants.add(uri) @@ -126,7 +148,7 @@ object PersistedUriPermissions { /** * Drops the pending grants [keep] has caught up with - those are reclaimable the ordinary way - * from now on - and hands back the ones whose document is still on its way onto the list. + * from now on - and hands back the ones that are still in flight. */ @Synchronized internal fun settlePending(keep: Set): Set { 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..110ea6994f5d --- /dev/null +++ b/app/src/main/java/app/opendocument/droid/background/SupportedDocumentTypes.kt @@ -0,0 +1,67 @@ +package app.opendocument.droid.background + +/** + * Whether a document is worth offering in the folder browser. + * + * Deliberately free of Android dependencies so it can be covered by plain JVM unit tests. + * + * 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. + * + * 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. + */ +object SupportedDocumentTypes { + + /** Mime types and prefixes taken from the STRICT_CATCH filter. */ + private val MIME_PREFIXES = + listOf( + "application/vnd.oasis.opendocument", + "application/vnd.openxmlformats-officedocument", + "application/msword", + "application/pdf", + "application/zip", + "text/plain", + "text/csv", + "image/", + ) + + /** The extensions the STRICT_CATCH pathPatterns spell out. */ + private val EXTENSIONS = + setOf( + "odt", + "ods", + "odp", + "odg", + "ott", + "ots", + "otp", + "otg", + "doc", + "docx", + "xlsx", + "pdf", + "txt", + "csv", + "zip", + ) + + 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 + val extension = MimeTypeResolver.parseExtension(filename)?.lowercase() + if (extension != null && extension in EXTENSIONS) { + return true + } + + if (mimeType == null) { + return false + } + + val normalized = mimeType.lowercase() + + return MIME_PREFIXES.any { normalized.startsWith(it) } + } +} diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/LandingFragment.kt b/app/src/main/java/app/opendocument/droid/ui/activity/LandingFragment.kt index 7d8d884fe5a2..92c3cd7d1ec0 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/LandingFragment.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/LandingFragment.kt @@ -1,23 +1,31 @@ package app.opendocument.droid.ui.activity +import android.app.Activity +import android.content.Intent import android.net.Uri import android.os.Bundle import android.view.LayoutInflater import android.view.View import android.view.ViewGroup +import androidx.activity.OnBackPressedCallback +import androidx.activity.result.contract.ActivityResultContracts import androidx.fragment.app.Fragment import androidx.lifecycle.ViewModelProvider import androidx.recyclerview.widget.LinearLayoutManager import androidx.recyclerview.widget.RecyclerView import app.opendocument.droid.R +import app.opendocument.droid.background.DocumentTreeBrowser +import app.opendocument.droid.background.PersistedUriPermissions import app.opendocument.droid.nonfree.AnalyticsConstants +import app.opendocument.droid.ui.SnackbarHelper import app.opendocument.droid.ui.widget.LandingAdapter import app.opendocument.droid.ui.widget.LandingItem import com.google.android.material.dialog.MaterialAlertDialogBuilder import com.google.android.material.floatingactionbutton.FloatingActionButton /** - * The screen the app opens on: the documents the user was last working on. + * The screen the app opens on: the documents the user was last working on, and the folders they + * granted access to. * * A fragment rather than more views inside MainActivity, following DocumentFragment - the list * state belongs in a ViewModel so it survives the recreation a theme or rotation change causes. @@ -35,6 +43,57 @@ class LandingFragment : Fragment(), LandingAdapter.Listener { private var landingVisible = true + /** + * Back goes up a folder before it goes anywhere else. + * + * Registered on viewLifecycleOwner so it stacks above MainActivity's own callback - the + * dispatcher runs the most recently added enabled callback first. + */ + private val folderBackCallback = + object : OnBackPressedCallback(false) { + override fun handleOnBackPressed() { + if (!viewModel.leaveFolder()) { + isEnabled = false + } + } + } + + /** + * ACTION_OPEN_DOCUMENT_TREE. Read only - the app asks for no write access anywhere, which is + * what keeps it free of storage permissions entirely. + */ + private val openTreeLauncher = + registerForActivityResult(ActivityResultContracts.StartActivityForResult()) { result -> + if (result.resultCode != Activity.RESULT_OK) { + return@registerForActivityResult + } + + val treeUri = result.data?.data ?: return@registerForActivityResult + + // has to happen before the transient grant on the result intent expires, and through + // PersistedUriPermissions rather than the resolver directly: addFolderTree writes the + // tree down on the view model's executor, so a prune in between would find a grant + // nothing points at yet and hand it straight back + if (!PersistedUriPermissions.takeRead(requireContext(), treeUri)) { + // some providers refuse to hand out a lasting grant + mainActivity.crashManager.log("could not persist the grant for $treeUri") + + SnackbarHelper.show( + requireActivity(), + R.string.landing_add_folder_failed, + null, + false, + true, + ) + + return@registerForActivityResult + } + + mainActivity.analyticsManager.report("folder_added") + + viewModel.addFolderTree(treeUri) + } + override fun onCreateView( inflater: LayoutInflater, container: ViewGroup?, @@ -65,6 +124,13 @@ class LandingFragment : Fragment(), LandingAdapter.Listener { mainActivity.findDocument() } + view.findViewById(R.id.landing_empty_add_folder).setOnClickListener { + onActionClicked(LandingItem.ACTION_ADD_FOLDER) + } + + requireActivity() + .onBackPressedDispatcher + .addCallback(viewLifecycleOwner, folderBackCallback) viewModel.state.observe(viewLifecycleOwner) { state -> render(state) } } @@ -81,7 +147,8 @@ class LandingFragment : Fragment(), LandingAdapter.Listener { * * The fragment is only hidden, never stopped - so nothing in the lifecycle fires when a * document is closed, and the list would keep showing what it held before the document was - * opened, missing the document that was just added to it. + * opened, missing the document that was just added to it. The back callback has to be turned + * off too, or a hidden landing screen would keep swallowing back inside a document. */ fun setLandingVisible(visible: Boolean) { landingVisible = visible @@ -92,14 +159,44 @@ class LandingFragment : Fragment(), LandingAdapter.Listener { return } + folderBackCallback.isEnabled = visible && viewModel.isInsideFolder + if (visible) { viewModel.refresh() } } private fun render(state: LandingViewModel.State) { + folderBackCallback.isEnabled = landingVisible && state.location != null + val items = ArrayList() + val location = state.location + if (location != null) { + renderFolder(items, state, location) + } else { + renderRoot(items, state) + } + + val isEmpty = location == null && state.documents.isEmpty() && state.trees.isEmpty() + + empty.visibility = if (isEmpty) View.VISIBLE else View.GONE + list.visibility = if (isEmpty) View.GONE else View.VISIBLE + + // the empty state offers the same thing with a label on it, so the bare fab would just + // be a second unexplained button next to it + fab.visibility = if (isEmpty) View.GONE else View.VISIBLE + + // a refresh can land while a document is open - the fragment is only hidden, so it keeps + // getting lifecycle callbacks - and the folder name must not take over the title then + if (landingVisible) { + mainActivity.title = location?.displayName ?: "" + } + + adapter.submitList(items) + } + + private fun renderRoot(items: ArrayList, state: LandingViewModel.State) { if (state.documents.isNotEmpty()) { items.add(LandingItem.Header(R.string.landing_section_recent)) for (document in state.documents) { @@ -107,33 +204,77 @@ class LandingFragment : Fragment(), LandingAdapter.Listener { } } + items.add(LandingItem.Header(R.string.landing_section_folders)) + for (tree in state.trees) { + items.add( + LandingItem.Folder( + tree.displayName, + tree.uri, + DocumentTreeBrowser.rootDocumentId(tree.uri), + ) + ) + } + items.add( + LandingItem.Action( + LandingItem.ACTION_ADD_FOLDER, + R.string.landing_add_folder, + R.drawable.ic_add, + ) + ) + items.add(LandingItem.Header(R.string.landing_section_settings)) items.add(LandingItem.CatchAll(state.catchAllEnabled)) + } - // with nothing to show but the setting, the list is just noise around one button - val isEmpty = state.documents.isEmpty() + private fun renderFolder( + items: ArrayList, + state: LandingViewModel.State, + location: LandingViewModel.FolderLocation, + ) { + items.add( + LandingItem.Action( + LandingItem.ACTION_UP, + R.string.landing_up_one_level, + R.drawable.ic_arrow_upward, + ) + ) - empty.visibility = if (isEmpty) View.VISIBLE else View.GONE - list.visibility = if (isEmpty) View.GONE else View.VISIBLE + if (state.children.isEmpty()) { + items.add(LandingItem.Message(R.string.landing_folder_empty)) - // the empty state offers the same thing with a label on it, so the bare fab would just - // be a second unexplained button next to it - fab.visibility = if (isEmpty) View.GONE else View.VISIBLE + return + } - adapter.submitList(items) + for (child in state.children) { + if (child.isDirectory) { + items.add(LandingItem.Folder(child.displayName, location.treeUri, child.documentId)) + } else { + items.add( + LandingItem.Document( + child.displayName, + DocumentTreeBrowser.documentUri(location.treeUri, child.documentId), + ) + ) + } + } } override fun onDocumentClicked(document: LandingItem.Document) { mainActivity.analyticsManager.report( AnalyticsConstants.EVENT_SELECT_CONTENT, AnalyticsConstants.PARAM_CONTENT_TYPE, - "recent", + if (viewModel.isInsideFolder) "folder" else "recent", ) mainActivity.loadUri(document.uri) } override fun onDocumentRemoveRequested(document: LandingItem.Document) { + // a document inside a folder is not ours to forget - it is simply there + if (viewModel.isInsideFolder) { + return + } + MaterialAlertDialogBuilder(requireContext()) .setTitle(document.filename) .setMessage(R.string.landing_remove_recent_message) @@ -144,6 +285,59 @@ class LandingFragment : Fragment(), LandingAdapter.Listener { .show() } + override fun onFolderClicked(folder: LandingItem.Folder) { + viewModel.enterFolder(folder.treeUri, folder.documentId, folder.name) + } + + override fun onFolderRemoveRequested(folder: LandingItem.Folder) { + // only the folders the user added themselves can be given back; a sub directory is just + // part of the tree above it + if (viewModel.isInsideFolder) { + return + } + + MaterialAlertDialogBuilder(requireContext()) + .setTitle(folder.name) + .setMessage(R.string.landing_remove_folder_message) + .setNegativeButton(android.R.string.cancel, null) + .setPositiveButton(R.string.landing_remove_folder) { _, _ -> + mainActivity.analyticsManager.report("folder_removed") + + viewModel.removeFolderTree(folder.treeUri) + } + .show() + } + + override fun onActionClicked(action: Int) { + when (action) { + LandingItem.ACTION_ADD_FOLDER -> addFolder() + LandingItem.ACTION_UP -> viewModel.leaveFolder() + } + } + + private fun addFolder() { + val intent = + Intent(Intent.ACTION_OPEN_DOCUMENT_TREE) + .addFlags( + Intent.FLAG_GRANT_READ_URI_PERMISSION or + Intent.FLAG_GRANT_PERSISTABLE_URI_PERMISSION + ) + + try { + openTreeLauncher.launch(intent) + } catch (e: Exception) { + mainActivity.crashManager.log(e) + + SnackbarHelper.show( + requireActivity(), + R.string.crouton_error_open_app, + null, + false, + true, + ) + } + } + override fun onCatchAllChanged(enabled: Boolean) { viewModel.setCatchAllEnabled(enabled) diff --git a/app/src/main/java/app/opendocument/droid/ui/activity/LandingViewModel.kt b/app/src/main/java/app/opendocument/droid/ui/activity/LandingViewModel.kt index ee42f45a46a6..b6043b320a0d 100644 --- a/app/src/main/java/app/opendocument/droid/ui/activity/LandingViewModel.kt +++ b/app/src/main/java/app/opendocument/droid/ui/activity/LandingViewModel.kt @@ -8,6 +8,8 @@ import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import app.opendocument.droid.background.CatchAllSetting +import app.opendocument.droid.background.DocumentTreeBrowser +import app.opendocument.droid.background.FolderTreesUtil import app.opendocument.droid.background.PersistedUriPermissions import app.opendocument.droid.background.RecentDocumentList import app.opendocument.droid.background.RecentDocumentsUtil @@ -17,22 +19,41 @@ import java.util.concurrent.Executors /** * What the landing screen shows. * - * Reading the recently opened documents touches a file and checking whether they still resolve - * talks to their content provider, so both happen on [executor] and are published through - * [LiveData] - which, unlike posting to a handler, drops the result when the fragment is gone. + * Reading the recently opened documents touches a file, listing a folder talks to its provider, and + * checking whether a document still resolves does too - so all of it happens on [executor] and is + * published through [LiveData], which, unlike posting to a handler, drops the result when the + * fragment is gone. * * There are no coroutines anywhere in this project, so this follows the executor plus handler shape * the loaders already use. */ class LandingViewModel(application: Application) : AndroidViewModel(application) { - class State(val documents: List, val catchAllEnabled: Boolean) + /** Where in a folder tree the browser currently is. */ + class FolderLocation(val treeUri: Uri, val documentId: String, val displayName: String) + + class State( + val documents: List, + val trees: List, + val catchAllEnabled: Boolean, + /** null while the roots are shown, otherwise the folder that is open. */ + val location: FolderLocation?, + val children: List, + ) private val executor: ExecutorService = Executors.newSingleThreadExecutor() private val mutableState = MutableLiveData() val state: LiveData = mutableState + // not persisted across process death on purpose: coming back into a folder whose grant was + // revoked while the app was away is a pile of edge cases for very little + private val backStack = ArrayList() + private var location: FolderLocation? = null + + val isInsideFolder: Boolean + get() = location != null + /** * Publishes what is on disk right away, then re-publishes once the documents that no longer * resolve have been dropped. Rendering does not wait on the provider round trip that way. @@ -42,9 +63,15 @@ class LandingViewModel(application: Application) : AndroidViewModel(application) val context = getApplication() val stored = RecentDocumentsUtil.getRecentDocuments(context) + val trees = FolderTreesUtil.getFolderTrees(context) val catchAll = CatchAllSetting.isEnabled(context) + val here = location - mutableState.postValue(State(stored, catchAll)) + val children = + if (here == null) emptyList() + else DocumentTreeBrowser.listChildren(context, here.treeUri, here.documentId) + + mutableState.postValue(State(stored, trees, catchAll, here, children)) val alive = stored.filter { isReadable(context, Uri.parse(it.uri)) } if (alive.size == stored.size) { @@ -56,10 +83,60 @@ class LandingViewModel(application: Application) : AndroidViewModel(application) } PersistedUriPermissions.prune(context) - mutableState.postValue(State(alive, catchAll)) + mutableState.postValue(State(alive, trees, catchAll, here, children)) + } + } + + fun addFolderTree(treeUri: Uri) { + executor.execute { + val context = getApplication() + + val displayName = DocumentTreeBrowser.displayNameOf(context, treeUri) + if (FolderTreesUtil.addFolderTree(context, treeUri, displayName).isNotEmpty()) { + PersistedUriPermissions.prune(context) + } + + refresh() } } + fun removeFolderTree(treeUri: Uri) { + executor.execute { + val context = getApplication() + + FolderTreesUtil.removeFolderTree(context, treeUri) + PersistedUriPermissions.prune(context) + + // the folder that was just dropped may be the one we are standing in + if (location?.treeUri == treeUri) { + backStack.clear() + location = null + } + + refresh() + } + } + + fun enterFolder(treeUri: Uri, documentId: String, displayName: String) { + location?.let { backStack.add(it) } + location = FolderLocation(treeUri, documentId, displayName) + + refresh() + } + + /** @return whether there was somewhere to go back to. */ + fun leaveFolder(): Boolean { + if (location == null) { + return false + } + + location = backStack.removeLastOrNull() + + refresh() + + return true + } + fun setCatchAllEnabled(enabled: Boolean) { CatchAllSetting.setEnabled(getApplication(), enabled) diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/LandingAdapter.kt b/app/src/main/java/app/opendocument/droid/ui/widget/LandingAdapter.kt index 157a9c649576..eb266f87ac8d 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/LandingAdapter.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/LandingAdapter.kt @@ -11,7 +11,10 @@ import androidx.recyclerview.widget.RecyclerView import app.opendocument.droid.R import com.google.android.material.materialswitch.MaterialSwitch -/** The rows of the landing screen: the recently opened documents and the catch-all setting. */ +/** + * The rows of the landing screen: the recently opened documents, the folders the user granted + * access to, and the settings. + */ class LandingAdapter(private val listener: Listener) : ListAdapter(DIFF) { @@ -21,6 +24,12 @@ class LandingAdapter(private val listener: Listener) : fun onDocumentRemoveRequested(document: LandingItem.Document) + fun onFolderClicked(folder: LandingItem.Folder) + + fun onFolderRemoveRequested(folder: LandingItem.Folder) + + fun onActionClicked(action: Int) + fun onCatchAllChanged(enabled: Boolean) } @@ -28,6 +37,8 @@ class LandingAdapter(private val listener: Listener) : when (getItem(position)) { is LandingItem.Header -> TYPE_HEADER is LandingItem.Document -> TYPE_DOCUMENT + is LandingItem.Folder -> TYPE_FOLDER + is LandingItem.Action -> TYPE_ACTION is LandingItem.CatchAll -> TYPE_CATCH_ALL is LandingItem.Message -> TYPE_MESSAGE } @@ -40,6 +51,7 @@ class LandingAdapter(private val listener: Listener) : TYPE_HEADER -> R.layout.item_landing_header TYPE_CATCH_ALL -> R.layout.item_landing_switch TYPE_MESSAGE -> R.layout.item_landing_message + // documents, folders and actions are all an icon plus a label else -> R.layout.item_landing_row } @@ -63,6 +75,24 @@ class LandingAdapter(private val listener: Listener) : } } + is LandingItem.Folder -> { + holder.icon.setImageResource(R.drawable.ic_folder) + holder.title.text = item.name + holder.itemView.setOnClickListener { listener.onFolderClicked(item) } + holder.itemView.setOnLongClickListener { + listener.onFolderRemoveRequested(item) + + true + } + } + + is LandingItem.Action -> { + holder.icon.setImageResource(item.icon) + holder.title.setText(item.label) + holder.itemView.setOnClickListener { listener.onActionClicked(item.action) } + holder.itemView.setOnLongClickListener(null) + } + is LandingItem.CatchAll -> { // set the state before the listener, so restoring it does not report a change holder.switch.setOnCheckedChangeListener(null) @@ -85,8 +115,10 @@ class LandingAdapter(private val listener: Listener) : private companion object { const val TYPE_HEADER = 0 const val TYPE_DOCUMENT = 1 - const val TYPE_CATCH_ALL = 2 - const val TYPE_MESSAGE = 3 + const val TYPE_FOLDER = 2 + const val TYPE_ACTION = 3 + const val TYPE_CATCH_ALL = 4 + const val TYPE_MESSAGE = 5 val DIFF = object : DiffUtil.ItemCallback() { @@ -102,10 +134,13 @@ class LandingAdapter(private val listener: Listener) : oldItem is LandingItem.Document && newItem is LandingItem.Document -> oldItem.filename == newItem.filename + oldItem is LandingItem.Folder && newItem is LandingItem.Folder -> + oldItem.name == newItem.name + oldItem is LandingItem.CatchAll && newItem is LandingItem.CatchAll -> oldItem.checked == newItem.checked - // headers and messages are fully described by their id + // headers, messages and actions are fully described by their id else -> true } } diff --git a/app/src/main/java/app/opendocument/droid/ui/widget/LandingItem.kt b/app/src/main/java/app/opendocument/droid/ui/widget/LandingItem.kt index d8658c7eec31..7beeee24c539 100644 --- a/app/src/main/java/app/opendocument/droid/ui/widget/LandingItem.kt +++ b/app/src/main/java/app/opendocument/droid/ui/widget/LandingItem.kt @@ -17,6 +17,15 @@ sealed class LandingItem { override val id: String = "document:$uri" } + class Folder(val name: String, val treeUri: Uri, val documentId: String) : LandingItem() { + override val id: String = "folder:$treeUri:$documentId" + } + + /** A tappable row that is not a document, such as "add a folder". */ + class Action(val action: Int, @param:StringRes val label: Int, val icon: Int) : LandingItem() { + override val id: String = "action:$action" + } + class CatchAll(val checked: Boolean) : LandingItem() { override val id: String = "catch_all" } @@ -25,4 +34,9 @@ sealed class LandingItem { class Message(@param:StringRes val text: Int) : LandingItem() { override val id: String = "message:$text" } + + companion object { + const val ACTION_ADD_FOLDER: Int = 1 + const val ACTION_UP: Int = 2 + } } diff --git a/app/src/main/res/drawable/ic_arrow_upward.xml b/app/src/main/res/drawable/ic_arrow_upward.xml new file mode 100644 index 000000000000..21a35e76a761 --- /dev/null +++ b/app/src/main/res/drawable/ic_arrow_upward.xml @@ -0,0 +1,10 @@ + + + diff --git a/app/src/main/res/layout/fragment_landing.xml b/app/src/main/res/layout/fragment_landing.xml index 2cd3940dd852..141d1e2357e6 100644 --- a/app/src/main/res/layout/fragment_landing.xml +++ b/app/src/main/res/layout/fragment_landing.xml @@ -57,6 +57,14 @@ android:text="@string/landing_open_file" app:icon="@drawable/ic_folder_open" /> + Settings Open all file types Open a file + Folders + Add a folder… + This folder can\'t be remembered. Try another one. + Remove folder + Stop showing this folder here? The folder and the documents in it are not deleted. + No documents in this folder. + Up one level Remove Remove this document from the recent list? The document itself is not deleted. Nothing here yet diff --git a/app/src/test/java/app/opendocument/droid/background/PersistedUriPermissionsTest.kt b/app/src/test/java/app/opendocument/droid/background/PersistedUriPermissionsTest.kt index 0bf0f78f1b22..029d3041d2ff 100644 --- a/app/src/test/java/app/opendocument/droid/background/PersistedUriPermissionsTest.kt +++ b/app/src/test/java/app/opendocument/droid/background/PersistedUriPermissionsTest.kt @@ -4,8 +4,9 @@ import org.junit.Assert import org.junit.Test /** - * Covers the bookkeeping that keeps a grant alive between taking it and the recent list naming it. - * Taking and releasing the grants themselves needs a content resolver, so that part is device work. + * Covers the bookkeeping that keeps a grant alive between taking it and a stored list naming it, + * and the decision prune makes about each grant it holds. Taking and releasing the grants + * themselves needs a content resolver, so that part is device work. */ class PersistedUriPermissionsTest { @@ -31,4 +32,29 @@ class PersistedUriPermissionsTest { Assert.assertFalse(leaked in PersistedUriPermissions.settlePending(emptySet())) } + + @Test + fun keepsWhatSitsBelowAGrantedTree() { + val tree = "content://com.android.externalstorage.documents/tree/primary%3ADocuments" + val trees = listOf(tree) + + // the tree itself is named by the folder list + Assert.assertTrue(PersistedUriPermissions.isReferenced(tree, setOf(tree), trees)) + + // and a document below it is reachable through that grant, so its own grant can go even + // though nothing names it - the uri spells the tree out + val below = "$tree/document/primary%3ADocuments%2Freport.odt" + Assert.assertTrue(PersistedUriPermissions.isReferenced(below, setOf(tree), trees)) + + // a sibling tree that merely starts the same way is not covered by it + val sibling = "${tree}Archive" + Assert.assertFalse(PersistedUriPermissions.isReferenced(sibling, setOf(tree), trees)) + } + + @Test + fun releasesWhatNothingPointsAt() { + val orphan = "content://com.android.providers.downloads.documents/document/17" + + Assert.assertFalse(PersistedUriPermissions.isReferenced(orphan, emptySet(), emptyList())) + } } 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..610064cece4b --- /dev/null +++ b/app/src/test/java/app/opendocument/droid/background/SupportedDocumentTypesTest.kt @@ -0,0 +1,71 @@ +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 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) +}