Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sync Worker #14166

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Add Sync Worker #14166

wants to merge 33 commits into from

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Dec 12, 2024

  • Tests written, or not not needed

Changes:

• Fixed the issue of scheduling too many FileDownloadWorker instances for folder sync.
• Resolved the issue of retrieving sub-folder items for the selected folder.
• Improved file download status observation, including states like syncing, downloading, and downloaded.
• Better constraints for worker
• Enhanced folder sync progress observation in notifications.
• Optimized performance using CoroutineWorker.

Demo

swd.mp4
big_file_test.mp4

Known Issues

• The current codebase does not efficiently handle the observation of the download states such as downloading, downloaded, cancelled. The swapDirectory() method must be called to display the green tick icon after each download, which is not an ideal approach. Additionally, invoking notifyItemChanged() does not impact the return value of the isDown() function, resulting in the downloaded file not being recognized as such. Thus, green tick cannot be set after file download completion.

A more robust solution could be considered; please feel free to suggest any viable alternatives.

@alperozturk96 alperozturk96 marked this pull request as draft December 12, 2024 10:16
@alperozturk96 alperozturk96 changed the title Feature/sync worker Feature - Sync Worker Dec 12, 2024
@alperozturk96 alperozturk96 force-pushed the feature/sync-worker branch 3 times, most recently from 57990c8 to 0c5db2c Compare December 19, 2024 08:40
@alperozturk96 alperozturk96 marked this pull request as ready for review December 19, 2024 11:14

private fun getFiles(folder: OCFile, storageManager: FileDataStorageManager): List<OCFile> =
storageManager.getAllFilesRecursivelyInsideFolder(folder)
.filter { !it.isFolder && !it.isDown }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same logic we have in the SynchronizeFolderOperation.prepareOpsFromLocalKnowledge()

@@ -500,7 +499,7 @@ private void startDirectDownloads() {
Log_OC.d(TAG, "Exception caught at startDirectDownloads" + e);
}
} else {
mFilesForDirectDownload.forEach(file -> fileDownloadHelper.downloadFile(user, file));
fileDownloadHelper.syncFolder(mLocalFolder);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since SyncWorker is responsible for preparing the files, there is no need to pass an array of files to the worker. Additionally, in scenarios where a user attempts to sync a large number of files (e.g., 1,000 files), the app could freeze or crash due to limitations associated with passing a large amount of data between classes via inputData.

Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Copy link

Codacy

Lint

TypemasterPR
Warnings5555
Errors33

SpotBugs

CategoryBaseNew
Bad practice6565
Correctness5757
Dodgy code294294
Experimental11
Internationalization77
Malicious code vulnerability11
Multithreaded correctness77
Performance5351
Security1818
Total503501

Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/14166.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant