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

"Freestyle" cropping does not maintain aspect ratio #135

Closed
yukuku opened this issue Jun 28, 2016 · 10 comments
Closed

"Freestyle" cropping does not maintain aspect ratio #135

yukuku opened this issue Jun 28, 2016 · 10 comments

Comments

@yukuku
Copy link

yukuku commented Jun 28, 2016

I'm using version 1.5.0 (because I don't want the native libraries)

When I enable the freestyle cropping, it seems to ignore the aspect ratio I set (1:1).

@shliama
Copy link
Contributor

shliama commented Jun 28, 2016

That is what freestyle cropping should do - it lets user to set any aspect ratio :) Therefore it's useless to combine this feature with pre-set aspect ratio.

@yukuku
Copy link
Author

yukuku commented Jun 28, 2016

Not really — even if the freestyle cropping is enabled (which means user can drag the crop rectangle instead of moving the image), aspect ratio can still be maintained. See another library's demo gif: https://raw.githubusercontent.com/ArthurHub/Android-Image-Cropper/master/art/demo.gif

@shliama
Copy link
Contributor

shliama commented Jun 28, 2016

Oh, that's great 👍
Please fork and pull-request, looking forward to add this feature :octocat:

@yukuku
Copy link
Author

yukuku commented Jun 28, 2016

Is it ok if I fork and make changes on 1.5.0? I do really like 1.5.0 more
than 2.x because:

  1. No native lib
  2. content:// uris are not resolved to file path. See
    https://commonsware.com/blog/2016/03/14/psa-file-scheme-ban-n-developer-preview.html
    for why it is important not to get file path from content:// uris.

On Tue, Jun 28, 2016 at 4:28 PM, Oleksii [email protected] wrote:

Oh, that's great 👍
Please fork and pull-request, looking forward to add this feature [image:
:octocat:]


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#135 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABV7Xa0-tTOMbhZMRytFYPg9eH6EVDciks5qQNssgaJpZM4I_0U7
.

@shliama
Copy link
Contributor

shliama commented Jun 28, 2016

I don't see anything wrong with the native libraries... however if you check the code, you may notice that I try to resolve a file path from content:// uris (and it still works for a large amount of cases), but if it fails - I use content:// uri directly via InputStream. I got the image file one way or another.

@yukuku
Copy link
Author

yukuku commented Jun 28, 2016

Native lib is subjective. I just don't want to make the APK bigger.

However, reading the file from content:// uri will fail if the app does not have the permission
to read external storage (or if the user denies the permission on M). It
crashes when I integrate the library without giving that permission.

@shliama
Copy link
Contributor

shliama commented Jun 28, 2016

The sample handles permission and works well on any platform. I don't think uCrop should handle permissions for you.

@yukuku
Copy link
Author

yukuku commented Jun 28, 2016

The sample requires read external storage permission even before the crop operation starts. It is not actually needed to select a photo using another app.

@suau
Copy link

suau commented Jul 8, 2016

@yukuku Agreed with the permissions. I started using uCrop 2.1.1 and I believe it's the best cropping library out there, however that permission thing struck me, as the permission isn't necessary when directly loading the passed in Uri.
With the native libraries concern however, I believe (didn't verify) uCrop is one of the few libraries which can crop large images without downsampling due to the native library, which doesn't need to hold the complete image in memory to crop (streaming ? again not verified @shliama are my assumptions correct ?).

@TeeRawk
Copy link
Contributor

TeeRawk commented Mar 6, 2017

outdated

@TeeRawk TeeRawk closed this as completed Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants