diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/core/SortedMerge.kt b/app/src/main/java/de/softwareapp_hb/filemanager/core/SortedMerge.kt new file mode 100644 index 0000000..0ff9468 --- /dev/null +++ b/app/src/main/java/de/softwareapp_hb/filemanager/core/SortedMerge.kt @@ -0,0 +1,42 @@ +package de.softwareapp_hb.filemanager.core + +/** + * Adds a freshly read batch to an already sorted list, in one pass. + * + * A folder is now read in batches so that the first screenful can be drawn while the rest is still being + * `stat`ed. Each batch is sorted on its own and merged here, which is O(n + k) — against O(n log n) for + * re-sorting everything after every batch — and it means the rows on screen are always in the order the + * user asked for, never a partly ordered jumble that reorders itself when the next batch lands. + */ +object SortedMerge { + + /** + * [sorted] must already be ordered by [comparator]; [batch] need not be. + * + * Ties keep the *earlier* entry first (`<=`), so an entry does not jump over an equal one just + * because it arrived in a later batch. + */ + fun merge( + sorted: List, + batch: List, + comparator: Comparator, + ): List { + if (batch.isEmpty()) return sorted + val ordered = batch.sortedWith(comparator) + if (sorted.isEmpty()) return ordered + + val merged = ArrayList(sorted.size + ordered.size) + var left = 0 + var right = 0 + while (left < sorted.size && right < ordered.size) { + if (comparator.compare(sorted[left], ordered[right]) <= 0) { + merged += sorted[left++] + } else { + merged += ordered[right++] + } + } + while (left < sorted.size) merged += sorted[left++] + while (right < ordered.size) merged += ordered[right++] + return merged + } +} 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 2b01ca8..da902b6 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 @@ -4,6 +4,9 @@ import android.annotation.SuppressLint import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.StoragePath import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.withContext import java.io.File import java.io.FileInputStream @@ -12,6 +15,8 @@ import java.io.InputStream import java.io.OutputStream import java.nio.channels.FileChannel import java.nio.channels.SeekableByteChannel +import java.nio.file.Files +import java.nio.file.Paths import java.nio.file.StandardOpenOption /** @@ -37,6 +42,30 @@ class AllFilesSource( if (children == null) emptyList() else children.map { it.toEntry() } } + /** + * The same entries, handed over every [batchSize] rows while the directory is still being read. + * + * `Files.newDirectoryStream` is lazy — it pulls one `readdir` result at a time as the loop asks — + * which is what makes a first batch possible at all; `File.listFiles` would materialise the whole + * folder before returning anything, and on this phone that is about two seconds for twenty thousand + * files. `flowOn(IO)` keeps the loop (and every `stat` in it) off the caller's thread; the caller + * only ever sees finished batches. + */ + override fun listBatches(directory: StoragePath, batchSize: Int): Flow> = + flow { + val batch = ArrayList(batchSize) + Files.newDirectoryStream(Paths.get(directory.value)).use { stream -> + for (path in stream) { + batch += path.toFile().toEntry() + if (batch.size >= batchSize) { + emit(batch.toList()) + batch.clear() + } + } + } + if (batch.isNotEmpty()) emit(batch.toList()) + }.flowOn(Dispatchers.IO) + /** * 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 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 4d39179..8750516 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 @@ -3,6 +3,8 @@ package de.softwareapp_hb.filemanager.data import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.StoragePath import java.io.InputStream +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.flow import java.io.OutputStream import java.nio.channels.SeekableByteChannel @@ -38,6 +40,32 @@ interface StorageSource { /** Children of [directory], in whatever order the backend returns them (unsorted). */ suspend fun list(directory: StoragePath): List + /** + * [list], in the order the backend reads it, a batch at a time. + * + * The reason is the first batch. Reading a folder means a `readdir` and a `stat` per child, and a + * phone's mounted storage makes each of those a round trip — measured at roughly 0.1 ms per file, so + * twenty thousand files is about two seconds in which nothing can be drawn. Batches let the screen + * show what has been read while the rest is being read, which is the difference between a slow + * folder and a frozen one. + * + * The default is a single batch holding everything, so a backend that cannot stream — and every test + * fake — behaves exactly as before. + */ + fun listBatches(directory: StoragePath, batchSize: Int = LIST_BATCH): Flow> = + flow { emit(list(directory)) } + + companion object { + /** + * How many entries are handed over at a time while a folder is read. + * + * Two hundred is roughly a few screens' worth: small enough that the first batch is drawn + * quickly, large enough that a folder of a thousand files arrives in five batches rather than + * fifty state updates. + */ + const val LIST_BATCH = 200 + } + /** * The names in [directory], **without reading metadata for each one**. * 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 dc44a44..3437ded 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 @@ -22,6 +22,7 @@ import de.softwareapp_hb.filemanager.core.NameAllocator import de.softwareapp_hb.filemanager.core.SortDirection import de.softwareapp_hb.filemanager.core.SortKey import de.softwareapp_hb.filemanager.core.SortSpec +import de.softwareapp_hb.filemanager.core.SortedMerge import de.softwareapp_hb.filemanager.core.StoragePath import de.softwareapp_hb.filemanager.core.TransferAction import de.softwareapp_hb.filemanager.core.isUsableName @@ -297,8 +298,9 @@ class BrowserViewModel( // Measured off the main thread, after the listing is up: see [fillFreeSpace]. viewModelScope.launch { fillFreeSpace(path) } viewModelScope.launch { - _state.update { it.copy(loading = true) } - listing = runCatching { source.list(path) }.getOrDefault(emptyList()) + // The folder is announced before anything is read, so leaving the previous one is immediate + // and the progress bar has something to sit above. + listing = emptyList() _state.update { it.copy( path = path, @@ -310,14 +312,43 @@ class BrowserViewModel( searchRoot = null, searching = false, freeBytes = null, - loading = false, + loading = true, + entries = emptyList(), ) } + + // Batch by batch: the first screenful is on screen while the rest is still being read. Each + // batch is merged into the list already on screen (see [SortedMerge]), so rows arrive in the + // user's order rather than shuffling into place afterwards. + runCatching { + source.listBatches(path).collect { batch -> + listing = SortedMerge.merge(listing, batch, _state.value.sort.comparator) + publishEntries() + } + } + + if (_state.value.path == path) _state.update { it.copy(loading = false) } + // Once more at the end, in case the sort was changed while the folder was still arriving: + // batches were merged with whatever order was current when each one landed. recompute() persistViewState() } } + /** + * Publishes what has been read so far, with the hidden filter applied. + * + * No sort here: [listing] is kept ordered by [SortedMerge] as batches arrive, so a batch costs one + * merge and one filter rather than a sort of the whole folder — which matters when the whole folder + * is twenty thousand rows being delivered a hundred times. + */ + private fun publishEntries() { + val current = _state.value + _state.update { state -> + state.copy(entries = listing.filter { current.showHidden || !it.isHidden }) + } + } + /** * Derive what is shown from the unfiltered listing. * diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/core/SortedMergeTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/core/SortedMergeTest.kt new file mode 100644 index 0000000..68d842a --- /dev/null +++ b/app/src/test/java/de/softwareapp_hb/filemanager/core/SortedMergeTest.kt @@ -0,0 +1,61 @@ +package de.softwareapp_hb.filemanager.core + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * How a batch that arrives late joins a list that is already on screen. + * + * The list has to be *ordered at every moment* — not "sorted once the folder has finished", because the + * folder may never finish visibly for twenty thousand rows — so each batch is merged rather than the + * whole list re-sorted. + */ +class SortedMergeTest { + + private val byName = SortSpec().comparator + + private fun file(name: String, size: Long = 0) = FileEntry( + path = StoragePath.of("/sdcard/$name"), + isDirectory = false, + sizeBytes = size, + lastModifiedMillis = 0, + ) + + @Test + fun `a later batch slots into the order already on screen`() { + val merged = SortedMerge.merge( + sorted = listOf(file("a.txt"), file("c.txt")), + batch = listOf(file("d.txt"), file("b.txt")), + comparator = byName, + ) + + assertEquals(listOf("a.txt", "b.txt", "c.txt", "d.txt"), merged.map { it.name }) + } + + @Test + fun `the first batch into an empty list is sorted on its own`() { + val merged = SortedMerge.merge(emptyList(), listOf(file("c.txt"), file("a.txt")), byName) + + assertEquals(listOf("a.txt", "c.txt"), merged.map { it.name }) + } + + @Test + fun `an empty batch changes nothing`() { + val shown = listOf(file("a.txt"), file("b.txt")) + + assertEquals(shown, SortedMerge.merge(shown, emptyList(), byName)) + } + + @Test + fun `an entry that ties on name does not jump ahead of one that arrived earlier`() { + // Two files with the same name can exist in different folders of a search result; the one + // already on screen keeps its position so the row under the user's finger does not change. + val merged = SortedMerge.merge( + sorted = listOf(file("same.txt", size = 1)), + batch = listOf(file("same.txt", size = 2)), + comparator = byName, + ) + + assertEquals(listOf(1L, 2L), merged.map { it.sizeBytes }) + } +} 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 52bd2e5..a877f16 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 @@ -24,6 +24,22 @@ class AllFilesSourceTest { private fun path(file: File) = StoragePath.of(file.absolutePath) + @Test + fun `reads a large folder in batches, covering every entry exactly once`() = runBlocking { + val dir = temp.newFolder("big") + repeat(250) { File(dir, "file-$it.txt").writeText("x") } + + val batches = mutableListOf>() + source(dir).listBatches(path(dir), batchSize = 100).collect { batch -> + batches += batch.map { it.name } + } + + // 250 files at 100 per batch: two full batches and a remainder, not one batch of everything. + assertEquals(listOf(100, 100, 50), batches.map { it.size }) + // And every file exactly once, which is what a streaming read is easy to get wrong about. + assertEquals(250, batches.flatten().toSet().size) + } + @Test fun `lists the children of a directory`() = runBlocking { val dir = temp.newFolder("docs") diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 6bc8c8f..2cb7723 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, 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. +79 source files, 13040 lines - 35 test files, 396 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`. @@ -24,6 +24,7 @@ Pure Kotlin, **no Android imports**: paths, file names, kinds, sorting, formatti | `NameAllocator.kt` | 30 | Picks a name that is not already in use, in the familiar "name (1)" style. NameAllocator | `NameMatcherTest` (10) | | `NameMatcher.kt` | 63 | The in-folder search matcher. NameMatcher | `NameMatcherTest` (10) | | `SortSpec.kt` | 82 | A complete, serialisable description of the ordering of a directory listing. SortKey, SortDirection, SortSpec, NaturalOrder | `SortSpecTest` (8) | +| `SortedMerge.kt` | 42 | Adds a freshly read batch to an already sorted list, in one pass. SortedMerge | `SortedMergeTest` (4) | | `StoragePath.kt` | 89 | A normalized, absolute, backend-agnostic storage path. StoragePath | `StoragePathTest` (8) | | `ThumbnailKey.kt` | 22 | The identity of a thumbnail: which file it came from, and which version of that file. ThumbnailKey | `ThumbnailKeyTest` (4) | | `Timestamps.kt` | 20 | Timestamps for the details sheet. Timestamps | - | @@ -35,7 +36,7 @@ Storage backends (all-files access and SAF), volumes, incoming shares, the SQLit | file | lines | what it holds | tests | |---|---|---|---| -| `AllFilesSource.kt` | 125 | The "All files access" backend: plain java.io.File over absolute paths. AllFilesSource, File() | `AllFilesSourceTest` (13) | +| `AllFilesSource.kt` | 154 | The "All files access" backend: plain java.io.File over absolute paths. AllFilesSource, File() | `AllFilesSourceTest` (14) | | `ApkInspector.kt` | 100 | What an APK says about itself, read without installing it. ApkInfo, interface(), PackageManagerApkInspector | - | | `BookmarkStore.kt` | 34 | The folders the user starred. BookmarkStore | - | | `IncomingSource.kt` | 48 | Something another app has handed to this one. IncomingSource, ContentResolverIncomingSource | - | @@ -50,7 +51,7 @@ Storage backends (all-files access and SAF), volumes, incoming shares, the SQLit | `StorageAccess.kt` | 80 | The Android-facing side of the storage-access decision. StorageAccess | - | | `StorageAccessPolicy.kt` | 68 | The decision rules behind StorageAccess. StorageAccessMode, StorageAccessPolicy | `StorageAccessPolicyTest` (6) | | `StorageLocation.kt` | 87 | Everything the browser needs to know about where files live and what it may touch. StorageLocation, StorageProvider | - | -| `StorageSource.kt` | 96 | The primitive file operations a storage backend must provide. StorageException, StorageSource | - | +| `StorageSource.kt` | 124 | The primitive file operations a storage backend must provide. StorageException, StorageSource | - | | `TrashIndex.kt` | 61 | The durable memory of what is in the trash. TrashRecord, TrashIndex | - | | `VolumeRepository.kt` | 121 | Discovers the device's storage volumes and reports their space. VolumeInfo, SpaceInfo, VolumeRepository | `VolumeRepositoryTest` (6) | @@ -102,7 +103,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` | 2206 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (90) | +| `BrowserViewModel.kt` | 2237 | 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() | - | @@ -289,6 +290,13 @@ Every test name in the suite, by file: the fastest way to see whether something - comparison ignores case - a prefix sorts before a longer name +### `core/SortedMergeTest.kt` (4) + +- a later batch slots into the order already on screen +- the first batch into an empty list is sorted on its own +- an empty batch changes nothing +- an entry that ties on name does not jump ahead of one that arrived earlier + ### `core/StoragePathTest.kt` (8) - normalizes separators and dot segments @@ -315,8 +323,9 @@ Every test name in the suite, by file: the fastest way to see whether something - a folder cannot be moved into itself or a descendant - a move into itself is skipped in the plan -### `data/AllFilesSourceTest.kt` (13) +### `data/AllFilesSourceTest.kt` (14) +- reads a large folder in batches, covering every entry exactly once - lists the children of a directory - names lists a folder without reading metadata - names of a folder that cannot be read is null, not empty @@ -676,4 +685,4 @@ Every test name in the suite, by file: the fastest way to see whether something --- -Purpose taken from the first KDoc rather than from a declaration sharing the file's name (15 of 77): `MainActivity.kt`, `ArchiveFormats.kt`, `FileNames.kt`, `ApkInspector.kt`, `OperationModels.kt`, `TextRes.kt`, `BrowserComponents.kt`, `BrowserDetailPane.kt`, `BrowserDialogs.kt`, `BrowserRoots.kt`, `BrowserScreen.kt`, `EntryIntents.kt`, `Color.kt`, `Theme.kt`, `Type.kt` +Purpose taken from the first KDoc rather than from a declaration sharing the file's name (15 of 78): `MainActivity.kt`, `ArchiveFormats.kt`, `FileNames.kt`, `ApkInspector.kt`, `OperationModels.kt`, `TextRes.kt`, `BrowserComponents.kt`, `BrowserDetailPane.kt`, `BrowserDialogs.kt`, `BrowserRoots.kt`, `BrowserScreen.kt`, `EntryIntents.kt`, `Color.kt`, `Theme.kt`, `Type.kt` diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index fa9f753..3ddd181 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -160,6 +160,28 @@ scheduler hiccup on top of that — unidentified, and worth a perfetto trace rat cheap in absolute terms, but it is work the frame has no reason to wait for, and the threshold keeps small folders on the simple synchronous path. +### The same cost, delivered in pieces + +Those two seconds of `readdir` and `stat` cannot be made much cheaper — they are round trips to another +process — so the fix is to **draw while they happen**: + +- `StorageSource.listBatches` hands a folder over 200 entries at a time. `AllFilesSource` implements it + with `Files.newDirectoryStream`, which is lazy: it pulls `readdir` results as the loop asks for them, + where `File.listFiles` materialised the whole folder before returning anything. The default + implementation is still a single batch, so the SAF backend and every test fake behave exactly as before. +- `BrowserViewModel.load` merges each batch into the list already on screen (`core/SortedMerge`, + O(n + k) instead of re-sorting the folder after every batch), so rows arrive in the user's order and the + list is never a partly ordered jumble that shuffles into place afterwards. +- `loading` stays true until the last batch, which is what the existing progress bar above the list already + draws — the feedback that keeps a slow folder from feeling like a frozen one. + +At 200 entries per batch, the first rows should appear after roughly 20 ms of reading plus a frame, against +two seconds of empty screen before. **That expectation is not measured yet**: confirming it needs a +20 000-file fixture, which takes about half an hour to build through MIUI's storage layer, so the numbers to +beat are the ones in the table above and the first thing to do with that fixture is to re-run +`openAFolderOfAThousandFiles` and look at the *min* iteration rather than the median — the minimum is what +"first rows visible" now governs. + 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.