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

Clean up api #256

Open
MarcelGarus opened this issue Mar 3, 2020 · 16 comments
Open

Clean up api #256

MarcelGarus opened this issue Mar 3, 2020 · 16 comments
Labels
v2 Epic feature or breaking change which isn't suitable for v1

Comments

@MarcelGarus
Copy link

MarcelGarus commented Mar 3, 2020

While the flutter_downloader works fine, its API doesn't conform to idiomatic Dart standards in some cases.
That's why I'm currently rewriting the Dart side of the plugin to make it more intuitive.
If implemented, this will be a breaking change and thus require a new major version update.

Especially the DownloadTasks are currently merely a wrapper around some data and not really actionable. I'm looking forward to rewriting the API so that something like the following is possible:

final task = await DownloadTask.create(
  url: 'https://...',
  downloadDirectory: getApplicationDirectory(),
);
task.updates.forEach(print);
task.onCompleted(() => task.openFile());

await Future.delayed(Duration(seconds: 3));
if (!task.isCompleted) {
  print('Download takes longer than three seconds.');
}

await task.waitUntilCompleted();

By using Streams rather than callbacks, it's possible to leverage Dart's strengths of async/await, applying stream transformations and support from the ecosystem. For example, one could easily build a Flutter widget like the following:

// inside a widget's State

DownloadTask task;

void _startDownload() {
  setState(() => task = DownloadTask.create(…));
}

Widget build(BuildContext context) {
  return Column(
    children: <Widget>[
      RaisedButton(onPressed: _startDownload, child: Text('Start download')),
      StreamBuilder<void>(
        stream: task?.updates ?? Stream.empty(),
        builder: (context, _) => Text('Progress: ${task?.progress}'),
      ),
    ],
  );
}

What do you think about this API?

@hnvn
Copy link
Member

hnvn commented Mar 4, 2020

Sounds great. Let me know if you need supports in native sides

@MarcelGarus
Copy link
Author

MarcelGarus commented Mar 7, 2020

When invoking the native method enqueue with the arguments {url: https://placekitten.com/263/300, saved_dir: /data/user/0/vn.hunghd.example/app_flutter, file_name: null, headers: , show_notification: true, open_file_from_notification: true, requires_storage_not_low: true}, I'm getting the following error:

I/flutter (18668): {url: https://placekitten.com/263/300, saved_dir: /data/user/0/vn.hunghd.example/app_flutter, file_name: null, headers: , show_notification: true, open_file_from_notification: true, requires_storage_not_low: true}
W/WM-WorkSpec(18668): Backoff delay duration less than minimum value
D/DownloadWorker(18668): DownloadWorker{url=https://placekitten.com/263/300,filename=null,savedDir=/data/user/0/vn.hunghd.example/app_flutter,header=,isResume=false
D/skia    (18668): --- Failed to create image decoder with message 'unimplemented'
D/DownloadWorker(18668): Open connection to https://placekitten.com/263/300
D/DownloadWorker(18668): Content-Type = image/jpeg
D/DownloadWorker(18668): Content-Length = -1
D/DownloadWorker(18668): Charset = null
D/DownloadWorker(18668): Content-Disposition = null
D/DownloadWorker(18668): fileName = 300
D/skia    (18668): --- Failed to create image decoder with message 'unimplemented'
D/skia    (18668): --- Failed to create image decoder with message 'unimplemented'
W/System.err(18668): java.lang.IllegalArgumentException: Failed to find configured root that contains /data/data/vn.hunghd.example/app_flutter/300
W/System.err(18668): 	at androidx.core.content.FileProvider$SimplePathStrategy.getUriForFile(FileProvider.java:744)
W/System.err(18668): 	at androidx.core.content.FileProvider.getUriForFile(FileProvider.java:418)
W/System.err(18668): 	at vn.hunghd.flutterdownloader.IntentUtils.buildIntent(IntentUtils.java:23)
W/System.err(18668): 	at vn.hunghd.flutterdownloader.IntentUtils.validatedFileIntent(IntentUtils.java:35)
W/System.err(18668): 	at vn.hunghd.flutterdownloader.DownloadWorker.downloadFile(DownloadWorker.java:361)
W/System.err(18668): 	at vn.hunghd.flutterdownloader.DownloadWorker.doWork(DownloadWorker.java:189)
W/System.err(18668): 	at androidx.work.Worker$1.run(Worker.java:85)
W/System.err(18668): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
W/System.err(18668): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
W/System.err(18668): 	at java.lang.Thread.run(Thread.java:919)
I/WM-WorkerWrapper(18668): Worker result FAILURE for Work [ id=b24c29d5-139c-45d8-a8fd-e4c00fa6512e, tags={ flutter_download_task, vn.hunghd.flutterdownloader.DownloadWorker } ]

Any idea why that occurs?

@MarcelGarus
Copy link
Author

I temporarily fixed this error with the help of @JonasWanke. The issue was that on Android, the package only supports saving to external directories, not to app directories. That's probably because the resolution of the DownloadedFileProvider fails. I'm no expert on native Android though, so somebody else should have a look at that. For now, I use the same workaround as the example and do Platform.isAndroid ? await getExternalStorageDirectory() : await getApplicationDocumentsDirectory().

Regarding some architectural decisions that I stumbled upon: Why was Java used instead of Kotlin? Why is there an SQL database in the background instead of just communicating with the Dart side?
And finally, I didn't change anything on the native side, but it gives me strange progress values like -101300. What's up with that?

@hnvn
Copy link
Member

hnvn commented Mar 17, 2020

Why was Java used instead of Kotlin?

I prefer Java to Kotlin because I don't have much experiences in Kotlin.

Why is there an SQL database in the background instead of just communicating with the Dart side?

If there's no SQL database, where do you store download task data? If you ask me that why the plugin need to maintain the download task data, my answer is that it's designed in that way, a library for downloading management that users can fire and forget instead of just creating a download task and leaving users with all stuff of download state management.

@hnvn
Copy link
Member

hnvn commented Mar 17, 2020

BTW, your issue related to Android Storage problem, it's not true that the plugin only supports saving to external directories. You can save your files in internal storage if you set the value of open_file_from_notification to false. It deals to the files management in Android. In order to open a file and preview it, the plugin needs to pass it to other applications on your device that can handle that file type (You can image it like the way you open files in your Windows computer, you want to open a FLV file, you need an application like VLC on your machine) and other applications can only access these files if they are stored in external storage.

@MarcelGarus
Copy link
Author

Ohhh I see. I'm not experienced with native Android and didn't know that limitation exists, but it makes sense. We should probably document that behavior in the API as well.

@MarcelGarus
Copy link
Author

MarcelGarus commented Mar 18, 2020

In the related issue in our app (hpi-schul-cloud/schulcloud-flutter#61), @JonasWanke mentioned that it's possible to store downloaded files in your app's private directory and grant other apps temporary read access to it: https://developer.android.com/training/secure-file-sharing/share-file#GrantPermissions
This is also implemented in the open_file package: https://github.com/crazecoder/open_file/blob/34dc2cbdeb0d49a83be59f66cbc6e1a258dc2420/android/src/main/java/com/crazecoder/openfile/OpenFilePlugin.java#L142

@hnvn
Copy link
Member

hnvn commented Mar 19, 2020

@nioncode
Copy link

nioncode commented Apr 6, 2020

@MarcelGarus Will this also take care of moving the callback from the background isolate of the native plugin to the main isolate in the UI so that we don't have to manage this ourselves?

@MarcelGarus
Copy link
Author

Of course! In Dart, most of the time, asynchronous operations are the way to go. So, on any DownloadTask, you'll be able to just call task.updates to receive a Stream of updates, which you can listen to or just plug into a StreamBuilder.
Also, the DownloadTasks update their fields automatically so you can do:

var task = ...;
await Future.delayed(Duration(seconds: 1));
print(task.status);

@sqrg
Copy link

sqrg commented Aug 26, 2020

Any updates on this? Is there any workaround to wait for a file to finish downloading?

@MarcelGarus
Copy link
Author

MarcelGarus commented Aug 31, 2020

There are some minor issues left in the PR #283.
Especially if you have macOS tooling and have some spare time, feel very welcome to look at the code from the PR.

In the meantime, you could register a callback for download progress, create a Completer and complete it as soon as you receive the callback for the task being finished.

@sqrg
Copy link

sqrg commented Sep 1, 2020

Sure, I can take a look on macOS. I don't have much experience nor knowledge, but I'll take a look

@qq326646683
Copy link

same issue on Android, these works for me:

provider_paths.xml:

<?xml version="1.0" encoding="utf-8"?>
<paths xmlns:android="http://schemas.android.com/apk/res/android">
    <root-path
        name="root"
        path="." />
</paths>

@arcanebrownie
Copy link

I'm new to flutter, so I apologize if my question is stupid, but how do I use the DownloadTask.create that you show in here? Is the import still import 'package:flutter_downloader/flutter_downloader.dart';? Since that is the import I saw you use in the example.

Thanks!

@bartekpacia
Copy link
Collaborator

It's a pity that this PR died, would be great if the API had better design.

@bartekpacia bartekpacia pinned this issue May 23, 2022
@bartekpacia bartekpacia added the v2 Epic feature or breaking change which isn't suitable for v1 label May 23, 2022
@bartekpacia bartekpacia unpinned this issue Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 Epic feature or breaking change which isn't suitable for v1
Projects
None yet
Development

No branches or pull requests

7 participants