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.

Sign up for GitHub

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

Jump to bottom

Use Work Manager For File Download #12308

Merged
merged 189 commits into from
Jan 12, 2024
Merged

Use Work Manager For File Download #12308

merged 189 commits into from
Jan 12, 2024

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Dec 20, 2023 •
edited
Loading

  • 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>"

JonasMayerDev
JonasMayerDev requested changes Dec 27, 2023
View reviewed changes
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

app/src/main/java/com/nextcloud/client/notifications/download/DownloadNotificationManager.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nextcloud/client/notifications/download/DownloadNotificationManager.kt Outdated Show resolved Hide resolved
@alperozturk96
Copy link
Collaborator Author

@JonasMayerDev Fix Sync

down_i.mp4

JonasMayerDev
JonasMayerDev requested changes Dec 28, 2023
View reviewed changes
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...

app/src/main/java/com/nextcloud/client/jobs/BackgroundJobManagerImpl.kt Outdated Show resolved Hide resolved
app/src/main/java/com/owncloud/android/files/FileMenuFilter.java Outdated Show resolved Hide resolved
app/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java Outdated Show resolved Hide resolved
app/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java Outdated Show resolved Hide resolved
app/src/main/java/com/owncloud/android/ui/adapter/OCFileListDelegate.kt Outdated Show resolved Hide resolved
@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

JonasMayerDev
JonasMayerDev requested changes Dec 28, 2023
View reviewed changes
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
@alperozturk96
No need for clear for each worker
d19b1d3 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Cancel action remove added
3e4cccf 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Cancel All notification when sync canncelled
b4e5416 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix cancel feature for individual file download inside folder
51bb589 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Use unique tag for all download type to fix sync problems
5ea707b 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix cancel for unique tag
1689aee 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Rename worker observer func
8ff2914 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Prevent unnecessary file download
f9d3305 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix cancel
7a0c591 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix cancel notification
5aa41ce 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix Cancel Notification Appearance and Progress Bar Visibility
17cdbc2 
Signed-off-by: alperozturk <[email protected]>
@JonasMayerDev @alperozturk96
Improve Notifications
85b68c2 
Signed-off-by: Jonas Mayer <[email protected]>
@alperozturk96
Add long running task support
7970ba7 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Improve notifications for different custom Android OS
687c557 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Rebase master
98dd0c7 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Dont show success notifications
c4ba0e0 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Cancel fix
15dedf8 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Prepare all files to cancel
eb0735e 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix recursive worker start for folder
3b2824e 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix cancel and isDownloading
2a0b754 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix kotlin spotless check
6fed74a 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix isDownloading for nested folders
ed24172 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Fix cancel in same root
3ba4721 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96
Better notifications
95e9be6 
Signed-off-by: alperozturk <[email protected]>
@alperozturk96 alperozturk96 force-pushed the refactor/use-work-manager-file-download branch from edab3e3 to 95e9be6 Compare January 12, 2024 07:51
@alperozturk96
Remove unused parameter
909c730 
Signed-off-by: alperozturk <[email protected]>
@github-actions GitHub Actions
Copy link

Codacy

Lint

TypemasterPR
Warnings6969
Errors33

SpotBugs

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

@github-actions GitHub Actions
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
Reviewers

@JonasMayerDev JonasMayerDev JonasMayerDev approved these changes

@ZetaTom ZetaTom Awaiting requested review from ZetaTom

@tobiasKaminsky tobiasKaminsky Awaiting requested review from tobiasKaminsky

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alperozturk96 @JonasMayerDev