feat(files): save selected files and folders to a device folder - #206
feat(files): save selected files and folders to a device folder#206junkerderprovinz wants to merge 2 commits into
Conversation
Adds a "Save to device" file action (single and multi-select) that lets the user pick a destination folder through the Storage Access Framework (ACTION_OPEN_DOCUMENT_TREE) and exports the selected files and folders there. Folders are recreated recursively and files that are not available locally are downloaded first, then copied into the target tree. Addresses opencloud-eu#180.
|
@zerox80 ^ @junkerderprovinz Thanks... How does this relate to #202 ? |
|
@guruz good question, they're not overlapping, they're inverse directions on the same Android API. #202 makes OpenCloud the SAF provider: DocumentsStorageProvider answers ACTION_OPEN_DOCUMENT_TREE so other apps can pick a folder that lives inside OpenCloud. This PR (#206) makes OpenCloud the SAF client: it issues ACTION_OPEN_DOCUMENT_TREE itself to let you pick a folder on the device and export selected files/folders out to it. No file overlap either, #202 touches DocumentsStorageProvider.kt, RootCursor.kt and the advanced-settings XML, this PR adds FileMenuOption.EXPORT, ExportFilesToDeviceUseCase and ExportFilesToDeviceWorker. Separately, the Woodpecker pipeline (ci/woodpecker/pr/integration-test) has been sitting on "pending approval" since it was opened, it's a draft specifically so CI can validate the build before marking it ready. Could you approve that run when you get a chance? |
zerox80
left a comment
zerox80
left a comment
There was a problem hiding this comment.
Static review of de5aadb90314, with the OpenCloud backend inspected read-only for WebDAV traversal and ETag semantics. I found four P1 and two P2 issues; details and compact suggestions are inline. Per project instruction, no tests were run.
| val localPath = ensureLocalCopy(ocFile) | ||
| val mimeType = ocFile.mimeType.ifBlank { MIME_OCTET_STREAM } | ||
| // Overwrite a previous export with the same name instead of creating a "(1)" duplicate. | ||
| parent.findFile(ocFile.fileName)?.delete() |
There was a problem hiding this comment.
[P1] Do not delete a same-named destination before replacement is safe
findFile() can return a directory as well as a file. If the chosen target already contains a directory named like the source file (for example, a folder README and a remote file README), this deletes the whole target tree. Even for an existing file, a later createFile, openOutputStream, or copy failure leaves the previous copy gone. Please reject directory collisions and replace regular files only after a complete staged write. As a minimal non-destructive fallback while choosing the overwrite policy:
| parent.findFile(ocFile.fileName)?.delete() | |
| val existingTarget = parent.findFile(ocFile.fileName) | |
| if (existingTarget != null) { | |
| throw IOException("Refusing to overwrite existing target ${ocFile.fileName}") | |
| } |
| failedCount++ | ||
| return | ||
| } | ||
| val children = ocFile.id?.let { |
There was a problem hiding this comment.
[P1] Enumerate the remote subtree instead of treating Room as authoritative
GetFolderContentUseCase ultimately calls only localFileDataSource.getFolderContent(). A selected folder that has never been opened (or whose descendants changed since the last refresh) therefore produces an empty/stale child list; getDataOrNull().orEmpty() also turns lookup errors into a valid empty folder. The worker then creates the destination directory and reports success while silently omitting files. OpenCloud disables Depth: infinity PROPFIND by default, so please enumerate the authoritative subtree folder-by-folder with supported Depth: 1 requests and propagate lookup failures instead of converting them to emptyList().
| val currentPath = ocFile.storagePath | ||
| if (ocFile.isAvailableLocally && !currentPath.isNullOrBlank() && File(currentPath).exists()) { | ||
| return currentPath | ||
| } |
There was a problem hiding this comment.
[P1] Do not export a stale local version as the current server file
The model intentionally keeps the ETag of the locally synchronized content in etag and the current server version in remoteEtag. This shortcut ignores that distinction, so after the file changes on another client an existing local copy can be exported silently as if it were current. Reuse the local bytes only when the version validators match; otherwise perform a full download and persist the new ETag using the same metadata path as DownloadFileWorker.
| val currentPath = ocFile.storagePath | |
| if (ocFile.isAvailableLocally && !currentPath.isNullOrBlank() && File(currentPath).exists()) { | |
| return currentPath | |
| } | |
| val currentPath = ocFile.storagePath | |
| if ( | |
| ocFile.isAvailableLocally && | |
| !currentPath.isNullOrBlank() && | |
| File(currentPath).exists() && | |
| !ocFile.etag.isNullOrBlank() && | |
| ocFile.etag == ocFile.remoteEtag | |
| ) { | |
| return currentPath | |
| } |
|
|
||
| val inputData = workDataOf( | ||
| ExportFilesToDeviceWorker.KEY_PARAM_ACCOUNT to params.accountName, | ||
| ExportFilesToDeviceWorker.KEY_PARAM_FILE_IDS to params.fileIds.toLongArray(), |
There was a problem hiding this comment.
[P1] Keep the unbounded selection out of WorkManager Data
WorkManager enforces a 10 KiB maximum for serialized Data and throws IllegalStateException synchronously when it is exceeded. Select all has no item limit, so the LongArray alone reaches the limit before roughly 1,280 IDs (earlier once account/URI and serialization overhead are included). Because workDataOf() runs in the SAF result callback, choosing the destination then crashes the app. Please persist the selection behind an export-job ID and pass only that ID to the worker, or split it into explicitly bounded requests. See https://developer.android.com/reference/androidx/work/Data.html.
| } | ||
| // Export / save to a device folder (files and folders, downloaded if needed) | ||
| if (!isAnyFileSynchronizing && !onlyAvailableOfflineFiles && !onlySharedByLinkFiles) { | ||
| optionsToShow.add(FileMenuOption.EXPORT) |
There was a problem hiding this comment.
[P2] Do not expose EXPORT in preview menus without an action handler
This central filter is also used by the audio, text, image, and video preview view models. Those screens inflate file_actions_menu, so this makes "Save to device" visible there, but none of their option handlers handles action_export_file; tapping it is a no-op. FileDetailsViewModel already removes EXPORT explicitly. Please make export availability a caller/context parameter restricted to the file list, or remove it in every preview until those screens implement the flow.
| private var checkedFiles: List<OCFile> = emptyList() | ||
|
|
||
| // Files/folders the user chose to export; consumed once the SAF folder picker returns. | ||
| private var pendingExportFiles: List<OCFile> = emptyList() |
There was a problem hiding this comment.
[P2] Persist the pending selection across the external picker lifecycle
This plain Fragment field is lost if the process or Fragment is recreated while the SAF picker is open. The Activity Result registry can still deliver the returned URI to the new instance, but pendingExportFiles is then empty and the callback silently does nothing. Store the selected IDs/account in SavedStateHandle or saved instance state and consume/clear that persisted state only after handling the result.
Six real findings, fixed through two rounds of adversarial review (each round re-read the actual code against the previous round's specific claims, not the implementer's own summary): 1. exportSingleFile() no longer deletes an existing target before the replacement is safe. Directory collisions are rejected up front. First-time exports write directly under the final name. Replacing an existing export stages the new content, renames the previous copy aside, swaps in the new one, and only then removes the backup -- restored unconditionally (try/finally) if any step fails. 2. Folder export now enumerates the live server (Depth:1 PROPFIND via SynchronizeFolderUseCase) instead of trusting Room, and a lookup failure fails that folder instead of exporting it as empty. The destination directory is only created after the listing succeeds. 3. A local copy is only reused when its etag matches the server's (remoteEtag); otherwise it's re-downloaded and the full DownloadFileWorker metadata set is persisted. A local copy with unsynced edits or an open conflict is never overwritten by the export. 4. The unbounded file-id selection no longer goes through WorkManager's 10 KiB Data limit. It's persisted as an export job and only the job id is passed to the worker. 5. "Save to device" no longer appears in the audio/text/image/video preview menus, which have no handler for it. 6. The pending export selection survives Activity/process recreation while the SAF folder picker is open (saved instance state, ids only). The first fix pass introduced its own regressions, caught by re-reading the code against the original findings a second time: - Chaining exports of one account (enqueueUniqueWork + APPEND_OR_REPLACE) meant one failed item in export A silently cancelled export B. Fixed: each export is its own unique work (KEEP, keyed by job id), with a synchronized prune of jobs whose work no longer exists. - The stop-safe retry (for finding 4) combined with the 10-minute JobScheduler window could retry forever with no user feedback. Fixed: foreground service notification, a persisted attempt cap, and per-item progress in the job row so a retry resumes instead of restarting. - Requiring a live folder refresh for every selected file (finding 3) broke offline export of already-downloaded files. Fixed: the parent refresh is best-effort; the etag guard still decides. Honestly still open: the Room schema JSON for migration 50 isn't committed (generated by the first real build, which this environment can't run) and nothing here was compiled -- no Android SDK available, verification was by reading the actual code, not by building it.
|
Thanks for the thorough review, @zerox80 — all six were real, and I went through them properly rather than patching around the symptoms. Pushed a fix that I put through two independent rounds of re-reading the actual code against your specific findings (not just trusting my own summary), because the first pass looked complete and wasn't. 1 — the destructive delete. Directory collisions are now rejected up front. A first-time export writes straight under the final name (nothing to lose). Replacing an existing export stages the new content, renames the previous copy aside to a backup name, swaps the staged copy in, and only then removes the backup — restored unconditionally if any step fails, including an exception (not just a 2 — Room as authoritative. Folder export now does a real Depth:1 PROPFIND per folder (via the existing 3 — stale local copy. Reuse is now gated on 4 — WorkManager Data limit. The selection is persisted as an export job; only the job id crosses into 5 — EXPORT in preview menus. Scoped to the file list only now, same mechanism 6 — selection lost on recreation. Survives via saved instance state (ids + account, not the Where it got interesting: my first fix pass for 1–4 introduced three of its own bugs, all caught by the second read-through rather than shipped:
Two things I want to be upfront about rather than let you find:
Happy to adjust anything above once real compilation/tests actually run against it. |
3 participants
Closes #180
What
Adds a Save to device file action (single file and multi-select) that lets the user pick a destination folder through the Storage Access Framework (
ACTION_OPEN_DOCUMENT_TREE) and exports the selected files and folders there. Folders are recreated recursively, and files that are not already available locally are downloaded first and then copied into the chosen tree.Today the app can only make files available offline (kept in the app's private storage); there is no way to save a file, or a whole folder, into a user chosen device folder such as Downloads. This addresses that (related: #69).
How
FileMenuOption.EXPORT, shown in the file list bottom sheet and the multi select action mode (removed from the details screen for now).ExportFilesToDeviceUseCaseenqueues anExportFilesToDeviceWorker(WorkManager) with the selected file ids and the picked tree URI (complex objects cannot be passed to a worker).GetFolderContentUseCase, recreating directories withDocumentFile; files copied into the tree. A file that is not local is downloaded into the app storage first (the same pathDownloadFileWorkeruses), then copied. It reports the result with a notification.Notes
Opening as a draft so CI can validate the build, and to gather feedback before polishing. Happy to adjust the UX (icon, label, where the action appears, the icon reuse, single vs. recursive behaviour) to your preference.