Clean up API

[MarcelGarus]

This PR changes none of the functionality of this package, only the API itself.

Now, there exist DownloadTasks that encapsulate all actions related to a specific download. DownloadTasks are also automatically kept up-to-date by the package.

Check out the example to see how the new API looks like when it's used.

Todo:

• document changes in README
• update version

[hnvn]

Great, it's more functional programming now.

[hnvn]

Do you think that we need a way to removePortNameMapping () somewhere?

[MarcelGarus]

I don't think so. FlutterDownloader only offers static methods, so there are no multiple intances of FlutterDownloader running at any time. Thus, there's never a naming conflict between ports.
Theoretically, we could add a dispose method, but when the app/DartVM is stopped, it automatically cleans up these resources for us, so I don't think there's a need for it.

[hnvn]

Ok, I think it makes sense

[MarcelGarus]

I still got a few extra thoughts:

• A little question regarding the newly created timeCreated field: Is it just an arbitrary increasing integer or something specific, like the number of seconds since Unix epoch? If it represents a concrete time, I'd rather change it to DateTime before merging.
• Also, I saw you added several try { … } on PlatformException catch (e) { … } blocks that fail silently by just returning null. What was the reasoning behind that? If an operation fails, I believe we should let the user know that by throwing an exception. If they occur, developers should open issues here so we can figure out what happened and either fix the flaw or instead throw a typed Dart exception that is more descriptive.
• Additionally, because this is a breaking change, I updated the version to 2.0.0 in accordance to semantic versioning so that the code of users that depend on this package using flutter_downloader: ^1.x.x doesn't get broken.
• Finally, what is the reasoning behind allowing custom queries with loadTasksWithRawQuery? I really don't like these kinds of escape hatches where the abstraction layer of the API gets broken – it makes it possible for users of this library to depend on how it's working internally rather than the actual API surface. I believe we should @Deprecate this methods. Developers that use this will be shown a warning and redirected to this repo, so they can file an issue with their use case.

[hnvn]

I just tried your new design for the example project in Android. There's a problem that causes user lost the status of current tasks in UI if they quit the application by using back button.

[MarcelGarus]

Sorry for the late reply, I had some other stuff to do. I implemented the restoring of tasks on app restarts – it would be great if you could have another look at it!
Also, do you have any update on points 1 and 4 of my previous comment, maybe also point 2? That would be really helpful – thanks in advance! 😊

[hnvn]

• The timeCreated is Unix epoch in milliseconds (codes and codes).
• My purpose to use try { … } on PlatformException catch (e) { … } that prevents the application from crashing by exceptions of platform channel (not dart exception).
• In the current api implementation, loadTasksWithRawQuery is very useful, it lets developers customize the tasks loaded from DB instead of loading all tasks. I think it makes sense when you have thousands (or millions) of tasks in DB.

[hnvn]

Your new design example project makes me quite confused:

• It doesn't show the process of downloading tasks in UI
• It doesn't let me know what is going on until I click (+) button.
• It doesn't update the status icon of downloading task (it sounds like a bug?)

[MarcelGarus]

I changed the example so it looks more like the previous one. Have a look:

I also changed the DownloadTask so it now has a created field, which is simply a DateTime.

loadTasksWithRawQuery

Concerning loadTasksWithRawQuery, I see that it does come in handy if you have thousands or millions of tasks in the DB, but I still don't like that it exposes some internal layers of the package to the users of this package. Here's how the package is currently architected:

The fact that loadTasksWithRawQuery exists makes the DB implementation part of the API (red arrow).
That means, if you change how tasks are stored in the future, this can't just be published as a minor version so that all users benefit from it the next time they run flutter pub get. Instead, it needs to be published as a major version, because it might break some implementations.
Also, giving developers access to the raw SQL DB means that they might accidentally mess things up. Currently, there are probably some really common use-cases (like, getting all tasks with a specific status) that are not supported because there's the loadTasksWithRawQuery method – there are probably multiple developers in the world who use this package and implemented the same functionality using raw SQL queries. That is inefficient and if some developers are not experienced with SQL, the might make their users vulnerable to SQL injections without knowing it.
If instead, we would @deprecate this method with a link to this repository, developers using this method would get notified, go here, and file issues about what functionality they need. For example, the use-case from above of getting tasks with a specific status could get implemented by the package. Perhaps, instead of loadAllTasks, we might implement a loadTasks method that optionally accepts several filter criteria, like id, status, url, destination, createdBefore, and createdAfter. Then you could use this to get specific tasks:

// Load failed tasks from the last week.
final tasks = FlutterDownloader.loadTasks(
  status: DownloadTaskStatus.failed,
  createdAfter: DateTime.now() - Duration(days: 7),
);

That would give us the opportunity to implement the feature once and everyone would benefit:

• You as a package maintainer would benefit because the package is more flexible. You could change the underlying database if something better comes up.
• Developers using this package benefit because there are some ready-to-use methods that probably fulfill their use-cases. They don't need to implement custom SQL queries anymore.
• Users would benefit by increased security. The code in this repository is reviewed by multiple people, so the SQL queries implemented here are probably safer than the ones that developers implement for their projects themselves. If some SQL expert knows some safer/faster queries, this could benefit all users.

