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

Add preserveAspectRatio to CameraOptions while resizing an image #3297

Closed
wants to merge 4 commits into from

Conversation

iphilgood
Copy link
Contributor

Hi guys

First of all, this is my very first PR to the capacitor repo.

This PR is a retry of PR #2050 and should also fix ionic-team/capacitor-plugins#2 (also the discussion #1952). I've used the Android implementation of @sandstrom (shout-out) and also did an implementation for iOS.

There's now a preserveAspectRatio flag on CameraOptions which is set to true by default.

I've also fixed a bug where the app would crash while saving an image to the gallery on Android. This only happens on certain devices but I was able to reproduce it by creating 32 images until it crashed. A random UUID gets now appended to the file name to make sure no name clashes happen anymore.

If there's anything I need to change or enhance, please let me know.

@sandstrom
Copy link
Contributor

@iphilgood Interesting with the Android crashes. We've had an issue like that which we couldn't track down (couldn't reliably reproduce). Could be something else, but eager to try this if this ends up being merged.

Minor, but I'd argue that preserveAspectRatio is what everyone want, so I wouldn't even add that as an option.

The only reason for adding an option would be backwards compatibility, but if that's the case I'd name it legacyIgnoreAspectRatio, make the default value true (to avoid a breaking change), but mention that 99% of people would want to have it set to false. Then only keep it until the next major release where that option would be dropped.

@imhoffd
Copy link
Contributor

imhoffd commented Jul 20, 2020

@iphilgood @sandstrom I'm guessing you want this before Capacitor 3? If so, you will need to switch the base branch of this PR to 2.x. main is for Capacitor 3 now that we're in a feature freeze. We could probably squeeze this in as a "fix" (even though it's really a feature).

We do want to maintain backwards compatibility, even for the weird behavior in this case. preserveAspectRatio will need to be defaulted to false. In Capacitor 3 we will change the default and perhaps even remove the option altogether.

@iphilgood
Copy link
Contributor Author

@sandstrom Yeah it took me some time to really find out what the issue was. I initially thought it has something to do with an inserted SD Card for the device. But after taking 32 images while testing I was able to reproduce it and I hope your crashes are gonna be fixed with this as well 😊

Regarding the aspect ratio I totally agree with you. But in my case I'm just happy to prevent distorted images in any way. In the meantime I've created my own Camera Plugin which prevents the distorted images. As soon as this gets hopefully merged, I'm gonna remove my custom plugin again. And thanks for your part of the implementation 👍🏽

@dwieeb Do you already know when Capacitor 3 is going to be released? Otherwise I think it would be nice if you could squeeze it in as a fix.

I'm going to close this PR and create a new one which is based on the 2.x branch. Just to maintain the history. I'm also going to set the default of preserveAspectRatio to false.

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.

Preserve aspect ratio
3 participants