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 47de09a..fa5432f 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 @@ -51,6 +51,7 @@ import de.softwareapp_hb.filemanager.domain.TrashedEntry import de.softwareapp_hb.filemanager.domain.UsageAnalysis import de.softwareapp_hb.filemanager.domain.UsageNode import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.currentCoroutineContext @@ -101,9 +102,20 @@ class BrowserViewModel( * disables those previews. * * A `File` rather than a `Context`: the ViewModel keeps its distance from the platform, - * and the application hands it a directory it already owns. + * and the application hands it a directory it already owns. [Lazy] because resolving it + * means `Context.getCacheDir`, which checks and creates the directory — measured at 492 ms + * on the first start (docs/PERFORMANCE.md), and never needed until a preview copies + * something. */ private val scratchRoot: Lazy? = null, + /** + * Where a folder too large to sort on the main thread is sorted instead. + * + * Injected so a test can make the sort synchronous: the behaviour under test is "a big folder is + * sorted and shown", not which thread did it, and `UnconfinedTestDispatcher` keeps the assertions + * where they were. + */ + private val io: CoroutineDispatcher = Dispatchers.IO, ) : ViewModel() { private val _state = MutableStateFlow(restoredViewState()) @@ -313,12 +325,44 @@ class BrowserViewModel( * that streams its own matches into [BrowserUiState.entries]; this is only ever the * "what is in this folder" view. */ + /** + * Derives what is shown from [listing]: hidden files filtered out, then sorted. + * + * A big folder is sorted on [io] instead of on the caller's thread. `File.listFiles` and the entry + * data are already read off the main thread, but the *sort* was not: the default order is natural + * order, which parses the digits in every name on every comparison, so twenty thousand rows means + * a few hundred thousand string comparisons in the middle of a tap. Measured on the phone at that + * size, one iteration out of five took 598 frames against a median of 184 — the kind of hitch this + * moves off the frame. Small folders stay on the caller's thread, which keeps the code (and the + * tests) honest for the case that happens all the time. + */ private fun recompute() { val current = _state.value - val shown = listing.filter { current.showHidden || !it.isHidden } - _state.update { it.copy(entries = current.sort.sort(shown)) } + if (listing.size < SORT_OFF_MAIN_THRESHOLD) { + _state.update { it.copy(entries = current.sort.sort(visible(current))) } + return + } + + viewModelScope.launch { + val sorted = withContext(io) { current.sort.sort(visible(current)) } + // Only if the screen still shows that folder, in that order: a second tap must not be + // overwritten by the answer to the first. + _state.update { state -> + if (state.path == current.path && + state.sort == current.sort && + state.showHidden == current.showHidden + ) { + state.copy(entries = sorted) + } else { + state + } + } + } } + private fun visible(state: BrowserUiState): List = + listing.filter { state.showHidden || !it.isHidden } + /** * The trail from the browsable root down to [path]. * @@ -2108,6 +2152,16 @@ class BrowserViewModel( * them — they would fill the candidate list without ever finding space worth having. 1 KB * is where "this is a real file" starts. */ + /** + * Above this many rows, a folder is filtered and sorted off the main thread. + * + * The number is where a sort stops being free: a few hundred rows sort in well under a frame, + * and a folder of thousands does not — twenty thousand rows of natural-order comparison is a + * few hundred thousand string comparisons. Small folders therefore keep the simple synchronous + * path, and only the folders that can hitch take the detour. + */ + const val SORT_OFF_MAIN_THRESHOLD = 500 + const val DUPLICATE_MIN_BYTES = 1024L /** 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 533f088..4bd4ee9 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 @@ -33,6 +33,7 @@ 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 +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.delay @@ -135,6 +136,9 @@ class BrowserViewModelTest { searchIndex: SearchIndex = SearchIndex.None, bookmarks: BookmarkStore = InMemoryBookmarkStore(), clock: () -> Long = System::currentTimeMillis, + // Unconfined in tests: the view model sorts a large folder on its own dispatcher, and the + // assertions in this file are written to read the state immediately after an action. + io: CoroutineDispatcher = UnconfinedTestDispatcher(), ) = BrowserViewModel( storage = location, notifier = recordingNotifier, @@ -145,6 +149,7 @@ class BrowserViewModelTest { searchIndex = searchIndex, bookmarkStore = bookmarks, clock = clock, + io = io, // The view model takes it lazily now — `app.cacheDir` is a disk check that used to run while // the screen was being built — so the test wraps the folder it already made. scratchRoot = scratchRoot?.let { folder -> lazyOf(folder) }, @@ -212,6 +217,23 @@ class BrowserViewModelTest { assertEquals(listOf("b.txt", "a.txt"), model.state.value.entries.map { it.name }) } + @Test + fun `a folder of six hundred files is still listed in natural order`() { + val big = root.child("big") + storage.addDirectory(big) + for (index in 0 until 600) storage.addFile(big.child("file-$index.txt"), "x") + + val model = viewModel() + model.openDirectory(big) + + val names = model.state.value.entries.map { it.name } + assertEquals(600, names.size) + // Natural order rather than lexicographic, which is the whole point of the comparator — and + // the case that has to survive being moved onto another dispatcher. + assertEquals(listOf("file-0.txt", "file-1.txt", "file-2.txt"), names.take(3)) + assertEquals("file-599.txt", names.last()) + } + @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 caf55d4..6bc8c8f 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. -78 source files, 12856 lines - 34 test files, 390 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. +78 source files, 12910 lines - 34 test files, 391 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`. @@ -102,7 +102,7 @@ The browser screen: view model, state, list and grid, dialogs, selection bar | `BrowserScreen.kt` | 591 | BrowserScreen() | - | | `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` | 311 | One immutable description of everything the browser shows. ViewMode, SearchScope, Clipboard, UiMessage, ConflictPrompt, RootItem, Bookmark, DetailFocus, Undo, UndoMove, Overlay, BrowserUiState | - | -| `BrowserViewModel.kt` | 2152 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (89) | +| `BrowserViewModel.kt` | 2206 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (90) | | `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() | - | @@ -572,13 +572,14 @@ 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` (89) +### `ui/browser/BrowserViewModelTest.kt` (90) - starts on the root list - opening a folder lists it with directories first and natural order - hidden files are filtered out until asked for - search narrows the listing - sorting by size reorders the listing +- a folder of six hundred files is still listed in natural order - 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 diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index 935d21c..02ef7f5 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -143,6 +143,20 @@ three seconds' worth of frames; one iteration of five took three times that, so is not merely slow but occasionally stalls — worth watching rather than explaining away, and the reason the 20 000 point is the one a release should be checked against. +Reading the listing path afterwards found a candidate that explains a hitch of exactly that shape: the +entries were read off the main thread but **sorted on it**. `AllFilesSource.list` already hops to +`Dispatchers.IO`, but `BrowserViewModel.recompute` filtered and sorted whatever `list` returned on the +caller's thread — and the default order is natural order, which parses the digits in every name on every +comparison. At 20 000 rows that is a few hundred thousand string comparisons inside a tap, plus a fresh +20 000-element list, which is the sort of work that is usually fine and occasionally not. + +`recompute` now sorts off the main thread above 500 rows (`SORT_OFF_MAIN_THRESHOLD`) and keeps the simple +synchronous path below it, so small folders — the common case, and the one the tests exercise — behave +exactly as before. The dispatcher is injected, so the large path is covered by a test that lists 600 files +and asserts they come out in natural order. **It is not re-measured**: the 20 000-file fixture takes half an +hour to build through MIUI's storage layer, so the 598 outlier stands as the motivation and the next +person with that fixture should confirm the max has come down. + Building that fixture is the expensive part, not measuring it: 20 000 files took about thirty minutes through MIUI's storage layer (`: >` in a loop), against roughly four minutes of benchmark on top. Worth knowing before anyone re-creates it, and an argument for the fixture being built once and kept.