diff --git a/CHANGELOG.md b/CHANGELOG.md index fca1c67..4434c68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -168,6 +168,33 @@ upload needs a **higher `versionCode`** than every previous upload. path in the row, and a rebuild reported progress and re-answered the query when it finished. Two honesty bugs found by watching it: the empty state claimed "Nothing matches" *while* the index was still being written, and two progress bars stacked on each other. + - **…and it no longer re-walks the volume to stay current.** A search everywhere used to + rebuild the whole index once it was older than fifteen minutes, which meant paying for a walk + of the entire volume every session — on a phone that is tens of seconds of reading before a + single result. Three changes remove almost all of it. **The answer comes first**: whatever + the index already knows is shown immediately, and it is brought up to date *behind* that + answer, so no walk is ever between the user and their results. **Staying current is now a + catch-up, not a walk**: `StorageSource.names` lists a folder's names without reading a size + and a date for every child — three `stat` round trips per entry is what made the walk slow — + and the catch-up compares those names against the ones already indexed, reading metadata only + for names it has never seen. **A walk is now the exception**: it runs for a volume that has + never been indexed, for one whose rows were built under the other hidden-files preference + (the rows mean different things, so they cannot be caught up), for one not walked in full for + a week, or when the user taps Rebuild. A catch-up compares names alone, so those are exactly + the things it structurally cannot see: a file swapped for a different file of the same name, + and the preference's effect on what was ever counted. Two timestamps per volume keep that + honest, which is a schema change — v2 adds `full_at` and `hidden_included` with a real + migration that keeps the rows, because throwing them away would cost the next search a walk + it need not do. A folder that cannot be read now answers `null` rather than "empty", and a + catch-up leaves what it knows about it exactly as it was: deleting rows on a transient + failure is how an index loses things that are still there. Covered by tests that assert what + a pass *reads* rather than only what it writes — a fresh index makes no call to the storage + at all, a stale one reads only the new names, and a walk and a catch-up over a real temporary + tree keep the index level with files added and deleted between them. **288 JVM tests, lint + clean.** No device pass this time: the emulator's `/dev/kvm` disappeared from this + environment partway through (the previous round's device checks had already run), so this + round's evidence is the suite plus `AllFilesSourceTest` exercising the real + `File.list()` path — which is the one piece a fake cannot stand in for. - **Six more kinds of file can be read in the app instead of guessed at.** Images and text had a preview and nothing else did, so a zip, a PDF, a video, a song or an APK offered only diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt index 518c8c8..2b01ca8 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/AllFilesSource.kt @@ -37,6 +37,15 @@ class AllFilesSource( if (children == null) emptyList() else children.map { it.toEntry() } } + /** + * One `readdir` and no `stat` per child, which is the whole reason a catch-up pass is + * cheap. `File.list()` returns `null` for a folder that cannot be read — exactly the + * "leave it alone" answer the caller wants. + */ + override suspend fun names(directory: StoragePath): List? = withContext(Dispatchers.IO) { + File(directory.value).list()?.toList() + } + override suspend fun stat(path: StoragePath): FileEntry? = withContext(Dispatchers.IO) { File(path.value).takeIf { it.exists() }?.toEntry() } diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/SafSource.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/SafSource.kt index 1f4a464..799a580 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/SafSource.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/SafSource.kt @@ -62,6 +62,29 @@ class SafSource( entries } + /** + * The same children query as [list], asking the provider for names only. + * + * A document provider answers a whole query in one round trip, so the saving here is + * smaller than on a filesystem — but it is still one cursor instead of one cursor plus a + * `FileEntry` per child, and the query is the expensive part either way. + */ + override suspend fun names(directory: StoragePath): List? = withContext(Dispatchers.IO) { + val parentId = SafDocumentIds.documentId(treeDocumentId, directory) + val childrenUri = DocumentsContract.buildChildDocumentsUriUsingTree(treeUri, parentId) + + val cursor = resolver.query(childrenUri, NAME_PROJECTION, null, null, null) + ?: return@withContext null + + val names = ArrayList() + cursor.use { + while (it.moveToNext()) { + it.getString(0)?.let(names::add) + } + } + names + } + override suspend fun stat(path: StoragePath): FileEntry? = withContext(Dispatchers.IO) { resolver.query(documentUri(path), PROJECTION, null, null, null)?.use { cursor -> if (cursor.moveToFirst()) cursor.toEntry(path) else null @@ -157,5 +180,8 @@ class SafSource( DocumentsContract.Document.COLUMN_SIZE, DocumentsContract.Document.COLUMN_LAST_MODIFIED, ) + + /** The one column a catch-up needs: what is in the folder, by name. */ + val NAME_PROJECTION = arrayOf(DocumentsContract.Document.COLUMN_DISPLAY_NAME) } } diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/SearchIndex.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/SearchIndex.kt index 4b0deef..73e83ba 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/SearchIndex.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/SearchIndex.kt @@ -19,6 +19,20 @@ data class IndexedFile( val modifiedMillis: Long, ) +/** + * What a volume's index is: how current it is, and how it was built. + * + * The two timestamps are separate on purpose. [indexedAtMillis] answers "may I trust this?"; + * [fullAtMillis] answers "when was every single entry last looked at?", which is the only + * thing that can catch what a catch-up structurally cannot — a file swapped for another of + * the same name, or a folder whose timestamps have been restored. + */ +data class VolumeStamp( + val indexedAtMillis: Long, + val fullAtMillis: Long, + val hiddenIncluded: Boolean, +) + /** * A by-name index of what is on the volumes, so a search does not have to walk them. * @@ -45,8 +59,15 @@ interface SearchIndex { /** Add a batch of rows for the volume being indexed. */ suspend fun add(rows: List) - /** Finish the pass, recording when it happened. */ - suspend fun endVolume(volume: String, indexedAtMillis: Long) + /** Finish a pass for [volume]. */ + suspend fun endVolume( + volume: String, + indexedAtMillis: Long, + /** `true` for a walk that replaced everything, `false` for a catch-up. */ + full: Boolean = false, + /** Whether this pass counted hidden entries, so a preference change can be noticed. */ + hiddenIncluded: Boolean = false, + ) /** * Rows whose name matches [query], at most [limit]. @@ -60,9 +81,24 @@ interface SearchIndex { /** Drop one row, because the file it pointed at has gone. */ suspend fun forget(path: StoragePath) + /** Drop [path] and everything the index knows beneath it. */ + suspend fun forgetTree(path: StoragePath) + + /** + * The rows for the *direct* children of [directory], and no deeper. + * + * A catch-up compares these against what the folder holds now, so what it needs is exactly + * one folder's worth — which is also why this is a query per visited folder rather than + * the whole volume held in memory. + */ + suspend fun children(directory: StoragePath): List + /** When the stalest volume was walked, or `null` when nothing has been indexed. */ suspend fun indexedAt(): Long? + /** What each volume's index is: how current, and how it was built. */ + suspend fun stamps(): Map + /** Forget everything, for a rebuild from nothing. */ suspend fun clear() @@ -75,14 +111,25 @@ interface SearchIndex { override suspend fun add(rows: List) = Unit - override suspend fun endVolume(volume: String, indexedAtMillis: Long) = Unit + override suspend fun endVolume( + volume: String, + indexedAtMillis: Long, + full: Boolean, + hiddenIncluded: Boolean, + ) = Unit override suspend fun find(query: String, limit: Int): List = emptyList() override suspend fun forget(path: StoragePath) = Unit + override suspend fun forgetTree(path: StoragePath) = Unit + + override suspend fun children(directory: StoragePath): List = emptyList() + override suspend fun indexedAt(): Long? = null + override suspend fun stamps(): Map = emptyMap() + override suspend fun clear() = Unit } } diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndex.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndex.kt index efc365b..f28a6c6 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndex.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndex.kt @@ -46,19 +46,89 @@ class SqliteSearchIndex(context: Context) : SearchIndex { } } - override suspend fun endVolume(volume: String, indexedAtMillis: Long) = withContext(Dispatchers.IO) { + override suspend fun endVolume( + volume: String, + indexedAtMillis: Long, + full: Boolean, + hiddenIncluded: Boolean, + ) = withContext(Dispatchers.IO) { helper.writableDatabase.insertWithOnConflict( TABLE_VOLUMES, null, ContentValues().apply { put(COLUMN_VOLUME, volume) put(COLUMN_INDEXED_AT, indexedAtMillis) + // A catch-up keeps the previous full-pass time: no walk has happened, and + // pretending otherwise would mean never noticing what only a walk can find. + put(COLUMN_FULL_AT, if (full) indexedAtMillis else fullAt(volume)) + put(COLUMN_HIDDEN_INCLUDED, if (hiddenIncluded) 1 else 0) }, SQLiteDatabase.CONFLICT_REPLACE, ) Unit } + override suspend fun stamps(): Map = withContext(Dispatchers.IO) { + helper.readableDatabase + .rawQuery("SELECT $COLUMN_VOLUME, $COLUMN_INDEXED_AT, $COLUMN_FULL_AT, $COLUMN_HIDDEN_INCLUDED FROM $TABLE_VOLUMES", null) + .use { cursor -> + buildMap { + while (cursor.moveToNext()) { + put( + cursor.getString(0), + VolumeStamp( + indexedAtMillis = cursor.getLong(1), + fullAtMillis = cursor.getLong(2), + hiddenIncluded = cursor.getInt(3) != 0, + ), + ) + } + } + } + } + + private fun fullAt(volume: String): Long = + helper.readableDatabase + .rawQuery("SELECT $COLUMN_FULL_AT FROM $TABLE_VOLUMES WHERE $COLUMN_VOLUME = ?", arrayOf(volume)) + .use { cursor -> if (cursor.moveToFirst()) cursor.getLong(0) else 0L } + + override suspend fun forgetTree(path: StoragePath) = withContext(Dispatchers.IO) { + val (lower, upper) = subtreeRange(path) + helper.writableDatabase.delete( + TABLE_FILES, + "$COLUMN_PATH = ? OR ($COLUMN_PATH >= ? AND $COLUMN_PATH < ?)", + arrayOf(path.value, lower, upper), + ) + Unit + } + + override suspend fun children(directory: StoragePath): List = withContext(Dispatchers.IO) { + val (lower, upper) = subtreeRange(directory) + helper.readableDatabase.query( + TABLE_FILES, + null, + // The range narrows to the subtree through the primary-key index; the `instr` then + // keeps only the direct children, which are the ones with no separator left in + // what follows the folder's own prefix. + "$COLUMN_PATH >= ? AND $COLUMN_PATH < ? AND instr(substr($COLUMN_PATH, ?), '/') = 0", + arrayOf(lower, upper, (directory.value.length + 2).toString()), + null, + null, + "$COLUMN_NAME ASC", + ).use { cursor -> buildList { while (cursor.moveToNext()) add(cursor.toRow()) } } + } + + /** + * The half-open range that contains everything inside [path] and nothing else. + * + * Every string beginning with `path/` sorts between `path/` and `path0`, because `0` is the + * character immediately after `/`. A range like this uses the primary-key index, where a + * `LIKE` would either scan the table or need its wildcards escaped — and a path is allowed + * to contain `%` or `_`, which is exactly the sort of thing an unescaped `LIKE` gets wrong. + */ + private fun subtreeRange(path: StoragePath): Pair = + "${path.value}/" to "${path.value}0" + override suspend fun find(query: String, limit: Int): List = withContext(Dispatchers.IO) { val (clause, args) = likeClause(query) helper.readableDatabase.query( @@ -173,7 +243,7 @@ class SqliteSearchIndex(context: Context) : SearchIndex { */ internal companion object { const val DATABASE_NAME = "search-index.db" - const val DATABASE_VERSION = 1 + const val DATABASE_VERSION = 2 const val TABLE_FILES = "indexed_files" const val TABLE_VOLUMES = "indexed_volumes" const val COLUMN_PATH = "path" @@ -183,6 +253,8 @@ class SqliteSearchIndex(context: Context) : SearchIndex { const val COLUMN_MODIFIED = "modified" const val COLUMN_VOLUME = "volume" const val COLUMN_INDEXED_AT = "indexed_at" + const val COLUMN_FULL_AT = "full_at" + const val COLUMN_HIDDEN_INCLUDED = "hidden_included" private const val LIKE_ESCAPE = '\\' @@ -206,8 +278,10 @@ class SqliteSearchIndex(context: Context) : SearchIndex { db.execSQL( """ CREATE TABLE $TABLE_VOLUMES ( - $COLUMN_VOLUME TEXT NOT NULL PRIMARY KEY, - $COLUMN_INDEXED_AT INTEGER NOT NULL + $COLUMN_VOLUME TEXT NOT NULL PRIMARY KEY, + $COLUMN_INDEXED_AT INTEGER NOT NULL, + $COLUMN_FULL_AT INTEGER NOT NULL DEFAULT 0, + $COLUMN_HIDDEN_INCLUDED INTEGER NOT NULL DEFAULT 0 ) """.trimIndent(), ) @@ -223,11 +297,28 @@ class SqliteSearchIndex(context: Context) : SearchIndex { */ fun upgrade(db: SQLiteDatabase, oldVersion: Int, newVersion: Int) { for (version in oldVersion until newVersion) { - error( - "No migration from search-index schema version $version to ${version + 1}. " + - "The index can be rebuilt from the device, so dropping and recreating " + - "is a valid step here — write it out deliberately and bump DATABASE_VERSION.", - ) + when (version) { + // v1 recorded only when a volume was last touched. v2 has to tell a full + // walk from a catch-up, and remember which way the hidden-files preference + // was set when it ran — both of which decide what the index may be trusted + // for. Added rather than recreated: the rows are still good, and a migration + // that threw them away would make the next search pay for a walk it need not. + 1 -> { + db.execSQL( + "ALTER TABLE $TABLE_VOLUMES ADD COLUMN $COLUMN_FULL_AT INTEGER NOT NULL DEFAULT 0", + ) + db.execSQL( + "ALTER TABLE $TABLE_VOLUMES ADD COLUMN $COLUMN_HIDDEN_INCLUDED " + + "INTEGER NOT NULL DEFAULT 0", + ) + } + + else -> error( + "No migration from search-index schema version $version to ${version + 1}. " + + "The index can be rebuilt from the device, so dropping and recreating " + + "is a valid step here — write it out deliberately and bump DATABASE_VERSION.", + ) + } } } } diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt b/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt index 75f9ac5..4d39179 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/data/StorageSource.kt @@ -38,6 +38,24 @@ interface StorageSource { /** Children of [directory], in whatever order the backend returns them (unsorted). */ suspend fun list(directory: StoragePath): List + /** + * The names in [directory], **without reading metadata for each one**. + * + * The difference is not cosmetic. [list] `stat`s every child for its size and date, and on + * a phone's mounted storage a `stat` is a round trip to another process — three of them per + * entry, which is what makes a full walk of a large volume take tens of seconds. A catch-up + * pass, whose only question is "what is here that I do not already know about?", needs + * nothing but the names. + * + * `null` when the folder could not be read. That is deliberately *not* the same as an empty + * list: a caller catching up must leave what it knows about an unreadable folder alone + * rather than conclude that everything inside it has gone. + * + * The default falls back to [list], so a backend that cannot do better still works — it + * simply pays the full price. + */ + suspend fun names(directory: StoragePath): List? = list(directory).map { it.name } + /** Metadata for [path], or `null` when it does not exist. */ suspend fun stat(path: StoragePath): FileEntry? diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/domain/SearchIndexer.kt b/app/src/main/java/de/softwareapp_hb/filemanager/domain/SearchIndexer.kt index 2139187..c274ead 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/domain/SearchIndexer.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/domain/SearchIndexer.kt @@ -2,6 +2,7 @@ package de.softwareapp_hb.filemanager.domain import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.StoragePath +import de.softwareapp_hb.filemanager.core.isHiddenName import de.softwareapp_hb.filemanager.data.IndexedFile import de.softwareapp_hb.filemanager.data.SearchIndex import de.softwareapp_hb.filemanager.data.StorageSource @@ -11,15 +12,17 @@ import kotlinx.coroutines.isActive /** * Fills a [SearchIndex] by walking a volume. * - * The walk is the same one the live search does — depth-first, hidden entries skipped unless - * the preference says otherwise — with two differences that only make sense in the - * background: it is not capped at a couple of hundred results, and it hands its rows over in - * batches rather than collecting a whole volume's listing in memory. + * Two passes, and the difference between them is the whole reason an index can be both instant + * and current: * - * Cancelling it is safe and leaves the index consistent: rows may have been added, but a - * volume's timestamp is only written once the entire pass has finished, so an interrupted - * walk leaves the old timestamp in place and reads as *older* than it is. That is the safe - * direction to be wrong in. + * - [index] walks everything and replaces the volume's rows. It reads the size and date of + * every entry, which is what makes it the expensive one. + * - [refresh] catches up: it compares the names each folder holds now against the names the + * index already has for it, and reads metadata only for what is new. + * + * Both are safe to cancel: rows may have been written, but a volume's stamp is only written + * once a pass has finished, so an interrupted pass leaves the old stamp in place and reads as + * *older* than it is. That is the safe direction to be wrong in. */ class SearchIndexer( private val source: StorageSource, @@ -27,7 +30,7 @@ class SearchIndexer( private val clock: () -> Long = System::currentTimeMillis, ) { - /** Walk everything under [volume], replacing what the index knows about it. Returns rows. */ + /** Walk everything under [volume], replacing what the index knows about it. Returns entries. */ suspend fun index( volume: StoragePath, showHidden: Boolean, @@ -72,10 +75,85 @@ class SearchIndexer( walk(volume) flush() - index.endVolume(key, clock()) + index.endVolume(key, clock(), full = true, hiddenIncluded = showHidden) return total } + /** + * Bring [volume] up to date without re-reading anything that has not changed. Returns how + * many names were looked at — the work done, which for a catch-up is most of the tree. + * + * A folder's own listing is all a *name* index needs: comparing the names it holds now + * against the names already indexed answers "what is new here?" without reading a single + * entry the index already knows. That is the difference between a walk and a catch-up — + * [StorageSource.list] reads a size and a date for every child, [StorageSource.names] reads + * only names, and on a phone's mounted storage each of those is a round trip to another + * process. + * + * Every folder still has to be *visited*: a change deep inside a tree leaves the folders + * above it untouched, so no timestamp can say "nothing below here moved", and a pass that + * trusted one would miss new files. What this cannot see, it does not invent — a file whose + * contents changed is still found by its name, and a periodic full [index] bounds the rest. + */ + suspend fun refresh( + volume: StoragePath, + showHidden: Boolean, + onProgress: (Int) -> Unit = {}, + ): Int { + val key = volume.value + var seen = 0 + var reported = 0 + + suspend fun visit(directory: StoragePath) { + if (!currentCoroutineContext().isActive) return + + // A folder that cannot be read is left exactly as it was: an empty answer here + // would look like "everything inside is gone", and deleting rows on a transient + // failure is how an index loses things that are still there. + val names = source.names(directory) ?: return + seen += names.size + if (seen - reported >= REPORT_EVERY) { + reported = seen + onProgress(seen) + } + + val known = index.children(directory).associateBy { it.name } + + val rows = ArrayList() + val folders = ArrayList() + for (name in names) { + if (!showHidden && isHiddenName(name)) continue + + val knownRow = known[name] + if (knownRow != null) { + // Known by name: nothing to read, and a folder stays a folder. + if (knownRow.isDirectory) folders += knownRow.path + continue + } + + // Only a name the index has never seen needs looking at. + val entry = source.stat(directory.child(name)) ?: continue + rows += entry.toRow(key) + if (entry.isDirectory) folders += entry.path + } + if (rows.isNotEmpty()) index.add(rows) + + // Whatever the index knew that the folder no longer holds. A folder that has gone + // takes its whole subtree with it. + for (row in known.values) { + if (names.contains(row.name) && (showHidden || !isHiddenName(row.name))) continue + if (row.isDirectory) index.forgetTree(row.path) else index.forget(row.path) + } + + for (folder in folders) visit(folder) + } + + visit(volume) + onProgress(seen) + index.endVolume(key, clock(), full = false, hiddenIncluded = showHidden) + return seen + } + private fun FileEntry.toRow(volume: String) = IndexedFile( volume = volume, path = path, 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 f4d52a4..49b23b4 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 @@ -349,11 +349,14 @@ class BrowserViewModel( /** * Search what the index knows, on every volume at once. * - * Two things make this honest rather than merely fast. Indexed rows are *candidates*: the - * filesystem is the truth, so every hit is checked and the ones whose file has gone are - * dropped — and forgotten, so the next search does not pay for them again. And the index - * is rebuilt when it is missing or old, because a search that silently omits what arrived - * since the last walk is worse than one that spends a minute being sure. + * Three rules make this honest rather than merely fast. **Indexed rows are candidates**, not + * answers: the filesystem is the truth, so every hit is checked and the ones whose file has + * gone are dropped — and forgotten, so the next search does not pay for them again. **The + * answer comes first**: whatever the index knows is shown before anything is done about + * whether it is current, because a search that waits for a walk before showing a single + * result is the thing an index exists to avoid. And **the index is brought up to date behind + * that answer**, because a search that silently omits what arrived since the last pass is + * worse than one that spends a second being sure. */ private fun searchEverywhere(query: String, source: StorageSource) { // Results can come from anywhere, so the row subtitle shows the whole parent path @@ -363,66 +366,112 @@ class BrowserViewModel( } searchJob = viewModelScope.launch { - val age = searchIndex.indexedAt() - if (age == null || clock() - age > INDEX_FRESHNESS_MILLIS) rebuild(source) + val volumes = volumes() + val stampAt = clock() + val stamps = searchIndex.stamps() + val hidden = _state.value.showHidden - val hits = runCatching { searchIndex.find(query, FileSearch.DEFAULT_LIMIT) } - .getOrDefault(emptyList()) - val confirmed = ArrayList(hits.size) - for (hit in hits) { - if (!currentCoroutineContext().isActive) return@launch - if (source.exists(hit.path)) { - confirmed += hit.toEntry() - } else { - // An index row whose file has gone is a row that has stopped being useful. - searchIndex.forget(hit.path) - } + if (stamps.isNotEmpty()) showIndexedResults(query, source) + + // A volume whose index was built with the other hidden-files preference, or which + // has not been walked end to end for a long time, needs a walk rather than a + // catch-up: a catch-up compares names, so it cannot see a name that is there both + // times but is not the file it was, and the preference is part of what rows mean. + val toWalk = volumes.filter { volume -> + val stamp = stamps[volume.value] + stamp == null || + stamp.hiddenIncluded != hidden || + stampAt - stamp.fullAtMillis > INDEX_FULL_PASS_AFTER_MILLIS } + if (toWalk.isNotEmpty()) pass(source, toWalk, full = true) + + val toCatchUp = volumes.filterNot(toWalk::contains).filter { volume -> + val stamp = stamps[volume.value] + stamp == null || stampAt - stamp.indexedAtMillis > INDEX_CATCH_UP_AFTER_MILLIS + } + if (toCatchUp.isNotEmpty()) pass(source, toCatchUp, full = false) + + // Answered again, because the pass may have found the file this search is for — + // which is the whole of "I just downloaded it, so why is it not here?". + showIndexedResults(query, source) _state.update { current -> if (current.query != query) { current } else { - current.copy( - entries = current.sort.sort(confirmed), - searching = false, - indexedAt = current.indexedAt ?: age, - ) + current.copy(searching = false, indexedAt = searchIndex.indexedAt()) } } } } - /** - * Walk every volume and replace what the index knows. - * - * Deliberately not a foreground service: a scan runs while this screen is up, and stops - * when it is not. Nothing is left half-written — an interrupted volume keeps its old - * timestamp — so stopping early costs freshness, never correctness. - */ - fun rebuildSearchIndex() { - val source = storage.activeSource() ?: return - viewModelScope.launch { - rebuild(source) - setQuery(_state.value.query) + /** Answer [query] from the index, confirming every candidate against the filesystem. */ + private suspend fun showIndexedResults(query: String, source: StorageSource) { + val hits = runCatching { searchIndex.find(query, FileSearch.DEFAULT_LIMIT) } + .getOrDefault(emptyList()) + + val confirmed = ArrayList(hits.size) + for (hit in hits) { + if (!currentCoroutineContext().isActive) return + if (source.exists(hit.path)) { + confirmed += hit.toEntry() + } else { + // An index row whose file has gone is a row that has stopped being useful. + searchIndex.forget(hit.path) + } + } + + _state.update { current -> + if (current.query != query) current else current.copy(entries = current.sort.sort(confirmed)) } } - private suspend fun rebuild(source: StorageSource) { - val volumes = storage.volumes().map { it.root }.ifEmpty { listOf(StoragePath.Root) } + /** + * Bring [volumes] up to date, and say so while it happens. + * + * A *walk* is for a volume the index has nothing good about — never built, built with a + * different hidden-files preference, or not walked in full for a week. Everything else is a + * *catch-up*, which reads names and nothing else. Neither is a foreground service: both run + * while this screen is up and stop when it is not, and stopping early costs freshness, + * never correctness. + */ + private suspend fun pass(source: StorageSource, volumes: List, full: Boolean) { val indexer = SearchIndexer(source, searchIndex, clock) _state.update { it.copy(indexing = true, indexedCount = 0) } var seen = 0 for (volume in volumes) { if (!currentCoroutineContext().isActive) break - seen += indexer.index(volume, _state.value.showHidden) { inVolume -> + val onProgress: (Int) -> Unit = { inVolume -> _state.update { it.copy(indexedCount = seen + inVolume) } } + seen += if (full) { + indexer.index(volume, _state.value.showHidden, onProgress) + } else { + indexer.refresh(volume, _state.value.showHidden, onProgress) + } } _state.update { it.copy(indexing = false, indexedAt = searchIndex.indexedAt()) } } + /** The volumes a pass should cover: every one this app can see. */ + private suspend fun volumes(): List = + storage.volumes().map { it.root }.ifEmpty { listOf(StoragePath.Root) } + + /** + * Walk every volume end to end and replace what the index knows. + * + * The explicit action, and the only thing that recovers from the index being *wrong* rather + * than merely old, which is why it stays one tap away on the search row. + */ + fun rebuildSearchIndex() { + val source = storage.activeSource() ?: return + viewModelScope.launch { + pass(source, volumes(), full = true) + setQuery(_state.value.query) + } + } + /** An indexed row as a listing row: the same shape the walk would have produced. */ private fun IndexedFile.toEntry() = FileEntry( path = path, @@ -1564,13 +1613,24 @@ class BrowserViewModel( private val SCRATCH_UNSAFE_CHARS = Regex("[^A-Za-z0-9._-]") /** - * How old the index may be before a search rebuilds it rather than trusting it. + * How old the index may be before a search catches it up. * - * Fifteen minutes is a judgement, not a measurement: long enough that a hunt through - * one folder does not re-walk a volume, short enough that "I just downloaded it" - * still finds it. What makes the number safe is that the age is on screen either way. + * A catch-up reads names and nothing else, so it is cheap enough to do whenever the + * user searches; the window is here only so that a burst of keystrokes does not repeat + * it per character. Two minutes is short enough that "I just downloaded it" works, and + * long enough that a hunt through one folder costs one pass rather than ten. */ - const val INDEX_FRESHNESS_MILLIS = 15L * 60L * 1000L + const val INDEX_CATCH_UP_AFTER_MILLIS = 2L * 60L * 1000L + + /** + * How long an index may go without a full walk. + * + * A catch-up compares names, so it cannot notice a file replaced by a different file of + * the same name, or a folder whose timestamps were restored — the two things only a walk + * sees. A week bounds how long that can go unnoticed, and it is exactly why a volume + * carries two timestamps instead of one. + */ + const val INDEX_FULL_PASS_AFTER_MILLIS = 7L * 24L * 60L * 60L * 1000L val Factory: ViewModelProvider.Factory = viewModelFactory { initializer { diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt index 4797417..52bd2e5 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/data/AllFilesSourceTest.kt @@ -38,6 +38,31 @@ class AllFilesSourceTest { assertTrue(entries.getValue("nested").isDirectory) } + @Test + fun `names lists a folder without reading metadata`() = runBlocking { + val dir = temp.newFolder("docs") + File(dir, "a.txt").writeText("hello") + File(dir, "nested").mkdirs() + + assertEquals(setOf("a.txt", "nested"), source(dir).names(path(dir))?.toSet()) + } + + @Test + fun `names of a folder that cannot be read is null, not empty`() = runBlocking { + val missing = StoragePath.of(temp.root.absolutePath).child("nope") + + // The difference matters: a catch-up has to leave an unreadable folder alone rather + // than conclude that everything inside it has gone. + assertNull(source(temp.root).names(missing)) + } + + @Test + fun `names of a file is null too, because a file holds no names`() = runBlocking { + val file = temp.newFile("a.txt") + + assertNull(source(temp.root).names(path(file))) + } + @Test fun `listing a missing directory yields nothing`() = runBlocking { val missing = StoragePath.of(temp.root.absolutePath).child("nope") diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndexTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndexTest.kt index b434faa..9e39a2c 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndexTest.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/data/SqliteSearchIndexTest.kt @@ -37,10 +37,15 @@ class SqliteSearchIndexTest { ) /** One finished pass over [volume], as an indexer would leave it. */ - private suspend fun SqliteSearchIndex.filled(vararg names: String, at: Long = 1000L) { + private suspend fun SqliteSearchIndex.filled( + vararg names: String, + at: Long = 1000L, + full: Boolean = true, + hidden: Boolean = false, + ) { beginVolume(volume) add(names.map { file(it) }) - endVolume(volume, at) + endVolume(volume, at, full = full, hiddenIncluded = hidden) } private fun names(rows: List) = rows.map { it.name } @@ -145,13 +150,108 @@ class SqliteSearchIndexTest { val db = scratchDatabase("search-migration") SqliteSearchIndex.create(db) - val failure = runCatching { SqliteSearchIndex.upgrade(db, 1, 2) } + val failure = runCatching { SqliteSearchIndex.upgrade(db, SqliteSearchIndex.DATABASE_VERSION, SqliteSearchIndex.DATABASE_VERSION + 1) } // Unlike the trash index this one *may* be dropped and rebuilt, but not by accident: // a bump with no written step is a bug to be reported. assertTrue("expected the missing step to be reported", failure.isFailure) } + /* + * The queries a catch-up is built on: one folder's rows, a subtree to drop, and the stamp + * that says whether an index may be trusted. + */ + + @Test + fun `children returns one folder's rows and nothing deeper`() = runBlocking { + val index = index() + index.beginVolume(volume) + index.add( + listOf( + file("top.txt"), + file("Documents").copy(isDirectory = true), + file("Documents/a.txt"), + file("Documents/deeper").copy(isDirectory = true), + file("Documents/deeper/b.txt"), + ), + ) + index.endVolume(volume, 1000L, full = true) + + val children = names(index.children(StoragePath.of("$volume/Documents"))) + + assertEquals(listOf("a.txt", "deeper"), children) + } + + @Test + fun `forgetting a folder takes its subtree and leaves its neighbours`() = runBlocking { + val index = index() + index.filled("holiday/beach.jpg", "holiday/notes.txt", "holidayish/still-here.txt") + + index.forgetTree(StoragePath.of("$volume/holiday")) + + // `holidayish` starts with the same letters as `holiday`, which is exactly what an + // unescaped prefix match gets wrong. + assertEquals(listOf("still-here.txt"), names(index.find("still", 10))) + } + + @Test + fun `forgetting a folder whose name contains a wildcard takes only that subtree`() = runBlocking { + val index = index() + index.filled("100%_done/a.txt", "100x-done/b.txt") + + index.forgetTree(StoragePath.of("$volume/100%_done")) + + assertEquals(listOf("b.txt"), names(index.find(".txt", 10))) + } + + @Test + fun `a catch-up keeps the time of the last full walk`() = runBlocking { + val index = index() + index.filled("a.txt", at = 1000L, full = true) + + index.endVolume(volume, 5000L, full = false, hiddenIncluded = false) + + val stamp = index.stamps().getValue(volume) + assertEquals(5000L, stamp.indexedAtMillis) + assertEquals("only a walk may move this", 1000L, stamp.fullAtMillis) + } + + @Test + fun `the stamp remembers which hidden-files preference built it`() = runBlocking { + val index = index() + index.filled("a.txt", hidden = true) + + assertTrue(index.stamps().getValue(volume).hiddenIncluded) + } + + /** + * v1 recorded only when a volume was last touched; v2 has to tell a walk from a catch-up. + * The rows are still good, so the migration adds columns instead of recreating anything — + * and a migration that threw them away would cost the next search a walk it need not do. + */ + @Test + fun `migrating from version 1 keeps the rows and adds the new columns`() { + val db = scratchDatabase("search-migration-v1") + db.execSQL( + "CREATE TABLE ${SqliteSearchIndex.TABLE_VOLUMES} " + + "(${SqliteSearchIndex.COLUMN_VOLUME} TEXT NOT NULL PRIMARY KEY, " + + "${SqliteSearchIndex.COLUMN_INDEXED_AT} INTEGER NOT NULL)", + ) + db.execSQL( + "INSERT INTO ${SqliteSearchIndex.TABLE_VOLUMES} " + + "(${SqliteSearchIndex.COLUMN_VOLUME}, ${SqliteSearchIndex.COLUMN_INDEXED_AT}) VALUES ('$volume', 1234)", + ) + + SqliteSearchIndex.upgrade(db, 1, 2) + + db.rawQuery("SELECT ${SqliteSearchIndex.COLUMN_FULL_AT}, ${SqliteSearchIndex.COLUMN_HIDDEN_INCLUDED} FROM ${SqliteSearchIndex.TABLE_VOLUMES}", null) + .use { cursor -> + assertTrue("the volume survived the migration", cursor.moveToFirst()) + assertEquals("a pre-v2 index has never been walked in full", 0L, cursor.getLong(0)) + assertEquals(0, cursor.getInt(1)) + } + } + /** A throwaway database, so a migration can be driven without disturbing the real index. */ private fun scratchDatabase(name: String): SQLiteDatabase = SQLiteDatabase.openOrCreateDatabase( diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/domain/SearchIndexerTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/domain/SearchIndexerTest.kt index a91050b..19b5f13 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/domain/SearchIndexerTest.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/domain/SearchIndexerTest.kt @@ -1,12 +1,17 @@ package de.softwareapp_hb.filemanager.domain +import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.StoragePath +import de.softwareapp_hb.filemanager.data.AllFilesSource import de.softwareapp_hb.filemanager.domain.testutil.InMemorySearchIndex import de.softwareapp_hb.filemanager.domain.testutil.InMemoryStorageSource +import java.io.File import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue +import org.junit.Rule import org.junit.Test +import org.junit.rules.TemporaryFolder /** * The walk that fills the index. It is the same shape as the live search, so what is worth @@ -15,6 +20,9 @@ import org.junit.Test */ class SearchIndexerTest { + @get:Rule + val temp = TemporaryFolder() + private val volume = StoragePath.of("/storage/emulated/0") private fun storage() = InMemoryStorageSource().apply { addDirectory(volume) } @@ -85,6 +93,171 @@ class SearchIndexerTest { assertEquals(listOf(120), index.addedBatches) } + /* + * The catch-up: the pass that runs whenever the user searches, and whose whole point is + * what it does *not* read. + */ + + @Test + fun `a catch-up reads names and the metadata of nothing it already knows`() = runBlocking { + val storage = storage().apply { + addDirectory(volume.child("Documents")) + addFile(volume.child("Documents/a.txt"), "x") + addFile(volume.child("top.txt"), "x") + } + val index = InMemorySearchIndex() + SearchIndexer(storage, index).index(volume, showHidden = false) + + storage.listCalls.clear() + storage.statCalls.clear() + storage.namesCalls.clear() + index.addedBatches.clear() + SearchIndexer(storage, index).refresh(volume, showHidden = false) + + // Three folders were looked at and not one metadata read: this is the difference + // between a catch-up and a walk, and the whole reason it can run on every search. + assertEquals(listOf(volume, volume.child("Documents")).sortedBy { it.value }, storage.namesCalls.sortedBy { it.value }) + assertTrue("a catch-up must not list with metadata", storage.listCalls.isEmpty()) + assertTrue("and must not stat what it knows", storage.statCalls.isEmpty()) + assertTrue("and must write nothing", index.addedBatches.isEmpty()) + } + + @Test + fun `a catch-up finds a file that arrived and leaves the rest alone`() = runBlocking { + val storage = storage().apply { + addDirectory(volume.child("Documents")) + addFile(volume.child("Documents/a.txt"), "x") + addFile(volume.child("top.txt"), "x") + } + val index = InMemorySearchIndex() + SearchIndexer(storage, index).index(volume, showHidden = false) + index.addedBatches.clear() + + storage.addDirectory(volume.child("DCIM")) + storage.addFile(volume.child("DCIM/new-photo.jpg"), "y") + SearchIndexer(storage, index).refresh(volume, showHidden = false) + + // Two writes of one row each: the folder that appeared, and the file inside it. + assertEquals(listOf(1, 1), index.addedBatches) + assertEquals(listOf(volume.child("DCIM/new-photo.jpg")), storage.statCalls.filter { it.name == "new-photo.jpg" }) + assertEquals(listOf("new-photo.jpg"), index.find("new-photo", 10).map { it.name }) + } + + @Test + fun `a catch-up drops a file that has gone`() = runBlocking { + val storage = storage().apply { addFile(volume.child("gone.txt"), "x") } + val index = InMemorySearchIndex() + SearchIndexer(storage, index).index(volume, showHidden = false) + + storage.delete(volume.child("gone.txt")) + SearchIndexer(storage, index).refresh(volume, showHidden = false) + + assertEquals(listOf(volume.child("gone.txt").value), index.forgotten) + assertTrue(index.find("gone", 10).isEmpty()) + } + + @Test + fun `a catch-up takes a folder that has gone with its whole subtree`() = runBlocking { + val storage = storage().apply { + addDirectory(volume.child("holiday")) + addFile(volume.child("holiday/beach.jpg"), "x") + addFile(volume.child("holiday/notes.txt"), "x") + } + val index = InMemorySearchIndex() + SearchIndexer(storage, index).index(volume, showHidden = false) + + storage.delete(volume.child("holiday")) + SearchIndexer(storage, index).refresh(volume, showHidden = false) + + assertEquals(listOf(volume.child("holiday").value), index.forgotten) + assertTrue("the folder and everything under it", index.find("holiday", 10).isEmpty()) + assertTrue(index.find("beach", 10).isEmpty()) + } + + @Test + fun `a folder that cannot be read is left exactly as it was`() = runBlocking { + val storage = storage().apply { + addDirectory(volume.child("Documents")) + addFile(volume.child("Documents/a.txt"), "x") + } + val index = InMemorySearchIndex() + SearchIndexer(storage, index).index(volume, showHidden = false) + + // An empty answer would look like "everything inside has gone"; a transient failure + // must therefore change nothing at all. + storage.unreadableNames += volume.child("Documents") + SearchIndexer(storage, index).refresh(volume, showHidden = false) + + assertTrue("nothing was forgotten on a failed listing", index.forgotten.isEmpty()) + assertEquals(listOf("a.txt"), index.find("a", 10).map { it.name }) + } + + @Test + fun `a catch-up records that it was a catch-up and not a walk`() = runBlocking { + val storage = storage().apply { addFile(volume.child("a.txt"), "x") } + val index = InMemorySearchIndex() + SearchIndexer(storage, index, clock = { 1000L }).index(volume, showHidden = false) + + SearchIndexer(storage, index, clock = { 5000L }).refresh(volume, showHidden = false) + + val stamp = index.stamps().getValue(volume.value) + assertEquals(5000L, stamp.indexedAtMillis) + // The full-pass time is what a catch-up cannot move: only a walk can see a file + // swapped for another of the same name. + assertEquals(1000L, stamp.fullAtMillis) + } + + @Test + fun `a catch-up with the other hidden-files preference forgets what it should not show`() = runBlocking { + val storage = storage().apply { + addFile(volume.child(".hidden.txt"), "x") + addFile(volume.child("visible.txt"), "x") + } + val index = InMemorySearchIndex() + SearchIndexer(storage, index).index(volume, showHidden = true) + assertEquals(2, index.find("txt", 10).size) + + SearchIndexer(storage, index).refresh(volume, showHidden = false) + + assertEquals(listOf("visible.txt"), index.find("txt", 10).map { it.name }) + } + + /** + * Both passes over a *real* filesystem, which is the only place [AllFilesSource.names] is + * exercised and the closest thing to what a device does: index a tree, change it the way a + * user would between two searches, catch up, and check the index tracked it. + */ + @Test + fun `a walk and a catch-up over a real tree keep the index level with it`() = runBlocking { + val root = temp.newFolder("sdcard") + File(root, "Documents").mkdirs() + File(root, "Documents/notes.txt").writeText("hello") + File(root, "top.txt").writeText("top") + + val volume = StoragePath.of(root.absolutePath) + val storage = AllFilesSource( + roots = listOf(FileEntry(volume, isDirectory = true, sizeBytes = 0L, lastModifiedMillis = 0L)), + ) + val index = InMemorySearchIndex() + + SearchIndexer(storage, index).index(volume, showHidden = false) + assertEquals( + setOf("Documents", "notes.txt", "top.txt"), + index.find("", 10).map { it.name }.toSet(), + ) + + // What a user does between two searches: add a file, delete one, make a folder. + File(root, "Documents/holiday.jpg").writeText("jpeg") + File(root, "top.txt").delete() + File(root, "DCIM").mkdirs() + SearchIndexer(storage, index).refresh(volume, showHidden = false) + + assertEquals(listOf("holiday.jpg"), index.find("holiday", 10).map { it.name }) + assertTrue("the deleted file has gone", index.find("top", 10).isEmpty()) + assertEquals(listOf("notes.txt"), index.find("notes", 10).map { it.name }) + assertEquals(listOf("DCIM"), index.find("DCIM", 10).map { it.name }) + } + @Test fun `the timestamp is stamped from the clock when the pass finishes`() = runBlocking { val index = InMemorySearchIndex() diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemorySearchIndex.kt b/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemorySearchIndex.kt index 3c49566..9682cd0 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemorySearchIndex.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemorySearchIndex.kt @@ -4,26 +4,33 @@ import de.softwareapp_hb.filemanager.core.NameMatcher import de.softwareapp_hb.filemanager.core.StoragePath import de.softwareapp_hb.filemanager.data.IndexedFile import de.softwareapp_hb.filemanager.data.SearchIndex +import de.softwareapp_hb.filemanager.data.VolumeStamp /** - * A [SearchIndex] in a map, for exercising the walk and the search without a database. + * A [SearchIndex] in a map, for exercising the passes and the search without a database. * * It matches with [NameMatcher] — the app's own matcher — where the real one narrows with * SQL. A test that passes here therefore says something about the app's *rules* and nothing * at all about its SQL, which is what [de.softwareapp_hb.filemanager.data.SqliteSearchIndexTest] * is for. * - * [addedBatches] records the size of every write, so a test can prove that rows are handed - * over in batches rather than one transaction per file. + * The call logs exist because the interesting difference between a walk and a catch-up is what + * they *do not* do: [beginVolumeCalls] distinguishes them, and [addedBatches] with + * [childrenQueries] shows that a catch-up writes only what changed. */ class InMemorySearchIndex : SearchIndex { private val rows = LinkedHashMap() - private val indexedAt = mutableMapOf() + private val stamps = mutableMapOf() val addedBatches: MutableList = mutableListOf() + val forgotten: MutableList = mutableListOf() + val childrenQueries: MutableList = mutableListOf() + var beginVolumeCalls: Int = 0 + private set override suspend fun beginVolume(volume: String) { + beginVolumeCalls++ rows.filterValues { it.volume == volume }.keys.toList().forEach(rows::remove) } @@ -32,8 +39,19 @@ class InMemorySearchIndex : SearchIndex { for (row in rows) this.rows[row.path.value] = row } - override suspend fun endVolume(volume: String, indexedAtMillis: Long) { - indexedAt[volume] = indexedAtMillis + override suspend fun endVolume( + volume: String, + indexedAtMillis: Long, + full: Boolean, + hiddenIncluded: Boolean, + ) { + val previous = stamps[volume] + stamps[volume] = VolumeStamp( + indexedAtMillis = indexedAtMillis, + // As in SQL: a catch-up leaves the last walk's time alone. + fullAtMillis = if (full) indexedAtMillis else previous?.fullAtMillis ?: 0L, + hiddenIncluded = hiddenIncluded, + ) } override suspend fun find(query: String, limit: Int): List = rows.values @@ -42,13 +60,27 @@ class InMemorySearchIndex : SearchIndex { .take(limit) override suspend fun forget(path: StoragePath) { + forgotten += path.value rows.remove(path.value) } - override suspend fun indexedAt(): Long? = indexedAt.values.minOrNull() + override suspend fun forgetTree(path: StoragePath) { + forgotten += path.value + val subtree = rows.keys.filter { it == path.value || it.startsWith("${path.value}/") } + subtree.forEach(rows::remove) + } + + override suspend fun children(directory: StoragePath): List { + childrenQueries += directory.value + return rows.values.filter { it.path.parent == directory }.sortedBy { it.name } + } + + override suspend fun indexedAt(): Long? = stamps.values.minOfOrNull { it.indexedAtMillis } + + override suspend fun stamps(): Map = stamps.toMap() override suspend fun clear() { rows.clear() - indexedAt.clear() + stamps.clear() } } diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryStorageSource.kt b/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryStorageSource.kt index ca3ca1f..fea90d0 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryStorageSource.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/domain/testutil/InMemoryStorageSource.kt @@ -40,6 +40,18 @@ class InMemoryStorageSource( /** Paths whose [openRead] throws, to exercise the per-item failure path. */ val unreadable = mutableSetOf() + /** Paths [names] cannot read, the way a folder can be unreadable without warning. */ + val unreadableNames = mutableSetOf() + + /** Every [list] call, so a test can prove a catch-up did not read metadata. */ + val listCalls = mutableListOf() + + /** Every [names] call, which is what a catch-up makes instead. */ + val namesCalls = mutableListOf() + + /** Every [stat] call. */ + val statCalls = mutableListOf() + fun addDirectory(path: StoragePath) { entries[path] = FileEntry(path, isDirectory = true, sizeBytes = 0L, lastModifiedMillis = 0L) } @@ -58,10 +70,21 @@ class InMemoryStorageSource( fun contains(path: StoragePath): Boolean = entries.containsKey(path) - override suspend fun list(directory: StoragePath): List = - entries.values.filter { it.path.parent == directory } + override suspend fun list(directory: StoragePath): List { + listCalls += directory + return entries.values.filter { it.path.parent == directory } + } - override suspend fun stat(path: StoragePath): FileEntry? = entries[path] + override suspend fun names(directory: StoragePath): List? { + namesCalls += directory + if (directory in unreadableNames) return null + return entries.values.filter { it.path.parent == directory }.map { it.name } + } + + override suspend fun stat(path: StoragePath): FileEntry? { + statCalls += path + return entries[path] + } override suspend fun exists(path: StoragePath): Boolean = entries.containsKey(path) diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModelTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModelTest.kt index ca848cf..1952bc2 100644 --- a/app/src/test/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModelTest.kt +++ b/app/src/test/java/de/softwareapp_hb/filemanager/ui/browser/BrowserViewModelTest.kt @@ -24,6 +24,7 @@ import de.softwareapp_hb.filemanager.domain.ArchiveEntry import de.softwareapp_hb.filemanager.domain.OperationKind import de.softwareapp_hb.filemanager.domain.OperationNotifier import de.softwareapp_hb.filemanager.domain.OperationProgress +import de.softwareapp_hb.filemanager.domain.SearchIndexer import de.softwareapp_hb.filemanager.domain.testutil.InMemorySearchIndex import de.softwareapp_hb.filemanager.domain.testutil.InMemoryStorageSource import de.softwareapp_hb.filemanager.ui.browser.testutil.FakeStorageLocation @@ -125,6 +126,7 @@ class BrowserViewModelTest { apkInspector: ApkInspector = ApkInspector.None, scratchRoot: File? = null, searchIndex: SearchIndex = SearchIndex.None, + clock: () -> Long = System::currentTimeMillis, ) = BrowserViewModel( storage = location, notifier = recordingNotifier, @@ -133,6 +135,7 @@ class BrowserViewModelTest { incoming = incoming, apkInspector = apkInspector, searchIndex = searchIndex, + clock = clock, scratchRoot = scratchRoot, ) @@ -503,7 +506,7 @@ class BrowserViewModelTest { indexed(root.child("ghost.png"), "ghost.png"), ), ) - index.endVolume(root.value, System.currentTimeMillis()) + index.endVolume(root.value, System.currentTimeMillis(), full = true, hiddenIncluded = false) } val model = viewModel(searchIndex = index) @@ -526,7 +529,7 @@ class BrowserViewModelTest { runBlocking { index.beginVolume(root.value) index.add(listOf(indexed(root.child("holiday.png"), "holiday.png"))) - index.endVolume(root.value, System.currentTimeMillis()) + index.endVolume(root.value, System.currentTimeMillis(), full = true, hiddenIncluded = false) } val model = viewModel(searchIndex = index) @@ -555,6 +558,82 @@ class BrowserViewModelTest { assertNotNull("the pass left a timestamp", runBlocking { index.indexedAt() }) } + @Test + fun `a fresh index is neither walked nor caught up`() { + val index = InMemorySearchIndex() + storage.addFile(root.child("holiday.png"), "x") + runBlocking { + index.beginVolume(root.value) + index.add(listOf(indexed(root.child("holiday.png"), "holiday.png"))) + index.endVolume(root.value, 10L * 60L * 1000L, full = true, hiddenIncluded = false) + } + storage.namesCalls.clear() + storage.listCalls.clear() + + val model = viewModel(searchIndex = index, clock = { 10L * 60L * 1000L }) + model.openDirectory(downloads) + model.setSearchScope(SearchScope.EVERYWHERE) + // Only what the search itself does from here counts as the search's own work. + storage.namesCalls.clear() + storage.listCalls.clear() + + model.setQuery("holiday") + + assertEquals(listOf("holiday.png"), model.state.value.entries.map { it.name }) + // This is the point of the whole exercise: an index that is current costs nothing at + // all to search — not a walk, not even a catch-up, and not one metadata read. + assertTrue("no catch-up was run", storage.namesCalls.isEmpty()) + assertTrue("and no walk", storage.listCalls.isEmpty()) + } + + @Test + fun `a stale index is caught up, and the answer includes what the catch-up found`() { + val index = InMemorySearchIndex() + // A real first pass, so the index matches what is really there — then stamped ten + // minutes ago: past the catch-up window, inside the full-walk window. + runBlocking { SearchIndexer(storage, index, clock = { 0L }).index(root, showHidden = false) } + // The file that arrived since, which no index row can know about. + storage.addFile(root.child("new-photo.png"), "x") + + val model = viewModel(searchIndex = index, clock = { 10L * 60L * 1000L }) + model.openDirectory(downloads) + model.setSearchScope(SearchScope.EVERYWHERE) + // Browsing lists folders too; only what the search does from here is the search's work. + storage.namesCalls.clear() + storage.listCalls.clear() + storage.statCalls.clear() + + model.setQuery("photo") + + assertEquals(listOf("new-photo.png"), model.state.value.entries.map { it.name }) + assertFalse("a catch-up is not a walk", storage.listCalls.isNotEmpty()) + // Only the name the index had never seen was looked at. + assertEquals(listOf(root.child("new-photo.png")), storage.statCalls) + } + + @Test + fun `a changed hidden-files preference asks for a walk rather than a catch-up`() { + val index = InMemorySearchIndex() + storage.addFile(root.child(".secret.txt"), "x") + runBlocking { + index.beginVolume(root.value) + // Built with hidden entries left out, which is what the rows mean. + index.endVolume(root.value, 0L, full = true, hiddenIncluded = false) + } + + val model = viewModel(searchIndex = index, clock = { 10L * 60L * 1000L }) + model.openDirectory(downloads) + model.toggleShowHidden() + model.setSearchScope(SearchScope.EVERYWHERE) + storage.listCalls.clear() + + model.setQuery("secret") + + // Only a walk can know about names the previous preference excluded. + assertTrue("a catch-up cannot add what was never counted", storage.listCalls.isNotEmpty()) + assertEquals(listOf(".secret.txt"), model.state.value.entries.map { it.name }) + } + private fun indexed(path: StoragePath, name: String) = IndexedFile( volume = root.value, path = path,