-
Notifications
You must be signed in to change notification settings - Fork 72
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
[WIP] Don't save incomplete downloaded files #258
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For coding discussions, if it's directly related to this PR, you can write it in comments to this PR, else you can also open an issue, link it if necessary.
I don't know exactly why the CheckDirectoryForFiles option has been removed originally. But in the meantime we added the filename template functionality which conflicts with it somewhat. As of now I see the CheckDirectoryForFiles more as a disaster recovery possibility, in case the tumblr index files have been lost and shall be rebuilt, under the precondition that default filenames have been used.
But only that it's not visible to the normal user doesn't mean it cannot be used any more or hasn't a meaning any longer.
@@ -289,7 +289,7 @@ protected virtual async Task<bool> DownloadBinaryPostAsync(TumblrPost downloadIt | |||
string fileName = FileName(downloadItem); | |||
UpdateProgressQueueInformation(Resources.ProgressSkipFile, fileName); | |||
} | |||
else if (!shellService.Settings.LoadAllDatabases && blog.CheckDirectoryForFiles && (blog.CheckIfBlogShouldCheckDirectory(FileNameUrl(downloadItem), FileNameNew(downloadItem)) | |||
/*else if (!shellService.Settings.LoadAllDatabases && blog.CheckDirectoryForFiles && (blog.CheckIfBlogShouldCheckDirectory(FileNameUrl(downloadItem), FileNameNew(downloadItem)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing functionality here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invisible options are a hidden danger. I removed it to reduce testing noise since it AddFileToDb
, this should probably be reverted but needs to be retested/rewritten.
@@ -310,6 +310,7 @@ protected virtual async Task<bool> DownloadBinaryPostAsync(TumblrPost downloadIt | |||
UpdateProgressQueueInformation(Resources.ProgressDownloadImage, fileName); | |||
if (!await DownloadBinaryFileAsync(fileLocation, fileLocationUrlList, Url(downloadItem))) | |||
{ | |||
RemoveFileFromDb(FileNameUrl(downloadItem)); // TODO: Temp plan, the addition should happen only when the download is successful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be a way to do it. But keep in mind that AddFileToDb is called early, because this url shall not be downloaded from another thread while this one is still working on it, and it also determines the final filename in case of using filename templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, I have no better way to improve/staticize the AddFileToDb
function. I'm not sure what you mean, but this PR is a proposal and not a final solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood that it's a draft and work in progress (WIP). Everything is ok, I only wanted to give more information as input.
@@ -55,7 +55,7 @@ public async Task<bool> DownloadFileWithResumeAsync(string url, string destinati | |||
|
|||
if (ct.IsCancellationRequested) return false; | |||
|
|||
var fileMode = totalBytesReceived > 0 ? FileMode.Append : FileMode.Create; | |||
var fileMode = File.Exists(destinationPath) ? FileMode.Append : FileMode.Create; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's another (unnecessary) filesystem access. But besides that I just see an error and an optimization possibility in the previous block (old code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the previous code is working fine for a created file of 0 bytes. Testcase may be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was unclear, I meant the lines 41-54 of the old code (not your changes).
I would try to avoid another filesystem access (File.Exists
). But as said WIP...
@@ -131,6 +131,8 @@ public async Task<bool> DownloadFileWithResumeAsync(string url, string destinati | |||
} | |||
else | |||
{ | |||
fileStream?.Dispose(); | |||
File.Delete(destinationPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not delete the file in all cases. What about a video download that timeouts short before the end? It prevents resume functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strangely enough, as of now, I don't see resume to be working, probably due to skipping any existing files in filesystem or DB (index).
Sometimes they are questions or proposals for code changes, refactoring or function change, and I don't really want to pollute real bug reports/feature requests, and it's intrusive for ordinary issues watcher. However, PRs are indeed a good place for a line-by-line review of code. |
Maybe we should set up the Discussions thing (to give the users a new forum). As usual, at the end it's not a problem of setting it up, rather the amount of people who will use it... |
Description
This seems to ensure that no 0-byte files are present. However, clicking stop button still cause some JPEG files to be incomplete while downloading.
CheckDirectoryForFiles option is not visible since 2017, I don't know why.
For coding discussions, any suitable place? Issues, GitHub discussion (not opened, I suggest turning it on and provide some topic), or something else?
Issue Resolution
#257.
Proposed Changes
New or Changed Features
Does this PR provide new or changed features or enhancements? If so, what is included in this PR?