diff --git a/CHANGELOG.md b/CHANGELOG.md index d2b82ec..850f34c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -242,6 +242,15 @@ upload needs a **higher `versionCode`** than every previous upload. ### Fixed +- **Typing in the search field no longer restarts the index, and neither does opening another library.** + The pass that keeps the index current was started from inside the search's own job, and that job is + cancelled and replaced on every keystroke — so each letter cancelled the walk that had just started, + before it could record that it had finished, and the next letter began again at the first folder. The + progress counter restarting with every character was the visible half; the hidden half was that on a + full volume the index could not finish while anyone was typing. The same thing happened when switching + libraries, which restarts its own job too. The pass now belongs to the view model: one at a time, + whoever asks, not cancelled by the screen that asked, re-answering what is on screen when it lands. + - **The search field told the truth everywhere except inside a library.** Opening the field from a library left the scope on "this folder" — the hint read *Search this folder* and a folder toggle sat beside it — while the search actually ran everywhere, because the view model has always known 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 f0c11d3..fec9abc 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 @@ -145,6 +145,17 @@ class BrowserViewModel( */ private val changedFolders = HashMap>() + /** + * The index pass that is running, if any. + * + * **Owned by the view model, not by whoever asked for it.** A search cancels and restarts its own job + * on every keystroke; a pass started from inside that job was therefore cancelled a few characters + * later — before it could stamp the volume — and the next keystroke started another one from the first + * folder. Typing "photo" on a phone walked the volume five times, finished none of them, and the + * counter on screen restarted with every letter. One job, one pass, whoever asked. + */ + private var indexJob: Job? = null + /** The running recursive search, so the next keystroke can supersede it. */ private var searchJob: Job? = null @@ -898,13 +909,20 @@ class BrowserViewModel( } searchJob = viewModelScope.launch { - if (searchIndex.stamps().isNotEmpty()) showIndexedResults(query, source) + val known = searchIndex.stamps().isNotEmpty() + if (known) showIndexedResults(query, source) - bringIndexCurrent(source) + // **Not awaited** when there is already an answer. The pass belongs to the view model, so + // typing neither cancels it nor queues behind it; it re-answers this query when it lands. + val refresh = bringIndexCurrent(source) - // 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) + if (!known) { + // The first search on a device has nothing to show until a walk has run, so this is the one + // case worth waiting for — with the progress row above it saying why. + refresh?.join() + if (!currentCoroutineContext().isActive) return@launch + showIndexedResults(query, source) + } _state.update { current -> if (current.query != query) { @@ -926,7 +944,33 @@ class BrowserViewModel( * and the preference is part of what a row means. Everything else needs a **catch-up**, which * reads names and nothing else. */ - private suspend fun bringIndexCurrent(source: StorageSource) { + private fun bringIndexCurrent(source: StorageSource): Job? { + // Already running: this caller gets that pass rather than a second one. + indexJob?.takeIf { it.isActive }?.let { return it } + + val job = viewModelScope.launch { + val (toWalk, toCatchUp) = staleVolumes() + if (toWalk.isNotEmpty()) pass(source, toWalk, full = true) + if (toCatchUp.isNotEmpty()) pass(source, toCatchUp, full = false) + + // What is on screen is answered again, because the pass may just have found the file the + // search is for — which is the whole of "I downloaded it, so why is it not here?". + refreshFromIndex() + } + indexJob = job + return job + } + + /** + * Which volumes need a walk, and which only a catch-up. + * + * The policy lives here rather than in its callers because search and the library views must agree + * about it: a volume that was never built, was built with the other hidden-files preference, or has + * not been walked end to end for a week needs a **walk** — 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 + * a row means. Everything else needs a **catch-up**, which reads names and nothing else. + */ + private suspend fun staleVolumes(): Pair, List> { val volumes = volumes() val stampAt = clock() val stamps = searchIndex.stamps() @@ -938,13 +982,24 @@ class BrowserViewModel( 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) + return toWalk to toCatchUp + } + + /** Re-answer the view that is on screen from the index, once a pass has landed. */ + private suspend fun refreshFromIndex() { + val source = storage.activeSource() ?: return + val current = _state.value + when { + current.query.isNotBlank() && current.searchRoot != null -> + showIndexedResults(current.query, source) + current.category != null -> showLibrary(current.category, source) + else -> Unit + } + _state.update { it.copy(indexedAt = searchIndex.indexedAt()) } } /** Answer [query] from the index, confirming every candidate against the filesystem. */ @@ -1008,7 +1063,10 @@ class BrowserViewModel( */ fun rebuildSearchIndex() { val source = storage.activeSource() ?: return - viewModelScope.launch { + // The explicit action wins over any pass in flight: it is the one thing that recovers from the index + // being *wrong* rather than merely old, so it must not be folded into a catch-up that is nearly done. + indexJob?.cancel() + indexJob = viewModelScope.launch { pass(source, volumes(), full = true) // Answered again, in whichever view asked for the rebuild: a search re-runs its query, // a library re-reads its rows, and the home screen's counts follow either way. 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 e5bfab8..a390fde 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 @@ -22,6 +22,8 @@ import de.softwareapp_hb.filemanager.data.DeleteRequestFactory import de.softwareapp_hb.filemanager.data.IncomingSource import de.softwareapp_hb.filemanager.data.IndexedFile import de.softwareapp_hb.filemanager.data.SearchIndex +import de.softwareapp_hb.filemanager.data.StorageSource +import de.softwareapp_hb.filemanager.data.StorageLocation import de.softwareapp_hb.filemanager.data.StorageAccessMode import de.softwareapp_hb.filemanager.data.VolumeInfo import de.softwareapp_hb.filemanager.data.testutil.InMemoryBookmarkStore @@ -36,6 +38,7 @@ import de.softwareapp_hb.filemanager.data.TrashIndex 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 +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -130,8 +133,12 @@ class BrowserViewModelTest { Dispatchers.resetMain() } + // The builder of the thing under test: every collaborator the view model can take, defaulted, so a test + // names only the one it is about. Ten parameters is the rule for production code, not for this. + @Suppress("LongParameterList") private fun viewModel( deleteRequests: DeleteRequestFactory = DeleteRequestFactory { _, _ -> null }, + storageLocation: StorageLocation = location, savedState: SavedStateHandle = SavedStateHandle(), incoming: IncomingSource? = fakeIncoming, apkInspector: ApkInspector = ApkInspector.None, @@ -143,7 +150,7 @@ class BrowserViewModelTest { // assertions in this file are written to read the state immediately after an action. io: CoroutineDispatcher = UnconfinedTestDispatcher(), ) = BrowserViewModel( - storage = location, + storage = storageLocation, notifier = recordingNotifier, deleteRequests = deleteRequests, savedState = savedState, @@ -586,6 +593,91 @@ class BrowserViewModelTest { assertTrue(model.state.value.overlay is Overlay.Trash) } + @Test + fun `typing while the index is being built does not start it again`() { + storage.addFile(downloads.child("photo.jpg"), "x") + val index = InMemorySearchIndex() + val gate = CompletableDeferred() + val model = viewModel( + storageLocation = GatedLocation(location, GatedSource(storage, gate)), + searchIndex = index, + ) + + model.setQuery("pho") + // The first search is inside the walk now, waiting at the gate. + assertTrue(model.state.value.indexing) + + model.setQuery("phot") + model.setQuery("photo") + + // Three keystrokes, one pass. The walk belongs to the view model; a keystroke cancels the *search* + // job, and used to cancel the walk with it — which is why the counter restarted with every letter. + assertEquals(1, index.beginVolumeCalls) + + gate.complete(Unit) + + assertEquals(1, index.beginVolumeCalls) + assertTrue(model.state.value.entries.any { it.name == "photo.jpg" }) + } + + @Test + fun `switching library while the index is being built does not start it again`() { + val index = InMemorySearchIndex() + val gate = CompletableDeferred() + val model = viewModel( + storageLocation = GatedLocation(location, GatedSource(storage, gate)), + searchIndex = index, + ) + + model.openCategory(FileCategory.IMAGES) + assertTrue(model.state.value.indexing) + + model.openCategory(FileCategory.VIDEOS) + model.openCategory(FileCategory.DOCUMENTS) + + assertEquals(1, index.beginVolumeCalls) + + gate.complete(Unit) + assertEquals(1, index.beginVolumeCalls) + } + + @Test + fun `a search is answered again when the pass finds what it was looking for`() { + var now = 1_000_000L + val index = InMemorySearchIndex() + val model = viewModel(searchIndex = index, clock = { now }) + // A first pass, so the index has a stamp; then a new file, and enough time for the stamp to age + // past the catch-up window. + model.openCategory(FileCategory.IMAGES) + storage.addFile(downloads.child("photo.jpg"), "x") + now += 3L * 60L * 1000L + + model.setQuery("photo") + + // The first answer came from what the index knew; the catch-up behind it found the new file and + // answered again — which is what makes "I just downloaded it" work without a walk per keystroke. + assertTrue(model.state.value.entries.any { it.name == "photo.jpg" }) + assertEquals(1, index.beginVolumeCalls) + } + + /** A location whose listing waits on [gate], so a test can stand inside a walk. */ + private class GatedLocation( + private val delegate: StorageLocation, + private val gated: StorageSource, + ) : StorageLocation by delegate { + override fun activeSource(): StorageSource = gated + } + + private class GatedSource( + private val delegate: StorageSource, + private val gate: CompletableDeferred, + ) : StorageSource by delegate { + override suspend fun list(directory: StoragePath): List { + gate.await() + return delegate.list(directory) + } + } + @Test fun `navigating up goes to the parent then to the roots`() { val model = viewModel() diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 0e277bd..fca584c 100644 --- a/docs/CODE_MAP.md +++ b/docs/CODE_MAP.md @@ -2,7 +2,7 @@ Generated from the source by `tools/make-code-map.py` - do not edit by hand. Regenerate it after a change (`python3 tools/make-code-map.py`); `--check` exits non-zero when this file is stale. -80 source files, 13841 lines - 37 test files, 427 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. +80 source files, 13899 lines - 37 test files, 430 tests. The behaviour index at the end lists every test name, and in this project a test name is a sentence about what the app does, so it doubles as the specification. Start at `AGENTS.md`. What is done, and why: `docs/ROADMAP.md` and `CHANGELOG.md`. What was verified by hand rather than by a test: `docs/EDGE_CASE_TESTING.md`. @@ -104,7 +104,7 @@ The browser screen: view model, state, list and grid, dialogs, selection bar | `BrowserScreen.kt` | 661 | The browser, and everything that can be on top of it. BrowserScreen(), messageText() | - | | `BrowserTopBar.kt` | 238 | The browser's app bar: title, up, the favourite star, and the search / sort / overflow actions. BrowserTopBar(), SEARCH_FIELD_WIDTH | `BrowserTopBarTest` (4) | | `BrowserUiState.kt` | 391 | One immutable description of everything the browser shows. ViewMode, SearchScope, Clipboard, CompressRequest, PickerFolder, TransferPicker, MessageAction, UiMessage, ConflictPrompt, RootItem, Bookmark, DetailFocus, Undo, UndoMove, Overlay, BrowserUiState | - | -| `BrowserViewModel.kt` | 2540 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (113) | +| `BrowserViewModel.kt` | 2598 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (116) | | `DetailsSheet.kt` | 109 | Name, location, size, type and modified date for one entry. DetailsSheet(), DetailLines(), DetailLine() | - | | `EntryIntents.kt` | 58 | Wraps an entry's path in a content:// URI another app may read. contentUriFor(), viewIntent(), shareIntent() | - | | `IncomingBar.kt` | 60 | The bar shown while another app is waiting on this one. IncomingBar() | - | @@ -595,7 +595,7 @@ Every test name in the suite, by file: the fastest way to see whether something - a library or the root list has nothing to star - the star gives way to the search field -### `ui/browser/BrowserViewModelTest.kt` (113) +### `ui/browser/BrowserViewModelTest.kt` (116) - starts on the root list - opening a folder lists it with directories first and natural order @@ -626,6 +626,9 @@ Every test name in the suite, by file: the fastest way to see whether something - opening the trash keeps what is still inside the window - emptying the trash removes what is in it - a shortcut opens the view it names +- typing while the index is being built does not start it again +- switching library while the index is being built does not start it again +- a search is answered again when the pass finds what it was looking for - navigating up goes to the parent then to the roots - selection survives toggling and clears on navigation - a file with no preview of its own is handed to the system