Quick review

[rekire]

I'm right now checking if I want to use this plugin. Here are lots of open issues (241 when I submit this) and 11 open PRs.

The code base seems to be quiet old which is nothing bad as log everything works as expected. However it seems that on Android the scoped storage is not supported. The way it is done with the provider is an way, but I think not the best idea. The downloads won't show up in file pickers. The native parts are written in "old" programming languages (ObjectiveC and Java). Notification requests are not supported on both platforms. The setup is complex and I have no idea why a database is used. I see no reason for that init call. The notifications seems to work very differently on iOS and Android. The localization is not centralized for both platforms.

Are you aware of all those issues?

[bartekpacia]

Hi @rekire! Thanks for reviewing the code of this plugin, I really appreciate this.

Here are lots of open issues (241 when I submit this) and 11 open PRs.

When I became a maintainer, there were more than 300 open issues. I'm doing my best to reduce this number, but it's just... a lot.

However it seems that on Android the scoped storage is not supported. The way it is done with the provider is an way, but I think not the best idea. The downloads won't show up in file pickers.

Agree, this is definitely an issue.

The native parts are written in "old" programming languages (ObjectiveC and Java).

Yeah, this bugs me as well, but it works. I'd like to convert the Android part to idiomatic Kotlin. Some work on rewrite to Swift is being done in #709.

Notification requests are not supported on both platforms.

Tracked in #710.

I have no idea why a database is used.

It's because this plugin not only downloads files, but also tracks the download tasks. This data has to persist between app restarts. Unfortunately it's done separately for Android and iOS, which makes this part of the codebase even harder to comprehend and refactor.

The localization is not centralized for both platforms.

I think the current solution is OK. What would you suggest instead?

Overall, I totally agree with you a lot that could be done better in this plugin. Some great ideas are mentioned in #283. I'm a maintainer, but unfortunately don't have enough time to actively develop the new features and fight the technical debt. I'm very open to PRs though – refactors, bug fixes, new features - I'll be happy to review anything. But the stability of the plugin is the most important thing.

I'd also like to have at least some basic test suite so that we could be more confident with introducing new changes, but... yeah, we don't have it.

[rekire]

I already read that the situation was much worse before. Glad that someone pick that task to maintain this package.

I just finished to convert the classes to Kotlin (but there are still a lot of bang operators) in a branch: https://github.com/rekire/flutter_downloader/tree/kotlin

Regarding the database I think that this is not required. I mean when I force close the app I won't expect that the download continues. My browser doesn't do that and as far I checked the code (while converting) there is also no code to continue downloads (by side that not all servers support range requests). Therefore I would just drop that feature, but that is just my own opinion. That would also reduce a lot of complexity.

Regarding the translations I am not absolutely sure, but it seems to be hard to translate this package e.g. to German. I would need to edit multiple files for both platforms to apply a new language or did I miss something?

[rekire]

Just one more thought: I'm thinking of tracking the progress of downloads with some kind of status bar like the download status bar (a quiet old plugin for Firefox). That would fix the case when the user rejects the permission that you get download feedback.

When I find some more time I will try to check if I can remove the database code for testing. In worst case that will be just a feature branch for myself.

[bartekpacia]

I just finished to convert the classes to Kotlin (but there are still a lot of bang operators) in a branch: https://github.com/rekire/flutter_downloader/tree/kotlin

Wow, nice! It'd be great if you created a PR with your changes if you think we could merge it.

Regarding the database I think that this is not required. I mean when I force close the app I won't expect that the download continues. My browser doesn't do that and as far I checked the code (while converting) there is also no code to continue downloads (by side that not all servers support range requests). Therefore I would just drop that feature, but that is just my own opinion. That would also reduce a lot of complexity.

That's an interesting point of view. I'd have to think a bit more about this.

Regarding the translations I am not absolutely sure, but it seems to be hard to translate this package e.g. to German. I would need to edit multiple files for both platforms to apply a new language or did I miss something?

Yes, to translate notification messages to German, you'll have to edit files both in the android and ios folders.

