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

feat: Add custom save path #2900

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

j-jonathan
Copy link
Contributor

@j-jonathan j-jonathan commented May 22, 2024

What

This PR adds a feature allowing users to define the output path when taking a photo/snapshot or recording a video.
The idea was presented in #893.

Additionally, this PR fixes an issue on Android where the quality parameter for takeSnapshot was mandatory, resulting in an error [EUNSPECIFIED: quality] if not provided. (#2804)

Changes

Added a new parameter "path" to specify the output directory for photos, snapshots, and recorded videos.
Updated the relevant functions to handle the custom save path.

Tested on

iPhone 12, iOS 17.4.1
Huawei ELE-L29, Android 12

Related issues

Copy link

vercel bot commented May 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-vision-camera ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 29, 2024 1:26pm

@j-jonathan j-jonathan changed the title save path Add custom save path May 22, 2024
@j-jonathan j-jonathan changed the title Add custom save path feat: Add custom save path May 22, 2024
@mrousavy
Copy link
Owner

Hi! Thank you so much for this contribution, this is good stuff.
I have a few code-organization/style concerns which I will think about and get back to you soon.

To share what's going on inside my head;

  • I am not a big fan of having one function for getting a path or creating a temp file.
  • I think maybe we can get the path at high-level using a sync native function, and basically always assume the path is non-null and writable on native side. In the JS func for startRecording/takePhoto/takeSnapshot we get a temp path from native if the current path is null (or not writable). This also reduces the amount of logic/branching on the native side and moves it to JS, where code is more flexible in general
  • message is a bit weird for the error parameter, I'd call it cause or something and pass an error I guess? Just a minor nitpick

Don't worry about changes right now, I'll get back to you in a few days about this. But I definitely think this should go into main, this is good nice work 👍

@j-jonathan
Copy link
Contributor Author

Thank you for taking the time to review my PR and for your positive feedback.

I agree that it could be a good idea to have a function for getting a temporary path and managing the path logic directly in the JS code. This would indeed simplify the native side and make the code more flexible overall.

I'll wait for your detailed feedback before making any changes. Looking forward to refining this feature based on your suggestions :)

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

Successfully merging this pull request may close these issues.

2 participants