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

Add ability to change [FileMode] of the download by introducing [Dio… #2281

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

shehabmohamed0
Copy link

@shehabmohamed0 shehabmohamed0 commented Aug 14, 2024

…FileMode]

New Pull Request Checklist

  • [Yes] I have read the Documentation
  • [Yes] I have searched for a similar pull request in the project and found none
  • [Yes] I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • [Yes] I have added the required tests to prove the fix/feature I'm adding
  • [Yes] I have updated the documentation (if necessary)
  • [Yes] I have run the tests without failures
  • [No] I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

Add the ability to change the [FileMode] of the download without altering the default value, which is [FileMode.write], to make resuming downloads easier to implement by introducing [DioFileMode] and mapping it to FileMode in [DioForNative]
without breaking web_adapter

@shehabmohamed0 shehabmohamed0 requested a review from a team as a code owner August 14, 2024 01:34
@ueman
Copy link
Contributor

ueman commented Aug 14, 2024

Can you please explain how this change enables resuming downloads?
Please also add tests for both cases, so it will not get broken in the future.

@shehabmohamed0
Copy link
Author

Can you please explain how this change enables resuming downloads? Please also add tests for both cases, so it will not get broken in the future.

When downloading a file a stream is opened and a Random Access File are opened in the client side to store bytes
the current implementation to open a Random Access File is [FileMode.write] this mode creates a new file and overwrite the existing one if it already exists

changing the file access mode to [FileMode.append] or [FileMode.writeOnlyAppend] with change the behavior to write to the end of the file

So if you had already started downloading a file and for some reason the download stopped let's say a network problem and you set [deleteOnError = false], the file had already downloaded 50% and it's not deleted it exists but is not complete

You check if the file exists and get it's size then you request to download the file again by requesting a partial content setting the range header to file size, that means skip downloading the 50% you already downloaded then you append the remaining 50% to the end of the file and here it's you resumed your download with minimum ease.

@shehabmohamed0
Copy link
Author

Can you please explain how this change enables resuming downloads? Please also add tests for both cases, so it will not get broken in the future.

Ok, I will add tests.

@ueman
Copy link
Contributor

ueman commented Aug 14, 2024

I wonder whether it makes sense to automatically add range header in the case of a resuming download, that way it would work out of the box, without the user needing to implement anything. The server needs to support them though. (This is not something which necessarily needs to be added to this PR though, I guess)

@shehabmohamed0
Copy link
Author

I wonder whether it makes sense to automatically add range header in the case of a resuming download, that way it would work out of the box, without the user needing to implement anything. The server needs to support them though. (This is not something which necessarily needs to be added to this PR though, I guess)

Yes, it's a good idea. I think it's rare to find a server that doesn't support range requests.
Should I change the implementation to just a boolean parameter in [Dio.download] called resume ?
If the server doesn't support range requests don't use it

@shehabmohamed0
Copy link
Author

I wonder whether it makes sense to automatically add range header in the case of a resuming download, that way it would work out of the box, without the user needing to implement anything. The server needs to support them though. (This is not something which necessarily needs to be added to this PR though, I guess)

I added test case for the file mode append, now can you review the code again

@shehabmohamed0 shehabmohamed0 requested a review from a team as a code owner October 15, 2024 01:25
/// - [DioFileMode.append]: Mode for opening a file for reading and writing
/// to the end of it. The file is created if it does not already exist.
/// {@endtemplate}
enum DioFileMode {
Copy link
Member

Choose a reason for hiding this comment

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

The name is a bit overwhelming. The type is only used for the download process.

Also the enhanced enum is not working on our minimum SDK support.

Copy link
Author

Choose a reason for hiding this comment

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

I will convert it to extension

Copy link
Author

Choose a reason for hiding this comment

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

any suggestion for naming

shehabmohamed0 and others added 4 commits October 21, 2024 22:29
change [DioFileMode] to [FileAccessMode]
change [Dio.download(fileMode:)] to  [dio.download(fileAccessMode:)]
@AlexV525
Copy link
Member

@ueman Could you follow above reviews and stamp this if applicable?

dio/lib/src/file_access_mode.dart Outdated Show resolved Hide resolved
dio/lib/src/file_access_mode.dart Outdated Show resolved Hide resolved
dio_test/lib/src/test/download_tests.dart Show resolved Hide resolved
shehabmohamed0 and others added 3 commits December 16, 2024 17:58
Co-authored-by: Alex Li <github@alexv525.com>
Signed-off-by: shehab mohame <shehabman@gmail.com>
@AlexV525
Copy link
Member

The CI is complaining about the v1 of dio_web_adapter, which should be updated after the PR has landed (I'll do that).

dio/lib/src/dio.dart Outdated Show resolved Hide resolved
dio_test/lib/src/test/download_tests.dart Show resolved Hide resolved
Co-authored-by: Alex Li <github@alexv525.com>
Signed-off-by: shehab mohame <shehabman@gmail.com>
Co-authored-by: Alex Li <github@alexv525.com>
Signed-off-by: shehab mohame <shehabman@gmail.com>
Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

RSLGTM. Also you need to update CHANGELOGs for the main package and the web adapter, just see how other PRs do this.

dio/lib/src/dio.dart Outdated Show resolved Hide resolved
dio/lib/src/dio_mixin.dart Outdated Show resolved Hide resolved
dio_test/lib/src/test/download_tests.dart Outdated Show resolved Hide resolved
Rename "mode" in `DioMixin.downloadUri` to "fileAccessMode"
Change var to int in "appeand bytes to previos download" test
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.

3 participants