Skip to content

Conversation

takameyer
Copy link
Contributor

This pull request saves the cropped image as a JPEG in iOS, reflecting the behavior of the previous library.
Android also preforms the same function.

with the .jpg extension.  The previous RN ImageEditor stored the cropped
image as a JPEG.  This commit fixes this issue.
@takameyer
Copy link
Contributor Author

Fixes issue #43

@Trancever
Copy link
Contributor

Trancever commented Nov 7, 2019

@takameyer We have recently added support on iOS for cropping transparent images (PNG) just like it is done on Android. We should change the extension of the saved file to "png" from "jpg" instead of using UIImageJPEGRepresentation back.

@takameyer
Copy link
Contributor Author

takameyer commented Nov 7, 2019

If that is the case, then i require an extension so that it can be saved as jpeg. The old version worked this way and the app I am working on requires it.

I will reword and rework the pull request.

@takameyer
Copy link
Contributor Author

I guess the big question is what the default should be and how the interface should behave. In our app, we are taking direct camera input and providing an interface to crop the picture. JPEG behaves well in this case since it will never have a transparency. This is also what iOS expects, which is the problem with issue #35 .

What I would like to see is:

const format = ImageCropFormat.JPEG
const croppedImagePath = await ImageEditor.cropImage(uri, cropData, format)

and ImageCropFormat would be defined as:

enum ImageCropFormat {
  JPEG = 'JPEG',
  PNG = 'PNG'
}

Since the old core library for ImageEditor used JPEG, I would set this as default.
Let me know if this is suitable and I will update the pull request.

@Trancever
Copy link
Contributor

@takameyer I am ok with having an optional format argument that defaults to "jpeg". We just need to make sure Android behaves in the same way. Feel free to submit a PR that implements it. Thanks!

the mimetype of the input image path and determines what it should
save it as.  If image/png is detected, then it will output a PNG.
If anything else is detected, then it uses JPEG.
@takameyer
Copy link
Contributor Author

I reviewed the Android code and found out that it determines the mimetype of the input image and produces saves the cropped image as the determined mimetype, or JPEG if it cannot be determined.
The changes in this pull request provide the same behavior in iOS. If a PNG is provided, than a PNG is the output. If anything else is provided, then JPEG is the output.

@takameyer takameyer changed the title ios cropped image is saved with .jpg file extension, but as PNG iOS - determine the output image type from the input image type Nov 12, 2019
@takameyer
Copy link
Contributor Author

@Trancever Are you okay with this pull request? It is a bit different than we discussed, but I feel it is the correct way to handle it.

@Trancever
Copy link
Contributor

@takameyer Let me check it tomorrow :)

@Trancever
Copy link
Contributor

@takameyer I've tested your changes and it doesn't work properly. contentTypeForImage function returns nil for both jpeg and png images. Looks like imageLoader from core of react-native somehow transforms the raw data or the code responsible for getting NSData from UIImage doesn't work correctly.

How did you test your changes?
Did you load image to crop from local e.g. camera roll or from remote e.g. web url?

@takameyer
Copy link
Contributor Author

I tested directly from camera and camera roll.

@Trancever
Copy link
Contributor

@takameyer So it doesn't work for me when I test images taken from web. Can you check that?

…d nil. This has been replaced with a simple file extension check on the input url. If a PNG is provided, then a PNG is returned. Otherwise JPEG is returned.
@takameyer
Copy link
Contributor Author

@Trancever So it seems that the method i was using to convert UIImage to NSData was not successful in any case. I have reverted to checking the input url for the file extension and creating a PNG if a PNG is provided. Otherwise JPEG is returned.
I have tested this both the camera roll and with a web image url.

@Trancever
Copy link
Contributor

@takameyer Awesome, I will test it tomorrow and if everything works properly I will merge it.

@azimgd
Copy link

azimgd commented Nov 18, 2019

Hey @Trancever can we merge this now please ?

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.

3 participants