From 667a85f80ca5f0041592db38614fde86acf9d358 Mon Sep 17 00:00:00 2001 From: Hadrian Burkhardt Date: Thu, 15 Oct 2026 10:25:37 +0200 Subject: [PATCH] Remember what is in the trash, and add a screen to restore from it The staged-delete work left a hole. The undo handle lived in BrowserUiState, so a delete from an earlier session was unreachable even though the files were sitting on the volume; and because nothing on disk recorded which folder an item came from, not even the user could put one back by hand with confidence. Adding a Trash screen alone would not have fixed it: the recovery data did not survive the process. So the origin is now written at delete time. data/ gains its first persistence: a SQLite index (staged path, original path, name, type, size, timestamp). SQLite rather than Room because an annotation processor is a large addition to this build for one table and three queries, and behind a TrashIndex port so domain/Trash keeps no Context while the tests drive it with a map and the real one is still tested against real SQLite. New Trash entry in the overflow menu lists batches and restores a whole one. The index is an index, not the truth: the staged files are the truth, and the two can disagree if a user empties the folder by hand or pulls a card. Reconciled both ways - restorableFrom drops entries whose file has gone, purgeExpired forgets entries whose folder has gone and sweeps folders no entry mentions - and rows are written after the move, so a crash leaves an unindexed folder (harmless, swept) rather than an entry that could never restore. Verified on hardware: delete, force-stop, relaunch, restore. Also corrects the docs where they claimed input swipe x y x y 900 is delivered as a plain tap on MIUI. It does open the selection bar - it was driven that way - but it has also landed as a tap, so it is flaky rather than impossible. --- CHANGELOG.md | 21 +++ README.md | 23 ++-- .../filemanager/FileManagerApplication.kt | 7 + .../filemanager/data/SqliteTrashIndex.kt | 130 ++++++++++++++++++ .../filemanager/data/TrashIndex.kt | 61 ++++++++ .../filemanager/domain/Trash.kt | 100 +++++++++++++- .../filemanager/ui/browser/BrowserScreen.kt | 12 ++ .../filemanager/ui/browser/BrowserTopBar.kt | 7 + .../filemanager/ui/browser/BrowserUiState.kt | 11 ++ .../ui/browser/BrowserViewModel.kt | 65 ++++++++- .../filemanager/ui/trash/TrashScreen.kt | 115 ++++++++++++++++ app/src/main/res/values/strings.xml | 6 + .../filemanager/data/SqliteTrashIndexTest.kt | 88 ++++++++++++ .../filemanager/domain/TrashTest.kt | 96 ++++++++++++- .../domain/testutil/InMemoryTrashIndex.kt | 27 ++++ docs/EDGE_CASE_TESTING.md | 11 +- 16 files changed, 758 insertions(+), 22 deletions(-) create mode 100644 app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt create mode 100644 app/src/main/java/de/softwareapp_hb/filemanager/data/TrashIndex.kt create mode 100644 app/src/main/java/de/softwareapp_hb/filemanager/ui/trash/TrashScreen.kt create mode 100644 app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt create mode 100644 app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryTrashIndex.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b086f2..1ef6d5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,27 @@ upload needs a **higher `versionCode`** than every previous upload. ### Added +- **A trash you can actually recover from, after the fact.** The staged-delete work left a + hole: the undo handle lived in memory, so a delete from a previous session was unreachable + even though the files were sitting right there on the volume — and because nothing on disk + recorded which folder an item came from, not even the *user* could put one back by hand + with any confidence. There is now a **Trash** entry in the overflow menu that lists what is + still there and puts a whole batch back. + + Backing it is `data/`'s first persistence: a SQLite index of staged path, original path, + name, type, size and timestamp. Deliberately SQLite rather than Room — an annotation + processor would be a large addition to this build for one table and three queries — and + deliberately behind a `TrashIndex` port, so `domain/Trash` stays free of a `Context` and + the tests drive it with a map while the real one is tested against real SQLite. + + The index is an **index, not the truth**: the staged files are the truth, and the two can + disagree — a user can delete `.plainfiles-trash` themselves, or pull the card. So it is + reconciled both ways. `restorableFrom` drops entries whose file has gone, `purgeExpired` + forgets entries whose folder has gone, and a row is written *after* the move, so a crash + between the two leaves an unindexed folder — harmless, and swept — rather than an entry + that could never restore anything. A `TrashScreen` row is one *batch*, not one file, + because a batch is what the user actually did and what the index records. + - **The app can receive a share, and act as a file picker.** The manifest had nothing but a `LAUNCHER` filter, so no other app could hand Plain Files anything at all. It now registers for `ACTION_SEND`/`SEND_MULTIPLE` — a bar appears over the browser, you navigate to the diff --git a/README.md b/README.md index f57d498..e88ab17 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,8 @@ app/src/main/java/de/softwareapp_hb/filemanager/ ├── core/ Pure-Kotlin engine (no Android imports!) — paths, sorting, formatting, │ search matching, copy/move planning, conflict policy ├── data/ Storage backends (all-files access + SAF), volumes, shares coming in -│ from other apps, file operations, archives, deletion +│ from other apps, the trash index (SQLite), file operations, archives, +│ deletion ├── domain/ Browser + operation controllers, models, permission state ├── service/ Foreground service for long-running file operations ├── ui/ Compose theme, browser screen, components, ViewModels @@ -139,11 +140,16 @@ UI.** Translated UI text has to be supplied in `values-/`. ## Known limitations -- **The trash has no screen of its own.** A delete now stages the item in a hidden - `.plainfiles-trash` folder on the same volume and keeps it for 24 hours, and the snackbar - offers Undo. Once that snackbar goes there is no in-app bin to browse or restore from — the - folder is reachable by turning on hidden files, but a proper trash screen is the obvious - follow-up. +- **The trash is a time window, not a bin you manage.** A delete stages the item in a hidden + `.plainfiles-trash` folder on the same volume and keeps it for 24 hours; the **Trash** entry + in the overflow menu lists what is still there and puts a whole batch back. There is no + "empty it now" action — expired batches are swept the next time something is deleted — and a + name taken again in the meantime is never overwritten, so a restored item can arrive beside + the new one rather than replacing it. +- **Where a trashed item came from is remembered in the app's own database**, not on the + volume. The staged files are the truth and the database is only an index, so the two are + reconciled in both directions: an entry whose file has gone is not offered back, and a + batch folder no entry mentions is swept. - **Sharing a file *out* only works in the all-files mode.** A SAF path is relative to the granted tree rather than a filesystem path, so it cannot be handed to `FileProvider`. Receiving a share and being picked from another app's chooser work either way: the first @@ -161,8 +167,9 @@ UI.** Translated UI text has to be supplied in `values-/`. - **Cancellation happens between entries**, so a single very large file finishes before a stop is honoured. - **View state is not persisted across process death** (`SavedStateHandle` is not wired up). -- **Long-press multi-select could not be driven from adb** on the MIUI test device, so the - compress/extract buttons want a manual pass (see +- **Long-press multi-select is awkward to script from the host.** The gesture that opens the + selection bar (`input swipe x y x y 900`) works on the MIUI test device but has also + registered as a plain tap, so compress/extract from the UI wants a manual pass (see [docs/EDGE_CASE_TESTING.md](docs/EDGE_CASE_TESTING.md)). - The display name is still the working title — decided before the first upload (M10). diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt b/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt index 59b0d99..94640c0 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt @@ -5,7 +5,9 @@ import de.softwareapp_hb.filemanager.data.ContentResolverIncomingSource import de.softwareapp_hb.filemanager.data.DeleteRequestFactory import de.softwareapp_hb.filemanager.data.IncomingSource import de.softwareapp_hb.filemanager.data.MediaStoreDeleter +import de.softwareapp_hb.filemanager.data.SqliteTrashIndex import de.softwareapp_hb.filemanager.data.StorageProvider +import de.softwareapp_hb.filemanager.data.TrashIndex import de.softwareapp_hb.filemanager.domain.OperationNotifier import de.softwareapp_hb.filemanager.service.ServiceOperationNotifier @@ -34,11 +36,16 @@ class FileManagerApplication : Application() { lateinit var incoming: IncomingSource private set + /** Remembers what is in the trash, and where each item came from. */ + lateinit var trashIndex: TrashIndex + private set + override fun onCreate() { super.onCreate() storage = StorageProvider(this) notifier = ServiceOperationNotifier(this) deleteRequests = MediaStoreDeleter(this).factory() incoming = ContentResolverIncomingSource(contentResolver) + trashIndex = SqliteTrashIndex(this) } } diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt new file mode 100644 index 0000000..8a94689 --- /dev/null +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndex.kt @@ -0,0 +1,130 @@ +package de.softwareapp_hb.filemanager.data + +import android.content.ContentValues +import android.content.Context +import android.database.Cursor +import android.database.sqlite.SQLiteDatabase +import android.database.sqlite.SQLiteOpenHelper +import de.softwareapp_hb.filemanager.core.StoragePath +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext + +/** + * The trash index, on SQLite. + * + * Deliberately **not** Room. Room would mean adding an annotation processor to a build that + * has none, for one table and three queries; the platform's own SQLite needs no dependency + * at all, and Robolectric runs it on the JVM, so this stays as testable as the rest of the app. + * + * If a general app database ever arrives — favourites, recents — Room becomes the better + * foundation and that is a decision worth taking on its own. + */ +class SqliteTrashIndex(context: Context) : TrashIndex { + + private val helper = Helper(context.applicationContext) + + override suspend fun record(records: List) = withContext(Dispatchers.IO) { + if (records.isEmpty()) return@withContext + val db = helper.writableDatabase + db.beginTransaction() + try { + for (record in records) { + db.insertWithOnConflict(TABLE, null, record.toValues(), SQLiteDatabase.CONFLICT_REPLACE) + } + db.setTransactionSuccessful() + } finally { + db.endTransaction() + } + } + + override suspend fun records(): List = withContext(Dispatchers.IO) { + // Newest batch first and stable inside a batch, so a trash screen reads in the order + // things were deleted rather than in whatever order the storage returns. + helper.readableDatabase.query( + TABLE, + null, + null, + null, + null, + null, + "$COLUMN_STAGED_AT DESC, $COLUMN_BATCH_ID DESC, $COLUMN_STAGED_PATH ASC", + ).use { cursor -> + buildList { + while (cursor.moveToNext()) add(cursor.toRecord()) + } + } + } + + override suspend fun forget(batchId: String) = withContext(Dispatchers.IO) { + helper.writableDatabase.delete(TABLE, "$COLUMN_BATCH_ID = ?", arrayOf(batchId)) + Unit + } + + private fun TrashRecord.toValues(): ContentValues = ContentValues().apply { + put(COLUMN_STAGED_PATH, stagedPath.value) + put(COLUMN_BATCH_ID, batchId) + put(COLUMN_BATCH_FOLDER, batchFolder.value) + put(COLUMN_ORIGINAL_PATH, originalPath.value) + put(COLUMN_NAME, name) + put(COLUMN_IS_DIRECTORY, if (isDirectory) 1 else 0) + put(COLUMN_SIZE_BYTES, sizeBytes) + put(COLUMN_STAGED_AT, stagedAtMillis) + } + + private fun Cursor.toRecord(): TrashRecord = TrashRecord( + batchId = getString(getColumnIndexOrThrow(COLUMN_BATCH_ID)), + batchFolder = StoragePath.of(getString(getColumnIndexOrThrow(COLUMN_BATCH_FOLDER))), + originalPath = StoragePath.of(getString(getColumnIndexOrThrow(COLUMN_ORIGINAL_PATH))), + stagedPath = StoragePath.of(getString(getColumnIndexOrThrow(COLUMN_STAGED_PATH))), + name = getString(getColumnIndexOrThrow(COLUMN_NAME)), + isDirectory = getInt(getColumnIndexOrThrow(COLUMN_IS_DIRECTORY)) != 0, + sizeBytes = getLong(getColumnIndexOrThrow(COLUMN_SIZE_BYTES)), + stagedAtMillis = getLong(getColumnIndexOrThrow(COLUMN_STAGED_AT)), + ) + + private class Helper(context: Context) : + SQLiteOpenHelper(context, DATABASE_NAME, null, DATABASE_VERSION) { + + override fun onCreate(db: SQLiteDatabase) { + db.execSQL( + """ + CREATE TABLE $TABLE ( + $COLUMN_STAGED_PATH TEXT NOT NULL PRIMARY KEY, + $COLUMN_BATCH_ID TEXT NOT NULL, + $COLUMN_BATCH_FOLDER TEXT NOT NULL, + $COLUMN_ORIGINAL_PATH TEXT NOT NULL, + $COLUMN_NAME TEXT NOT NULL, + $COLUMN_IS_DIRECTORY INTEGER NOT NULL, + $COLUMN_SIZE_BYTES INTEGER NOT NULL, + $COLUMN_STAGED_AT INTEGER NOT NULL + ) + """.trimIndent(), + ) + db.execSQL("CREATE INDEX trashed_batch ON $TABLE ($COLUMN_BATCH_ID)") + db.execSQL("CREATE INDEX trashed_staged_at ON $TABLE ($COLUMN_STAGED_AT)") + } + + override fun onUpgrade(db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { + // Dropping rather than migrating, which is only acceptable while nothing has + // shipped: unlike every other kind of cache here, this one cannot be rebuilt + // from the device, because the device never recorded where the items came from. + // A released version needs a real migration. + db.execSQL("DROP TABLE IF EXISTS $TABLE") + onCreate(db) + } + } + + private companion object { + const val DATABASE_NAME = "trash-index.db" + const val DATABASE_VERSION = 1 + const val TABLE = "trashed" + const val COLUMN_STAGED_PATH = "staged_path" + const val COLUMN_BATCH_ID = "batch_id" + const val COLUMN_BATCH_FOLDER = "batch_folder" + const val COLUMN_ORIGINAL_PATH = "original_path" + const val COLUMN_NAME = "name" + const val COLUMN_IS_DIRECTORY = "is_directory" + const val COLUMN_SIZE_BYTES = "size_bytes" + const val COLUMN_STAGED_AT = "staged_at" + } +} diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/TrashIndex.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/TrashIndex.kt new file mode 100644 index 0000000..76314ad --- /dev/null +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/TrashIndex.kt @@ -0,0 +1,61 @@ +package de.softwareapp_hb.filemanager.data + +import de.softwareapp_hb.filemanager.core.StoragePath + +/** + * One staged delete: an item sitting in the trash and the path it came from. + * + * [sizeBytes] is the entry's own size, not a folder's recursive total — measuring a tree at + * delete time would make deleting as slow as copying. + */ +data class TrashRecord( + val batchId: String, + val batchFolder: StoragePath, + val originalPath: StoragePath, + val stagedPath: StoragePath, + val name: String, + val isDirectory: Boolean, + val sizeBytes: Long, + val stagedAtMillis: Long, +) + +/** + * The durable memory of what is in the trash. + * + * Without one, a staged delete is only recoverable for as long as the snackbar is on screen: + * the files are still on disk, but nothing remembers which folder they came from, so not + * even the user can put them back by hand with any confidence. + * + * **This is an index, not the truth.** The staged files are the truth, and the two can + * disagree: a user can delete `.plainfiles-trash` themselves, an SD card can be removed, or + * app data can be cleared while the folders stay. So a reader must drop records whose file + * has gone, and a sweeper must clear folders no record mentions — see [Trash] for both. + * + * It lives in app-private storage, which is the point of having it rather than a manifest + * beside the files: the index names folders all over the device, and that is nobody else's + * business. + */ +interface TrashIndex { + + /** Add [records], replacing any that describe the same staged path. */ + suspend fun record(records: List) + + /** Every record, newest batch first. */ + suspend fun records(): List + + /** Drop everything about one batch. */ + suspend fun forget(batchId: String) + + /** + * No index. The trash still stages and still sweeps, it just cannot offer anything back + * after the process that deleted it is gone — which is the default for a `Trash` built + * without one, and what the tests that do not care about persistence use. + */ + object None : TrashIndex { + override suspend fun record(records: List) = Unit + + override suspend fun records(): List = emptyList() + + override suspend fun forget(batchId: String) = Unit + } +} diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/domain/Trash.kt b/app/src/main/java/de/softwareapp_hb/filemanager/domain/Trash.kt index 21faac5..59833f4 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/domain/Trash.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/domain/Trash.kt @@ -3,6 +3,8 @@ package de.softwareapp_hb.filemanager.domain import de.softwareapp_hb.filemanager.core.ConflictPolicy import de.softwareapp_hb.filemanager.core.StoragePath import de.softwareapp_hb.filemanager.data.StorageSource +import de.softwareapp_hb.filemanager.data.TrashIndex +import de.softwareapp_hb.filemanager.data.TrashRecord import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.isActive import kotlin.coroutines.cancellation.CancellationException @@ -11,7 +13,12 @@ import kotlin.coroutines.cancellation.CancellationException data class TrashedEntry(val original: StoragePath, val staged: StoragePath) /** One staged delete: everything [Trash.stage] moved aside, and where it went. */ -data class TrashBatch(val folder: StoragePath, val entries: List) +data class TrashBatch( + val folder: StoragePath, + val entries: List, + /** When it was staged, so a trash screen can say how long ago. */ + val stagedAtMillis: Long, +) /** * A staging folder that makes a delete undoable. @@ -33,12 +40,18 @@ data class TrashBatch(val folder: StoragePath, val entries: List) * - **Retention is a time window, not a bin to manage.** [purgeExpired] drops batches older * than [RETENTION_HOURS]; the caller runs it after staging, so space comes back without * the app needing a startup hook or the user needing to empty anything. + * - **Where each item came from is remembered in a [TrashIndex]**, not on the volume. The + * staged files are the truth and the index is only an index, so the two are reconciled in + * both directions: [restorable] drops rows whose file has gone, and [purgeExpired] sweeps + * folders no row mentions. * - **A delete that went through the platform's own dialog is never staged.** Android owns * that deletion, and its own trash with it, so it is not ours to reverse. */ class Trash( private val source: StorageSource, private val volumeRoot: StoragePath, + /** Remembers where each staged item came from; [TrashIndex.None] forgets it again. */ + private val index: TrashIndex = TrashIndex.None, private val clock: () -> Long = System::currentTimeMillis, ) { @@ -59,7 +72,8 @@ class Trash( return null to OperationResult(OperationKind.DELETE, 0, emptyList(), false) } - val batch = root().child(BATCH_PREFIX + clock()) + val stagedAt = clock() + val batch = root().child(BATCH_PREFIX + stagedAt) val operations = FileOperations(source) val staged = ArrayList() val failures = ArrayList() @@ -101,7 +115,28 @@ class Trash( } val outcome = OperationResult(OperationKind.DELETE, staged.size, failures, cancelled) - return batch.takeIf { staged.isNotEmpty() }?.let { TrashBatch(it, staged) } to outcome + if (staged.isEmpty()) return null to outcome + + // Recorded *after* the move, never before: a row is only worth having next to a file + // that is really there. A crash between the two leaves a folder nothing indexes, + // which [purgeExpired] sweeps; the other order would leave a row that can never + // restore anything. + index.record( + staged.map { entry -> + val stat = source.stat(entry.staged) + TrashRecord( + batchId = batch.name, + batchFolder = batch, + originalPath = entry.original, + stagedPath = entry.staged, + name = entry.original.name, + isDirectory = stat?.isDirectory ?: false, + sizeBytes = stat?.sizeBytes ?: 0L, + stagedAtMillis = stagedAt, + ) + }, + ) + return TrashBatch(batch, staged, stagedAt) to outcome } /** @@ -144,24 +179,49 @@ class Trash( } } + // Forgotten only once everything is accounted for: a partial restore keeps its rows + // so a second attempt still knows what it was trying to put back. + if (failures.isEmpty()) index.forget(batch.folder.name) + return OperationResult(OperationKind.MOVE, restored, failures, false) } /** Remove batches older than [RETENTION_HOURS]. Returns how many were dropped. */ suspend fun purgeExpired(): Int { + // Rows whose batch folder has gone are dead weight — the user emptied the trash by + // hand, or a card was pulled. Nothing can restore them, so let them go first. + for (record in index.records()) { + if (!source.exists(record.batchFolder)) index.forget(record.batchId) + } + val root = root() if (!source.exists(root)) return 0 val expiry = clock() - RETENTION_MILLIS var purged = 0 for (child in source.list(root)) { + // The age lives in the folder name, so a folder no row mentions is still swept. + // That is the reconciliation for a crash between the move and the index write. val stagedAt = batchTimestamp(child.name) ?: continue if (stagedAt >= expiry) continue - if (source.delete(child.path)) purged++ + if (source.delete(child.path)) { + index.forget(child.name) + purged++ + } } return purged } + /** + * What the index says is still recoverable on this volume, newest batch first. + * + * Records whose staged file has gone are skipped as they are found: the index is an + * index, and a row that no longer describes a real file cannot restore anything. This is + * what makes a delete reversible after the app has been restarted — the snackbar handle + * never could, because it only ever lived in memory. + */ + suspend fun restorable(): List = restorableFrom(source, index) + /** * The epoch millis a batch folder was created, or `null` when the name is not one of * ours. @@ -182,8 +242,36 @@ class Trash( * The trash a batch belongs to, derived from where the batch actually sits — * `/.plainfiles-trash/batch-n` — so a restore needs no volume lookup. */ - fun of(source: StorageSource, batch: TrashBatch): Trash = - Trash(source, batch.folder.parent?.parent ?: StoragePath.Root) + fun of(source: StorageSource, batch: TrashBatch, index: TrashIndex = TrashIndex.None): Trash = + Trash(source, batch.folder.parent?.parent ?: StoragePath.Root, index) + + /** + * Everything [index] can still restore, newest batch first. + * + * No `Trash` instance is needed because reading does not care which volume a batch + * sits on: every record already carries its own folder. Asking per volume would + * return the same batches once per volume. + */ + suspend fun restorableFrom(source: StorageSource, index: TrashIndex): List { + val grouped = LinkedHashMap() + for (record in index.records()) { + if (!source.exists(record.stagedPath)) continue + val group = grouped.getOrPut(record.batchId) { + Group(record.batchFolder, record.stagedAtMillis, mutableListOf()) + } + group.entries += TrashedEntry( + original = record.originalPath, + staged = record.stagedPath, + ) + } + return grouped.values.map { TrashBatch(it.folder, it.entries, it.stagedAtMillis) } + } + + private class Group( + val folder: StoragePath, + val stagedAtMillis: Long, + val entries: MutableList, + ) private const val BATCH_PREFIX = "batch-" diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt index 78261d7..1d87aff 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt @@ -66,6 +66,7 @@ import de.softwareapp_hb.filemanager.R import de.softwareapp_hb.filemanager.core.ConflictPolicy import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.StoragePath +import de.softwareapp_hb.filemanager.ui.trash.TrashScreen import de.softwareapp_hb.filemanager.ui.usage.UsageScreen import de.softwareapp_hb.filemanager.ui.viewer.ImageViewerScreen import de.softwareapp_hb.filemanager.ui.viewer.TextEditorScreen @@ -189,6 +190,16 @@ fun BrowserScreen( return } + is Overlay.Trash -> { + TrashScreen( + batches = overlay.batches, + loading = overlay.loading, + onRestore = viewModel::restoreFromTrash, + onClose = viewModel::closeOverlay, + ) + return + } + Overlay.None -> Unit } @@ -237,6 +248,7 @@ fun BrowserScreen( onPaste = viewModel::paste, onRefresh = viewModel::refresh, onUsage = viewModel::openUsage, + onTrash = viewModel::openTrash, ) }, floatingActionButton = { diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserTopBar.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserTopBar.kt index cbb3501..f834835 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserTopBar.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserTopBar.kt @@ -5,6 +5,7 @@ import androidx.compose.foundation.layout.width import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.filled.ArrowBack import androidx.compose.material.icons.filled.Close +import androidx.compose.material.icons.filled.Delete import androidx.compose.material.icons.filled.MoreVert import androidx.compose.material.icons.filled.PieChart import androidx.compose.material.icons.filled.ContentPaste @@ -46,6 +47,7 @@ fun BrowserTopBar( onPaste: () -> Unit, onRefresh: () -> Unit, onUsage: () -> Unit, + onTrash: () -> Unit, ) { var menuOpen by remember { mutableStateOf(false) } @@ -109,6 +111,11 @@ fun BrowserTopBar( leadingIcon = { Icon(Icons.Filled.PieChart, contentDescription = null) }, onClick = { menuOpen = false; onUsage() }, ) + DropdownMenuItem( + text = { Text(stringResource(R.string.browser_action_trash)) }, + leadingIcon = { Icon(Icons.Filled.Delete, contentDescription = null) }, + onClick = { menuOpen = false; onTrash() }, + ) DropdownMenuItem( text = { Text(stringResource(R.string.browser_action_refresh)) }, leadingIcon = { Icon(Icons.Filled.Refresh, contentDescription = null) }, diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt index e9a465a..43fae95 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt @@ -102,6 +102,17 @@ sealed interface Overlay { val nodes: List, val loading: Boolean, ) : Overlay + + /** + * What the trash still holds, rebuilt from the index rather than from memory. + * + * The snackbar's undo handle dies with the process, so this is the only way back to a + * delete from earlier. + */ + data class Trash( + val batches: List, + val loading: Boolean, + ) : Overlay } /** diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt index b586875..54f1b89 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModel.kt @@ -26,6 +26,7 @@ import de.softwareapp_hb.filemanager.data.IncomingSource import de.softwareapp_hb.filemanager.data.StorageAccessMode import de.softwareapp_hb.filemanager.data.StorageLocation import de.softwareapp_hb.filemanager.data.StorageSource +import de.softwareapp_hb.filemanager.data.TrashIndex import de.softwareapp_hb.filemanager.domain.ArchiveEngine import de.softwareapp_hb.filemanager.domain.FileOperations import de.softwareapp_hb.filemanager.domain.FileSearch @@ -34,6 +35,7 @@ import de.softwareapp_hb.filemanager.domain.OperationKind import de.softwareapp_hb.filemanager.domain.OperationNotifier import de.softwareapp_hb.filemanager.domain.OperationResult import de.softwareapp_hb.filemanager.domain.Trash +import de.softwareapp_hb.filemanager.domain.TrashBatch import de.softwareapp_hb.filemanager.domain.UsageAnalysis import de.softwareapp_hb.filemanager.domain.UsageNode import kotlinx.coroutines.CancellationException @@ -67,6 +69,7 @@ class BrowserViewModel( private val deleteRequests: DeleteRequestFactory = DeleteRequestFactory { _, _ -> null }, private val savedState: SavedStateHandle = SavedStateHandle(), private val incoming: IncomingSource? = null, + private val trashIndex: TrashIndex = TrashIndex.None, ) : ViewModel() { private val _state = MutableStateFlow(restoredViewState()) @@ -466,6 +469,61 @@ class BrowserViewModel( } } + /** + * Show what the trash still holds. + * + * Rebuilt from the index, so it includes deletes made in earlier sessions — which is the + * whole difference between this and the snackbar's undo, whose handle died with the + * process that made it. + */ + fun openTrash() { + val source = storage.activeSource() ?: return + _state.update { it.copy(overlay = Overlay.Trash(emptyList(), loading = true)) } + + viewModelScope.launch { + val batches = runCatching { Trash.restorableFrom(source, trashIndex) } + .getOrDefault(emptyList()) + _state.update { current -> + val overlay = current.overlay as? Overlay.Trash ?: return@update current + current.copy(overlay = overlay.copy(batches = batches, loading = false)) + } + } + } + + /** + * Put one batch back where it came from, then re-read the trash. + * + * A name taken again in the meantime is not overwritten — see [Trash.restore] — so the + * item comes back beside whatever holds the name now. + */ + fun restoreFromTrash(batch: TrashBatch) { + val source = storage.activeSource() ?: return + + viewModelScope.launch { + _state.update { it.copy(loading = true) } + notifier.start(OperationKind.MOVE) + val result = try { + Trash.of(source, batch, trashIndex).restore(batch) { notifier.update(it) } + } catch (cancellation: CancellationException) { + throw cancellation + } finally { + notifier.stop() + } + _state.update { it.copy(loading = false) } + + if (result.succeeded) { + messagePlural(R.plurals.message_undone, result.completed) + } else { + report(result) + } + + // Re-read rather than dropping the batch locally: the truth is the index *and* + // the filesystem, and a partial restore changes both. + openTrash() + _state.value.path?.let { load(it) } + } + } + fun closeOverlay() { _state.update { it.copy(overlay = Overlay.None) } } @@ -717,7 +775,9 @@ class BrowserViewModel( is Undo.Relocate -> relocateBack(source, pending.moves) is Undo.Trashed -> - Trash.of(source, pending.batch).restore(pending.batch) { notifier.update(it) } + Trash.of(source, pending.batch, trashIndex).restore(pending.batch) { + notifier.update(it) + } } } catch (cancellation: CancellationException) { throw cancellation @@ -849,7 +909,7 @@ class BrowserViewModel( */ private suspend fun deleteDirectly(paths: List) { val source = storage.activeSource() ?: return - val trash = Trash(source, volumeRootFor(paths)) + val trash = Trash(source, volumeRootFor(paths), trashIndex) _state.update { it.copy(loading = true, selection = emptySet()) } notifier.start(OperationKind.DELETE) @@ -987,6 +1047,7 @@ class BrowserViewModel( deleteRequests = app.deleteRequests, savedState = createSavedStateHandle(), incoming = app.incoming, + trashIndex = app.trashIndex, ) } } diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/trash/TrashScreen.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/trash/TrashScreen.kt new file mode 100644 index 0000000..ac3304a --- /dev/null +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/trash/TrashScreen.kt @@ -0,0 +1,115 @@ +package de.softwareapp_hb.filemanager.ui.trash + +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.lazy.LazyColumn +import androidx.compose.foundation.lazy.items +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.Close +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton +import androidx.compose.material3.LinearProgressIndicator +import androidx.compose.material3.ListItem +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Scaffold +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.material3.TopAppBar +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.style.TextOverflow +import de.softwareapp_hb.filemanager.R +import de.softwareapp_hb.filemanager.core.Timestamps +import de.softwareapp_hb.filemanager.domain.TrashBatch +import de.softwareapp_hb.filemanager.ui.browser.EmptyState +import de.softwareapp_hb.filemanager.ui.browser.itemCountLabel + +/** + * What the trash still holds, and the way to put it back. + * + * A row is one *batch*, not one file: a batch is what a single delete did, which is the unit + * the user performed and the unit the index records. Restoring a batch is therefore also the + * unit that makes sense — "put back what I deleted at 14:20", not "resurrect one file". + * + * This is the screen the snackbar could never be. Its undo handle lived in memory, so a + * delete from a previous session was unreachable even though the files were sitting right + * there on the volume. + */ +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun TrashScreen( + batches: List, + loading: Boolean, + onRestore: (TrashBatch) -> Unit, + onClose: () -> Unit, +) { + Scaffold( + modifier = Modifier.fillMaxSize(), + topBar = { + TopAppBar( + title = { Text(stringResource(R.string.trash_title)) }, + navigationIcon = { + IconButton(onClick = onClose) { + Icon( + imageVector = Icons.Filled.Close, + contentDescription = stringResource(R.string.browser_action_close), + ) + } + }, + ) + }, + ) { innerPadding -> + Box( + modifier = Modifier + .fillMaxSize() + .padding(innerPadding), + ) { + if (batches.isEmpty() && !loading) { + EmptyState( + message = stringResource(R.string.trash_empty), + modifier = Modifier.fillMaxSize(), + ) + } else { + LazyColumn(modifier = Modifier.fillMaxSize()) { + items(items = batches, key = { it.folder.value }) { batch -> + TrashRow(batch = batch, onRestore = { onRestore(batch) }) + } + } + } + + if (loading) { + LinearProgressIndicator( + modifier = Modifier.fillMaxWidth().align(Alignment.TopCenter), + ) + } + } + } +} + +@Composable +private fun TrashRow(batch: TrashBatch, onRestore: () -> Unit) { + ListItem( + headlineContent = { Text(itemCountLabel(batch.entries.size)) }, + supportingContent = { + Text( + text = listOfNotNull( + Timestamps.format(batch.stagedAtMillis).takeIf { it.isNotEmpty() }, + batch.entries.joinToString(", ") { it.original.name }, + ).joinToString(" · "), + style = MaterialTheme.typography.bodySmall, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + ) + }, + trailingContent = { + TextButton(onClick = onRestore) { + Text(stringResource(R.string.trash_restore)) + } + }, + ) +} diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index ec6d5c3..bbc44cf 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -64,6 +64,12 @@ Save here Navigate to a file and tap it to hand it back Cancel + + + Trash + Nothing in the trash + Restore + Trash More options Refresh Cancel diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt new file mode 100644 index 0000000..d283242 --- /dev/null +++ b/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteTrashIndexTest.kt @@ -0,0 +1,88 @@ +package de.softwareapp_hb.filemanager.data + +import de.softwareapp_hb.filemanager.core.StoragePath +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.annotation.Config + +/** + * The index is the only thing that makes a staged delete reversible after a restart, so it is + * worth proving against real SQLite rather than only against a map. Ordering and + * replace-on-conflict are exactly where a fake would paper over a mistake. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class SqliteTrashIndexTest { + + private val batch = StoragePath.of("/storage/emulated/0/.plainfiles-trash/batch-1000") + + private fun index() = SqliteTrashIndex(RuntimeEnvironment.getApplication()) + + private fun record( + name: String, + stagedAt: Long = 1000L, + batchId: String = "batch-1000", + ) = TrashRecord( + batchId = batchId, + batchFolder = batch, + originalPath = StoragePath.of("/storage/emulated/0/Documents/$name"), + stagedPath = batch.child("0").child(name), + name = name, + isDirectory = false, + sizeBytes = 42L, + stagedAtMillis = stagedAt, + ) + + @Test + fun `a recorded item reads back whole`() = runBlocking { + val index = index() + index.record(listOf(record("report.pdf"))) + + val row = index.records().single() + + assertEquals("report.pdf", row.name) + assertEquals("/storage/emulated/0/Documents/report.pdf", row.originalPath.value) + assertEquals(batch, row.batchFolder) + assertEquals(42L, row.sizeBytes) + assertEquals(1000L, row.stagedAtMillis) + } + + @Test + fun `recording the same staged path twice replaces instead of duplicating`() = runBlocking { + val index = index() + index.record(listOf(record("a.txt"))) + index.record(listOf(record("a.txt"))) + + assertEquals(1, index.records().size) + } + + @Test + fun `records come back newest batch first`() = runBlocking { + val index = index() + index.record(listOf(record("old.txt", stagedAt = 1000L, batchId = "batch-1000"))) + index.record(listOf(record("new.txt", stagedAt = 2000L, batchId = "batch-2000"))) + + assertEquals(listOf("new.txt", "old.txt"), index.records().map { it.name }) + } + + @Test + fun `forgetting one batch leaves the others alone`() = runBlocking { + val index = index() + index.record(listOf(record("a.txt", stagedAt = 1000L, batchId = "batch-1000"))) + index.record(listOf(record("b.txt", stagedAt = 2000L, batchId = "batch-2000"))) + + index.forget("batch-1000") + + assertEquals(listOf("b.txt"), index.records().map { it.name }) + } + + @Test + fun `an empty index reads back empty rather than throwing`() = runBlocking { + assertTrue(index().records().isEmpty()) + } +} diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/domain/TrashTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/domain/TrashTest.kt index 271b286..82fa816 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/domain/TrashTest.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/domain/TrashTest.kt @@ -1,7 +1,9 @@ package de.softwareapp_hb.filemanager.domain import de.softwareapp_hb.filemanager.core.StoragePath +import de.softwareapp_hb.filemanager.data.TrashIndex import de.softwareapp_hb.filemanager.domain.testutil.InMemoryStorageSource +import de.softwareapp_hb.filemanager.domain.testutil.InMemoryTrashIndex import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -28,8 +30,11 @@ class TrashTest { addFile(docs.child("report.pdf"), "report") } - private fun trash(source: InMemoryStorageSource, at: Long = TRASHED_AT) = - Trash(source, volume, clock = { at }) + private fun trash( + source: InMemoryStorageSource, + at: Long = TRASHED_AT, + index: TrashIndex = TrashIndex.None, + ) = Trash(source, volume, index, clock = { at }) @Test fun `staging moves an item aside and records where it came from`() = runBlocking { @@ -153,6 +158,93 @@ class TrashTest { assertEquals(volume.child(Trash.TRASH_DIR_NAME), trash(storage()).root()) } + @Test + fun `staging records where the item came from`() = runBlocking { + val storage = storage() + val index = InMemoryTrashIndex() + + trash(storage, index = index).stage(listOf(notes)) + + val row = index.records().single() + assertEquals(notes, row.originalPath) + assertEquals("notes.txt", row.name) + assertFalse(row.isDirectory) + assertEquals(5L, row.sizeBytes) + } + + @Test + fun `a staged delete is still recoverable after the process forgets everything`() = runBlocking { + // The point of the index. The snackbar's handle is in-memory only, so before this a + // restart turned a staged delete into a folder nobody could place. + val storage = storage() + val index = InMemoryTrashIndex() + trash(storage, index = index).stage(listOf(notes, docs)) + assertFalse(storage.contains(notes)) + + // A brand-new Trash, as a relaunched app would build: it shares only the index. + val reopened = trash(storage, index = index) + val batches = reopened.restorable() + assertEquals(1, batches.size) + + val result = reopened.restore(batches.single()) + + assertTrue(result.succeeded) + assertTrue(storage.contains(notes)) + assertTrue(storage.contains(docs)) + assertEquals("hello", storage.contentOf(notes)) + assertEquals("report", storage.contentOf(docs.child("report.pdf"))) + } + + @Test + fun `the index is emptied once a restore has succeeded`() = runBlocking { + val storage = storage() + val index = InMemoryTrashIndex() + val (batch, _) = trash(storage, index = index).stage(listOf(notes)) + + trash(storage, index = index).restore(batch!!) + + assertTrue(index.records().isEmpty()) + } + + @Test + fun `a row whose file has gone is not offered back`() = runBlocking { + val storage = storage() + val index = InMemoryTrashIndex() + trash(storage, index = index).stage(listOf(notes)) + // Someone emptied the folder by hand, or a card was pulled out. + storage.delete(index.records().single().stagedPath) + + assertTrue(trash(storage, index = index).restorable().isEmpty()) + } + + @Test + fun `purging forgets the rows it deletes`() = runBlocking { + val storage = storage() + val index = InMemoryTrashIndex() + val (old, _) = trash(storage, at = 0L, index = index).stage(listOf(notes)) + trash(storage, at = 0L, index = index).restore(old!!) + val (fresh, _) = trash(storage, at = Trash.RETENTION_MILLIS + 1, index = index) + .stage(listOf(notes)) + + trash(storage, at = Trash.RETENTION_MILLIS + 2, index = index).purgeExpired() + + val left = index.records() + assertEquals(1, left.size) + assertEquals(fresh!!.folder.name, left.single().batchId) + } + + @Test + fun `purging drops rows whose folder has gone, so the index cannot grow forever`() = runBlocking { + val storage = storage() + val index = InMemoryTrashIndex() + val (batch, _) = trash(storage, index = index).stage(listOf(notes)) + storage.delete(batch!!.folder) + + trash(storage, index = index).purgeExpired() + + assertTrue(index.records().isEmpty()) + } + private companion object { const val TRASHED_AT = 1_700_000_000_000L } diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryTrashIndex.kt b/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryTrashIndex.kt new file mode 100644 index 0000000..76b9684 --- /dev/null +++ b/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryTrashIndex.kt @@ -0,0 +1,27 @@ +package de.softwareapp_hb.filemanager.domain.testutil + +import de.softwareapp_hb.filemanager.data.TrashIndex +import de.softwareapp_hb.filemanager.data.TrashRecord + +/** + * A [TrashIndex] in a map, for exercising the trash without a database. + * + * Ordered the way [de.softwareapp_hb.filemanager.data.SqliteTrashIndex] orders, so a test + * that passes here says something about the real one. + */ +class InMemoryTrashIndex : TrashIndex { + + private val rows = LinkedHashMap() + + override suspend fun record(records: List) { + for (record in records) rows[record.stagedPath.value] = record + } + + override suspend fun records(): List = rows.values.sortedWith( + compareByDescending { it.stagedAtMillis }.thenBy { it.stagedPath.value }, + ) + + override suspend fun forget(batchId: String) { + rows.filterValues { it.batchId == batchId }.keys.toList().forEach(rows::remove) + } +} diff --git a/docs/EDGE_CASE_TESTING.md b/docs/EDGE_CASE_TESTING.md index d516f4e..83029c1 100644 --- a/docs/EDGE_CASE_TESTING.md +++ b/docs/EDGE_CASE_TESTING.md @@ -50,10 +50,13 @@ An udev rule widening the mode for Android vendors fixes it ## Not verified, and why -- **Long-press multi-select via adb.** `input swipe x y x y 900` is delivered as a plain - tap on this MIUI build, so the selection action bar (and therefore compress/extract from - the UI) could not be driven from the host. The archive engine itself is covered by - end-to-end JVM tests, including a zip-slip archive. **Worth a manual pass.** +- **Long-press multi-select via adb: it works, but not every time.** `input swipe x y x y 900` + *does* open the selection action bar on this MIUI build — it was driven that way to reach + Delete, Rename and Compress — but the same gesture has also landed as a plain tap, which + opens the row instead of selecting it. So the bar is reachable from the host, and yet + compress/extract from the UI is still worth a manual pass rather than trusted to a scripted + gesture. The archive engine itself is covered by end-to-end JVM tests, including a zip-slip + archive. - **The SAF folder-picker round trip.** Driving the system picker and granting a tree was not attempted; the code path is unit-tested against a stand-in `DocumentsProvider`. - **Image viewer and thumbnails.** Verified on the AVD in M4/M5; not repeated on the phone.