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

Use Work Manager For File Download #12308

Merged
merged 189 commits into from
Jan 12, 2024

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Dec 20, 2023

  • Tests written, or not not needed

@alperozturk96 alperozturk96 changed the title Use Work Manager File Download Use Work Manager For File Download Dec 20, 2023
@alperozturk96 alperozturk96 force-pushed the refactor/use-work-manager-file-download branch 2 times, most recently from d984c32 to df0be0a Compare December 21, 2023 12:36
@alperozturk96
Copy link
Collaborator Author

/rebase

2 similar comments
@alperozturk96
Copy link
Collaborator Author

/rebase

@alperozturk96
Copy link
Collaborator Author

/rebase

@alperozturk96 alperozturk96 force-pushed the refactor/use-work-manager-file-download branch from 584ed8b to 28cffe1 Compare December 22, 2023 10:11
@alperozturk96 alperozturk96 force-pushed the refactor/use-work-manager-file-download branch 3 times, most recently from a450370 to 041e836 Compare December 26, 2023 12:29
@alperozturk96
Copy link
Collaborator Author

Only one test is failing - "download_progress_is_updated[android(AVD) - 8.1.0] FAILED - java.lang.AssertionError: expected:<6> but was:<2>"

Copy link
Collaborator

@JonasMayerDev JonasMayerDev left a comment

Choose a reason for hiding this comment

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

Synchronizing Folders only downloads one file of the folder not all as expected

@alperozturk96
Copy link
Collaborator Author

@JonasMayerDev Fix Sync

down_i.mp4

Copy link
Collaborator

@JonasMayerDev JonasMayerDev left a comment

Choose a reason for hiding this comment

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

New implemented function "isDownloading" does not the same thing as old one. Maybe reimplement it and make sure it does the same thing. Be careful when changing the functionality of a function to make sure everywhere it is used it is still fitting...

@JonasMayerDev
Copy link
Collaborator

Maybe it is a better idea to not change the download functionality and switch to chained worker in another pull request to make sure we don't introduce issues with new untested logic...? @alperozturk96

@alperozturk96
Copy link
Collaborator Author

Maybe it is a better idea to not change the download functionality and switch to chained worker in another pull request to make sure we don't introduce issues with new untested logic...? @alperozturk96

getRequestDownloads() was causing the problem. I fixed it but for future implementation chained worker can be used as well.

@alperozturk96
Copy link
Collaborator Author

New implemented function "isDownloading" does not the same thing as old one. Maybe reimplement it and make sure it does the same thing. Be careful when changing the functionality of a function to make sure everywhere it is used it is still fitting...

New function added into BackgroundJobManagerImpl that checks status of the current file. You can test it.

@alperozturk96
Copy link
Collaborator Author

@JonasMayerDev Sync icon same as master, fyi.

Master

master_c.mp4

PR

pr_c.mp4

Copy link
Collaborator

@JonasMayerDev JonasMayerDev left a comment

Choose a reason for hiding this comment

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

Screen_recording_20231228_170425.webm
  • Download of folder happens in parallel, which messes with the progress notification.
  • Blue sync icon for a folder is only visible for first download
  • Sync of a folder can not be stopped

@alperozturk96 alperozturk96 force-pushed the refactor/use-work-manager-file-download branch from dcba572 to 99fe9f3 Compare December 29, 2023 08:06
alperozturk96 and others added 24 commits January 12, 2024 08:51
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: Jonas Mayer <jonas.a.mayer@gmx.net>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96 alperozturk96 force-pushed the refactor/use-work-manager-file-download branch from edab3e3 to 95e9be6 Compare January 12, 2024 07:51
Signed-off-by: alperozturk <alper_ozturk@proton.me>
Copy link

Codacy

Lint

TypemasterPR
Warnings6969
Errors33

SpotBugs

CategoryBaseNew
Bad practice2626
Correctness6970
Dodgy code353351
Experimental22
Internationalization99
Malicious code vulnerability22
Multithreaded correctness96
Performance5454
Security1818
Total542538

Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12308.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.

@alperozturk96 alperozturk96 merged commit ed88a47 into master Jan 12, 2024
20 checks passed
@delete-merged-branch delete-merged-branch bot deleted the refactor/use-work-manager-file-download branch January 12, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants