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

Camera #33

Merged
merged 71 commits into from
Dec 19, 2020
Merged

Camera #33

merged 71 commits into from
Dec 19, 2020

Conversation

ikeith
Copy link
Contributor

@ikeith ikeith commented Sep 1, 2020

@dalepo
Copy link

dalepo commented Sep 21, 2020

Just a FYI there's an open issue regarding the camera plugin resetting the application (Android)
ionic-team/capacitor#2931

@asztal
Copy link

asztal commented Oct 20, 2020

I've been working on a plugin of my own to implement video capture, as I don't see any capacitor plugins for it. It's at a very early stage at the moment but if I were to make a PR to the capacitor camera plugin, would it be considered, or would I have to make a community plugin?

I would just use the cordova plugin for capturing video but in my case I have received a lot of crash reports relating to the cordova camera plugin when used with Capacitor.

@imhoffd
Copy link
Contributor

imhoffd commented Oct 21, 2020

@asztal Thanks for bringing it up! Video capture would belong in the Camera plugin for sure. I've added this issue to track it: #78

We'd be happy to look at a PR that adds that functionality! We're hoping to merge this PR into main sometime this week.

@barrcodes
Copy link

barrcodes commented Oct 26, 2020

@ikeith @dwieeb I'm curious what the plan is here? Our enterprise application had a bug with Samsung devices which I was able to create a hotfix for by patching Camera.java. I was coming here to submit a PR to fix the issue, and noticed that the Camera plugin is gone.

Is a new plugin on the horizon?

Edit: I think I see what's happening here. There's a draft PR into the plugins repo. Sorry, this was confusing to read. What does the roadmap look like for releasing this?

@ikeith
Copy link
Contributor Author

ikeith commented Oct 26, 2020

@barrcodes As part of the work on capacitor 3, the core plugins are being split out into separate packages (this PR is for the migrated plugin into its own package). We are cleaning up a few things in the process but it is incomplete at the moment, as-is capacitor 3. If you have a critical fix, please open a PR for it against the camera plugin in the 2.x branch of capacitor.

@ikeith
Copy link
Contributor Author

ikeith commented Dec 9, 2020

Also, the Camera Intent on Android don't really need the Camera permission, so if the Camera permission is not present on the AndroidManifest.xml, the plugin should not require the permission and everything should still work.
In v2 it requested the permission because the apps shipped with it, but on v3 won't be the case, so we should have that into account. Should first check if the permission is in the manifest, and only request it if present.

This behavior has been changed but it is dependent on ionic-team/capacitor#3911

@ikeith
Copy link
Contributor Author

ikeith commented Dec 11, 2020

Also, the Camera Intent on Android don't really need the Camera permission, so if the Camera permission is not present on the AndroidManifest.xml, the plugin should not require the permission and everything should still work.
In v2 it requested the permission because the apps shipped with it, but on v3 won't be the case, so we should have that into account. Should first check if the permission is in the manifest, and only request it if present.

This behavior has been changed but it is dependent on ionic-team/capacitor#3911

This has been updated again, now dependent on ionic-team/capacitor#3939

@ikeith ikeith requested a review from jcesarmobile December 15, 2020 23:42
@imhoffd imhoffd mentioned this pull request Dec 16, 2020
2 tasks
@imhoffd
Copy link
Contributor

imhoffd commented Dec 16, 2020

I wonder if limited should be granted-limited or something so it starts with the more general state, much like prompt-with-rationale.

Also, we may have talked about this before, but what are the actual implications of limited photo access? Why does the app developer need to know if a user has granted only a subset of photos?

@ikeith
Copy link
Contributor Author

ikeith commented Dec 17, 2020

granted-limited makes sense but, on the other hand, limited is what the platform API returns so it has the benefit of being easier to look up. But I don't have a strong opinion either way.

The practical result of limited is that we cannot get the underlying PHAsset for any image that the user has not chosen to include in the list of selected images. Which means we cannot get any metadata for it. The user can still select any image in the picker regardless of what permissions they grant, but we will only be able to read metadata on the images for which they have granted access. So the state is a signal to the developer that metadata is less likely to be returned but we can't know for sure until the user actually picks one.

@imhoffd imhoffd merged commit 4864928 into main Dec 19, 2020
@imhoffd imhoffd deleted the camera branch December 19, 2020 20:02
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.

7 participants