flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
169.4k stars 28.25k forks source link

[cross_file] Platform-aware factories. #91869

Open ditman opened 3 years ago

ditman commented 3 years ago

The current implementation of cross_file relies on cross-platform constructors that "need to work" across all platforms. With time, and as features are added, these constructors end up growing, and now some parameters are being ignored by some platforms and used by others. This causes confusion/problems.

It seems that cross-platform constructors are not the way to go for this particular package; instead XFile users should be able to choose what factory they need for the platform that they're implementing. So, in places where dart:io is available, they could do:

XFileIO.fromFile(io.File file) or XFileIO.fromPath(String path)

But if dart:html is available

XFileWeb.fromHtmlFile(html.File file) or XFileWeb.fromBlob(html.Blob blob, String name) or XFileWeb.fromBlobUrl(String blobUrl, String name, int length...)...

or any other "specialty" constructors that would make sense per platform.

The XFile class that most people use would end up being a read-only interface to retrieve data from the file, and the different backing implementations would extend it.

(This would also allow us to return XFile as the base-class name for all the objects, instead of being the top-most classname, which is weird from an OOP standpoint!)

Some questions:

ditman commented 3 years ago

@stuartmorgan said:

I think we could make it relatively painless if we:

  • Add new constructors with the APIs we want, and comment (but don't actively deprecate) the old APIs as being soft-deprecated
  • Update all our plugins to use the new APIs
  • Give the ecosystem a little while to catch up
  • At some point deprecated the old constructors
  • Eventually remove them in a breaking change
Jjagg commented 3 years ago

I ran into a pretty serious limitation caused by this issue.

Because XFile does not have a Blob constructor for web, it can't refer to a file on the user's machine. And without a Blob, there's no way to get the data in the file back from the XFile. So, currently, the only way to pass the file data in an XFile is by reading the full file into memory and passing the Uint8List in the constructor. I ran into this with a drag and drop implementation for web.

ditman commented 3 years ago

@Jjagg yes, having a Blob constructor on the web is my main motivation for this change. The current web version loads a file, internally stores its blob URL, and then it attempts to re-hydrate the Blob when reading data from it. It should just... store the File/Blob and be done with it.

ditman commented 1 year ago

I've seen a huge improvement of the performance of file_selector by implementing a constructor fromFile for HTML Files (as suspected).

Here's a demo app: https://dit-tests.web.app; when selecting a large file you'll see that it'll get processed immediately, instead of being copied over as a network "resource".

The prototype also includes a fix for this issue:

bparrishMines commented 1 year ago

I think this can be extended to every platform and that we should federate the plugin. Android and iOS have there own systems for reading and writing to files that are external to the application:

Android: https://developer.android.com/training/data-storage/shared/documents-files#java iOS: https://developer.apple.com/documentation/uikit/uidocumentpickerviewcontroller?language=objc

For Android, I wasn't able to use dart:io to write or read from external files because I needed to use a ContentResolver. I'm not sure if this is something that we should expect dart:io to handle or if we need to write a plugin (like cross_file_android) to interact with this API.

ditman commented 1 year ago

I think this can be extended to every platform and that we should federate the plugin.

Indeed! @stuartmorgan and I discussed this briefly a couple of weeks ago and decided to push it to after new year, but it's on the radar!

flutter-triage-bot[bot] commented 11 months ago

This issue is assigned to @stuartmorgan but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

flutter-triage-bot[bot] commented 8 months ago

This issue was assigned to @stuartmorgan but has had no status updates in a long time. To remove any ambiguity about whether the issue is being worked on, the assignee was removed.

stuartmorgan commented 8 months ago

This is still on my list to work on eventually, but not in the short term, so unassigned is correct for now.

EvertonMJunior commented 1 month ago

Are there any updates on this? Running into issues with large files on web.

ditman commented 1 day ago

I have a PR / proposal for this, but I haven't had time to work on it, if anybody's interested, this is what I've been planning to do:

The TL;DR is that when you read a XFile you use its XFile interface (without a specific class), but when your app or plugin is producing an XFile, it can produce "the most optimal" version (which really only has different implementations on the web).

In the rare cases where you need to be able to submit really big files, I think it's likely that the app will need to look at the internal "Blob" of the file, so the browser can handle that directly without having to look at its bytes (when doing fetch, for example). I think this is not possible with the current implementation I proposed, but should be easy to add.

Anyway, haven't had time to work on the above, but if anybody wants to take it and bring it up to speed, I can help review and land the new implementation.