diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a54ff8..cf8f6ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,8 @@ upload needs a **higher `versionCode`** than every previous upload. folder **without leaving the one you are in**. The files stay selected afterwards, so sending the same set to a second destination is two taps. A name that already exists there asks exactly what Paste asks, naming that folder; progress belongs to the same operation notification, and a move clears the selection - because those rows are gone. Underneath it is the paste path with an explicit target: the engine was + because those rows are gone. A finished copy answers with **"Copied to *folder*"** and a **Show** button + that opens the destination — so the file you just put somewhere is one tap away from being looked at. Underneath it is the paste path with an explicit target: the engine was always target-agnostic, so the new part is the picker and the rules around it. - **Several files can be renamed at once, numbered in a series that carries on.** Selecting more 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 071d516..589f9b4 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 @@ -320,17 +320,18 @@ fun BrowserScreen( val message = state.message - val messageText = message?.let { - it.pluralQuantity?.let { quantity -> - pluralStringResource(it.textRes, quantity, *it.args.toTypedArray()) - } ?: stringResource(it.textRes, *it.args.toTypedArray()) + // A message that names a folder cannot be built from the path alone: a volume root has no name of its + // own (`0`), which is the one thing only the screen can resolve — the same rule breadcrumbs use. + val destinationLabel = message?.destination?.let { folder -> + folder.labelRes?.let { stringResource(it) } ?: folder.title } + val messageText = message?.let { messageText(it, destinationLabel) } // Resolved out here because `stringResource` is a composable call and the effect is not. - val undoLabel = message?.actionLabelRes?.let { stringResource(it) } + val actionLabel = message?.actionLabelRes?.let { stringResource(it) } LaunchedEffect(message?.id) { if (messageText != null) { - val outcome = snackbarHostState.showSnackbar(messageText, actionLabel = undoLabel) - if (outcome == SnackbarResult.ActionPerformed) viewModel.undo() + val outcome = snackbarHostState.showSnackbar(messageText, actionLabel = actionLabel) + if (outcome == SnackbarResult.ActionPerformed) viewModel.performMessageAction() viewModel.consumeMessage() } } @@ -601,3 +602,21 @@ fun BrowserScreen( } } +/** + * The text of a snackbar message. + * + * A message that names a folder cannot carry its label as an argument — a volume root has no name of its own + * (internal storage's is `0`) — so the screen resolves it the way breadcrumbs are resolved and passes it as + * the first argument. + * + * `SpreadOperator` is suppressed on purpose: the `vararg` resource helpers take an array, and this runs once + * per message rather than once per row or per frame, so the copy the rule warns about is not a hot path. + */ +@Suppress("SpreadOperator") +@Composable +private fun messageText(message: UiMessage, destinationLabel: String?): String { + val args = if (destinationLabel != null) listOf(destinationLabel) + message.args else message.args + return message.pluralQuantity?.let { quantity -> + pluralStringResource(message.textRes, quantity, *args.toTypedArray()) + } ?: stringResource(message.textRes, *args.toTypedArray()) +} 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 b1b7c92..3c25188 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 @@ -4,6 +4,7 @@ import android.net.Uri import androidx.activity.result.IntentSenderRequest import androidx.annotation.AnyRes import androidx.annotation.StringRes +import de.softwareapp_hb.filemanager.R import de.softwareapp_hb.filemanager.core.ConflictPolicy import de.softwareapp_hb.filemanager.core.FileCategory import de.softwareapp_hb.filemanager.core.FileEntry @@ -71,6 +72,21 @@ data class TransferPicker( val loading: Boolean = false, ) +/** + * What a snackbar's action button does. + * + * The label follows from the action rather than being passed beside it, so the two cannot disagree: an + * action that offers "Undo" is an undo, and one that offers "Show" is a jump. + */ +sealed interface MessageAction { + + /** Take the change back — the undo of the operation that has just finished. */ + data object Undo : MessageAction + + /** Open the folder an operation wrote to. */ + data class Show(val path: StoragePath) : MessageAction +} + /** A one-shot message for a snackbar. [id] makes repeats of the same text distinguishable. */ data class UiMessage( /** A `@StringRes` or `@PluralsRes` value; see [pluralQuantity]. */ @@ -78,10 +94,25 @@ data class UiMessage( val args: List = emptyList(), /** Set when [textRes] names a `` resource; the UI resolves it accordingly. */ val pluralQuantity: Int? = null, - /** When set, the snackbar offers a single action button carrying this label. */ - @StringRes val actionLabelRes: Int? = null, + /** What the snackbar's action button does; `null` for a message with no action. */ + val action: MessageAction? = null, + /** + * A folder the text names, whose label only the UI can resolve: a volume root has no name of its own — + * internal storage's is `0`. When set, its title becomes the text's first argument. + */ + val destination: PickerFolder? = null, val id: Long = System.nanoTime(), -) +) { + + /** The action button's label, or `null` when there is no action. */ + @get:StringRes + val actionLabelRes: Int? + get() = when (action) { + MessageAction.Undo -> R.string.browser_action_undo + is MessageAction.Show -> R.string.browser_action_show + null -> null + } +} /** * A paste that hit existing names, waiting for the user to choose how to resolve it. 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 6e404c1..960e82a 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 @@ -555,7 +555,14 @@ class BrowserViewModel( it.copy(conflict = ConflictPrompt(picker.paths, target, picker.cut, conflicts)) } } else { - transfer(picker.paths, target, picker.cut, ConflictPolicy.KEEP_BOTH) + transfer( + paths = picker.paths, + target = target, + cut = picker.cut, + policy = ConflictPolicy.KEEP_BOTH, + // Only a copy: a move's message is the undoable one, and its sources are gone. + announce = if (picker.cut) null else picker.crumbs.lastOrNull() ?: PickerFolder(target), + ) } } } @@ -1807,6 +1814,8 @@ class BrowserViewModel( target: StoragePath, cut: Boolean, policy: ConflictPolicy, + /** Set when the destination was chosen by hand, so the message can name it and offer to show it. */ + announce: PickerFolder? = null, ) { val source = storage.activeSource() ?: return val operations = FileOperations(source) @@ -1838,6 +1847,8 @@ class BrowserViewModel( _state.update { it.copy(undo = Undo.Relocate(moves)) } } messageUndoablePlural(R.plurals.message_moved, result.completed) + } else if (announce != null) { + messageCopiedTo(announce, result.completed) } else { messagePlural(R.plurals.message_copied, result.completed) } @@ -2224,6 +2235,20 @@ class BrowserViewModel( // ----------------------------------------------------------------- messages + /** + * Run what the current message's action button offered. + * + * Undo is the usual one. A **Copy to…** offers Show instead, because a copy leaves its sources where + * they were — the interesting place is the one it wrote to, which may be several folders away. + */ + fun performMessageAction() { + when (val action = _state.value.message?.action) { + MessageAction.Undo -> undo() + is MessageAction.Show -> openDirectory(action.path) + null -> Unit + } + } + fun consumeMessage() { _state.update { it.copy(message = null) } } @@ -2246,7 +2271,7 @@ class BrowserViewModel( /** A message whose snackbar also offers to take the change back. */ private fun messageUndoable(@StringRes textRes: Int, vararg args: Any) { postMessage( - UiMessage(textRes, args.toList(), actionLabelRes = R.string.browser_action_undo), + UiMessage(textRes, args.toList(), action = MessageAction.Undo), ) } @@ -2256,7 +2281,20 @@ class BrowserViewModel( textRes, listOf(count), pluralQuantity = count, - actionLabelRes = R.string.browser_action_undo, + action = MessageAction.Undo, + ), + ) + } + + /** A copy into a folder the user picked: names it, and offers to go there. */ + private fun messageCopiedTo(destination: PickerFolder, count: Int) { + postMessage( + UiMessage( + R.plurals.message_copied_to, + listOf(count), + pluralQuantity = count, + action = MessageAction.Show(destination.path), + destination = destination, ), ) } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 185c4f3..a1a757f 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -125,6 +125,8 @@ Select all Clear selection Undo + + Show @@ -238,6 +240,12 @@ Folder created Renamed That name is already in use + + + Copied to %1$s + Copied %2$d items to %1$s + + Copied %1$d item Copied %1$d items 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 3fe3ef6..56e4d5d 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 @@ -353,6 +353,92 @@ class BrowserViewModelTest { assertTrue(recordedNotifications.isEmpty()) } + @Test + fun `a copy says where it went and offers to show it`() { + val model = viewModel() + model.openDirectory(downloads) + model.toggleSelection(downloads.child("a.txt")) + + model.startTransferTo(cut = false) + model.pickerOpen(documents) + model.confirmTransferTo() + + val message = model.state.value.message + assertNotNull(message) + assertEquals(R.plurals.message_copied_to, message!!.textRes) + // The count is the only argument the view model owns; the folder's label is the screen's job. + assertEquals(listOf(1), message.args) + assertEquals(documents, message.destination?.path) + assertEquals(MessageAction.Show(documents), message.action) + assertEquals(R.string.browser_action_show, message.actionLabelRes) + } + + @Test + fun `show takes the user to the folder the copy went to`() { + val model = viewModel() + model.openDirectory(downloads) + model.toggleSelection(downloads.child("a.txt")) + + model.startTransferTo(cut = false) + model.pickerOpen(documents) + model.confirmTransferTo() + model.performMessageAction() + + assertEquals(documents, model.state.value.path) + assertTrue(storage.contains(documents.child("a.txt"))) + } + + @Test + fun `a paste into the folder on screen names no destination and offers no action`() { + val model = viewModel() + model.openDirectory(root) + model.copyToClipboard(listOf(downloads.child("a.txt")), cut = false) + + model.openDirectory(documents) + model.paste() + + val message = model.state.value.message + assertNotNull(message) + assertEquals(R.plurals.message_copied, message!!.textRes) + assertNull(message.destination) + assertNull(message.action) + assertNull(message.actionLabelRes) + } + + @Test + fun `the message's undo action is the undo`() { + val model = viewModel() + model.openDirectory(downloads) + model.toggleSelection(downloads.child("a.txt")) + + model.startTransferTo(cut = true) + model.pickerOpen(documents) + model.confirmTransferTo() + assertFalse(storage.contains(downloads.child("a.txt"))) + + // What the snackbar's button does — the UI no longer calls `undo()` itself, so this is the path + // that has to work for Undo to work at all. + model.performMessageAction() + + assertTrue(storage.contains(downloads.child("a.txt"))) + } + + @Test + fun `a move to a chosen folder offers undo rather than show`() { + val model = viewModel() + model.openDirectory(downloads) + model.toggleSelection(downloads.child("a.txt")) + + model.startTransferTo(cut = true) + model.pickerOpen(documents) + model.confirmTransferTo() + + val message = model.state.value.message + assertNotNull(message) + assertEquals(MessageAction.Undo, message!!.action) + assertEquals(R.string.browser_action_undo, message.actionLabelRes) + } + @Test fun `navigating up goes to the parent then to the roots`() { val model = viewModel() diff --git a/config/detekt/baseline.xml b/config/detekt/baseline.xml index 60e3f52..3088118 100644 --- a/config/detekt/baseline.xml +++ b/config/detekt/baseline.xml @@ -22,7 +22,6 @@ LongParameterList:BrowserContent.kt$( state: BrowserUiState, contentPadding: PaddingValues, crumbLabel: (StoragePath) -> String, onOpenEntry: (FileEntry) -> Unit, onFocusEntry: (FileEntry) -> Unit, onFocusRoot: (RootItem) -> Unit, onToggleSelection: (StoragePath) -> Unit, onOpenRoot: (StoragePath) -> Unit, onNavigate: (StoragePath) -> Unit, onOpenCategory: (FileCategory) -> Unit, onOpenBookmark: (StoragePath) -> Unit, onRemoveBookmark: (StoragePath) -> Unit, onGrantAllFiles: () -> Unit, onPickFolder: () -> Unit, onRebuildIndex: () -> Unit, /** Hoisted by the screen so the scroll position outlives a viewer taking over. */ listState: LazyListState = rememberLazyListState(), gridState: LazyGridState = rememberLazyGridState(), ) LongParameterList:BrowserContent.kt$( state: BrowserUiState, crumbLabel: (StoragePath) -> String, wide: Boolean, listState: LazyListState, gridState: LazyGridState, onOpenEntry: (FileEntry) -> Unit, onFocusEntry: (FileEntry) -> Unit, onToggleSelection: (StoragePath) -> Unit, onNavigate: (StoragePath) -> Unit, onGrantAllFiles: () -> Unit, onRebuildIndex: () -> Unit, modifier: Modifier = Modifier, ) LongParameterList:BrowserTopBar.kt$( title: String, canNavigateUp: Boolean, /** Whether this view is a folder, and so can be starred at all. */ canBookmark: Boolean, bookmarked: Boolean, query: String, searchOpen: Boolean, searchScope: SearchScope, /** Whether the scope is the user's to choose at all; see the note beside the toggle. */ scopeChoosable: Boolean, canPaste: Boolean, onUp: () -> Unit, onQuery: (String) -> Unit, onSearchOpen: (Boolean) -> Unit, onSearchScope: () -> Unit, onSort: () -> Unit, onPaste: () -> Unit, onToggleBookmark: () -> Unit, onRefresh: () -> Unit, onUsage: () -> Unit, onDuplicates: () -> Unit, onTrash: () -> Unit, ) - LongParameterList:SelectionBar.kt$( state: BrowserUiState, onCopy: () -> Unit, onCut: () -> Unit, onDelete: () -> Unit, onDetails: () -> Unit, onRename: () -> Unit, onOpenWith: () -> Unit, onShare: () -> Unit, onCompress: () -> Unit, onExtract: () -> Unit, onSelectAll: () -> Unit, onClear: () -> Unit, ) LoopWithTooManyJumpStatements:ArchiveEngine.kt$ArchiveEngine$while LoopWithTooManyJumpStatements:BatchRename.kt$BatchRename$for LoopWithTooManyJumpStatements:DuplicateFinder.kt$DuplicateFinder$while @@ -68,8 +67,6 @@ RethrowCaughtException:BrowserViewModel.kt$BrowserViewModel$throw cancellation ReturnCount:ArchiveEngine.kt$ArchiveEngine$private suspend fun withEntry( archive: StoragePath, name: String, sink: suspend (InputStream) -> Unit, ): Boolean ReturnCount:BrowserScreen.kt$@OptIn(ExperimentalMaterial3Api::class) @Composable fun BrowserScreen( incoming: IncomingRequest? = null, onDeliver: (FileEntry) -> Unit = {}, onDone: () -> Unit = {}, viewModel: BrowserViewModel = viewModel(factory = BrowserViewModel.Factory), ) - SpreadOperator:BrowserScreen.kt$(it.textRes, *it.args.toTypedArray()) - SpreadOperator:BrowserScreen.kt$(it.textRes, quantity, *it.args.toTypedArray()) TooGenericExceptionCaught:ArchiveEngine.kt$ArchiveEngine$failure: Exception TooGenericExceptionCaught:BrowserViewModel.kt$BrowserViewModel$failure: Exception TooGenericExceptionCaught:FileOperations.kt$FileOperations$failure: Exception diff --git a/docs/CODE_MAP.md b/docs/CODE_MAP.md index 78ac18d..bec9655 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, 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. +79 source files, 13481 lines - 35 test files, 409 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`. @@ -100,10 +100,10 @@ The browser screen: view model, state, list and grid, dialogs, selection bar | `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` | 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` | 603 | BrowserScreen() | - | +| `BrowserScreen.kt` | 622 | The text of a snackbar message. BrowserScreen(), messageText() | - | | `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` | 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) | +| `BrowserUiState.kt` | 379 | One immutable description of everything the browser shows. ViewMode, SearchScope, Clipboard, PickerFolder, TransferPicker, MessageAction, UiMessage, ConflictPrompt, RootItem, Bookmark, DetailFocus, Undo, UndoMove, Overlay, BrowserUiState | - | +| `BrowserViewModel.kt` | 2430 | Drives the browser screen. BrowserViewModel, ArchivePreview | `BrowserViewModelTest` (102) | | `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` (97) +### `ui/browser/BrowserViewModelTest.kt` (102) - starts on the root list - opening a folder lists it with directories first and natural order @@ -596,6 +596,11 @@ Every test name in the suite, by file: the fastest way to see whether something - the picker walks up to the volumes and back down - a conflicting destination asks before overwriting, naming that folder - cancelling the picker copies nothing +- a copy says where it went and offers to show it +- show takes the user to the folder the copy went to +- a paste into the folder on screen names no destination and offers no action +- the message's undo action is the undo +- a move to a chosen folder offers undo rather than show - 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/EDGE_CASE_TESTING.md b/docs/EDGE_CASE_TESTING.md index ab9293c..83f290d 100644 --- a/docs/EDGE_CASE_TESTING.md +++ b/docs/EDGE_CASE_TESTING.md @@ -298,6 +298,10 @@ Driven on the phone through the real UI (`adb` taps on `uiautomator` dumps), wit - **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 message and its Show button.** After a copy into `AADst` the snackbar read *Copied to AADst* with + **Show**, and tapping Show opened `AADst` with the copied file in it. Repeated into a volume root — the + case where the path has no name of its own — it read *Copied to Internal storage*, which is the label + rule doing its job rather than `0` leaking into a sentence. - **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 / diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 7445f87..1cf7510 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -64,8 +64,10 @@ would no longer have. ## The queue after 1.0 -Ordered by what it protects, not by what is fun. Nothing here is scheduled; the first two are what a -release should not go out without. +Ordered by what it protects, not by what is fun. **Four of the five items are done** — thumbnails, the +scaling numbers (which turned into progressive listing), Copy to… / Move to…, and the CI benchmark. What is +left is the baseline profile's *measurement*, which needs a physical API 33+ device and cannot be finished +on this machine; everything else in the list says what it produced. 1. **Baseline profile** — **generated and committed**: `app/src/main/baseline-prof.txt` is bundled in the release APK, written by the `:baselineprofile` module. What remains is its *measurement*: collection @@ -90,8 +92,10 @@ release should not go out without. 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. 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. + the picker naming internal storage by its raw name `0`. A copy into a picked folder says **"Copied to + "** and offers **Show**, which opens that folder — the destination's label resolved the way + breadcrumbs are, so a volume root reads *Internal storage* rather than `0`. A move keeps the undoable + message instead, because its sources are gone. 5. **A CI run of the benchmarks on an emulator** — **done**: `.github/workflows/benchmark.yml` runs the four benchmarks on API 35 (`reactivecircus/android-emulator-runner`, `google_apis`, x86_64, SwiftShader) on demand and every Monday, installs the app and resets its compilation before instrumenting it, pulls the