[file_selector] Updates cross_file to 0.4.0 and updates to platform interface to latest standard - #13147

This pattern has drawbacks:

1. It depends on knowledge of the Darwin cross file implementation.
2. It doesn't extend to out-of-tree platforms

An alternative might be to add an API to XFile to try to convert it to a File. If that operation succeeds, we can use Image.file, otherwise we fallback to Image.memory. What would you think of that approach?

This is true, but you also need to know that web only works with Image.network anyways. I don't know if there is a way to avoid needing platform specific knowledge without proving an Image toImage(XFile) somewhere or adding XFile support to Image directly.

Added a TODO that includes flutter/flutter#196306 for this. I think it could still be worthwhile for a platform implementation to handle returning a file path if possible. That would at least make it so Uri.parse(file.uri).toFilePath() doesn't need to be called everywhere. Also, just attempting to create a File with the URI from XFile wouldn't be enough. One could make a valid file path from any URI like 123 and the user wouldn't know that you cant access the file that way. So the issue leaves it up to the platform implementation to handle whether the URI can be provided as a file path.

I also don't want to return a File in XFile because i think I want to avoid importing dart:io in the app-facing package. I could be wrong, but I remember a weird interaction with pub where if a package imports dart:io, it removes the flag that web is supported for a package.

I could be wrong, but I remember a weird interaction with pub where if a package imports dart:io, it removes the flag that web is supported for a package.

Ah that's a very good point 👍

Added a TODO that includes flutter/flutter#196306 for this.

Ah nice, after this TODO I imagine the code will look something like this?

Future<Image> _getImage() async {
  if (kIsWeb) {
    return Image.network(file.uri);
  } else if (file.asFilePath case String path) {
    return Image.file(File(path));
  } else {
    return Image.memory(await file.readAsBytes());
  }
}

If so, that addresses my concern and SGTM 👍

If this is a ScopedStorageXFile, we need to dispose() it. On iOS/macOS, forgetting to call NSURL.stopAccessingSecurityScopedResource can result in the OS preventing the app from accessing more resources. Some thoughts:

• We should update the examples to properly dispose XFiles
• It seems we should add a XFile.dispose method. Apps shouldn't need to cast an XFile to check whether they need to dispose the XFile.
• Your app now needs to call dispose() when it didn't need before. This is a breaking change (even though your app will continue to compile). The CHANGELOG.md should explain this and provide migration instructions

Yes, this was also brought up in the cross_file 1.0 PR and the design doc. To avoid requiring users to call dispose or potentially just forgetting, cross_file_darwin and cross_file_web automatically clean up resources when the ScopedStorageXFile is garbage collected. (cross_file_android doesn't need it)

I didn't consider this a breaking change because web and macOS need to clean up resources with the current implementation, but they don't. I think it's more of a bug:

Web: https://github.com/flutter/packages/blob/main/packages/file_selector/file_selector_web/lib/src/dom_helper.dart#L70
MacOS: NSURL.startAccessingSecurityScopedResource is called automatically and NSURL.stopAccessingSecurityScopedResource is supposed to be called.

I prefer not requiring calling dispose with every XFile and I don't want ScopedStorage platforms to expect the user to remember to do it. Unfortunately, requiring to call dispose affects more than just file_selector. Any API that returns or uses a XFile would have to remember to call dispose. But I think this is more of a cross_file plugin discussion and shouldn't affect this PR since it's currently handled automatically on all platforms.

I added documentation to the file_selector methods to recommend calling the dispose method though:

/// If the returned [XFile] is a [ScopedStorageXFile], it is recommended that
/// the [ScopedStorageXFile.dispose] is called when the app no longer needs
/// access to the resource.

Maurice and I discussed this offline. Adding XFile.dispose() has the following concerns:

• It complicates XFile. One could argue this complexity is good: you get one API that works well across all implementations. Also, Flutter uses this pattern (e.g., BoxPainter has an empty dispose method that not all subclasses implement).
• XFile.dispose() could cause different implementations to behave differently. Calling a method after dispose() would error for ScopedStorageXFile but not FileSystemXFile. We can mitigate this by adding a debugDisposed property so that FileSystemXFile can error if methods are called after dispose().

We agreed to add XFile.dispose() after this discussion.

I imagine that an app that uses file_selector might mock FileSelectorPlatform with an implements. This is no longer possible now that FileSelectorPlatform is an abstract base class. Could we add a quick migration guide for how to mock the file selector plugin?

It looks like FileSelectorWeb web doesn't implement getDirectories, so it throws an UnimplementedError. This is a pre-existing bug, but since we're introducing a new getDirectories method could you update FileSelectorWeb to return an empty array with a test?

Can we remove this now?

Should we rename this to getDirectoryUri? If yes, don't forget to update the doc above :)

Should this be update to returns a file URI.? Same comment for openFiles below

Can we replace this with import 'package:flutter/foundation.dart';? It doesn't look like we use Cupertino here.

+1