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 13 forks source link

Files opened by "Copy to" get filenames mangled #4

Closed amake closed 4 years ago

amake commented 4 years ago

See #3 for a discussion of "Copy-to" files.

Problem

When you open a file by "Copy to", a copy of the file is placed in /private/var/mobile/Containers/Data/Application/$SANDBOX_UUID/Documents/Inbox/. Say the file is foobar.txt.

If you don't do anything with that file, it will just sit there forever.

Then when you "Copy to" foobar.txt again, the new copy will be renamed to foobar-2.txt to avoid overwriting the old copy.

(Why would you "Copy to" the same file twice? For instance opening files from Google Drive only allows doing "Copy to", not "Open in", so if you want to open files from Google Drive in a Flutter app using file_picker_writable then you will encounter this issue.)

Proposal

The only workaround I can see is to delete the copied file when you're done with it. You can delete the file just fine: File.fromUri(Uri.parse(fileInfo.uri)).delete() will succeed. However it's not clear which files are safe to delete this way.

So I would propose a flag: isCopy or isLocalFile or something, to indicate that the file was copied rather than being accessed in-place.

Other solutions I don't like

Just delete all of Documents/Inbox

This seems like overkill. What about use cases where some such files should be retained?

Just delete if the file URI is within the app's sandbox

This is surprisingly cumbersome to do correctly: Using e.g. path_provider you can obtain the app's Documents folder, but the URI is slightly different due to symlinks (one starts with /var, the other /private/var; the former is symlinked to the latter).

final docsDir = await getApplicationDocumentsDirectory();
final realDocsDir = docsDir.resolveSymbolicLinksSync();
final filePath = Uri.parse(fileInfo.uri).toFilePath();
if (filePath.startsWith(realDocsDir)) {
  // File is in app's sandbox
}

So we need another Flutter plugin, a roundtrip through a platform channel, and to hit the filesystem, all to determine something that flutter_picker_writable already knows.

hpoul commented 4 years ago

I also didn't realize that there is a Copy As with the document APIs on iOS, I thought it would be editable in place.. good to know.

Regarding flags: I think this is pretty similar to persistable - how about reusing it? it's basically the same thing? (It can only be read, and not written back.. and when we implement the proposed fix, we'd delete the file from the Inbox, it is also no longer readable)

I would guess from the name Inbox that we could also delete all of the files, because there shouldn't be any retained files anyway, those are only files which are copied there so they can be imported by the app. But I agree, it would be safer to only delete files we explicitly pass on..

To be honest, I think the file handling is generally a bit wrong right now with many left-over files by default.. Every operation should take a callback within which the caller on the dart side has access to the file, and once the callback returns, the file is always deleted. So for example:

final result = await FilePickerWritable().readFileWithIdentifier(fileIdentifier, (fileInfo, file) async {
  // now we can access `file`
  final contents = await file.readAsString();
  return contents;
});
// now `file` is deleted again.

because the FileInfo.file right now should actually only be read from.. because it is just a copy in most cases (e.g. android content uris, ios urls). but nobody is deleting those files.

I could imagine a similar write operation which takes care of creating a temporary file the caller can use to persist changes..

await FilePickerWritable().writeFileWithIdentifier(fileIdentifier, (file) async {
  // now we can access `file`
  await file.writeString('lorem ipsum);
});
// now `file` is deleted again.

ie. the FileInfo would actually no longer have the file property, but it is only available while inside the callback..

amake commented 4 years ago

Regarding flags: I think this is pretty similar to persistable - how about reusing it? it's basically the same thing? (It can only be read, and not written back.. and when we implement the proposed fix, we'd delete the file from the Inbox, it is also no longer readable)

If you're OK with this solution, I do think it's best. I was assuming that because the identifier is technically persistable (it just points to the "wrong" file) that you would prefer persistable: true. But that does violate the spirit and main use case of the plugin.

Actually it turns out you can't write to files in Documents/Inbox, so treating them as non-persistable seems like very much the right choice.

To be honest, I think the file handling is generally a bit wrong right now with many left-over files by default

You are properly using NSTemporaryDirectory to get tmp/; according to the docs:

Use this directory to write temporary files that do not need to persist between launches of your app. Your app should remove files from this directory when they are no longer needed; however, the system may purge this directory when your app is not running.

The contents of this directory are not backed up by iTunes or iCloud.

Since the OS will clean it up I think it's OK to leave them there. Also the user can delete FileInfo.file from Dart if they really want to.

It's just files in Inbox that need additional handling, and I think it's unreasonable to do so from the Dart side.

Every operation should take a callback within which the caller on the dart side has access to the file, and once the callback returns, the file is always deleted

ie. the FileInfo would actually no longer have the file property, but it is only available while inside the callback.

I understand and agree with your desire here, but I have to caution against this kind of API:

I will update my PR with:

amake commented 4 years ago

This was addressed by #3