I'm thinking of tracking the progress of downloads with some kind of status bar like the download status bar (a quiet old plugin for Firefox). That would fix the case when the user rejects the permission that you get download feedback.

I'm not sure what you mean by this. Do you want this plugin to provide some default "progress indicator" widget? Please elaborate :)

I'm on the side that this is the developer's responsibility. If they want to keep their users informed about the progress, they can listen to the download tasks updates stream.

When I find some more time I will try to check if I can remove the database code for testing. In worst case that will be just a feature branch for myself.

Sure, good luck. When you're done please share the results :)

[rekire]

I just finished to convert the classes to Kotlin (but there are still a lot of bang operators) in a branch: https://github.com/rekire/flutter_downloader/tree/kotlin

Wow, nice! It'd be great if you created a PR with your changes if you think we could merge it.

I'll do that when I'm sure that I added no bugs. By the way feel free to test the changes yourself, since you might know the edge cases. Do you prefer a draft PR for that?

[...] to translate notification messages to German, you'll have to edit files both in the android and ios folders.

I would prefer something centralized in dart code, but I need to think a bit more about it what is the best way.

I'm thinking of tracking the progress of downloads with some kind of status bar like the download status bar (a quiet old plugin for Firefox). That would fix the case when the user rejects the permission that you get download feedback.

I'm not sure what you mean by this. Do you want this plugin to provide some default "progress indicator" widget? Please elaborate :)

I'm on the side that this is the developer's responsibility. If they want to keep their users informed about the progress, they can listen to the download tasks updates stream.

Yes something like done before here, but that would require to expose the progress which is AFIK currently not supported (but I could be wrong). I need to find out what you mean with that updates stream, might be that is already supported.

When I find some more time I will try to check if I can remove the database code for testing. In worst case that will be just a feature branch for myself.

Sure, good luck. When you're done please share the results :)

I'll do that.

[bartekpacia]

Do you prefer a draft PR for that?

Yeah, I'd prefer it.

Yes something like done before here, but that would require to expose the progress which is AFIK currently not supported (but I could be wrong)

It is, you can look at the example app and how it displays the current progress to the user.

[rekire]

Here we go https://github.com/fluttercommunity/flutter_downloader/pull/719

I'll check the sample app, sorry for my wrong guess.

[rekire]

I should have checked the sample app before, there is actually really a download continuation functionality. Nice! But it is broken: When the server doesn't support range requests and simply returns the entire file it will be ignored and simply appended, at least on the Android part. Not sure if that was reported before. I don't actually understand the iOS part so I cannot find out if there is the same bug.

Likely not really an issue, but also the etag is not stored for checking if the file content has changed before continuing the download.

However I would download the file preferred with dart code, instead of maintaining it per platform. That would make things easier for other target platforms.

[bartekpacia]

But it is broken: When the server doesn't support range requests and simply returns the entire file it will be ignored and simply appended, at least on the Android part.

Maybe we could check if the server supports range requests (link), or just add info to docs about this behavior? I bet many users of this plugin depend on this feature.

Likely not really an issue, but also the etag is not stored for checking if the file content has changed before continuing the download.

Thanks for raising this, it definitely deserves its own issue. Feel free to create one :)

However I would download the file preferred with dart code, instead of maintaining it per platform. That would make things easier for other target platforms.

Some work on it is (is? was?) done in https://github.com/fluttercommunity/flutter_downloader/issues/693, but I'm afraid the original creator had his reasons to implement this natively twice.

[rekire]

I want to spend some hours to check how it would work when I move that part to the dart side and eliminate the database at once. In my mind that reduces a lot of complexity.

I could create one or two issues for that tomorrow, it's just that I had no motivation yet to create a proper bug report with steps to reproduce 😅

[bartekpacia]

Sure, no problem :) I'm not going anywhere, haha, take your time

[rekire]

