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 8 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. |
|
ill test it no worries and thanks for the Work |
|
could u make sure the new files created use "openCloud" instead OwnCloud in copyright? |
|
Good catch @zerox80, thanks. Fixed in 77481e3: the nine new files that carry a license header now read On the casing, I went with The other two new files, |
zerox80
left a comment
zerox80
left a comment
There was a problem hiding this comment.
Follow-up static review of 77481e3 after the changes in 51bf12b. The original six findings were addressed; these are follow-on issues in the new implementation: two P1 and three P2. No tests were run, per the repository instruction.
| private fun listFolderContentFromServer(ocFolder: OCFile): List<OCFile> { | ||
| val folderId = ocFolder.id ?: throw IOException("Unknown folder ${ocFolder.remotePath}") | ||
|
|
||
| val refreshResult = synchronizeFolderUseCase( |
There was a problem hiding this comment.
[P1] Do not use a destructive cache refresh for export discovery
Both this call and refreshFolder() above execute SynchronizeFolderUseCase with REFRESH_FOLDER. That reaches OCFileRepository.refreshFolder(), whose reconciliation unconditionally removes every local row and local file that is absent from the PROPFIND response - even when the local copy has unsynced changes or etagInConflict is set. A user choosing "Save to device" after another client deleted the server entry can therefore lose the only local copy before it is exported. Please add a read-only Depth:1 listing use case for export traversal and return those server children without reconciling Room/local storage. If the existing refresh has to stay, dirty/conflicted local descendants need to be snapshotted or excluded from deletion before this call.
| return ContentToExport(path = currentPath, fileToDiscard = null) | ||
| } | ||
|
|
||
| val temporalFolderPath = FileStorageUtils.getTemporalPath(account.name, ocFile.spaceId) |
There was a problem hiding this comment.
[P1] Give each export worker its own download staging directory
DownloadRemoteFileOperation derives its file as localFolderPath + remotePath. Since each export job has its own unique work name and jobs intentionally run in parallel, two exports of the same remote file - or an export and DownloadFileWorker - write the same temporary file. One worker can truncate, rename, or delete it while the other is still writing/copying, yielding a failed or truncated export. Isolate the staging root by worker id (and clean that root after the job); the minimal path isolation is:
| val temporalFolderPath = FileStorageUtils.getTemporalPath(account.name, ocFile.spaceId) | |
| val temporalFolderPath = File( | |
| FileStorageUtils.getTemporalPath(account.name, ocFile.spaceId), | |
| "export-$id", | |
| ).absolutePath |
| override fun onSaveInstanceState(outState: Bundle) { | ||
| super.onSaveInstanceState(outState) | ||
| // The SAF folder picker is another app, this fragment may be recreated while it is shown. | ||
| outState.putLongArray(KEY_PENDING_EXPORT_FILE_IDS, pendingExportFileIds.toLongArray()) |
There was a problem hiding this comment.
[P2] Keep the unbounded selection out of Fragment saved state
Select all has no item limit, and this LongArray becomes part of the Activity saved-state transaction when the external SAF picker backgrounds the app. A sufficiently large selection can therefore throw TransactionTooLargeException instead of surviving recreation. Persist a pending export record before launching the picker and put only its generated id in this Bundle; after the picker returns, attach the tree URI, enqueue the work, and delete or consume the pending record.
| context = appContext, | ||
| contentTitle = appContext.getString(titleRes), | ||
| notificationChannelId = DOWNLOAD_NOTIFICATION_CHANNEL_ID, | ||
| notificationId = DOWNLOAD_NOTIFICATION_ID_DEFAULT, |
There was a problem hiding this comment.
[P2] Allocate notification ids per export job
All running exports use foreground id 124 at line 618, and every result here uses download id 123. Concurrent exports overwrite or cancel each other's progress notifications, while export results overwrite download results. Derive two stable ids from this worker/job - one for foreground progress and a different one for the terminal result. As a minimal result-side fix (paired with id.hashCode().and(Int.MAX_VALUE).coerceAtLeast(1) at line 618):
| notificationId = DOWNLOAD_NOTIFICATION_ID_DEFAULT, | |
| notificationId = ((id.hashCode() xor 0x40000000) and Int.MAX_VALUE).coerceAtLeast(1), |
| import androidx.sqlite.db.SupportSQLiteDatabase | ||
| import eu.opencloud.android.data.ProviderMeta.ProviderTableMeta | ||
|
|
||
| val MIGRATION_49_50 = object : Migration(49, 50) { |
There was a problem hiding this comment.
[P2] Commit schema 50 and migration coverage
The database version is now 50 with exportSchema = true, but the committed Room schema directory still ends at 49.json, and there is no MigrationToDB50Test. That leaves the new migration without the canonical target schema used by MigrationTestHelper and by future migrations. Please generate and commit opencloudData/schemas/eu.opencloud.android.data.OpencloudDatabase/50.json and add a 49-to-50 migration test that verifies both migration validation and preservation of existing rows. I know this was already called out in the PR conversation; keeping it inline so it cannot be missed before merge.
|
To avoid duplicate work: the remaining review fixes are already implemented in junkerderprovinz/android#1. It is based on the current head of this PR and targets |
fix: make save-to-device exports safe
|
Merged, thanks for going the extra mile on this. I went through the diff before merging: schema 50 matches MIGRATION_49_50 exactly, the per-worker temp download root is torn down in the finally block, the notification ids are derived from the work id, and the |
fix: address save-to-device review findings
|
@guruz looks fine to me, what do u think? |
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.