The picker names a volume the way the browser does
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.
This commit is contained in:
@@ -71,8 +71,12 @@ fun TransferToDialog(
|
|||||||
text = {
|
text = {
|
||||||
Column {
|
Column {
|
||||||
if (picker.crumbs.isNotEmpty()) {
|
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(
|
||||||
text = picker.crumbs.joinToString(" / ") { it.name },
|
text = trail.joinToString(" / "),
|
||||||
style = MaterialTheme.typography.bodySmall,
|
style = MaterialTheme.typography.bodySmall,
|
||||||
maxLines = 1,
|
maxLines = 1,
|
||||||
)
|
)
|
||||||
@@ -102,7 +106,7 @@ fun TransferToDialog(
|
|||||||
)
|
)
|
||||||
Spacer(modifier = Modifier.width(12.dp))
|
Spacer(modifier = Modifier.width(12.dp))
|
||||||
Text(
|
Text(
|
||||||
text = folder.title,
|
text = folder.labelRes?.let { stringResource(it) } ?: folder.title,
|
||||||
maxLines = 1,
|
maxLines = 1,
|
||||||
overflow = TextOverflow.Ellipsis,
|
overflow = TextOverflow.Ellipsis,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -582,11 +582,9 @@ fun BrowserScreen(
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
state.conflict?.let { prompt ->
|
state.transferPicker?.let { picker ->
|
||||||
val transferPicker = state.transferPicker
|
|
||||||
if (transferPicker != null) {
|
|
||||||
TransferToDialog(
|
TransferToDialog(
|
||||||
picker = transferPicker,
|
picker = picker,
|
||||||
onOpen = viewModel::pickerOpen,
|
onOpen = viewModel::pickerOpen,
|
||||||
onUp = viewModel::pickerUp,
|
onUp = viewModel::pickerUp,
|
||||||
onConfirm = viewModel::confirmTransferTo,
|
onConfirm = viewModel::confirmTransferTo,
|
||||||
@@ -594,6 +592,7 @@ fun BrowserScreen(
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
state.conflict?.let { prompt ->
|
||||||
ConflictDialog(
|
ConflictDialog(
|
||||||
prompt = prompt,
|
prompt = prompt,
|
||||||
onChoice = { policy -> viewModel.resolveConflict(policy) },
|
onChoice = { policy -> viewModel.resolveConflict(policy) },
|
||||||
|
|||||||
@@ -42,9 +42,14 @@ data class Clipboard(val paths: List<StoragePath>, val cut: Boolean) {
|
|||||||
* [label] is the platform's name for a volume, which is the only place the picker has something better
|
* [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`.
|
* 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
|
val title: String get() = label ?: path.name
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -61,8 +66,8 @@ data class TransferPicker(
|
|||||||
/** The folder being shown, or `null` for the volumes. */
|
/** The folder being shown, or `null` for the volumes. */
|
||||||
val path: StoragePath? = null,
|
val path: StoragePath? = null,
|
||||||
val folders: List<PickerFolder> = emptyList(),
|
val folders: List<PickerFolder> = emptyList(),
|
||||||
/** The trail back to the volumes, ending with [path] when one is open. */
|
/** The trail back to the volumes, ending with [path] when one is open; a root keeps its label. */
|
||||||
val crumbs: List<StoragePath> = emptyList(),
|
val crumbs: List<PickerFolder> = emptyList(),
|
||||||
val loading: Boolean = false,
|
val loading: Boolean = false,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -517,7 +517,7 @@ class BrowserViewModel(
|
|||||||
fun pickerUp() {
|
fun pickerUp() {
|
||||||
val picker = _state.value.transferPicker ?: return
|
val picker = _state.value.transferPicker ?: return
|
||||||
val crumbs = picker.crumbs
|
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() {
|
fun cancelTransferTo() {
|
||||||
@@ -574,7 +574,15 @@ class BrowserViewModel(
|
|||||||
}
|
}
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
val folders = if (path == null) {
|
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 {
|
} else {
|
||||||
val source = storage.activeSource()
|
val source = storage.activeSource()
|
||||||
val entries = if (source == null) {
|
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. */
|
/** The trail down to [path], starting at its volume root; empty for the volume list itself. */
|
||||||
private fun pickerCrumbs(path: StoragePath?): List<StoragePath> {
|
private fun pickerCrumbs(path: StoragePath?): List<PickerFolder> {
|
||||||
if (path == null) return emptyList()
|
if (path == null) return emptyList()
|
||||||
val roots = storage.volumes().mapTo(HashSet()) { it.root }
|
val volumes = storage.volumes().associateBy { it.root }
|
||||||
val crumbs = ArrayList<StoragePath>()
|
val crumbs = ArrayList<StoragePath>()
|
||||||
var current: StoragePath? = path
|
var current: StoragePath? = path
|
||||||
while (current != null) {
|
while (current != null) {
|
||||||
crumbs += current
|
crumbs += current
|
||||||
if (current in roots) break
|
if (current in volumes) break
|
||||||
current = current.parent
|
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
|
// ----------------------------------------------------------------- libraries
|
||||||
|
|||||||
@@ -284,6 +284,23 @@ class BrowserViewModelTest {
|
|||||||
assertEquals(setOf(documents, downloads), folders.toSet())
|
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
|
@Test
|
||||||
fun `the picker walks up to the volumes and back down`() {
|
fun `the picker walks up to the volumes and back down`() {
|
||||||
val model = viewModel()
|
val model = viewModel()
|
||||||
@@ -292,16 +309,16 @@ class BrowserViewModelTest {
|
|||||||
|
|
||||||
model.startTransferTo(cut = false)
|
model.startTransferTo(cut = false)
|
||||||
// The picker opens on the volumes, with no trail behind it.
|
// The picker opens on the volumes, with no trail behind it.
|
||||||
assertEquals(emptyList<StoragePath>(), model.state.value.transferPicker!!.crumbs)
|
assertTrue(model.state.value.transferPicker!!.crumbs.isEmpty())
|
||||||
|
|
||||||
model.pickerOpen(downloads)
|
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()
|
model.pickerUp()
|
||||||
assertEquals(listOf(root), model.state.value.transferPicker!!.crumbs)
|
assertEquals(listOf(root), model.state.value.transferPicker!!.crumbs.map { it.path })
|
||||||
|
|
||||||
model.pickerUp()
|
model.pickerUp()
|
||||||
assertEquals(emptyList<StoragePath>(), model.state.value.transferPicker!!.crumbs)
|
assertTrue(model.state.value.transferPicker!!.crumbs.isEmpty())
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|||||||
+7
-6
@@ -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.
|
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`.
|
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() | - |
|
| `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() | - |
|
| `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() | - |
|
| `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) |
|
| `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) |
|
| `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 | - |
|
| `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` | 2376 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (96) |
|
| `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() | - |
|
| `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() | - |
|
| `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() | - |
|
| `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
|
- a library or the root list has nothing to star
|
||||||
- the star gives way to the search field
|
- the star gives way to the search field
|
||||||
|
|
||||||
### `ui/browser/BrowserViewModelTest.kt` (96)
|
### `ui/browser/BrowserViewModelTest.kt` (97)
|
||||||
|
|
||||||
- starts on the root list
|
- starts on the root list
|
||||||
- opening a folder lists it with directories first and natural order
|
- 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
|
- 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
|
- move to a chosen folder takes the rows with it
|
||||||
- the picker offers folders and nothing else
|
- 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
|
- the picker walks up to the volumes and back down
|
||||||
- a conflicting destination asks before overwriting, naming that folder
|
- a conflicting destination asks before overwriting, naming that folder
|
||||||
- cancelling the picker copies nothing
|
- cancelling the picker copies nothing
|
||||||
|
|||||||
@@ -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
|
cache — a disk cache would hold a second copy of pictures that are already on the device, and survive
|
||||||
the file changing.
|
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
|
## Not verified, and why
|
||||||
|
|
||||||
- **Long-press multi-select via adb: it works, but not every time.** `input swipe x y x y 900`
|
- **Long-press multi-select via adb: it works, but not every time.** `input swipe x y x y 900`
|
||||||
|
|||||||
+3
-2
@@ -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
|
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
|
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
|
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
|
the selection rule. Driven on the phone for both verbs (see `EDGE_CASE_TESTING.md`), which is what caught
|
||||||
copy reports itself the way a paste does.
|
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
|
5. **A CI run of the benchmarks on an emulator** — removes the phone from the loop and reports
|
||||||
`frameDurationCpuMs`, which this device cannot.
|
`frameDurationCpuMs`, which this device cannot.
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user