diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt b/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt index 23b675a..1519a9c 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/FileManagerApplication.kt @@ -6,6 +6,7 @@ import android.content.pm.ApplicationInfo import android.os.StrictMode import coil3.ImageLoader import coil3.SingletonImageLoader +import coil3.memory.MemoryCache import coil3.video.VideoFrameDecoder import de.softwareapp_hb.filemanager.data.ApkInspector import de.softwareapp_hb.filemanager.data.BookmarkStore @@ -122,5 +123,29 @@ class FileManagerApplication : Application(), SingletonImageLoader.Factory { override fun newImageLoader(context: Context): ImageLoader = ImageLoader.Builder(context) .components { add(VideoFrameDecoder.Factory()) } + // Memory only, and a size decided rather than inherited. + // + // A disk cache would hold a second copy of pictures that are already on this device: the + // sources are local files, the decode is cheap, and the copy is a liability — it survives the + // file being edited or replaced and would then serve the old picture from an entry nothing + // invalidates. The memory cache is what makes scrolling back up a thousand rows instant, and + // it is bounded explicitly so a big folder cannot grow it into the app's whole heap. + .diskCache(null) + .memoryCache { + MemoryCache.Builder() + .maxSizePercent(context, MEMORY_CACHE_PERCENT) + .build() + } .build() + private companion object { + /** + * How much of the app's heap the thumbnail memory cache may use. + * + * Coil's own default is a quarter. A file manager's thumbnails are small and numerous, and the + * rows are also holding on to their own state, so this leaves the rest of the heap to the listing + * and to Compose — a size chosen with the screen in mind rather than inherited. + */ + const val MEMORY_CACHE_PERCENT = 0.15 + } + } diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/core/ThumbnailKey.kt b/app/src/main/java/de/softwareapp_hb/filemanager/core/ThumbnailKey.kt new file mode 100644 index 0000000..8515703 --- /dev/null +++ b/app/src/main/java/de/softwareapp_hb/filemanager/core/ThumbnailKey.kt @@ -0,0 +1,22 @@ +package de.softwareapp_hb.filemanager.core + +/** + * The identity of a thumbnail: which file it came from, and which *version* of that file. + * + * Coil keys its caches by whatever a request says, and by default that is the `File` — its path. Keyed + * by path alone, a file that is replaced, edited or renamed-over outside the app keeps showing the + * picture the cache already has: nothing tells Coil the pixels changed. In a file manager that is not a + * cosmetic bug — the app exists to show what is on disk — so the key carries the size and the + * modification time, and a file that changes becomes a different entry. + * + * It is a pure function over data the listing already carries, deliberately: computing the key must not + * `stat` the file. The listing has just read those numbers, and asking the filesystem again for every + * visible row would put disk reads back on the thread that draws them — the exact cost that was taken + * off the startup path in [docs/PERFORMANCE.md]. + */ +object ThumbnailKey { + + /** A cache key that changes when the file's path, size or modification time changes. */ + fun of(path: String, sizeBytes: Long, lastModifiedMillis: Long): String = + "$path|$sizeBytes|$lastModifiedMillis" +} diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserComponents.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserComponents.kt index b9f0b37..9509a4c 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserComponents.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserComponents.kt @@ -4,6 +4,7 @@ import android.content.Context import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.combinedClickable import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxWidth @@ -53,6 +54,7 @@ import de.softwareapp_hb.filemanager.core.FileCategory import de.softwareapp_hb.filemanager.core.FileEntry import de.softwareapp_hb.filemanager.core.FileKind import de.softwareapp_hb.filemanager.core.FileSize +import de.softwareapp_hb.filemanager.core.ThumbnailKey import java.io.File /** The thumbnail an entry's leading visual gets in a list row. */ @@ -89,21 +91,10 @@ fun EntryIcon(entry: FileEntry, size: Dp) { // what it settled on, which is the icon. var failed by remember(entry.path.value) { mutableStateOf(false) } - if (entry.hasThumbnail() && !failed) { - val context = LocalContext.current - // Remembered, because Coil keys its caches by the request: a new request object on every - // recomposition would be a new cache entry on every recomposition. - val model = remember(entry.path.value, entry.kind) { entry.thumbnailRequest(context) } - AsyncImage( - model = model, - contentDescription = null, - contentScale = ContentScale.Crop, - onError = { failed = true }, - modifier = Modifier - .size(size) - .clip(RoundedCornerShape(6.dp)), - ) - } else { + // The icon is drawn first and the thumbnail paints over it. That is the placeholder: a row scrolled + // into view while a thousand others are still decoding shows the file's kind immediately instead of + // an empty square, and the picture replaces it without a flicker. + Box(modifier = Modifier.size(size), contentAlignment = Alignment.Center) { Icon( imageVector = iconFor(entry.kind), contentDescription = null, @@ -114,6 +105,25 @@ fun EntryIcon(entry: FileEntry, size: Dp) { MaterialTheme.colorScheme.onSurfaceVariant }, ) + + if (entry.hasThumbnail() && !failed) { + val context = LocalContext.current + // Remembered, because Coil keys its caches by the request: a new request object on every + // recomposition would be a new cache entry on every recomposition. The size and timestamp are + // part of the key (and of this `remember`), so a file that changes is decoded again. + val model = remember(entry.path.value, entry.kind, entry.sizeBytes, entry.lastModifiedMillis) { + entry.thumbnailRequest(context) + } + AsyncImage( + model = model, + contentDescription = null, + contentScale = ContentScale.Crop, + onError = { failed = true }, + modifier = Modifier + .size(size) + .clip(RoundedCornerShape(6.dp)), + ) + } } } @@ -132,14 +142,20 @@ private fun FileEntry.hasThumbnail(): Boolean = * The frame is taken [VIDEO_FRAME_MILLIS] in rather than at zero, because the first frame of a * recording is very often black or the middle of a fade-in, and a thumbnail of a black rectangle is * no more useful than the icon it replaced. + * + * Both carry an explicit cache key built from the listing's own numbers, so a file that has changed since + * it was cached decodes again instead of showing the previous picture; see [ThumbnailKey] for why the key + * carries more than the path, and why it does not `stat` the file. */ private fun FileEntry.thumbnailRequest(context: Context): Any { val file = File(path.value) - if (kind != FileKind.VIDEO) return file - return ImageRequest.Builder(context) + val key = ThumbnailKey.of(path.value, sizeBytes, lastModifiedMillis) + val request = ImageRequest.Builder(context) .data(file) - .videoFrameMillis(VIDEO_FRAME_MILLIS) - .build() + .memoryCacheKey(key) + .diskCacheKey(key) + if (kind == FileKind.VIDEO) request.videoFrameMillis(VIDEO_FRAME_MILLIS) + return request.build() } /** Where in a video its thumbnail is taken: far enough in to be past the usual fade-in. */ diff --git a/app/src/test/java/de/softwareapp_hb/filemanager/core/ThumbnailKeyTest.kt b/app/src/test/java/de/softwareapp_hb/filemanager/core/ThumbnailKeyTest.kt new file mode 100644 index 0000000..c75b49f --- /dev/null +++ b/app/src/test/java/de/softwareapp_hb/filemanager/core/ThumbnailKeyTest.kt @@ -0,0 +1,47 @@ +package de.softwareapp_hb.filemanager.core + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Test + +/** + * What a thumbnail's cache entry is allowed to remember. + * + * These are the two ways a file manager shows a wrong picture, and the key is what prevents both: a file + * whose contents were replaced (same path, new size and timestamp) and a file that was edited in place + * (same size, new timestamp). + */ +class ThumbnailKeyTest { + + @Test + fun `the same file with the same size and timestamp is the same cache entry`() { + assertEquals( + ThumbnailKey.of("/storage/emulated/0/DCIM/a.jpg", 4096, 1_700_000_000_000), + ThumbnailKey.of("/storage/emulated/0/DCIM/a.jpg", 4096, 1_700_000_000_000), + ) + } + + @Test + fun `a replaced file is a different cache entry even though its path is unchanged`() { + assertNotEquals( + ThumbnailKey.of("/storage/emulated/0/DCIM/a.jpg", 4096, 1_700_000_000_000), + ThumbnailKey.of("/storage/emulated/0/DCIM/a.jpg", 8192, 1_700_000_000_000), + ) + } + + @Test + fun `a file edited in place is a different cache entry even though its size is unchanged`() { + assertNotEquals( + ThumbnailKey.of("/storage/emulated/0/DCIM/a.jpg", 4096, 1_700_000_000_000), + ThumbnailKey.of("/storage/emulated/0/DCIM/a.jpg", 4096, 1_700_000_060_000), + ) + } + + @Test + fun `two files with the same size and timestamp do not share an entry`() { + assertNotEquals( + ThumbnailKey.of("/storage/emulated/0/DCIM/a.jpg", 4096, 1_700_000_000_000), + ThumbnailKey.of("/storage/emulated/0/DCIM/b.jpg", 4096, 1_700_000_000_000), + ) + } +} diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 4c1268a..caf55d4 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. -77 source files, 12793 lines - 33 test files, 386 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, 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. 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`. @@ -25,6 +25,7 @@ Pure Kotlin, **no Android imports**: paths, file names, kinds, sorting, formatti | `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) | | `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 | - | | `TransferPlanner.kt` | 105 | Turns "copy/paste these items into that folder" into a per-item decision, before a single byte is written. ConflictPolicy, TransferAction, TransferSource, TransferItem, TransferPlanner | `TransferPlannerTest` (5) | @@ -93,7 +94,7 @@ The browser screen: view model, state, list and grid, dialogs, selection bar | file | lines | what it holds | tests | |---|---|---|---| -| `BrowserComponents.kt` | 301 | The thumbnail an entry's leading visual gets in a list row. ListThumbnailSize, GridThumbnailSize, GridCellMinSize, EntryIcon(), FileEntry(), FileEntry(), VIDEO_FRAME_MILLIS, iconFor(), iconFor(), EntryRow(), EntryGridCell(), FileEntry() | - | +| `BrowserComponents.kt` | 317 | The thumbnail an entry's leading visual gets in a list row. ListThumbnailSize, GridThumbnailSize, GridCellMinSize, EntryIcon(), FileEntry(), FileEntry(), VIDEO_FRAME_MILLIS, iconFor(), iconFor(), EntryRow(), EntryGridCell(), FileEntry() | - | | `BrowserContent.kt` | 463 | Everything below the app bar: the permission prompt, the roots, or a directory. WIDE_LAYOUT_MIN_WIDTH, MasterPaneWidth, BrowserContent(), LazyListScope(), LIBRARY_SECTION_KEY, LazyListScope(), BOOKMARK_SECTION_KEY, DirectoryView(), FileEntry(), IndexStatusRow(), FileEntry() | - | | `BrowserDetailPane.kt` | 131 | The second pane of the wide layout: what is inside the folder the user picked, or a preview and the details of the file they picked. DetailPane(), FolderPreview(), ImagePreview() | - | | `BrowserDialogs.kt` | 279 | Renaming several files at once, with the numbering worked out before anything moves. BatchRenameDialog(), PREVIEW_NAMES, batchRenameNote(), NameDialog(), ConfirmDialog(), ConflictDialog() | `BrowserDialogsTest` (5) | @@ -161,7 +162,7 @@ Compose theme, colours, type | file | lines | what it holds | tests | |---|---|---|---| -| `FileManagerApplication.kt` | 126 | Application entry point and the app's tiny object graph. FileManagerApplication | - | +| `FileManagerApplication.kt` | 151 | Application entry point and the app's tiny object graph. FileManagerApplication | - | | `MainActivity.kt` | 65 | MainActivity | - | ## benchmark/ @@ -299,6 +300,13 @@ Every test name in the suite, by file: the fastest way to see whether something - ancestor and descendant are strict - is a value type +### `core/ThumbnailKeyTest.kt` (4) + +- the same file with the same size and timestamp is the same cache entry +- a replaced file is a different cache entry even though its path is unchanged +- a file edited in place is a different cache entry even though its size is unchanged +- two files with the same size and timestamp do not share an entry + ### `core/TransferPlannerTest.kt` (5) - no conflict copies straight in @@ -667,4 +675,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 76): `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 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` diff --git a/docs/EDGE_CASE_TESTING.md b/docs/EDGE_CASE_TESTING.md index ec91dfe..a11d44e 100644 --- a/docs/EDGE_CASE_TESTING.md +++ b/docs/EDGE_CASE_TESTING.md @@ -256,6 +256,37 @@ Deliberately not done on the device: a favourite whose folder disappears. The ca JVM tests, and staging it would have meant deleting a folder on someone else's phone; the missing row's wording and the message are the same strings those tests assert. +## The thumbnail hardening session + +The one screen that had never been pushed past what a photo album happens to contain: a folder that is +mostly *not* pictures, next to a folder that is nothing but pictures. Both built on the phone, both driven +by hand through `adb`. + +| pass | result | +|---|---| +| **6 200 files, 300 of them real JPEGs** (`AAThumbs`) — opened and scrolled five times | **Correct and stable.** No crash, no `OutOfMemoryError`, no decode failure that left an empty square; text rows kept their document icon and the JPEGs further down would have had their frames. PSS 86.6 MB while scrolling the text rows. | +| **300 real JPEGs** (`AAImages`) — opened, scrolled eight swipes down and one back | **Correct.** Every row drew its own picture, not an icon and not a neighbour's picture — 300 copies of one photo, so a cache-key collision would have been invisible here and is covered by `ThumbnailKeyTest` instead. No crash, no OOM. PSS grew 83.9 MB → 149.6 MB as the memory cache filled, then stayed. | + +Two things that pass is *not* evidence of: + +- **It is not a jank measurement.** This device reports no frame durations at all (`docs/PERFORMANCE.md`), + so "it scrolled" is all that can be said here. The emulator path reports `frameDurationCpuMs`, so the + number that would matter can be taken there or on a newer phone. +- **It is not a 20 000 *image* folder.** The 6 200-file folder held 300 images, and 300 is where the memory + cache tops out. A folder of 20 000 pictures is the case that would test the cache bound rather than the + listing, and it is not staged — writing 20 000 files through MIUI's storage layer took five minutes for + 6 000, so it is a fixture to build once and keep, not something to improvise. + +What the pass did change, before it was run: + +- thumbnails are keyed by path **and size and modification time** (`core/ThumbnailKey`), so a file that is + replaced or edited outside the app re-decodes instead of showing the previous picture — the one failure + a cache can cause that a user would mistake for data loss; +- the icon is drawn *under* the thumbnail rather than the row showing an empty square while decoding; +- the image loader's caches are decided explicitly: **memory only**, at 15 % of the heap, and no disk + cache — a disk cache would hold a second copy of pictures that are already on the device, and survive + the file changing. + ## Not verified, and why - **Long-press multi-select via adb: it works, but not every time.** `input swipe x y x y 900` diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index dfa3a92..b56ecb1 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -73,9 +73,12 @@ release should not go out without. resolve the delta. One run on a physical API 33+ device — `compile --reset` versus `compile -m speed-profile` — turns it from "bundled" into a number in [PERFORMANCE](PERFORMANCE.md). -2. **Thumbnail hardening before the next release** — an explicit Coil `ImageLoader` with a decided cache - policy and a placeholder, plus a large-folder device pass recorded in - [EDGE_CASE_TESTING](EDGE_CASE_TESTING.md). The only item here that touches code changed late in 1.0. +2. **Thumbnail hardening** — **done**: thumbnails are keyed by path *and* size *and* modification time + (`core/ThumbnailKey`, four tests), the icon is drawn under the thumbnail instead of an empty square + while decoding, and the image loader's caches are decided explicitly — memory only at 15 % of the heap, + no disk cache. The device pass is in [EDGE_CASE_TESTING](EDGE_CASE_TESTING.md): 6 200 files with 300 + images and a folder of 300 images, correct and stable, no crash or OOM. A 20 000-*image* folder is + still unmeasured, and jank on this screen needs a device that reports frame durations. 3. **The scaling numbers** — `./tools/benchmark-fixture.sh --count 20000` and the existing benchmarks: answers whether a full SD card is a performance problem before a user with one finds out. 4. **Copy to… / Move to… (M16)** — the comfort feature, specified below.