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

[cross_file] Platform-aware factories. #91869

Open
ditman opened this issue Oct 15, 2021 · 7 comments
Open

[cross_file] Platform-aware factories. #91869

ditman opened this issue Oct 15, 2021 · 7 comments
Assignees
Labels
c: new feature Nothing broken; request for a new capability Bot is counting down the days until it unassigns the issue p: cross_file The cross_file plugin P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team

Comments

@ditman
Copy link
Member

ditman commented Oct 15, 2021

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:

  • What to do when the user wants to modify the contents of a file programmatically?
  • How to perform those operations so the modified file still uses an efficient backend?
  • Should we keep an "inefficient" Uint8List-backed constructor so people can build raw files from memory?
@ditman ditman added c: new feature Nothing broken; request for a new capability plugin P3 Issues that are less important to the Flutter project p: cross_file The cross_file plugin labels Oct 15, 2021
@ditman ditman added this to Not Started in Flutter Web Plugins via automation Oct 15, 2021
@ditman
Copy link
Member Author

ditman commented Oct 15, 2021

@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
Copy link
Contributor

Jjagg commented Dec 30, 2021

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
Copy link
Member Author

ditman commented Jan 5, 2022

@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.

@stuartmorgan stuartmorgan added package flutter/packages repository. See also p: labels. and removed plugin labels Mar 6, 2023
@flutter-triage-bot flutter-triage-bot bot added team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team labels Jul 8, 2023
@stuartmorgan stuartmorgan added P2 Important issues not at the top of the work list and removed P3 Issues that are less important to the Flutter project labels Jul 26, 2023
@stuartmorgan stuartmorgan self-assigned this Jul 26, 2023
@ditman
Copy link
Member Author

ditman commented Oct 17, 2023

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
Copy link
Contributor

bparrishMines commented Dec 20, 2023

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
Copy link
Member Author

ditman commented Dec 21, 2023

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
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: new feature Nothing broken; request for a new capability Bot is counting down the days until it unassigns the issue p: cross_file The cross_file plugin P2 Important issues not at the top of the work list package flutter/packages repository. See also p: labels. team-ecosystem Owned by Ecosystem team triaged-ecosystem Triaged by Ecosystem team
Projects
Flutter Web Plugins
  
Not Started
Development

No branches or pull requests

6 participants