From 1ec71381a0965c975f35e7634c40b35a8db07783 Mon Sep 17 00:00:00 2001 From: Hadrian Burkhardt Date: Sat, 17 Oct 2026 22:44:53 +0200 Subject: [PATCH] The picker names a volume the way the browser does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from driving Copy to… and Move to… on the phone. The dialog was nested inside the conflict-prompt block, so it could only appear when a conflict was already on screen - which is never. Nothing in the unit tests could see that: they test the view model, and the view model was right. And the picker named internal storage by the platform's own name for it, 0: as the volume row, and in the trail above it (0 / AADst). The roots screen has always shown Internal storage there; the picker now follows the same rule, through a label resource on PickerFolder rather than a second copy of the string logic. A test pins the label on both the row and the crumb. On the device: copy leaves the source and the selection in place, move empties the source, clears the selection and says Moved 1 item. Recorded in EDGE_CASE_TESTING.md. --- .../filemanager/ui/browser/BrowserDialogs.kt | 8 ++++-- .../filemanager/ui/browser/BrowserScreen.kt | 21 +++++++------- .../filemanager/ui/browser/BrowserUiState.kt | 13 ++++++--- .../ui/browser/BrowserViewModel.kt | 28 +++++++++++++++---- .../ui/browser/BrowserViewModelTest.kt | 25 ++++++++++++++--- docs/CODE_MAP.md | 13 +++++---- docs/EDGE_CASE_TESTING.md | 16 +++++++++++ docs/ROADMAP.md | 5 ++-- 8 files changed, 94 insertions(+), 35 deletions(-) diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserDialogs.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserDialogs.kt index 525b0cc..83139ee 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserDialogs.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserDialogs.kt @@ -71,8 +71,12 @@ fun TransferToDialog( text = { Column { if (picker.crumbs.isNotEmpty()) { + // `map` is inline, so the labels can be resolved here; the trail is then plain text. + val trail = picker.crumbs.map { crumb -> + crumb.labelRes?.let { res -> stringResource(res) } ?: crumb.title + } Text( - text = picker.crumbs.joinToString(" / ") { it.name }, + text = trail.joinToString(" / "), style = MaterialTheme.typography.bodySmall, maxLines = 1, ) @@ -102,7 +106,7 @@ fun TransferToDialog( ) Spacer(modifier = Modifier.width(12.dp)) Text( - text = folder.title, + text = folder.labelRes?.let { stringResource(it) } ?: folder.title, maxLines = 1, overflow = TextOverflow.Ellipsis, ) diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt index ea231ba..071d516 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserScreen.kt @@ -582,18 +582,17 @@ fun BrowserScreen( ) } - state.conflict?.let { prompt -> - val transferPicker = state.transferPicker - if (transferPicker != null) { - TransferToDialog( - picker = transferPicker, - onOpen = viewModel::pickerOpen, - onUp = viewModel::pickerUp, - onConfirm = viewModel::confirmTransferTo, - onDismiss = viewModel::cancelTransferTo, - ) - } + state.transferPicker?.let { picker -> + TransferToDialog( + picker = picker, + onOpen = viewModel::pickerOpen, + onUp = viewModel::pickerUp, + onConfirm = viewModel::confirmTransferTo, + onDismiss = viewModel::cancelTransferTo, + ) + } + state.conflict?.let { prompt -> ConflictDialog( prompt = prompt, onChoice = { policy -> viewModel.resolveConflict(policy) }, diff --git a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt index 245d4ac..b1b7c92 100644 --- a/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt +++ b/app/src/main/java/de/softwareapp_hb/filemanager/ui/browser/BrowserUiState.kt @@ -42,9 +42,14 @@ data class Clipboard(val paths: List, val cut: Boolean) { * [label] is the platform's name for a volume, which is the only place the picker has something better * than a path to show — internal storage's own name is `0`. */ -data class PickerFolder(val path: StoragePath, val label: String? = null) { +data class PickerFolder( + val path: StoragePath, + val label: String? = null, + /** A localised name for the volumes the platform does not name — internal storage is `0` to it. */ + @StringRes val labelRes: Int? = null, +) { - /** What the row says: the volume's label when there is one, else the folder's own name. */ + /** What the row says: the volume's own name, else the folder's name. */ val title: String get() = label ?: path.name } @@ -61,8 +66,8 @@ data class TransferPicker( /** The folder being shown, or `null` for the volumes. */ val path: StoragePath? = null, val folders: List = emptyList(), - /** The trail back to the volumes, ending with [path] when one is open. */ - val crumbs: List = emptyList(), + /** The trail back to the volumes, ending with [path] when one is open; a root keeps its label. */ + val crumbs: List = emptyList(), val loading: Boolean = false, ) 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 8d861c0..6e404c1 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 @@ -517,7 +517,7 @@ class BrowserViewModel( fun pickerUp() { val picker = _state.value.transferPicker ?: return val crumbs = picker.crumbs - openPicker(if (crumbs.size > 1) crumbs[crumbs.size - 2] else null) + openPicker(if (crumbs.size > 1) crumbs[crumbs.size - 2].path else null) } fun cancelTransferTo() { @@ -574,7 +574,15 @@ class BrowserViewModel( } viewModelScope.launch { val folders = if (path == null) { - storage.volumes().map { PickerFolder(path = it.root, label = it.label) } + storage.volumes().map { volume -> + PickerFolder( + path = volume.root, + label = volume.label, + // The same rule as the roots screen: internal storage has no platform label, and + // showing its own name (`0`) in a copy dialog would be a riddle. + labelRes = if (volume.label == null) R.string.browser_internal_storage else null, + ) + } } else { val source = storage.activeSource() val entries = if (source == null) { @@ -599,17 +607,25 @@ class BrowserViewModel( } /** The trail down to [path], starting at its volume root; empty for the volume list itself. */ - private fun pickerCrumbs(path: StoragePath?): List { + private fun pickerCrumbs(path: StoragePath?): List { if (path == null) return emptyList() - val roots = storage.volumes().mapTo(HashSet()) { it.root } + val volumes = storage.volumes().associateBy { it.root } val crumbs = ArrayList() var current: StoragePath? = path while (current != null) { crumbs += current - if (current in roots) break + if (current in volumes) break current = current.parent } - return crumbs.reversed() + return crumbs.reversed().map { crumb -> + // A volume root in the trail is named the way the volume list names it. + val volume = volumes[crumb] ?: return@map PickerFolder(path = crumb) + PickerFolder( + path = crumb, + label = volume.label, + labelRes = if (volume.label == null) R.string.browser_internal_storage else null, + ) + } } // ----------------------------------------------------------------- libraries 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 862a964..3fe3ef6 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 @@ -284,6 +284,23 @@ class BrowserViewModelTest { assertEquals(setOf(documents, downloads), folders.toSet()) } + @Test + fun `the picker names a volume the way the browser does`() { + val model = viewModel() + model.openDirectory(downloads) + model.toggleSelection(downloads.child("a.txt")) + + model.startTransferTo(cut = false) + val volumes = model.state.value.transferPicker!!.folders + assertEquals(listOf(root), volumes.map { it.path }) + // Internal storage has no platform label; showing its own name, `0`, would be a riddle. + assertEquals(R.string.browser_internal_storage, volumes.single().labelRes) + + model.pickerOpen(downloads) + val crumb = model.state.value.transferPicker!!.crumbs.first() + assertEquals(R.string.browser_internal_storage, crumb.labelRes) + } + @Test fun `the picker walks up to the volumes and back down`() { val model = viewModel() @@ -292,16 +309,16 @@ class BrowserViewModelTest { model.startTransferTo(cut = false) // The picker opens on the volumes, with no trail behind it. - assertEquals(emptyList(), model.state.value.transferPicker!!.crumbs) + assertTrue(model.state.value.transferPicker!!.crumbs.isEmpty()) model.pickerOpen(downloads) - assertEquals(listOf(root, downloads), model.state.value.transferPicker!!.crumbs) + assertEquals(listOf(root, downloads), model.state.value.transferPicker!!.crumbs.map { it.path }) model.pickerUp() - assertEquals(listOf(root), model.state.value.transferPicker!!.crumbs) + assertEquals(listOf(root), model.state.value.transferPicker!!.crumbs.map { it.path }) model.pickerUp() - assertEquals(emptyList(), model.state.value.transferPicker!!.crumbs) + assertTrue(model.state.value.transferPicker!!.crumbs.isEmpty()) } @Test diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 71aed83..78ac18d 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. -79 source files, 13369 lines - 35 test files, 403 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, 13393 lines - 35 test files, 404 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`. @@ -98,12 +98,12 @@ The browser screen: view model, state, list and grid, dialogs, selection bar | `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` | 381 | The Copy to… / Move to… picker. TransferToDialog(), titleFor(), hereFor(), BatchRenameDialog(), PREVIEW_NAMES, batchRenameNote(), NameDialog(), ConfirmDialog(), ConflictDialog() | `BrowserDialogsTest` (5) | +| `BrowserDialogs.kt` | 385 | The Copy to… / Move to… picker. TransferToDialog(), titleFor(), hereFor(), BatchRenameDialog(), PREVIEW_NAMES, batchRenameNote(), NameDialog(), ConfirmDialog(), ConflictDialog() | `BrowserDialogsTest` (5) | | `BrowserRoots.kt` | 270 | A row on the root screen: a volume, with its free space when the platform reports it. RootRow(), itemCountLabel(), EmptyState(), LibrarySectionHeader(), BookmarkSectionHeader(), SectionHeader(), BookmarkRow(), LibraryRow(), Breadcrumbs() | `BrowserRootsTest` (4) | -| `BrowserScreen.kt` | 604 | BrowserScreen() | - | +| `BrowserScreen.kt` | 603 | 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` | 343 | One immutable description of everything the browser shows. ViewMode, SearchScope, Clipboard, PickerFolder, TransferPicker, UiMessage, ConflictPrompt, RootItem, Bookmark, DetailFocus, Undo, UndoMove, Overlay, BrowserUiState | - | -| `BrowserViewModel.kt` | 2376 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (96) | +| `BrowserUiState.kt` | 348 | One immutable description of everything the browser shows. ViewMode, SearchScope, Clipboard, PickerFolder, TransferPicker, UiMessage, ConflictPrompt, RootItem, Bookmark, DetailFocus, Undo, UndoMove, Overlay, BrowserUiState | - | +| `BrowserViewModel.kt` | 2392 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (97) | | `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() | - | @@ -581,7 +581,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` (96) +### `ui/browser/BrowserViewModelTest.kt` (97) - starts on the root list - opening a folder lists it with directories first and natural order @@ -592,6 +592,7 @@ Every test name in the suite, by file: the fastest way to see whether something - copy to a chosen folder leaves the user where they are and keeps the selection - move to a chosen folder takes the rows with it - the picker offers folders and nothing else +- the picker names a volume the way the browser does - the picker walks up to the volumes and back down - a conflicting destination asks before overwriting, naming that folder - cancelling the picker copies nothing diff --git a/docs/EDGE_CASE_TESTING.md b/docs/EDGE_CASE_TESTING.md index a11d44e..ab9293c 100644 --- a/docs/EDGE_CASE_TESTING.md +++ b/docs/EDGE_CASE_TESTING.md @@ -287,6 +287,22 @@ What the pass did change, before it was run: cache — a disk cache would hold a second copy of pictures that are already on the device, and survive the file changing. +## The copy-to session (M16) + +Driven on the phone through the real UI (`adb` taps on `uiautomator` dumps), with `/sdcard/AASrc`, +`/sdcard/AADst` and `/sdcard/AAMv` standing in for two user folders. + +- **Copy** into a picked folder: one file selected in `AASrc`, overflow → *Copy to…*, *Internal + storage* → `AADst` → *Copy here*. The file appeared in `AADst`, **stayed** in `AASrc`, the browser + came back to `AASrc`, and the bar still read *1 item* — the selection really does survive a copy. +- **Move** into a picked folder: the same walk into `AAMv`. `AASrc` ended up empty, `AAMv` held the + file, the snackbar said *Moved 1 item*, the listing said *This folder is empty*, and the selection bar + was gone. +- **The trail and the volume row.** Both first read the platform's own name for internal storage — + `0 / AADst` and a row labelled `0` — which is correct as a path and useless as a label. Fixed by + giving the picker the same label rule as the roots screen; the second pass reads *Internal storage / + AADst*. This is the kind of thing only a device shows: the JVM tests use paths, not labels. + ## 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 6dbc437..9767d55 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -86,8 +86,9 @@ release should not go out without. picked folder without leaving the current one. The selection survives a copy so a second destination is two taps; a move clears it. Conflicts ask the same question as a paste, naming the picked folder. Seven tests cover the verbs, the picker's walk, folders-only listing, the conflict target, cancellation, and - the selection rule. **Not done**: the "copied to X — Show" message with a jump to the destination; the - copy reports itself the way a paste does. + the selection rule. Driven on the phone for both verbs (see `EDGE_CASE_TESTING.md`), which is what caught + the picker naming internal storage by its raw name `0`. **Not done**: the "copied to X — Show" message + with a jump to the destination; the copy reports itself the way a paste does. 5. **A CI run of the benchmarks on an emulator** — removes the phone from the loop and reports `frameDurationCpuMs`, which this device cannot.