The index pass belongs to the view model, not to the keystroke that started it
Reported: the index looked recreated on every keystroke, and switching library started it over. Both are the same bug, and it is in the ownership of the pass. searchEverywhere cancels and re-creates searchJob on every keystroke, and loadLibrary does the same with libraryJob. bringIndexCurrent was a suspend function called from inside those jobs, so the walk ran in the job that the next interaction cancelled. A cancelled pass never reaches endVolume, so the volume's stamp stayed old, so the next interaction found the index stale and started again from the first folder. With a 20 000-file volume on a phone, where a walk takes tens of seconds and typing takes milliseconds, the index could not finish while anyone was typing - and the progress counter restarting with every character was the visible half of it. The pass now has its own job on the view model: one at a time, shared by whoever asks, not cancelled by the screen that asked. Search does not wait for it once there is an answer to show (it did before, which queued the UI behind the walk), and when the pass lands it re-answers whatever is on screen - the query or the library - so "I just downloaded it" still works. A rebuild cancels any pass in flight and takes over: it is the one action that recovers from the index being wrong rather than old. Three tests, each standing inside a walk through a gated StorageSource: typing does not start a second pass, switching library does not start a second pass, and a finished pass answers the search again when it finds the file. 430 tests total.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -145,6 +145,17 @@ class BrowserViewModel(
|
||||
*/
|
||||
private val changedFolders = HashMap<StoragePath, MutableSet<StoragePath>>()
|
||||
|
||||
/**
|
||||
* 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?".
|
||||
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<StoragePath>, List<StoragePath>> {
|
||||
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.
|
||||
|
||||
@@ -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<Unit>()
|
||||
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<Unit>()
|
||||
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<Unit>,
|
||||
) : StorageSource by delegate {
|
||||
override suspend fun list(directory: StoragePath): List<FileEntry> {
|
||||
gate.await()
|
||||
return delegate.list(directory)
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `navigating up goes to the parent then to the roots`() {
|
||||
val model = viewModel()
|
||||
|
||||
+6
-3
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user