hpoul / file_picker_writable

Flutter plugin to choose files which can be read, referenced and written back at a later time.
https://pub.dev/packages/file_picker_writable
MIT License
17 stars 12 forks source link

Errors opening from intent/open-in/copy-to are swallowed #9

Open amake opened 4 years ago

amake commented 4 years ago

If an error/exception occurs while trying to open a file sent from another app by intent (Android) or "Open in"/"Copy to" (iOS), the error is logged (e.g. here) but nothing is reported to the Dart layer.

In case the application wants to show an error in this case, it would be good to have an ErrorHandler that one could register (alongside UriHandler, FileInfoHandler) to be notified when an error occurs.

hpoul commented 4 years ago

right, but this looks pretty much like the only place? I guess it would make sense to add a dedicated OpenErrorHandler which is for this specific case - when the plugin receives an open intent/callback from the OS.. otherwise for open file/write file/etc. method calls should continue to result in exceptions.

the only thing i'm not sure about is whether the init method should also result in an thrown error when an exception happens during _handleUrl .. i guess to keep consistent it should succeed.. and the error should be reported to the OpenErrorHandler

amake commented 4 years ago

right, but this looks pretty much like the only place?

Yes, sorry, I said “e.g.” because the equivalent entry point also exists on Android.

a dedicated OpenErrorHandler which is for this specific case

Yes, it would be single-purpose, but there’s really no other option to catch this case.

i guess to keep consistent it should succeed.. and the error should be reported to the OpenErrorHandler

I agree, I don’t think there’s a better choice.

hpoul commented 4 years ago

@amake I have now done a bit of refactoring, and also added such error handler for open-in/copy-to - only iOS right now.. and it only contains a string message .. this should probably something more useful the application can parse.. not sure yet what though..

https://github.com/hpoul/file_picker_writable/commit/f83262129a251adf7ec4ae764948418188e4e3e6#diff-f540102ae087b792214ddead720f73e1R337-R339

the handling works basically the same as for file open handlers.. https://github.com/hpoul/file_picker_writable/commit/f83262129a251adf7ec4ae764948418188e4e3e6#diff-d9a718a3042c2ebab14df5f34ef46a52R102-R112

btw. this is still a bit of a WIP, and I also did a quite bit of refactoring and want to move away from providing temporary files in FileInfo to only providing access to files via a callback..

https://github.com/hpoul/file_picker_writable/blob/f83262129a251adf7ec4ae764948418188e4e3e6/example/lib/main.dart#L162-L169

this way there will be no more left over temporary files.. once the app returns from the callback, file will be deleted immediately.

amake commented 4 years ago

Thanks for the update. I think a string message is good enough. Just being able to receive a notification is actually enough for me.

Regarding callbacks, I have to say I'm not a fan. My code is very much designed around Futures and async/await, so it will probably be painful to move to the new API. On the other hand I don't have any other good suggestions for achieving your goals.

hpoul commented 4 years ago

@amake the old api still works the same way for now, it's just deprecated.. and it does use futures and async.. basically nothing changes, except you have to process or copy the file to some location before the callback completes..

in AuthPass the change wasn't too painful https://github.com/authpass/authpass/commit/8f7a9528539d277ae469da7eb496d6856c168cf2

The result of the callbacks is passed back to the caller, so if you simply read the whole file at once, you could basically just do that in the callback and return a tuple of the FileInfo plus Uint8List (or string) with the file content ..

Maybe it would even make sense to put this into the package, a default callback which returns a future with the FileInfo plus the file content (either string or bytes), i guess not many use cases have to stream the file contents.. 🤔