Of course, this isn't as powerful as providing a raw SQL interface, but gives developers less opportunity to mess something up – all queries will be guaranteed to be safe. We can then implement higher-level methods (or additional filter parameters for loadTasks) that support these use-cases. After some time (like a year or so), loadTasksWithRawQuery could get removed.

That being said, I'm not even sure this change would affect that many people – like you said, loadTasksWithRawQuery only makes sense if you have thousands of downloads.

Usage of try-catch

I'm not sure wrapping all platform calls in try-catch is the right approach. Exception throwing is a powerful error-handling feature of Dart and we should use it. It's more powerful than just returning null for several reasons:

• It gives developers information about what error happened. If a method returns null, users of this package have no idea whatsoever about what went wrong.
• It allows non-local catching. By using exceptions, they can choose to handle the exception not right where they called this package, but in another function higher up on the call stack.

Also, PlatformExceptions don't just appear out of nowhere – there's probably a reason they get thrown. If they get thrown, we (as package maintainers) should investigate why they are thrown and then decide what to do.

• If it's a valid exception (something bad happened, but it should be possible to happen), we should catch it and then create our own exception class (for example, NoStoragePermissionException). Developers using this package can then catch these exceptions explicitly and get notified when something goes wrong on the native side:

  try {
    await DownloadTask.create(...);
  } on NoStoragePermissionException {
    // Do something.
  }

  Note that if we create a generic FlutterDownloaderException and let all other exceptions extend/implement that, users could also do try-on FlutterDownloaderException to catch all possible valid exceptions from this package.
• If this is an exception because some code on the native side is broken, we should still throw the PlatformException so that developers see something is wrong and can come to this repository and file an issue. We can then investigate the issue in more detail and fix it.

By the way, with the upcoming NNBD feature of Dart, types are non-nullable by default (aka type T cannot be null, but T? can). If we would continue just returning null if an error occurred, we would be forced to return DownloadTask?, making the quite unintuitive.

---

What do you think?

[MarcelGarus]

@hnvn Any updates?

[hnvn]

Sorry for late response, I am quite busy this time. I will try to work on this PR next week.

[hnvn]

I'm working on your source codes but I cannot get packages with this error:

[example] flutter pub get
Running "flutter pub get" in example...                         
Because every version of flutter_test from sdk depends on path 1.7.0 and dartx >=0.2.0 <0.4.0 depends on path >=1.6.4 <1.7.0, flutter_test from sdk is incompatible with dartx >=0.2.0 <0.4.0.

And because black_hole_flutter >=0.2.10 depends on dartx ^0.3.0, flutter_test from sdk is incompatible with black_hole_flutter >=0.2.10.

So, because flutter_downloader_example depends on both black_hole_flutter ^0.2.11 and flutter_test any from sdk, version solving failed.
pub get failed (1; So, because flutter_downloader_example depends on both black_hole_flutter ^0.2.11 and flutter_test any from sdk, version solving failed.)
exit code 1

My Flutter version:

Flutter 1.18.0-11.1.pre • channel beta • https://github.com/flutter/flutter.git
Framework • revision 2738a1148b (2 weeks ago) • 2020-05-13 15:24:36 -0700
Engine • revision ef9215ceb2
Tools • Dart 2.9.0 (build 2.9.0-8.2.beta)

[MarcelGarus]

cc @JonasWanke

[JonasWanke]

I can publish a new version of black_hole_flutter with support for dartx v0.4.x. It seems that their v0.4.1 release now supports both Flutter stable and beta.

[hnvn]

Any update @MarcelGarus , I still cannot run the example project on beta channel

[MarcelGarus]

Yup, still waiting for @JonasWanke to push an update of black_hole_flutter, which is used in the example

[JonasWanke]

Sorry for the delay, I hope I'll get the update out tomorrow. Note that you can specify dartx: ^0.4.0 in the dependency_overrides-section of your pubspec.yaml to solve the problem in the meantime.

[MarcelGarus]

True. Did that, (still) works fine on my device

[JonasWanke]

Finally had the time to push the update: v0.2.12

[hnvn]

Any update @MarcelGarus ?

[MarcelGarus]

Oh sure, here we go.
@hnvn Do you have an opinion on the points from my comment above?

[hnvn]

I agree with two points you mention above but your example project still has issues:

• Lost download state when user exit the app and come back later (Android).
• The downloaded file can not be opened (iOS). I run the example project on iOS simulator, the files downloaded with current source can be opened but the files downloaded with your new source codes can not, it's quite curious 🤔

[MarcelGarus]

Glad that you agree.
I'll look into the issue on the Android side, but I don't have iOS tooling, so someone would have the help with the second one.

[MarcelGarus]

Okay, so I debugged and restructured the code in the example to be even more readable.

Now, on Android, if you close the app and restart it, already downloaded tasks are recognized and are displayed as downloaded – the example gets the tasks from the native side and then binds them to the existing files. However, it seems like the native side restarts the download every time the app is opened. I can try to look into that, but I'm not really experienced with native Android development, so it would be really great if someone could help out there.

Regarding iOS, I have no idea how that could be – the changes shouldn't affect the opening of files in any way. I have no iPhone or MacBook, so I can't reproduce and fix the bug.

[bartekpacia]

This was a great discussion.

An awesome upgrade is happening in #793, so I'm closing this.