-
Notifications
You must be signed in to change notification settings - Fork 523
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
Draft: Downloads in dart with simpler interface without Databases #793
Conversation
Apply suggestions from code review Co-authored-by: Bartek Pacia <[email protected]>
FYI: I had the last days not a lot of time, but I will continue |
awesome :D |
I have a question about how the API/usage of the plugin will change with this PR. In the 1.0 version, I would schedule my download and let the platform handle everything. I would get a taskId when scheduling the download so I can keep track of the downloads on my side of things. This seems to be gone now, is that correct? I get a Also, the listener I can attach does not actually inform me about individual downloads, does it? I can only ask about all downloads and match them myself. The |
@lyio, you point out something interesting which I had not yet in mind. With the code structure I have in mind you can query all downloads, then you need to filter out the download you are interested in. Each download has as you correctly found out a progress (in permille) and a status. Changes of those value will be notified (e.g. for UI). I had in mind that you can observe in that way a single As said you would need to find the download you are interested in yourself. I will think about a simpler way to give you the progress of a single download in a simpler way. I had already in mind to provide the progress of a group of downloads. But I want to have the basics done first. By the way I'm currently working on the migration to the JSON structure, which takes (as expected) relativ high amount of time since I need to handle it differently on multiple platforms. When this works as expected I will push an update. |
@rekire in v1 the plugin returned a |
I agree that each task having a unique ID of some sorts is a nice feature. |
To be honest I see nothing useful in a random identifier... What about the hash of the url? I'm using it as id internal |
@rekire I've made quite a bit of progress on the background_downloader package that we have discussed before, and in issue #823 I am suggesting we could consider that package as a candidate for V2.0 of flutter_downloader. I know you're working on this too, so just wanted to let you know that I have no horse in this race, as they say! If you all would rather go further down this path, I am totally fine with that. I just wanted to offer an alternative that is already fully functional. |
@bartekpacia what does the CI try to say me with the broken test? Out of my view the file which breaks the test does not exist. |
@781flyingdutchman I just looked into your repo again, you made huge improvements since the last time I checked it. To take your metaphor of the horse race you would be ahead. I see that you added in the mean time also uploads and progress for groups of downloads. The only thing I still don't really like is that you have negative progress values for some error cases. I'm currently fighting with that resuming downloads on mobile after app restarts, but what is actually working on other platforms right now. I count pausing and resuming as important, what is your timeline for your 5.0 and will this really be a breaking change what the major version change implies? In this small detail I'm might be a bit ahead. Just general speaking you should just close bugs when they are done that was confusing me a bit while checking your progress. Anyway great work and I'm not really sure what is better now, your or my approach |
It's a false positive. One of the analyzer rules ( |
I have now the basic functionally done. I have just a bug on iOS that the current progress (just the progressbar) cannot be recovered after an app restart, but the download will continue on all Platforms with JSON files. |
@bartekpacia I'm now happy with the current status could you review my changes now? I would like to split out the open points into separate PRs:
What is still open:
Do you see something else missing? Might be even considering to use the work of 781flyingdutchman it's up to you |
HI @rekire and @bartekpacia apologies I didn't see your message, but I did just publish V5 which supports pause/resume. It passes all my tests, and I'm curious to see how it behaves in the wild! |
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.
Thank you for your work. I left some comments.
@@ -1,8 +1,8 @@ | |||
group 'vn.hunghd.flutterdownloader' | |||
group 'com.github.fluttercommunity.flutterdownloader' |
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.
Let's use double quotes for all strings. It'll be easier to convert to Gradle Kotlin DSL in the future.
part 'legacy_api.dart'; | ||
|
||
/// The target where the download should be visible for the user. | ||
enum DownloadTarget { |
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.
It'd be great if you added what methods can be used to access files under each of these paths.
For example, explain that path_provider.getDownloadsDirectory() can be used to access the file if its download target is downloadsFolder
.
part 'legacy_api.dart'; | ||
|
||
/// The target where the download should be visible for the user. | ||
enum DownloadTarget { |
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.
Can the user specify an arbitrary path where they want to save the file?
@override | ||
DownloadStatus get status => throw UnimplementedError(); | ||
|
||
// TODO check if that is required in the future |
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.
todo
final files = await baseDir.list().toList(); | ||
final downloads = <Download>[]; | ||
for (final file in files) { | ||
//print('checking File: ${file.path}'); |
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.
Let's make FlutterDownloader
accept an optional function argument called logger
. By default it's null, but the user can pass whatever they want to get logs from the internals of FlutterDownloader.
} | ||
|
||
/// Left for compatibility returns false. | ||
@Deprecated('There is no replacement') |
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 a pity. How hard would it be to provide a replacement?
|
||
/// A platform specific [Download] which invokes the method channel and make the | ||
/// download native if the platform has this requirement. | ||
class PlatformDownload extends DesktopPlatformDownload { |
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 can use PlatformDownload
on mobile, yet it inherits from DesktopPlatformDownload
? Seems a bit wrong to me.
static Future<PlatformDownload> fromDirectory( | ||
String baseDir, | ||
String id, | ||
) async => |
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.
Let's use the block body.
I use arrow body only when the whole function fits into a single line, otherwise it looks ugly, imho.
} | ||
|
||
/// Run a test in a temp directory, which will be deleted after the execution. | ||
dynamic Function() isolated(Future<dynamic> Function(Directory testDir) body) { |
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.
You might want to use package:file
for this - it abstracts the concrete file system and provides a useful MemoryFileSystem
for use in testing.
@rekire Please rebase with |
Just a quick update from me. I'm observing now for month the progress of the background_downloader of @781flyingdutchman and he is doing a great job out of my view. I have simply not the time to apply the changes as quickly. Therefore I would suggest to check if both packages could be merged to get a bigger audience. |
Thank you for your work @rekire. I agree with the comment above. background_downloader does look nice. However, I'd like it to:
I didn't use it personally though, and honestly, I think I'll try to write the code myself. I'm realizing that 90% of Flutter plugins are of very low quality, become abandoned easily, and in the long run bring more harm than good. I tried my best to improve this plugin but failed. Sorry about this. |
This is my draft for a 2.0 refactoring. I would like to get some feedback.
Progress
Things I want to do before merging:
Improvements
FlutterDownloader
and theDownload
s with theChangeNotifier
APIBreaking changes
Technical details
The
Download
class/interface is the front end the user should use. There are two implementations:DartDownload
is the implementation of theDownload
which has the ability to download files in pure dart code which is used for Desktop platforms and provides the status updates etc.PlatformDownload
is an extended Version of theDartDownload
which delegates downloads to platform implementations and get notified about progress from the native world and forward those details to the Flutter world.Usage samples
Start a download
Pause and resume a download
Get downloads (e.g. after restart)
Observe all downloads at once