capawesome-team / capacitor-plugins

⚡️ Community plugins for Capacitor. Supports Android, iOS and the Web.
https://capawesome.io/plugins/
196 stars 28 forks source link

feat(file-picker): save all picked files in a temporary subfolder on iOS #13

Closed massimoliani closed 3 months ago

massimoliani commented 1 year ago

Working on iOS the plugin has to copy with filemanager a picked media to a tmp folder. The tmp folder today is located on the root folder of app container.

Is it possible to locate this temporary folder inside a capacitor/filesystem folders? For example inside /Library/Caches?

Could be a great feature to locate all files inside a sub folder for the picker "picker-tmp" so we can check for lost content and cleanup if necessary.

Last one to improve final user experience to have media stored on a sub folder, name generated as today filename, the filename inside using original filename for the media.

Thank you.

robingenz commented 1 year ago

Is it possible to locate this temporary folder inside a capacitor/filesystem folders? For example inside /Library/Caches?

This is indeed already possible, although the path looks different.

public async pickFile(): Promise<void> {
  const { files } = await FilePicker.pickFiles();
  const file = files[0];
  const result = await Filesystem.stat({
    path: file.name,
    directory: Directory.Cache,
  });
  console.log(result);
}

But I will unify it (see capawesome-team/capacitor-file-picker#95).

Could be a great feature to locate all files inside a sub folder for the picker "picker-tmp" so we can check for lost content and cleanup if necessary.

I will think about that.

Last one to improve final user experience to have media stored on a sub folder, name generated as today filename, the filename inside using original filename for the media.

The file name of the temporary file is already the same as the name of the original file. Or what do you mean exactly?

nvahalik commented 1 year ago

@robingenz Why does the FilePicker copy the file exactly?

If the user has access to the photo/video, why does it need to be duplicated?

When accessing large files, this "feature" makes our app unresponsive. There is no process or indication of what is going on.

Can we make the duplication process optional, especially if we don't intend to just use the original without making a modification?

robingenz commented 1 year ago

@nvahalik As far as I remember, we had the problem that the app only had access to the selected file via the file picker and copying the file to a temporary app folder was a way to be able to process the file with other plugins. However, I've also been planning to remove this copying process again (or make it optional) and take a closer look at the original problem. If you have more information or ideas feel free to share them here. It's definitely on my todo list.

nvahalik commented 1 year ago

Would you accept a PR if I made that change?

Perhaps just adding a createCacheCopy: boolean to the pick* options and had it default to true?

Or should I just fork it and figure it out for our own needs?

robingenz commented 1 year ago

@nvahalik I would first like to find out what the problem was at that time and how to actually solve it. I don't know how you process the picked file but you should have a similar problem without this workaround. If you can solve it I would be very happy about a PR. If a createCacheCopy property is the solution, then I have no problem with that. I just don`t want to add a property that might be deprecated in a month, because there is a better way to solve the problem.

nvahalik commented 1 year ago

I'm doing some research into this and I think I've found the reason why the copy exists.

Note: My iOS knowledge is pretty sub-par, so this is just based off research and testing.

In iOS 14+, PHPickerViewController is used. The result is a PHPickerResult. The results returned have NSItemProviders (result.itemProvider) attached to them, which, in turn, contain the actual results.

NSItemProvider is a bit more generic in the sense that it can represent any number of types of documents/files and can have a number of different sources: real files, the clipboard, or anything that extends the NSExtensionItem.

Therefore, it seems like the real reason why a temporary file is made is because there is no guarantee that the PHPickerResult will be an actual file. You wanted a result, and now you've got it. This works really well if you're just wanting an AppKit Image, because you can get just that very easily. The difficulty comes when you're wanting to do something else with this image.

This stands in contrast to the UIImagePickerController. What we get from that is a real URI. This is essentially what I want.

In other words, we can't just simply disable creating the local copy... we have to signal to the plugin that we don't want to use PHPickerViewController at all. Then we can get what we want.

nvahalik commented 1 year ago

Oh, and as far as the reason why the copy is required... it is because of this:

This method writes a copy of the file’s data to a temporary file, which the system deletes when the completion handler returns.

Source: https://developer.apple.com/documentation/foundation/nsitemprovider/2888338-loadfilerepresentation

robingenz commented 1 year ago

@nvahalik Thank you for your research. What would be your proposed solution?

From Apple.com:

PHPicker is the modern replacement for UIImagePickerController. In addition to its privacy-focused approach, the API also provides additional features for your app like search, multi-image selection, and the ability to zoom in or out on on the photo grid. We'll show you how PHPicker can help most apps avoid asking for direct library access and how you can implement it to improve the overall experience for people interacting with your app.

So in the long run the UIImagePickerController will be replaced by the PHPickerViewController.

nvahalik commented 1 year ago

@robingenz Given the circumstances, I don't think the code needs the PHPickerViewController implementation. You always want the URI and the UIImagePickerController always gives it to you. If you want to keep the new PHPickerViewController, then the implementation that currently exists is probably the best.

This problem is very similar to the issues we were facing on Android: there are 2 types of results: Content Provider URIs (content://) and "legacy" file URIs (file://).

In those instances, older code needed to be updated to support both code paths. In most cases it was a minimal change. This is mainly due to the fact that both are represented by Strings.

The case of iOS is different in that we have two distinct types at play here: NSURLs and NSItemProvider. Given how the APIs work, the best approach would be to just lean into the NSItemProvider and increase support for them at the plugin level:

  1. Don't discard them: "retain" them and keep them around until the app "releases"/deletes them. Maybe return them back as a URI item://<uuid>.
  2. Extend Capacitor to be able to return data back directly from an item:// URI and also support something like filters. That is, you can load a file from the fileystem, if Capacitor supported item://<uuid> directly, you could skip the file conversion altogether and just show them. A nice piece would be for instance defining things like filters which could resize images (e.g. item://<uuid>/image would show an image, and item://<uuid>/op:scale,w:800,h:600,f:jpeg,q:90 would give you an 800x600 JPEG at 90% quality.
  3. For operations which can, extend them to support hydrating the NSItemProvider and allowing those to be used. This would create two code paths, but you've already got them. e.g. Want to upload the file somewhere? Or resize it? Just send it over to a method that directly takes the item:// URI directly...

I'm assuming a couple of normative use-cases here:

  1. Selecting/displaying images
    1. Displaying the originals
    2. Gathering EXIF data
    3. Type conversions
    4. Resizing/optimizing originals
    5. Displaying thumbnails/variants
  2. Selecting/displaying videos
    1. Loading metadata
    2. Asset conversion/transcoding
    3. Generating thumbnails/posters
    4. Playback
    5. Trimming and other operations

If you want to avoid doing the extra work of copying files, especially large ones like we tend to use, then putting off the work of transcoding is important... you'd ideally want to pass in the NSItemProvider information to that process and just have it use the item directly.

Ultimately, given what Capacitor provides, a custom implementation that does all of this on a per use-case basis I think ultimately makes sense. If you're really wanting that level of control, then you've got it and having a plugin do everything is likely asking too much. It is almost trivial to just copy this picker into your own project and just have it customized. The only real thing lacking would be Capacitor natively supporting NSItemProvider resources being sent back to the WebView.

robingenz commented 1 year ago

Given the circumstances, I don't think the code needs the PHPickerViewController implementation. You always want the URI and the UIImagePickerController always gives it to you. If you want to keep the new PHPickerViewController, then the implementation that currently exists is probably the best.

@nvahalik Unfortunately, UIImagePickerController lacks a few features of PHPickerViewController like selectionLimit and preferredAssetRepresentationMode. For that reason, the plugin relies on PHPickerViewController unless we remove features again.

Thank you very much for all the feedback. There are some very interesting ideas in there that I will take a closer look at. If you are interested in creating a prototype for a new plugin version, feel free to create a PR and we'll take a closer look at everything there. Otherwise, I'll get back to you as soon as I've had time to take a closer look.

nvahalik commented 1 year ago

For that reason, the plugin relies on PHPickerViewController unless we remove features again.

I completely understand. My increasing understanding of the problem is hopefully evident from the comments.

The PHPPickerViewController implementation is arguably more useful from a native standpoint. What hinders the implementation here is both our particular use-case and the fact that Capacitor can't natively return back this information. It is "stuck in the past" where everything must exist on the filesystem.

@robingenz Would you have any guidance on approaching this with the main Capacitor team?

robingenz commented 11 months ago

@nvahalik Sorry for my late response.

Would you have any guidance on approaching this with the main Capacitor team?

Yes, but before that I need to take a closer look at the problem. The issue is still on my todo list this year.

nvahalik commented 11 months ago

Thanks for the response.

This is still on our radar. Our final solution was that we "forked" this module internally and reverted back to the UIImagePicker implementation for now. This serves us best.

However, I'd love to move this discussion up the chain if possible. Will you be at Ionic conf?

robingenz commented 11 months ago

Will you be at Ionic conf?

Unfortunately, I can't make it this year because I'm currently writing my master's thesis. That's why I have so little time for Capawesome at the moment. :/ But next year it is definitely planned.

CricketLaChica commented 7 months ago

Thanks for the response.

This is still on our radar. Our final solution was that we "forked" this module internally and reverted back to the UIImagePicker implementation for now. This serves us best.

However, I'd love to move this discussion up the chain if possible. Will you be at Ionic conf?

Is your forked module publicly available? I'm having a hard time accessing videos over 30 seconds using this plugin as it becomes too slow. @nvahalik

robingenz commented 7 months ago

I'm having a hard time accessing videos over 30 seconds using this plugin as it becomes too slow.

Did you set skipTranscoding to true (see https://github.com/capawesome-team/capacitor-plugins/issues/12#issuecomment-1767933290)?

robingenz commented 3 months ago

I am closing this issue as not planned for the time being as there have been no further requests for this feature.