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) +}