Fix recursive folder sync#16602
Conversation
- Make FileDataStorageManager.getFolderContent() public to allow MetadataWorker access - Add recursive parameter to BackgroundJobManager.downloadFolder() with default false - Add job dependency in BackgroundJobManagerImpl to ensure MetadataWorker runs before FolderDownloadWorker - Add recursive parameter to FileDownloadHelper.downloadFolder() - Pass recursive parameter in SynchronizeFolderOperation
- Add FORCE_REFRESH flag to bypass eTag caching and always fetch fresh content - Implement BFS-based recursive folder processing to fetch metadata for ALL nested subfolders - Before: Only processed one level of subfolders (e.g., Artists but not ABBA inside it) - After: Recursively processes all nested folders at every depth level - Also fetches files at each folder level to ensure FolderDownloadWorker has access to all files
- Add warning log when no files found for recursive download - Add detailed debug logging in getAllFilesRecursive() to track folder processing - Log content count at each folder level to help diagnose issues
- Add DOWNLOAD_FOLDER_RECURSIVE action in FileAction - Add action ID in ids.xml - Add UI strings for recursive download confirmation dialogs - Add showDownloadFolderRecursiveConfirmation() in FileActivity - Add downloadFolderRecursive() method in FileOperationsHelper - Add overloaded syncFile() method with recursive parameter
- FileDetailFragment: Add handler for action_sync_file_recursive to show recursive download dialog - OCFileListFragment: Add handler for recursive download action in file list - PreviewTextFileFragment: Add confirmation dialog for folder sync
|
Going to paste a message from the linked issue here:
|
| storageManager.getFolderContent(folder, false) | ||
| .filter { !it.isFolder && !it.isDown } | ||
| private fun getFiles(folder: OCFile, storageManager: FileDataStorageManager, recursive: Boolean): List<OCFile> { | ||
| if (recursive) { |
There was a problem hiding this comment.
Could you please use FiledatastorageManager.getAllFilesRecursivelyInsideFolder() together with the OCFile.isDown check instead of introducing a new function?
Using isDown should not negatively impact this process; in fact, it helps reduce unnecessary operations.
Also, let’s remove the comments where possible. Ideally, the code should be self-explanatory and clear enough that extensive comments aren’t needed.
| val files = getFiles(folder, storageManager, recursiveDownload) | ||
|
|
||
| // Add warning log when no files found for recursive download | ||
| if (files.isEmpty()) { |
There was a problem hiding this comment.
Why do we need warning log here? It is just extra info if file list is empty.
Please change with info log and remove the emoji along with comment since Log already prints.
| /** | ||
| * Recursively get all files in the folder and its subfolders | ||
| */ | ||
| private fun getAllFilesRecursive(folder: OCFile, storageManager: FileDataStorageManager): List<OCFile> { |
There was a problem hiding this comment.
Please remove this and use FiledatastorageManager.getAllFilesRecursivelyInsideFolder()
| } | ||
|
|
||
| override fun downloadFolder(folder: OCFile, accountName: String) { | ||
| override fun downloadFolder(folder: OCFile, accountName: String, recursive: Boolean) { |
There was a problem hiding this comment.
Thank you for the PR and your time.
I’d suggest keeping the scope of this PR strictly limited. The changes proposed outside of FolderDownloadWorker are quite significant and would require thorough testing.
For now, let’s focus only on FolderDownloadWorker with a minimal adjustment, for example, adding the new boolean parameter. From the UI side, we can introduce a separate action for this behavior, and have the worker that uses either the full nested structure or only the first-level depth based on that parameter.
Keeping the scope small will help us review more effectively and move forward with confidence. Once this is merged and validated, we can discuss and plan the next steps separately.
Thanks for understanding and we can test, review other findings in separate PR.
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
I’ll be closing this PR for now to keep things organized. Please feel free to reopen it once the changes are addressed. Thank you again for your contribution. |
Certainly not the place to inquire, but if these doesn't meet your standards, is there chance of further investigation of the issue that this attempted to fix? It's confusing how this is not the number one priority for nextcloud core contributors. It's a 10-year bug that renders the android app completely unusable for almost every possible use case. Android users largely don't use nextcloud, and this is the reason. I'd direct you here: |
4 participants
Summary
This PR fixes #269 , the inability to recursively sync folders (an issue that is 10 years old). Previously, when users tried to sync a folder with subfolders recursively, only the immediate children were synced. I spent three days of time I should have been focusing on my programming assignments on this, so please let me know if there is anything wrong with my code instead of just throwing it out. I am happy to collaborate, but I needed this feature so that I can properly sync and listen to my music.
Root Cause
The issue had three contributing factors:
Race Condition: MetadataWorker and FolderDownloadWorker were running without proper dependency, causing FolderDownloadWorker to query the database before metadata was populated.
eTag Caching: MetadataWorker was skipping folder content refresh when eTag hadn't changed, leaving the database empty.
Single-Level Processing: MetadataWorker only fetched one level of subfolders, not recursively traversing into nested folders like
Artists/ABBA, for example.Changes
🏁 Checklist