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] Adding custom sources to feed data to XFile #6625

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

theskyblockman
Copy link

This PR adds a new constructor for XFile (XFile.fromCustomSource) that only takes an implementation of a new abstract class XFileSource. This new class enables users of the cross_file plugin to construct an XFile and have outputs totally managed by the user.
I removed multiple TODOs as the version required to remove them was met.
New tests have also been added for this new constructor for both dart:io and web versions.
NOTE: I got the web version tests to run in a suboptimal environment because of flutter/flutter-intellij#5550, although it seemed like it didn't change the behavior of the tests.
It seems like a move to a federated plugin for cross_file is in the works (flutter/flutter#91869), it could split XFile into multiple classes and break this PR. Should this feature be delayed for the move to a federated or do we merge now and see how it goes then improve it via feedback when the package will be federated?

This PR will enable support for content:// URIs to be implemented with XFiles (It is the first step to close flutter/flutter#147037).

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

theskyblockman and others added 4 commits April 27, 2024 20:55
- Added lazy-loading of a blob for the file content which fixes saveTo with html.dart when using a custom source
- Fixed incorrect constructor in interface.dart
- Moved TextXFileSource to common.dart file
@ditman
Copy link
Member

ditman commented Apr 29, 2024

(One of) The problems with XFile in its current version, is that it should have been modeled as an interface which people use to read the contents of the file, but then we would have per-platform versions of the creation of the file, so we can use the most appropriate version for each platform (from bytes, from a Blob, from a io:File...)

This PR may help by introducing the extra complexity of the XFileSource (which I think is fine). However, it might have been nicer to migrate the current data sources to internal implementations of the XFileSource, if possible, so you don't have to keep an internal _file and a _source, and compare which one is not null to read from one or the other.

This approach may also fail on the web, where the implementation of Stream openRead is not that great (to put it mildly, there's a TODO linked in the relevant bit of the source).

Anyway, what do you think @stuartmorgan?

@theskyblockman
Copy link
Author

theskyblockman commented Apr 29, 2024

it might have been nicer to migrate the current data sources to internal implementations of the XFileSource

I have successfully kept only the source in the XFile with dart:io, but the web version needs to have access to the blob to read its content which is tricky to get both inside the source and in XFile for saveTo. This could be resolved by making XFileSource require saveTo, but this would mean the implementer would need to deal with differences between web and dart:io.
For example if we construct an XFile using XFile.fromData on web, internally we could have used a shared XFileSource implementation for both web and dart:io but because of that we would need to implement saveTo twice.

This would make the source manage the destination and we don't want that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants