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

[Android] Handles file creation failures when attempting to retry failed downloads #771

Closed
wants to merge 1 commit into from
Closed

[Android] Handles file creation failures when attempting to retry failed downloads #771

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions android/src/main/kotlin/vn/hunghd/flutterdownloader/DownloadWorker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,17 +497,33 @@ class DownloadWorker(context: Context, params: WorkerParameters) :
private fun createFileInAppSpecificDir(filename: String, savedDir: String): File? {
val newFile = File(savedDir, filename)
try {
val rs: Boolean = newFile.createNewFile()
if (rs) {
return newFile
} else {
logError("It looks like you are trying to save file in public storage but not setting 'saveInPublicStorage' to 'true'")
if (newFile.exists()) {
val deleted = newFile.delete()
if (!deleted) {
logError("Unable to delete existing file: ${newFile.absolutePath}")
return null
}
}
Comment on lines +500 to +506
Copy link
Collaborator

@bartekpacia bartekpacia Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's expected to delete the old file to make place for the the newly downloaded file.

If new file creation fails, we wrongly assume that the user is creating a file in public storage but not setting saveInPublicStorage to true.

Can we modify the error message to state not setting saveInPublicStorage is one of the possible causes, not the cause?

Copy link
Contributor Author

@rupinderjeet rupinderjeet Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's expected to delete the old file to make place for the the newly downloaded file.

What should be done in this case? fileName passed in createFileInAppSpecificDir(String, String) already exists. Can we create a file with different (randomised index) name and return it? I think it will cause issues upstream from this function.

Can we modify the error message to state not setting saveInPublicStorage is one of the possible causes, not the cause?

Yes, I wrote it as a question to the user (in the commit):

"Unable to create new file: ${newFile.absolutePath}.
Are are trying to save file in public storage but not setting 'saveInPublicStorage' to 'true'?"

This can definitely be updated to state it differently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I wrote it as a question to the user (in the commit):

Yes, but the developer sees this log after the original file is already deleted.

Please also see this issue, I remember some agreement was reached in that issue what should we do in that case. But it's kinda abandoned, feel free to continue their work if you have time and will :)


val created: Boolean = newFile.createNewFile()
if (!created) {
logError(
"""
Unable to create new file: ${newFile.absolutePath}.
Are are trying to save file in public storage
but not setting 'saveInPublicStorage' to 'true'?
""".trimIndent()
)
return null
}

return newFile

} catch (e: IOException) {
e.printStackTrace()
logError("Create a file using java.io API failed ")
return null
}
return null
}

/**
Expand Down