Found while testing my pr another bug (but I'm not sure if that is only a sample app bug): When you double tap the download button you might get a -1% progress :-D

[781flyingdutchman]

Thanks @rekire for working on this. I was also working on a refactor (more than a translation of Java and Obj-C) and thought about the sqlite database a lot - for example if you could move it to the Dart side and have only one version to maintain. However, the issue is on the iOS side: different than in Android, the equivalent of the 'WorkManager' concept is a urlSession object that accepts urlDownloadTasks. However, there is no way to attach metadata to those tasks, and therefore it is not possible to track the status of your downloads in any way unless you add a database where you store a reference to the task. So unfortunately I do think we need to keep the database around, and I also think we need to keep them separate in iOS and Android because otherwise every progress update call requires teh system to fire up the Flutter Engine again to make a call to Dart, and although I have not tried it I think this will create significant overhead that on iOS will likely lead to the tasks getting shut down (iOS is MUCH more aggressive about backgroudn behavior, so we need to stay as close as possible to the iOS native mechanism of urlSession).

There is another issue with the current downloader that I think really needs to be addressed because it is truly broken and cannot be fixed without a breaking change in the API. This is the issue where on iOS you are not allowed to store absolute paths, as the path to the app data changes with every app start. As a result of this, the current API with 'savedDir' as absolute simply breaks on iOS for long or many background downloads. In my implemenation I change the requirement for saveDir to be a relative directory, and add to the DownloadTask class an enum for the base directory with .applicationDocuments (as provided by getApplicationDocumentsDirectory()), .temporary (as provided by getTemporaryDirectory()) and .applicationSupport (as provided by getApplicationSupportDirectory()).

Finally, I would agree that the functionality is currently far from equivalent between iOS and Android, and the plugin is trying to do many things. For a V2 design - besides the changes you suggest plus the one I mention here - I think a narrower function set (e.g. without notification options, external storage, etc) might be better, because the more the plugin behaves differently between platforms, the harder it is to use in an actual app that targets both. For example, if the plugin cannot handle notifications for both platforms, then why not leave out the notification code and let users use a separate plugin that does notifications well for both iOS and Android?

[rekire]

I'm working right now on a PoC to download files with pure dart code (even without any other plugins). This is primarily to understand how this works. Right now I have only this "meta data":

• url
• detected file name (based on first response)
• expected file size (based on first response)
• etag (when provided in the first response)
• resumable (when first response contains "accept-ranges: bytes")
• optional: request headers (but with a generated user-agent)

I had the plan that I create a hash of the url and to store the already downloaded data in a file with the schema hashed-url.part and store the metadata above in a file hashed-url.meta (since I'm still the opinion that metadata are required but not necessary in a database)

Regarding the background tasks I was thinking to use Isolates for the background tasks. To be honest I am not sure if that works on all platforms as I would expect, might be that becomes a show stopper on iOS. In that case it might required to keep the old behavior.

That way I have in mind would work for future desktop implementations, or does that iOS restrictions also apply for MacOS?

How are downloads managed on Safari on iOS? Is there a progress when the app is in background? If so I would expect that there should be an API to do the same.

I'm not sure if I would remove the notifications in general. I need some more insights how it works on iOS on Android I know it. Since Android 13 you need also a runtime permission like on iOS (can you tell since which version?). I have in mind a widget as some kind of status bar to show the download progress that might be nice too. In my app I cannot image a simple percent view like in the sample app

[bartekpacia]

Since Android 13 you need also a runtime permission like on iOS (can you tell since which version?)

Since Android 13, that is API level 33.

Anyway, I'm extremely happy to see you work on improving this plugin :)

[781flyingdutchman]

I wrote an alternative, simpler version of a background file downloader and published it as background_downloader because I needed something that works reliably, without all the bells and whistles of the flutter_downloader package. Per @rekire it doesn't use a Sqlite database, and mostly works via registered callbacks for status updates and progress updates. It works really well for my purpose (large number of smaller files to download reliably in the background, when the app is suspended, plus a few large files where progress tracking is important) and has a few extra features (like grouping requests and handling updates differently for different groups).

Feel free to take a look and use the code in the flutter_downloader if you'd like (or submit improvements to background_downloader!). Personally, I think the API is much cleaner and nicer, plus it addresses some of the fundamental issues with flutter_downloader that cannot be fixed without breaking that API anyway.

[rekire]

@781flyingdutchman to be honest I do not really understand yet how your library works. Which code is actually calling doWork() which seems to be the only caller of the method downloadFile()?

Do I understand the Android part correctly that you download the file in a coroutine with ktor client?

I have bad news for you, that just works when your app is in foreground. But when you put the app for a couple of seconds (I was unable to find out the exact timing) then the download will be interrupted. I also found nothing of pausing or resuming downloads, but this seems not to relevant for your usecase. By the way the connection timeout of 8 minutes is huge has that some meaning or is that just random?

---

@bartekpacia do you know more about threading in dart? I think that the download itself has to be offloaded to e.g. a foreground service, but I'm not sure if the DartVM will use those threads and even if it is possible to call some dart code directly from the JVM. I think this only works with MethodChannels and there I would bet that this relays on the NDK code of the Flutter SDK and there in the DartVM. So I'm thinking that the downloads must be really done native on Android and iOS since this required a lot of native APIs. What do you think?

[781flyingdutchman]

@rekire the Android code uses the WorkManager which is the recommended way to run background tasks on Android. It is somewhat similar to URLSession on iOS in that you enqueue a task and the system executes it. It is the Workmanager that calls doWork() and because the ktor client is run in that function, it does not get interrupted when the app switches to the background - that is the point of using the workmanager. The 8 minute timeout is for completion of the download (it is not the typical reponse timeout, that is usually set at seconds) and is there because a workmanager task gets interrupted after about 10 minutes by the Android system.

As for your question about threading in Dart: threads (using isolates in Dart) will help unblock the UI, but this will not solve your background problem, as threads won't survive a switch to background. This gets to the problem with flutter_downloader: it promises something it doesn't reliably deliver, as the way the background downloads are implemented is just not correct

[rekire]

@781flyingdutchman ah that is cool I missed that while checking your code. I need to check the WorkManager documentation. On the first view of the documentation did you test the setForeground() call? That should allow even bigger downloads (or at least on slow connections).

That isolates way might be still reasonable when used on the desktop implementation what do you think?

[rekire]

I'm still working on some experimental changes to resolve lots of issues and to reduce the complexity. I'll provide here my toughs to get some feedback:

I'm rewriting the way how the download progress is tracked. My current idea is to have the FlutterDownloader class as entry point which maintains all the current downloads. So it had some methods like:

• List<Download> getAllDownloads()
• Download startNewDownload(...)

That class should also implement a DownloadProgress interface to observe the progress of all active downloads. This interface should be also implemented by the Download class.

The idea of that DownloadProgress is to add listeners to show the progress in the UI like a progress bar, but this could be used also to show the progress e.g. on Windows in the Taskbar. Since this would be decoupled this could also allow a implementation for notifications on Android including the permission management.

As already pointed out the downloads itself must be made native on mobile platforms, however on desktop this can be done in pure dart.

The Download class has methods like:

• pause()
• cancel()
• delete()
• and it should provide its status like the progress if it is continueable (based on the headers etc)

Do you think that this refactoring makes sense to you?

[rekire]

Regarding to my last comment here is my idea of the new FlutterDownloader interface. I already kept in mind a migration path.

My download interface as mentioned above can you see here.

The current version does not work on mobile and the sample (desktop) app just downloads (for testing) the downloads for exact 500ms. I had no time yet to wire the sample app to make it work smooth with my new API idea.

Since I have absolutely no idea how to do it on iOS I would try to keep it as it is, the Android part should be quiet similar to the work of @781flyingdutchman with the WorkManager API.

For Android I have also in mind to put the download in the "new" scoped storage to make things easier, but this will take some time. This would also fix the issue of https://github.com/fluttercommunity/flutter_downloader/issues/676

[rekire]

I created a preview pull request to show my progress. Any feedback?

[rekire]

Closed via #854