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

fix: fixes for downloading #23

Merged
merged 9 commits into from
Jul 19, 2024
Merged

Conversation

rnr
Copy link
Collaborator

@rnr rnr commented Jul 17, 2024

This PR fixes next issues:
openedx#485
openedx#458

To fix the remaining tmp files, DownloadRequest.Destination was added with the setting .removePreviousFile.
The way we cancel downloads at the chapter level has also been changed: since we have no individual control at this level, we must cancel and remove all files in the chapter to prevent a situation where we cancel a download but the files remain.

@rnr rnr requested review from forgotvas and saeedbashir July 17, 2024 12:44
@rnr rnr changed the title Fixes for downloading fix: fixes for downloading Jul 17, 2024
@saeedbashir
Copy link

I'm not sure whether this is related to this, but downloaded videos are not deleting.
https://github.com/user-attachments/assets/23205715-2c4c-4952-b8f0-9a265153e2a4

@rnr
Copy link
Collaborator Author

rnr commented Jul 17, 2024

I'm not sure whether this is related to this, but downloaded videos are not deleting. https://github.com/user-attachments/assets/23205715-2c4c-4952-b8f0-9a265153e2a4

@saeedbashir Do you see it in this branch only? Could you please provide some details to reproduce this. Thank you

@forgotvas forgotvas changed the base branch from 2U/develop to develop July 17, 2024 20:23
@forgotvas forgotvas changed the base branch from develop to 2U/develop July 17, 2024 20:23
@saeedbashir
Copy link

@saeedbashir Do you see it in this branch only? Could you please provide some details to reproduce this. Thank you

I was playing around with the downloads and it happened. Now, I'm not able to replicate that issue but I am facing another issue, the downloads continue even after canceling the download.

Screen.Recording.2024-07-18.at.8.17.16.AM.mov

@@ -268,7 +268,8 @@ public class DownloadManager: DownloadManagerProtocol {
public func deleteFile(blocks: [CourseBlock]) async {
for block in blocks {
do {
if let fileURL = fileUrl(for: block.id) {
if let fileURL = fileUrl(for: block.id),

Choose a reason for hiding this comment

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

Getting Trailing Whitespace Violation: warning here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@rnr
Copy link
Collaborator Author

rnr commented Jul 18, 2024

@saeedbashir Do you see it in this branch only? Could you please provide some details to reproduce this. Thank you

I was playing around with the downloads and it happened. Now, I'm not able to replicate that issue but I am facing another issue, the downloads continue even after canceling the download.

Screen.Recording.2024-07-18.at.8.17.16.AM.mov

@saeedbashir Yes we are facing this bug too - it's not just cancelling issue it's how downloads start. @forgotvas is looking on this. We will create ticket/issue and perhaps separate branch/PR will be better for this. Thank you

@saeedbashir
Copy link

@saeedbashir Do you see it in this branch only? Could you please provide some details to reproduce this. Thank you

I was playing around with the downloads and it happened. Now, I'm not able to replicate that issue but I am facing another issue, the downloads continue even after canceling the download.
Screen.Recording.2024-07-18.at.8.17.16.AM.mov

@saeedbashir Yes we are facing this bug too - it's not just cancelling issue it's how downloads start. @forgotvas is looking on this. We will create ticket/issue and perhaps separate branch/PR will be better for this. Thank you

If that is the case, we can merge this PR. Please create a issue for this so it wont lost and we have to fix it before MVP.

Copy link

@saeedbashir saeedbashir left a comment

Choose a reason for hiding this comment

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

Approving this as per the discussion 👍

@rnr
Copy link
Collaborator Author

rnr commented Jul 19, 2024

Sergey made these tickets:
openedx#508
openedx#506
merging this

@rnr rnr merged commit 5e8e116 into 2U/develop Jul 19, 2024
3 checks passed
rnr added a commit that referenced this pull request Oct 1, 2024
rnr added a commit that referenced this pull request Oct 8, 2024
* Merge pull request #23 from edx/small-fix-for-downloading-cancelling

fix: fixes for downloading

* chore: fix for Xcode 16 and after merge

* Merge pull request #24 from edx/2U/fix/download-states

fix: [iOS] On Course "Home" tab the row height

* fix: after merge, deleted IAP part

fix: [iOS] On Course "Home" tab the row height

* Merge pull request #25 from edx/2U/feat/primary-horizontal

feat: Landscape mode Improvement

* fix: removed IAP part

* chore: remove snack bar error for course dates info API on course home (#27)

* Merge pull request #28 from shafqat-muneer/Shafqat/LEARNER-10020-ErrorHandling

feat: Course Level Error Handling for Empty States

* chore: remove IAP part after merging

---------

Co-authored-by: Anton Yarmolenko <[email protected]>
Co-authored-by: Saeed Bashir <[email protected]>
@rnr rnr deleted the small-fix-for-downloading-cancelling branch December 17, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] Changing the number of files available for download
3 participants